[PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[Chao Gao]

According to SDM "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
"EXTENDED XAPIC (X2APIC)" -> "x2APIC State Transitions", the APIC mode
and APIC ID are preserved when handling INIT signal, no matter the
current mode is x2APIC mode or xAPIC. All the other APIC registers are
initialized, exactly like the result of a reset.  This patch
introduces a new function lapic_INIT() and replaces the wrongly used
lapic_reset().

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
Why we have this restriction? As a consequence, This patch isn't
tested in that case.
---
 xen/arch/x86/hvm/vlapic.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 2653ba8..5f19253 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -83,6 +83,8 @@ static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
     ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
      == APIC_TIMER_MODE_TSC_DEADLINE)
 
+static void vlapic_INIT(struct vlapic *vlapic);
+
 static int vlapic_find_highest_vector(const void *bitmap)
 {
     const uint32_t *word = bitmap;
@@ -281,7 +283,7 @@ static void vlapic_init_sipi_one(struct vcpu *target, uint32_t icr)
         rc = vcpu_reset(target);
         ASSERT(!rc);
         target->fpu_initialised = fpu_initialised;
-        vlapic_reset(vcpu_vlapic(target));
+        vlapic_INIT(vcpu_vlapic(target));
         domain_unlock(target->domain);
         break;
     }
@@ -1237,18 +1239,15 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic)
             !(vlapic_get_reg(vlapic, APIC_LVTPC) & APIC_LVT_MASKED));
 }
 
-/* Reset the VLPAIC back to its power-on/reset state. */
-void vlapic_reset(struct vlapic *vlapic)
+/* VLAPIC handles INIT signal */
+static void vlapic_INIT(struct vlapic *vlapic)
 {
-    struct vcpu *v = vlapic_vcpu(vlapic);
     int i;
 
-    if ( !has_vlapic(v->domain) )
+    if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
         return;
 
-    vlapic_set_reg(vlapic, APIC_ID,  (v->vcpu_id * 2) << 24);
     vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
-
     for ( i = 0; i < 8; i++ )
     {
         vlapic_set_reg(vlapic, APIC_IRR + 0x10 * i, 0);
@@ -1262,7 +1261,6 @@ void vlapic_reset(struct vlapic *vlapic)
     vlapic_set_reg(vlapic, APIC_TMICT,   0);
     vlapic_set_reg(vlapic, APIC_TMCCT,   0);
     vlapic_set_tdcr(vlapic, 0);
-
     vlapic_set_reg(vlapic, APIC_DFR, 0xffffffffU);
 
     for ( i = 0; i < VLAPIC_LVT_NUM; i++ )
@@ -1275,6 +1273,18 @@ void vlapic_reset(struct vlapic *vlapic)
     destroy_periodic_time(&vlapic->pt);
 }
 
+/* Reset the VLPAIC back to its power-on/reset state. */
+void vlapic_reset(struct vlapic *vlapic)
+{
+    struct vcpu *v = vlapic_vcpu(vlapic);
+
+    if ( !has_vlapic(v->domain) )
+        return;
+
+    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
+    vlapic_INIT(vlapic);
+}
+
 /* rearm the actimer if needed, after a HVM restore */
 static void lapic_rearm(struct vlapic *s)
 {
-- 
1.8.3.1


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

Re: [PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[Jan Beulich]

>>> On 18.04.17 at 23:51, <chao.gao@intel.com> wrote:
> According to SDM "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
> "EXTENDED XAPIC (X2APIC)" -> "x2APIC State Transitions", the APIC mode
> and APIC ID are preserved when handling INIT signal, no matter the
> current mode is x2APIC mode or xAPIC. All the other APIC registers are
> initialized, exactly like the result of a reset.  This patch
> introduces a new function lapic_INIT() and replaces the wrongly used
> lapic_reset().

Please refer to the correct function names. Also according to the
patch there's nothing wrong with mode handling (you only correct
the ID part), yet the text above reads as if both were wrong (but
see below for a mode related aspect wrt RESET).

> HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
> Why we have this restriction? As a consequence, This patch isn't
> tested in that case.

HVM guests very well can use x2APIC, I'm seeing them use it all
the time. Please be more specific with your question.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -83,6 +83,8 @@ static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
>      ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
>       == APIC_TIMER_MODE_TSC_DEADLINE)
>  
> +static void vlapic_INIT(struct vlapic *vlapic);

Why the capitals? Yes, we do have a vlapic_init() function already,
but that doesn't preclude the new one being named e.g.
_vlapic_init(), vlapic_do_init(), or vlapic_handle_init().

> @@ -1237,18 +1239,15 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic)
>              !(vlapic_get_reg(vlapic, APIC_LVTPC) & APIC_LVT_MASKED));
>  }
>  
> -/* Reset the VLPAIC back to its power-on/reset state. */
> -void vlapic_reset(struct vlapic *vlapic)
> +/* VLAPIC handles INIT signal */

