[Xen-devel] [PATCH v2 0/2] x86/apic: improvements to disconnect_bsp_APIC

[Xen-devel] [PATCH v2 0/2] x86/apic: improvements to disconnect_bsp_APIC

[Roger Pau Monne]

Hello,

The series contain some improvements to disconnect_bsp_APIC, which
started as a way to fix the "APIC error on CPU0: 40(00)" error printed
by some Intel boxes on reboot or shutdown. First patch is the fix for
the error, second patch is a cleanup.

Roger Pau Monne (2):
  x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
  x86/apic: simplify disconnect_bsp_APIC setup of LVT{0/1}

 xen/arch/x86/apic.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/2] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC

[Roger Pau Monne]

The Intel SDM states:

"When an illegal vector value (0 to 15) is written to a LVT entry and
the delivery mode is Fixed (bits 8-11 equal 0), the APIC may signal an
illegal vector error, without regard to whether the mask bit is set or
whether an interrupt is actually seen on the input."

And that's exactly what's currently done in disconnect_bsp_APIC when
virt_wire_setup is true and LVT LINT0 is being masked. By writing only
APIC_LVT_MASKED Xen is actually setting the vector to 0 and the
delivery mode to Fixed (0), and hence it triggers an APIC error even
when the LVT entry is masked.

This would usually manifest when Xen is being shut down, as that's
where disconnect_bsp_APIC is called:

(XEN) APIC error on CPU0: 40(00)

Fix this by calling clear_local_APIC prior to setting the LVT LINT
registers which already clear LVT LINT0, and hence the troublesome
write can be avoided as the register is already cleared.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Use clear_local_APIC in order to clear LINT0.
---
 xen/arch/x86/apic.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index a6a7754d77..508b1586f2 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -262,6 +262,8 @@ void disconnect_bsp_APIC(int virt_wire_setup)
         /* Go back to Virtual Wire compatibility mode */
         unsigned long value;
 
+        clear_local_APIC();
+
         /* For the spurious interrupt use vector F, and enable it */
         value = apic_read(APIC_SPIV);
         value &= ~APIC_VECTOR_MASK;
@@ -279,10 +281,6 @@ void disconnect_bsp_APIC(int virt_wire_setup)
             value = SET_APIC_DELIVERY_MODE(value, APIC_MODE_EXTINT);
             apic_write(APIC_LVT0, value);
         }
-        else {
-            /* Disable LVT0 */
-            apic_write(APIC_LVT0, APIC_LVT_MASKED);
-        }
 
         /* For LVT1 make it edge triggered, active high, nmi and enabled */
         value = apic_read(APIC_LVT1);
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/2] x86/apic: simplify disconnect_bsp_APIC setup of LVT{0/1}

[Roger Pau Monne]

There's no need to read the current values of LVT{0/1} for the
purposes of the function, which seem to be to save the currently
selected vector: in the destination modes used (ExtINT and NMI) the
vector field is ignored and hence can be set to 0.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/apic.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 508b1586f2..c18314c1a3 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -273,23 +273,12 @@ void disconnect_bsp_APIC(int virt_wire_setup)
 
         if (!virt_wire_setup) {
             /* For LVT0 make it edge triggered, active high, external and enabled */
-            value = apic_read(APIC_LVT0);
-            value &= ~(APIC_MODE_MASK | APIC_SEND_PENDING |
-                       APIC_INPUT_POLARITY | APIC_LVT_REMOTE_IRR |
-                       APIC_LVT_LEVEL_TRIGGER | APIC_LVT_MASKED );
-            value |= APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING;
-            value = SET_APIC_DELIVERY_MODE(value, APIC_MODE_EXTINT);
+            value = APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING | APIC_DM_EXTINT;
             apic_write(APIC_LVT0, value);
         }
 
         /* For LVT1 make it edge triggered, active high, nmi and enabled */
-        value = apic_read(APIC_LVT1);
-        value &= ~(
-            APIC_MODE_MASK | APIC_SEND_PENDING |
-            APIC_INPUT_POLARITY | APIC_LVT_REMOTE_IRR |
-            APIC_LVT_LEVEL_TRIGGER | APIC_LVT_MASKED);
-        value |= APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING;
-        value = SET_APIC_DELIVERY_MODE(value, APIC_MODE_NMI);
+        value = APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING | APIC_DM_NMI;
         apic_write(APIC_LVT1, value);
     }
 }
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC

[Jan Beulich]

On 23.01.2020 19:06, Roger Pau Monne wrote:
> The Intel SDM states:
> 
> "When an illegal vector value (0 to 15) is written to a LVT entry and
> the delivery mode is Fixed (bits 8-11 equal 0), the APIC may signal an
> illegal vector error, without regard to whether the mask bit is set or
> whether an interrupt is actually seen on the input."
> 
> And that's exactly what's currently done in disconnect_bsp_APIC when
> virt_wire_setup is true and LVT LINT0 is being masked. By writing only
> APIC_LVT_MASKED Xen is actually setting the vector to 0 and the
> delivery mode to Fixed (0), and hence it triggers an APIC error even
> when the LVT entry is masked.
> 
> This would usually manifest when Xen is being shut down, as that's
> where disconnect_bsp_APIC is called:
> 
> (XEN) APIC error on CPU0: 40(00)
> 
> Fix this by calling clear_local_APIC prior to setting the LVT LINT
> registers which already clear LVT LINT0, and hence the troublesome
> write can be avoided as the register is already cleared.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/apic: simplify disconnect_bsp_APIC setup of LVT{0/1}

