1. Patchew
  2. Xen
  3. View series

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

Roger Pau Monne posted 1 patch 4 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200117150948.45014-1-roger.pau@citrix.com
xen/arch/x86/apic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
Posted by Roger Pau Monne 4 years, 3 months ago 
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 <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 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);
         }
 
         /* For LVT1 make it edge triggered, active high, nmi and enabled */
-- 
2.25.0


_______________________________________________
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
Posted by Andrew Cooper 4 years, 3 months ago 
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 <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  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
Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC
Posted by Roger Pau Monné 4 years, 3 months ago 
On Fri, Jan 17, 2020 at 03:30:44PM +0000, 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 <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  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
Posted by Jan Beulich 4 years, 3 months ago 
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
Posted by Roger Pau Monné 4 years, 3 months ago 
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
Posted by Jan Beulich 4 years, 3 months ago 
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
Posted by Roger Pau Monné 4 years, 3 months ago 
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
Posted by Jan Beulich 4 years, 3 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.