x86/pvh: do not forward MADT Local APIC NMI structures to dom0

[PATCH] x86/pvh: do not forward MADT Local APIC NMI structures to dom0

Posted by Roger Pau Monne 1 year, 5 months ago

Currently Xen will passthrough any Local APIC NMI Structure found in
the native ACPI MADT table to a PVH dom0.  This is wrong because PVH
doesn't have access to the physical local APIC, and instead gets an
emulated local APIC by Xen, that doesn't have the LINT0 or LINT1
pins wired to anything.  Furthermore the ACPI Processor UIDs used in
the APIC NMI Structures are likely to not match the ones generated by
Xen for the Local x2APIC Structures, creating confusion to dom0.

Fix this by removing the logic to passthrough the Local APIC NMI
Structure for PVH dom0.

Fixes: 1d74282c45 ('x86: setup PVHv2 Dom0 ACPI tables')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 34 +---------------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 307edc6a8c..d44de7f2b2 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -58,9 +58,6 @@
 static unsigned int __initdata acpi_intr_overrides;
 static struct acpi_madt_interrupt_override __initdata *intsrcovr;
 
-static unsigned int __initdata acpi_nmi_sources;
-static struct acpi_madt_nmi_source __initdata *nmisrc;
-
 static unsigned int __initdata order_stats[MAX_ORDER + 1];
 
 static void __init print_order_stats(const struct domain *d)
@@ -763,25 +760,6 @@ static int __init cf_check acpi_set_intr_ovr(
     return 0;
 }
 
-static int __init cf_check acpi_count_nmi_src(
-    struct acpi_subtable_header *header, const unsigned long end)
-{
-    acpi_nmi_sources++;
-    return 0;
-}
-
-static int __init cf_check acpi_set_nmi_src(
-    struct acpi_subtable_header *header, const unsigned long end)
-{
-    const struct acpi_madt_nmi_source *src =
-        container_of(header, struct acpi_madt_nmi_source, header);
-
-    *nmisrc = *src;
-    nmisrc++;
-
-    return 0;
-}
-
 static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
 {
     struct acpi_table_madt *madt;
@@ -797,16 +775,11 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
     acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE,
                           acpi_count_intr_ovr, UINT_MAX);
 
-    /* Count number of NMI sources in the MADT. */
-    acpi_table_parse_madt(ACPI_MADT_TYPE_NMI_SOURCE, acpi_count_nmi_src,
-                          UINT_MAX);
-
     max_vcpus = dom0_max_vcpus();
     /* Calculate the size of the crafted MADT. */
     size = sizeof(*madt);
     size += sizeof(*io_apic) * nr_ioapics;
     size += sizeof(*intsrcovr) * acpi_intr_overrides;
-    size += sizeof(*nmisrc) * acpi_nmi_sources;
     size += sizeof(*x2apic) * max_vcpus;
 
     madt = xzalloc_bytes(size);
@@ -862,12 +835,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
     acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE, acpi_set_intr_ovr,
                           acpi_intr_overrides);
 
