[v2,2/2] x86/apic: simplify disconnect_bsp_APIC setup of LVT{0/1}
diff mbox series

Message ID 20200123180615.69370-3-roger.pau@citrix.com
State New, archived
Headers show
Series
  • x86/apic: improvements to disconnect_bsp_APIC
Related show

Commit Message

Roger Pau Monné Jan. 23, 2020, 6:06 p.m. UTC
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(-)

Comments

Jan Beulich Jan. 27, 2020, 4:43 p.m. UTC | #1
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
Andrew Cooper Jan. 27, 2020, 4:47 p.m. UTC | #2
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
Roger Pau Monné Jan. 27, 2020, 5:21 p.m. UTC | #3
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.
Andrew Cooper Jan. 27, 2020, 5:34 p.m. UTC | #4
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

Patch
diff mbox series

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);
     }
 }