| Message ID | 20200123180615.69370-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/apic: improvements to disconnect_bsp_APIC | expand |
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
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
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.
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
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); } }
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(-)