-    /* Setup NMI sources. */
-    nmisrc = (void *)intsrcovr;
-    acpi_table_parse_madt(ACPI_MADT_TYPE_NMI_SOURCE, acpi_set_nmi_src,
-                          acpi_nmi_sources);
-
-    ASSERT(((void *)nmisrc - (void *)madt) == size);
+    ASSERT(((void *)intsrcovr - (void *)madt) == size);
     madt->header.length = size;
     /*
      * Calling acpi_tb_checksum here is a layering violation, but
-- 
2.37.3

Re: [PATCH] x86/pvh: do not forward MADT Local APIC NMI structures to dom0

Posted by Jan Beulich 1 year, 5 months ago

On 16.11.2022 17:42, Roger Pau Monne wrote:
> Currently Xen will passthrough any Local APIC NMI Structure found in
> the native ACPI MADT table to a PVH dom0.  This is wrong because PVH
> doesn't have access to the physical local APIC, and instead gets an
> emulated local APIC by Xen, that doesn't have the LINT0 or LINT1
> pins wired to anything.  Furthermore the ACPI Processor UIDs used in
> the APIC NMI Structures are likely to not match the ones generated by
> Xen for the Local x2APIC Structures, creating confusion to dom0.

Plus we should have passed through Local x2APIC NMI Structures then as
well.

> Fix this by removing the logic to passthrough the Local APIC NMI
> Structure for PVH dom0.
> 
> Fixes: 1d74282c45 ('x86: setup PVHv2 Dom0 ACPI tables')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit with the implied ack in there provisional upon Andrew accepting
your response to his reply.

Jan

Re: [PATCH] x86/pvh: do not forward MADT Local APIC NMI structures to dom0

Posted by Andrew Cooper 1 year, 5 months ago

On 17/11/2022 09:27, Jan Beulich wrote:
> On 16.11.2022 17:42, Roger Pau Monne wrote:
>> Currently Xen will passthrough any Local APIC NMI Structure found in
>> the native ACPI MADT table to a PVH dom0.  This is wrong because PVH
>> doesn't have access to the physical local APIC, and instead gets an
>> emulated local APIC by Xen, that doesn't have the LINT0 or LINT1
>> pins wired to anything.  Furthermore the ACPI Processor UIDs used in
>> the APIC NMI Structures are likely to not match the ones generated by
>> Xen for the Local x2APIC Structures, creating confusion to dom0.
> Plus we should have passed through Local x2APIC NMI Structures then as
> well.
>
>> Fix this by removing the logic to passthrough the Local APIC NMI
>> Structure for PVH dom0.
>>
>> Fixes: 1d74282c45 ('x86: setup PVHv2 Dom0 ACPI tables')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit with the implied ack in there provisional upon Andrew accepting
> your response to his reply.

I'm confident that removing this code is better than leaving it present,
so I don't have an issue with the patch going in like this.


But, at the moment, I'm not convinced that this is the end of the
necessary changes.

~Andrew

Re: [PATCH] x86/pvh: do not forward MADT Local APIC NMI structures to dom0

Posted by Roger Pau Monné 1 year, 5 months ago

On Thu, Nov 17, 2022 at 10:27:41AM +0100, Jan Beulich wrote:
> On 16.11.2022 17:42, Roger Pau Monne wrote:
> > Currently Xen will passthrough any Local APIC NMI Structure found in
> > the native ACPI MADT table to a PVH dom0.  This is wrong because PVH
> > doesn't have access to the physical local APIC, and instead gets an
> > emulated local APIC by Xen, that doesn't have the LINT0 or LINT1
> > pins wired to anything.  Furthermore the ACPI Processor UIDs used in
> > the APIC NMI Structures are likely to not match the ones generated by
> > Xen for the Local x2APIC Structures, creating confusion to dom0.
> 
> Plus we should have passed through Local x2APIC NMI Structures then as
> well.

Sadly this is not possible for PVH dom0, Linux will use the ACPI
Processor UID as vCPU ID in hypercalls, so if the UIDs don't start at
0 and are sequential Linux will panic during boot because vCPU
operations will fail.

> > Fix this by removing the logic to passthrough the Local APIC NMI
> > Structure for PVH dom0.
> > 
> > Fixes: 1d74282c45 ('x86: setup PVHv2 Dom0 ACPI tables')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit with the implied ack in there provisional upon Andrew accepting
> your response to his reply.

Thanks, Roger.

Re: [PATCH] x86/pvh: do not forward MADT Local APIC NMI structures to dom0

Posted by Jan Beulich 1 year, 5 months ago

On 17.11.2022 11:23, Roger Pau Monné wrote:
> On Thu, Nov 17, 2022 at 10:27:41AM +0100, Jan Beulich wrote:
>> On 16.11.2022 17:42, Roger Pau Monne wrote:
>>> Currently Xen will passthrough any Local APIC NMI Structure found in
>>> the native ACPI MADT table to a PVH dom0.  This is wrong because PVH
>>> doesn't have access to the physical local APIC, and instead gets an
>>> emulated local APIC by Xen, that doesn't have the LINT0 or LINT1
>>> pins wired to anything.  Furthermore the ACPI Processor UIDs used in
>>> the APIC NMI Structures are likely to not match the ones generated by
>>> Xen for the Local x2APIC Structures, creating confusion to dom0.
>>
>> Plus we should have passed through Local x2APIC NMI Structures then as
>> well.
> 
> Sadly this is not possible for PVH dom0, Linux will use the ACPI
> Processor UID as vCPU ID in hypercalls, so if the UIDs don't start at
> 0 and are sequential Linux will panic during boot because vCPU
> operations will fail.

Sure - I was merely hinting at the original attempt having been incomplete
(besides being flawed as per the description here).

Jan

Re: [PATCH] x86/pvh: do not forward MADT Local APIC NMI structures to dom0

Posted by Andrew Cooper 1 year, 5 months ago

On 16/11/2022 16:42, Roger Pau Monne wrote:
> Currently Xen will passthrough any Local APIC NMI Structure found in
> the native ACPI MADT table to a PVH dom0.  This is wrong because PVH
> doesn't have access to the physical local APIC, and instead gets an
> emulated local APIC by Xen, that doesn't have the LINT0 or LINT1
> pins wired to anything.  Furthermore the ACPI Processor UIDs used in
> the APIC NMI Structures are likely to not match the ones generated by
> Xen for the Local x2APIC Structures, creating confusion to dom0.
>
> Fix this by removing the logic to passthrough the Local APIC NMI
> Structure for PVH dom0.
>
> Fixes: 1d74282c45 ('x86: setup PVHv2 Dom0 ACPI tables')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Yeah, that was never going to work correctly.

That said, I'm not certain we can get away with discarding them either. 
Some systems really do use ExtINT in IO-APIC entries, and dom0 is
capable of configuring this if it thinks it wants virtual wire mode.

Is dom0 likely to get more or less confused with the LAPIC not
defaulting to regular x86 norms?  (The answer to this question is
whether we should fake up up an NMI structure for each vCPU.)

~Andrew

Re: [PATCH] x86/pvh: do not forward MADT Local APIC NMI structures to dom0

Posted by Roger Pau Monné 1 year, 5 months ago

On Wed, Nov 16, 2022 at 04:56:41PM +0000, Andrew Cooper wrote:
> On 16/11/2022 16:42, Roger Pau Monne wrote:
> > Currently Xen will passthrough any Local APIC NMI Structure found in
> > the native ACPI MADT table to a PVH dom0.  This is wrong because PVH
> > doesn't have access to the physical local APIC, and instead gets an
> > emulated local APIC by Xen, that doesn't have the LINT0 or LINT1
> > pins wired to anything.  Furthermore the ACPI Processor UIDs used in
> > the APIC NMI Structures are likely to not match the ones generated by
> > Xen for the Local x2APIC Structures, creating confusion to dom0.
> >
> > Fix this by removing the logic to passthrough the Local APIC NMI
> > Structure for PVH dom0.
> >
> > Fixes: 1d74282c45 ('x86: setup PVHv2 Dom0 ACPI tables')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Yeah, that was never going to work correctly.
> 
> That said, I'm not certain we can get away with discarding them either. 
> Some systems really do use ExtINT in IO-APIC entries, and dom0 is
> capable of configuring this if it thinks it wants virtual wire mode.

But the MADT entries discussed here (Local APIC NMI Structure) deal
exclusively with the LAPIC LINT# pins, not IO-APIC pins.

Interrupt Source Override Structure on the MADT are the ones that deal
with IO-APIC pins, and those we do forward them to dom0 and are setup
correctly (because they don't reference any Processor UID at all).

Thanks, Roger.