[v3,4/8] x86/psp: Add IRQ support

  The ACPI PSP device provides a mailbox irq that needs to be configured
through the ACPI mailbox register first. This requires passing a CPU
vector and physical CPU id and then enabling interrupt delivery.
Allocate the irq directly from the default irq domain
(x86_vector_domain) to get access to the required information. By
passing a cpumask through irq_alloc_info the vector is immediately
allocated (and not later during activation) and can be retrieved.

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 arch/x86/kernel/psp.c | 185 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 181 insertions(+), 4 deletions(-)
  

On Mon, Mar 20 2023 at 19:19, Jeremi Piotrowski wrote:
> The ACPI PSP device provides a mailbox irq that needs to be configured
> through the ACPI mailbox register first. This requires passing a CPU
> vector and physical CPU id and then enabling interrupt delivery.
> Allocate the irq directly from the default irq domain
> (x86_vector_domain) to get access to the required information. By
> passing a cpumask through irq_alloc_info the vector is immediately
> allocated (and not later during activation) and can be retrieved.

Sorry, but this is a horrible hack which violates _all_ design rules
for interrupts in one go.

 1) What's so special about this PSP device that it requires a vector
    directly from the vector domain and evades interrupt remapping?

 2) Why is this interrupt enabled _before_ it is actually requested?

 3) Why is this interrupt required to be bound to CPU0 and still exposes
    a disfunctional and broken affinity setter interface in /proc?

There is absolutely zero reason and justification to fiddle in the guts
of the x86 vector configuration data just because it's possible.

This is clearly a custom MSI implementation and the obvious solution is
a per device MSI interrupt domain.

Thanks,

        tglx
  

On 21/03/2023 11:31, Thomas Gleixner wrote:
> On Mon, Mar 20 2023 at 19:19, Jeremi Piotrowski wrote:
>> The ACPI PSP device provides a mailbox irq that needs to be configured
>> through the ACPI mailbox register first. This requires passing a CPU
>> vector and physical CPU id and then enabling interrupt delivery.
>> Allocate the irq directly from the default irq domain
>> (x86_vector_domain) to get access to the required information. By
>> passing a cpumask through irq_alloc_info the vector is immediately
>> allocated (and not later during activation) and can be retrieved.
> 
> Sorry, but this is a horrible hack which violates _all_ design rules
> for interrupts in one go.
>

Ouch. But I agree and it's not like i was trying to sneak it past you -
I just didn't know how to map the hardware behavior to kernel constructs
any better so was hoping to get some guidance.
 
>  1) What's so special about this PSP device that it requires a vector
>     directly from the vector domain and evades interrupt remapping?
>

The PSP interrupt configuration requires passing an APIC id and interrupt
vector that it will assert. The closest thing I found that provides me with
those was the x86_vector_domain. Here's the link to the ACPI interface, the
relevant info is on pages 13-15 (it's not very detailed, but that's all I
had when implementing this):
https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf

The platform that I have access to for testing does not have interrupt remapping
so it's not something I thought about. I don't know how this thing would behave
with interrupt remapping.
 
>  2) Why is this interrupt enabled _before_ it is actually requested?
> 

In this implementation: so that I can configure everything before setting the
irq number on the platform device. I had a version with an irq domain that I
wasn't happy with, I'll talk about that below.


>  3) Why is this interrupt required to be bound to CPU0 and still exposes
>     a disfunctional and broken affinity setter interface in /proc?> 

It's not required to be bound to CPU0. But yes, with this implementation
smp_affinity does not work (effective_affinity is correct though).

> There is absolutely zero reason and justification to fiddle in the guts
> of the x86 vector configuration data just because it's possible.
> 
> This is clearly a custom MSI implementation and the obvious solution is
> a per device MSI interrupt domain.
> 

This is an interesting suggestion, and it wasn't at all obvious to me that
this maps to an MSI interrupt domain. I'd love to get that to work, let me
ask some follow-up questions:

1) do I need both a standard irq domain and an msi domain?

2) what domain do I take as a parent?

3) i will still need to extract apic id + vector from somewhere. irq_compose_msi_msg
   or irq_write_msi_msg seem like candidates, but then i'd still be relying on knowledge
   about the hierarchy and need to poke at irqd_cfg or read msi_msg.arch_data. Am I missing
   something, do you see a better way?

