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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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