On 4/3/2023 8:20 AM, Thomas Gleixner wrote: > On Sun, Apr 02 2023 at 17:44, Borislav Petkov wrote: >> On Fri, Mar 24, 2023 at 06:10:09PM +0100, Jeremi Piotrowski wrote: >>> Since the AMD PSP is a privileged device, there is a desire to not have to trust the >>> ACPI stack, >> >> And yet you do: >> >> + err = acpi_parse_aspt(&res[0], &pdata); >> + if (err) >> + return err; >> >> You don't trust the ACPI stack, and yet you're parsing an ACPI table?!?! >> You have to make up your mind here. I gave you background on why Microsoft system designers like to use the ASPT on *physical hardware* in our datacenters. It is because it allows them to setup a highly privileged system component through an isolated ACPI table, without needing to depend on the *rest of the ACPI stack* (other ACPI tables and/or the ACPI interpreter). The same reason they use IVRS for AMD IOMMU. I thought it might be good to write this down, as this shows that the ASPT is a hardware interface that has *some* value. I don't think further discussion on this point helps us make forward progress. We're trying to adhere to a specification for a physical device when modeling that same device in a virtual environment. Yes, this requires parsing an ACPI table. >> >> Btw, you still haven't answered my question about doing: >> >> devm_request_irq(dev, 9, ..) >> >> where 9 is the default ACPI interrupt. >> >> You can have some silly table tell you what to map or you can simply map >> IRQ 9 and be done with it. In this second case you can *really* not >> trust ACPI because you know which IRQ it is. > So I originally thought I answered when i said "because we're trying to not deviate from the hardware specification for the PSP". Interrupt configuration is part of that specification. But when I think about what you're suggesting, I can interpret it two ways: 1. Configure the PSP to raise the vector corresponding to ACPI IRQ 9. This might work and would look similar to the first version I posted. I'd fetch 'struct irq_cfg' for acpi_sci_irq, write the corresponding APIC-ID/vector into the PSP, enable PSP interrupt generation and then probe the "ccp" driver so that it can call "devm_request_irq(9)". I assume this would also require registering an irq affinity notifier, much like drivers/iommu/amd/init.c did before commit d1adcfbb520c. 2. Deviate from the hardware specification. >From reading acpi code (not at all an expert on this), that "9" does not look like a static value to me, so it requires either: a) passing a GSI number in an ACPI table b) defining it as being the same interrupt as the SCI, which comes from the FADT table. c) using the GPE mechanism of the ACPI SCI interrupt. So I'd need to define a third way for the PSP to interrupt the OS, one that would only be supported on Hyper-V. Work with our hypervisor and/or virtual firmware teams to make sure that the PSP model supports generating the interrupt in this way. Work with the Windows team to make Windows support it (the same virtual hardware model/virtual firmware is used regardless of the OS). I have no objection to doing "1." if it works. I don't see it as a big win over using an irq_domain. I don't think "2." is a reasonable thing to ask. We do regularly make suggestions to hypervisor/firmware teams on how to make things better supported in Linux without requiring hacks. But modelling a piece of hardware in a custom way to avoid following hardware specs is questionable. I also think that soon, when other people deploy more SEV-SNP hardware in their datacenters, they will also want to rely on the ASPT for the reasons listed at the top of the email, so we'll be adding support for it anyway. Which way do you suggest we go Boris? I'm not attached to the code at all but I am attached to adhering to hardware specifications. I can try to do "1." or stick with the irq_domain approach that i posted. Thanks, Jeremi