4) will this actually work considering that the PSP will not actually write to an
   MSI address, and that I can't use all the data contained in an msi_msg?

My first attempt that "worked" used a plain irq domain and can be found here:
https://github.com/jepio/linux/commit/43c9ed6de82a3ae6c3f2d7894b3da049cb1ea4e4

It had this structure:

static struct irq_chip psp_irq_chip = {
	.name = "PSP",
	.irq_enable = psp_irq_enable,
	.irq_disable = psp_irq_disable,
	.irq_set_affinity = psp_irq_set_affinity, // calls psp_configure_irq(data, cpu num)
};

static int psp_irq_domain_map(struct irq_domain *d, unsigned int irq,
	irq_hw_number_t hwirq)
{
	struct psp_irq_domain_data *data = d->host_data;
	// this used a global system vector instead of x86_vector_domain
        // and i needed to program to some cpu otherwise psp_irq_enable fails
	psp_configure_irq(data, 0); 
	irq_set_chip_and_handler(irq, &psp_irq_chip, handle_simple_irq);
	irq_set_chip_data(irq, d->host_data);
	return 0;
}
static const struct irq_domain_ops psp_irq_domain_ops = {
	.map = psp_irq_domain_map,
};

static int psp_init_irq()
{
	// eliding lots of things
	psp_domain = irq_domain_create_linear(NULL, 1, &psp_irq_domain_ops, &psp_irq_data);
	return irq_create_mapping(psp_domain, 0);
}


This version had many flaws: it wasn't hierarchical, it relied on a static
system vector (it was only later that i figured out how to dynamically allocate the
vector), and setting irq affinity didn't actually work (due to lack of mask/unmask
I think).

Jeremi

> Thanks,
> 
>         tglx
>
  

On Tue, Mar 21 2023 at 20:16, Jeremi Piotrowski wrote:
> On 21/03/2023 11:31, Thomas Gleixner wrote:
>  
>>  1) What's so special about this PSP device that it requires a vector
>>     directly from the vector domain and evades interrupt remapping?
>
> The PSP interrupt configuration requires passing an APIC id and interrupt
> vector that it will assert. The closest thing I found that provides me with
> those was the x86_vector_domain. Here's the link to the ACPI interface, the
> relevant info is on pages 13-15 (it's not very detailed, but that's all I
> had when implementing this):
> https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf

That seriously expects an (extended) APIC-ID so that firmware can fiddle
with X2APIC ICR directly.

Why can't firmware people just use something which is properly
manageable by the OS, e.g. a MSI message or something like the ACPI
interrupt? Because that would just be too useful and not require
horrible hacks.

So my initial suggestion to piggy pack that on device MSI is moot. Let
me think about it.

Thanks,

        tglx
  

On Wed, Mar 22, 2023 at 11:07:25AM +0100, Thomas Gleixner wrote:
> On Tue, Mar 21 2023 at 20:16, Jeremi Piotrowski wrote:
> > On 21/03/2023 11:31, Thomas Gleixner wrote:
> >  
> >>  1) What's so special about this PSP device that it requires a vector
> >>     directly from the vector domain and evades interrupt remapping?
> >
> > The PSP interrupt configuration requires passing an APIC id and interrupt
> > vector that it will assert. The closest thing I found that provides me with
> > those was the x86_vector_domain. Here's the link to the ACPI interface, the
> > relevant info is on pages 13-15 (it's not very detailed, but that's all I
> > had when implementing this):
> > https://www.amd.com/system/files/TechDocs/58028_1.00-PUB.pdf
> 
> That seriously expects an (extended) APIC-ID so that firmware can fiddle
> with X2APIC ICR directly.
> 
> Why can't firmware people just use something which is properly
> manageable by the OS, e.g. a MSI message or something like the ACPI
> interrupt? Because that would just be too useful and not require
> horrible hacks.
> 
> So my initial suggestion to piggy pack that on device MSI is moot. Let
> me think about it.
> 
> Thanks,
> 
>         tglx
> 
> 

So this is what it looks when done properly with an irq_domain. It definitely
feels less like a hack to me now (thank you): setting affinity works and the
irq is enabled only when requested. The lack of support for interrupt remapping
and the need to configure with APIC-ID/vector comes from the fact that for
firmware designers this PSP operates at the same level as the AMD IOMMU (and
the IOMMU is responsible for interrupt remapping).

Let me know what you think.

Jeremi