Please make the comment state what the _function_ does (not the
vlapic). Using the original comment, perhaps "Put the VLAPIC back
into its init state"?

> +static void vlapic_INIT(struct vlapic *vlapic)
>  {
> -    struct vcpu *v = vlapic_vcpu(vlapic);
>      int i;
>  
> -    if ( !has_vlapic(v->domain) )
> +    if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
>          return;
>  
> -    vlapic_set_reg(vlapic, APIC_ID,  (v->vcpu_id * 2) << 24);
>      vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
> -
>      for ( i = 0; i < 8; i++ )

Please don't drop blank lines like this.

> @@ -1262,7 +1261,6 @@ void vlapic_reset(struct vlapic *vlapic)
>      vlapic_set_reg(vlapic, APIC_TMICT,   0);
>      vlapic_set_reg(vlapic, APIC_TMCCT,   0);

Upwards from here is an LDR write. Judging from set_x2apic_id()
this may need to remain untouched in the INIT case? The register
is r/o in x2APIC mode after all. While the documentation includes
LDR in the set of registers being set to zero, I'm wondering whether
that's wrong.


>      vlapic_set_tdcr(vlapic, 0);
> -
>      vlapic_set_reg(vlapic, APIC_DFR, 0xffffffffU);

Same here.

> @@ -1275,6 +1273,18 @@ void vlapic_reset(struct vlapic *vlapic)
>      destroy_periodic_time(&vlapic->pt);
>  }
>  
> +/* Reset the VLPAIC back to its power-on/reset state. */

VLAPIC

> +void vlapic_reset(struct vlapic *vlapic)
> +{
> +    struct vcpu *v = vlapic_vcpu(vlapic);

const

> +    if ( !has_vlapic(v->domain) )
> +        return;
> +
> +    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
> +    vlapic_INIT(vlapic);

With what is left here it now very much looks like mode isn't being
(and hasn't been) reset to xAPIC - wouldn't you need to correct
this too? This might be particularly relevant for the call from
hvm_s3_suspend().

Jan


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

Re: [PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[Chao Gao]

On Wed, Apr 19, 2017 at 02:48:57AM -0600, Jan Beulich wrote:
>>>> On 18.04.17 at 23:51, <chao.gao@intel.com> wrote:
>> According to SDM "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
>> "EXTENDED XAPIC (X2APIC)" -> "x2APIC State Transitions", the APIC mode
>> and APIC ID are preserved when handling INIT signal, no matter the
>> current mode is x2APIC mode or xAPIC. All the other APIC registers are
>> initialized, exactly like the result of a reset.  This patch
>> introduces a new function lapic_INIT() and replaces the wrongly used
>> lapic_reset().
>
>Please refer to the correct function names. Also according to the
>patch there's nothing wrong with mode handling (you only correct
>the ID part), yet the text above reads as if both were wrong (but
>see below for a mode related aspect wrt RESET).

Agree. Regarding to the mode, I also think it should be resetting in 
vlapic_reset(). I will reset the mode and preserve the APIC base
address.

>
>> HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
>> Why we have this restriction? As a consequence, This patch isn't
>> tested in that case.
>
>HVM guests very well can use x2APIC, I'm seeing them use it all
>the time. Please be more specific with your question.

OK. I failed to enable guest's x2apic, with the output: IRQ Remapping doesn't
support X2APIC mode. Eventually, I found the xen_x2apic_para_available()
in linux kernel and it returns false when Xen exposes XENFEAT_hvm_pirqs to
guest.

>> @@ -1262,7 +1261,6 @@ void vlapic_reset(struct vlapic *vlapic)
>>      vlapic_set_reg(vlapic, APIC_TMICT,   0);
>>      vlapic_set_reg(vlapic, APIC_TMCCT,   0);
>
>Upwards from here is an LDR write. Judging from set_x2apic_id()
>this may need to remain untouched in the INIT case? The register
>is r/o in x2APIC mode after all. While the documentation includes
>LDR in the set of registers being set to zero, I'm wondering whether
>that's wrong.

agree.

>
>> +
>> +    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
>> +    vlapic_INIT(vlapic);
>
>With what is left here it now very much looks like mode isn't being
>(and hasn't been) reset to xAPIC - wouldn't you need to correct
>this too? This might be particularly relevant for the call from
>hvm_s3_suspend().

will do.

Thanks
Chao

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

Re: [PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[Jan Beulich]

>>> On 19.04.17 at 06:56, <chao.gao@intel.com> wrote:
> On Wed, Apr 19, 2017 at 02:48:57AM -0600, Jan Beulich wrote:
>>>>> On 18.04.17 at 23:51, <chao.gao@intel.com> wrote:
>>> HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
>>> Why we have this restriction? As a consequence, This patch isn't
>>> tested in that case.
>>
>>HVM guests very well can use x2APIC, I'm seeing them use it all
>>the time. Please be more specific with your question.
> 
> OK. I failed to enable guest's x2apic, with the output: IRQ Remapping doesn't
> support X2APIC mode. Eventually, I found the xen_x2apic_para_available()
> in linux kernel and it returns false when Xen exposes XENFEAT_hvm_pirqs to
> guest.

Looks more like a Linux/pvops question, which I continue to not
really be an expert on. Let's ask the maintainers (now Cc-ed)
(but it may well be that this simply depends on whether you
enable PVHVM mode)...

Jan


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

Re: [PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[Boris Ostrovsky]

On 04/19/2017 09:21 AM, Jan Beulich wrote:
>>>> On 19.04.17 at 06:56, <chao.gao@intel.com> wrote:
>> On Wed, Apr 19, 2017 at 02:48:57AM -0600, Jan Beulich wrote:
>>>>>> On 18.04.17 at 23:51, <chao.gao@intel.com> wrote:
>>>> HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
>>>> Why we have this restriction? As a consequence, This patch isn't
>>>> tested in that case.
>>>
>>> HVM guests very well can use x2APIC, I'm seeing them use it all
>>> the time. Please be more specific with your question.
>>
>> OK. I failed to enable guest's x2apic, with the output: IRQ Remapping doesn't
>> support X2APIC mode. Eventually, I found the xen_x2apic_para_available()
>> in linux kernel and it returns false when Xen exposes XENFEAT_hvm_pirqs to
>> guest.
>
> Looks more like a Linux/pvops question, which I continue to not
> really be an expert on. Let's ask the maintainers (now Cc-ed)
> (but it may well be that this simply depends on whether you
> enable PVHVM mode)...

Interestingly enough, I was looking at this last week and wondering why 
it was there. Ankur (copied) found an issue related to x2apic he thought 
was PVH-specific, but that was because HVM guests don't turn it on.

Konrad couldn't remember why it was there neither.

-boris

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

Re: [PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[Juergen Gross]

On 19/04/17 15:43, Boris Ostrovsky wrote:
> 
> 
> On 04/19/2017 09:21 AM, Jan Beulich wrote:
>>>>> On 19.04.17 at 06:56, <chao.gao@intel.com> wrote:
>>> On Wed, Apr 19, 2017 at 02:48:57AM -0600, Jan Beulich wrote:
>>>>>>> On 18.04.17 at 23:51, <chao.gao@intel.com> wrote:
>>>>> HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
>>>>> Why we have this restriction? As a consequence, This patch isn't
>>>>> tested in that case.
>>>>
>>>> HVM guests very well can use x2APIC, I'm seeing them use it all
>>>> the time. Please be more specific with your question.
>>>
>>> OK. I failed to enable guest's x2apic, with the output: IRQ Remapping
>>> doesn't
>>> support X2APIC mode. Eventually, I found the xen_x2apic_para_available()
>>> in linux kernel and it returns false when Xen exposes
>>> XENFEAT_hvm_pirqs to
>>> guest.
>>
>> Looks more like a Linux/pvops question, which I continue to not
>> really be an expert on. Let's ask the maintainers (now Cc-ed)
>> (but it may well be that this simply depends on whether you
>> enable PVHVM mode)...
> 
> Interestingly enough, I was looking at this last week and wondering why
> it was there. Ankur (copied) found an issue related to x2apic he thought
> was PVH-specific, but that was because HVM guests don't turn it on.
> 
> Konrad couldn't remember why it was there neither.

I think this is the answer:

https://lists.xen.org/archives/html/xen-devel/2010-12/msg00373.html

Basically some very old assumption...

I think we can remove the offending test and do some more cleanup in
this area (I think there are some #ifdef's and tests which can be
removed, too).

I'll post some patches.


Juergen


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

Re: [PATCH] x86/vlapic: Don't reset APIC mode/ID when handling INIT signal

[Juergen Gross]

On 19/04/17 15:58, Juergen Gross wrote:
> On 19/04/17 15:43, Boris Ostrovsky wrote:
>>
>>
>> On 04/19/2017 09:21 AM, Jan Beulich wrote:
>>>>>> On 19.04.17 at 06:56, <chao.gao@intel.com> wrote:
>>>> On Wed, Apr 19, 2017 at 02:48:57AM -0600, Jan Beulich wrote:
>>>>>>>> On 18.04.17 at 23:51, <chao.gao@intel.com> wrote:
>>>>>> HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
>>>>>> Why we have this restriction? As a consequence, This patch isn't
>>>>>> tested in that case.
>>>>>
>>>>> HVM guests very well can use x2APIC, I'm seeing them use it all
>>>>> the time. Please be more specific with your question.
>>>>
>>>> OK. I failed to enable guest's x2apic, with the output: IRQ Remapping
>>>> doesn't
>>>> support X2APIC mode. Eventually, I found the xen_x2apic_para_available()
>>>> in linux kernel and it returns false when Xen exposes
>>>> XENFEAT_hvm_pirqs to
>>>> guest.
>>>
>>> Looks more like a Linux/pvops question, which I continue to not
>>> really be an expert on. Let's ask the maintainers (now Cc-ed)
>>> (but it may well be that this simply depends on whether you
>>> enable PVHVM mode)...
>>
>> Interestingly enough, I was looking at this last week and wondering why
>> it was there. Ankur (copied) found an issue related to x2apic he thought
>> was PVH-specific, but that was because HVM guests don't turn it on.
>>
>> Konrad couldn't remember why it was there neither.
> 
> I think this is the answer:
> 
> https://lists.xen.org/archives/html/xen-devel/2010-12/msg00373.html
> 
> Basically some very old assumption...
> 
> I think we can remove the offending test and do some more cleanup in
> this area (I think there are some #ifdef's and tests which can be
> removed, too).
> 
> I'll post some patches.

Thinking more about it: now is a bad time. This would make the planned
revert of 72a9b186292 and da72ff5bfcb0 even more difficult.


Juergen


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