Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
On 23.01.2020 16:43, Roger Pau Monné wrote:
> On Fri, Jan 17, 2020 at 05:25:12PM +0100, Jan Beulich wrote:
>> On 17.01.2020 17:08, Roger Pau Monné wrote:
>>> On Fri, Jan 17, 2020 at 04:56:00PM +0100, Jan Beulich wrote:
On 17.01.2020 16:09, 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.
But there are many more instances where we (have a risk to) do so,
most notably in clear_local_APIC(). The two step logic there is
anyway somewhat in conflict with the citation above.
>>>
>>> clear_local_APIC masks the error vector before doing any write, and
>>> clears ESR afterwards, there's a comment at the top:
>>>
>>> "Masking an LVT entry on a P6 can trigger a local APIC error
>>> if the vector is zero. Mask LVTERR first to prevent this."
>>>
>>> We could do the same (ie: mask LVTERR first and clear ESR afterwards)
>>> if that seems preferable. There's a maxlvt check in clear_local_APIC,
>>> but the sdm doesn't specify anyway to check if the lapic will accept a
>>> masked vector 0 write or not, so not sure whether we should replicate
>>> that check or just do it unconditionally on both disconnect_bsp_APIC
>>> and clear_local_APIC.
>>
>> I think doing it the most careful way is going to be best. I find it
>> surprising anyway that disconnect_bsp_APIC() doesn't write LVTERR
>> (or other LVTs except for LVT1) at all. The function looks to have a
>> goal of putting the APIC back into the state that we found it when
>> booting.
>
> Maybe it would be better to just call clear_local_APIC before trying
> to configure LVT{0/1}, which will leave LVT0 in a reset state and thus
> no write would be required in the !virt_wire_setup case?
Half of me was implying this as on option from the earlier reply.
The other half was thinking that this would be quite a lot of
behavioral change in one step. But since you think so too, why
don't we give this a try?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
On Fri, Jan 17, 2020 at 05:25:12PM +0100, Jan Beulich wrote:
> On 17.01.2020 17:08, Roger Pau Monné wrote:
> > On Fri, Jan 17, 2020 at 04:56:00PM +0100, Jan Beulich wrote:
> >> On 17.01.2020 16:09, 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.
> >>
> >> But there are many more instances where we (have a risk to) do so,
> >> most notably in clear_local_APIC(). The two step logic there is
> >> anyway somewhat in conflict with the citation above.
> >
> > clear_local_APIC masks the error vector before doing any write, and
> > clears ESR afterwards, there's a comment at the top:
> >
> > "Masking an LVT entry on a P6 can trigger a local APIC error
> > if the vector is zero. Mask LVTERR first to prevent this."
> >
> > We could do the same (ie: mask LVTERR first and clear ESR afterwards)
> > if that seems preferable. There's a maxlvt check in clear_local_APIC,
> > but the sdm doesn't specify anyway to check if the lapic will accept a
> > masked vector 0 write or not, so not sure whether we should replicate
> > that check or just do it unconditionally on both disconnect_bsp_APIC
> > and clear_local_APIC.
>
> I think doing it the most careful way is going to be best. I find it
> surprising anyway that disconnect_bsp_APIC() doesn't write LVTERR
> (or other LVTs except for LVT1) at all. The function looks to have a
> goal of putting the APIC back into the state that we found it when
> booting.
Maybe it would be better to just call clear_local_APIC before trying
to configure LVT{0/1}, which will leave LVT0 in a reset state and thus
no write would be required in the !virt_wire_setup case?
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
On 17.01.2020 17:08, Roger Pau Monné wrote: > On Fri, Jan 17, 2020 at 04:56:00PM +0100, Jan Beulich wrote: >> On 17.01.2020 16:09, 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. >> >> But there are many more instances where we (have a risk to) do so, >> most notably in clear_local_APIC(). The two step logic there is >> anyway somewhat in conflict with the citation above. > > clear_local_APIC masks the error vector before doing any write, and > clears ESR afterwards, there's a comment at the top: > > "Masking an LVT entry on a P6 can trigger a local APIC error > if the vector is zero. Mask LVTERR first to prevent this." > > We could do the same (ie: mask LVTERR first and clear ESR afterwards) > if that seems preferable. There's a maxlvt check in clear_local_APIC, > but the sdm doesn't specify anyway to check if the lapic will accept a > masked vector 0 write or not, so not sure whether we should replicate > that check or just do it unconditionally on both disconnect_bsp_APIC > and clear_local_APIC. I think doing it the most careful way is going to be best. I find it surprising anyway that disconnect_bsp_APIC() doesn't write LVTERR (or other LVTs except for LVT1) at all. The function looks to have a goal of putting the APIC back into the state that we found it when booting. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
On Fri, Jan 17, 2020 at 03:30:44PM +, Andrew Cooper wrote:
> On 17/01/2020 15:09, 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 reusing the current LVT LINT0 value and just adding the
> > mask bit to it.
> >
> > Reported-by: Andrew Cooper
> > Signed-off-by: Roger Pau Monné
> > ---
> > xen/arch/x86/apic.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> > index a6a7754d77..e4363639bd 100644
> > --- a/xen/arch/x86/apic.c
> > +++ b/xen/arch/x86/apic.c
> > @@ -281,7 +281,8 @@ void disconnect_bsp_APIC(int virt_wire_setup)
> > }
> > else {
> > /* Disable LVT0 */
> > -apic_write(APIC_LVT0, APIC_LVT_MASKED);
> > +value = apic_read(APIC_LVT0);
> > +apic_write(APIC_LVT0, value | APIC_LVT_MASKED);
> > }
>
> This really is ugly. It seems that we can't write LVT0 to the same
> state that it has after reset/INIT.
>
> For the code however, both halves of the if() condition do a
> read/modify/write. It would be nicer to have the read and write common,
> with modify alone having the if().
As said on my reply to Jan, we could do the same as clear_local_APIC
if that seems preferable.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
On Fri, Jan 17, 2020 at 04:56:00PM +0100, Jan Beulich wrote: > On 17.01.2020 16:09, 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. > > But there are many more instances where we (have a risk to) do so, > most notably in clear_local_APIC(). The two step logic there is > anyway somewhat in conflict with the citation above. clear_local_APIC masks the error vector before doing any write, and clears ESR afterwards, there's a comment at the top: "Masking an LVT entry on a P6 can trigger a local APIC error if the vector is zero. Mask LVTERR first to prevent this." We could do the same (ie: mask LVTERR first and clear ESR afterwards) if that seems preferable. There's a maxlvt check in clear_local_APIC, but the sdm doesn't specify anyway to check if the lapic will accept a masked vector 0 write or not, so not sure whether we should replicate that check or just do it unconditionally on both disconnect_bsp_APIC and clear_local_APIC. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
On 17.01.2020 16:09, 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. But there are many more instances where we (have a risk to) do so, most notably in clear_local_APIC(). The two step logic there is anyway somewhat in conflict with the citation above. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
On 17/01/2020 15:09, 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 reusing the current LVT LINT0 value and just adding the
> mask bit to it.
>
> Reported-by: Andrew Cooper
> Signed-off-by: Roger Pau Monné
> ---
> xen/arch/x86/apic.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index a6a7754d77..e4363639bd 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -281,7 +281,8 @@ void disconnect_bsp_APIC(int virt_wire_setup)
> }
> else {
> /* Disable LVT0 */
> -apic_write(APIC_LVT0, APIC_LVT_MASKED);
> +value = apic_read(APIC_LVT0);
> +apic_write(APIC_LVT0, value | APIC_LVT_MASKED);
> }
This really is ugly. It seems that we can't write LVT0 to the same
state that it has after reset/INIT.
For the code however, both halves of the if() condition do a
read/modify/write. It would be nicer to have the read and write common,
with modify alone having the if().
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