[Jan Beulich]

On 23.01.2020 19:06, Roger Pau Monne wrote:
> There's no need to read the current values of LVT{0/1} for the
> purposes of the function, which seem to be to save the currently
> selected vector: in the destination modes used (ExtINT and NMI) the
> vector field is ignored and hence can be set to 0.

The question is - is there firmware putting data in these fields
that it expects to survive unmodified? Such behavior would be
highly suspect (to me at least), but you never know. There ought
to be a reason it's been done this way not just in Xen, but also
in Linux. IOW may I ask that you at least make an attempt to
submit the same change for Linux, to see what the feedback is?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/apic: simplify disconnect_bsp_APIC setup of LVT{0/1}

[Andrew Cooper]

On 27/01/2020 16:43, Jan Beulich wrote:
> On 23.01.2020 19:06, Roger Pau Monne wrote:
>> There's no need to read the current values of LVT{0/1} for the
>> purposes of the function, which seem to be to save the currently
>> selected vector: in the destination modes used (ExtINT and NMI) the
>> vector field is ignored and hence can be set to 0.
> The question is - is there firmware putting data in these fields
> that it expects to survive unmodified? Such behavior would be
> highly suspect (to me at least), but you never know. There ought
> to be a reason it's been done this way not just in Xen, but also
> in Linux. IOW may I ask that you at least make an attempt to
> submit the same change for Linux, to see what the feedback is?

I can tell you what the Linux feedback will be.

This is not a fastpath.  Always do RMW, because life is too short to
keep on unbreaking hardware which makes such assumptions.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/apic: simplify disconnect_bsp_APIC setup of LVT{0/1}

[Roger Pau Monné]

On Mon, Jan 27, 2020 at 04:47:48PM +0000, Andrew Cooper wrote:
> On 27/01/2020 16:43, Jan Beulich wrote:
> > On 23.01.2020 19:06, Roger Pau Monne wrote:
> >> There's no need to read the current values of LVT{0/1} for the
> >> purposes of the function, which seem to be to save the currently
> >> selected vector: in the destination modes used (ExtINT and NMI) the
> >> vector field is ignored and hence can be set to 0.
> > The question is - is there firmware putting data in these fields
> > that it expects to survive unmodified? Such behavior would be
> > highly suspect (to me at least), but you never know. There ought
> > to be a reason it's been done this way not just in Xen, but also
> > in Linux. IOW may I ask that you at least make an attempt to
> > submit the same change for Linux, to see what the feedback is?
> 
> I can tell you what the Linux feedback will be.
> 
> This is not a fastpath.  Always do RMW, because life is too short to
> keep on unbreaking hardware which makes such assumptions.

We already do such kinds of direct writes to LVT{0/1}, see
clear_local_APIC where Xen clears the registers by directly writing
APIC_LVT_MASKED to them (no RMW). As the modified code follows a call
to clear_local_APIC there's nothing to preserve at this point.

Note that init_bsp_APIC and smp_callin also call clear_local_APIC, so
there's no data in those registers that could survive (regardless of
my added call to clear_local_APIC in the previous patch).

I'm not arguing this is correct (not sure it's incorrect either), but
given how things are handled ATM it seems quite dumb to do a RMW in
disconnect_bsp_APIC: it gives the false impression Xen is preserving
something, when the register has already been wiped at startup.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/apic: simplify disconnect_bsp_APIC setup of LVT{0/1}

[Andrew Cooper]

On 27/01/2020 17:21, Roger Pau Monné wrote:
> On Mon, Jan 27, 2020 at 04:47:48PM +0000, Andrew Cooper wrote:
>> On 27/01/2020 16:43, Jan Beulich wrote:
>>> On 23.01.2020 19:06, Roger Pau Monne wrote:
>>>> There's no need to read the current values of LVT{0/1} for the
>>>> purposes of the function, which seem to be to save the currently
>>>> selected vector: in the destination modes used (ExtINT and NMI) the
>>>> vector field is ignored and hence can be set to 0.
>>> The question is - is there firmware putting data in these fields
>>> that it expects to survive unmodified? Such behavior would be
>>> highly suspect (to me at least), but you never know. There ought
>>> to be a reason it's been done this way not just in Xen, but also
>>> in Linux. IOW may I ask that you at least make an attempt to
>>> submit the same change for Linux, to see what the feedback is?
>> I can tell you what the Linux feedback will be.
>>
>> This is not a fastpath.  Always do RMW, because life is too short to
>> keep on unbreaking hardware which makes such assumptions.
> We already do such kinds of direct writes to LVT{0/1}, see
> clear_local_APIC where Xen clears the registers by directly writing
> APIC_LVT_MASKED to them (no RMW). As the modified code follows a call
> to clear_local_APIC there's nothing to preserve at this point.
>
> Note that init_bsp_APIC and smp_callin also call clear_local_APIC, so
> there's no data in those registers that could survive (regardless of
> my added call to clear_local_APIC in the previous patch).
>
> I'm not arguing this is correct (not sure it's incorrect either), but
> given how things are handled ATM it seems quite dumb to do a RMW in
> disconnect_bsp_APIC: it gives the false impression Xen is preserving
> something, when the register has already been wiped at startup.

The problem isn't with the currently defined fields (when we're trying
to clear them).  It is with implementations which depend on set reserved
bits not changing, and/or new fields defined at some future point.

If we already have a mix, then some more probably isn't a problem, but
you did ask what the Linux feedback would be...

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel