Re: [Xen-devel] [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.05.17 at 09:46, <chao.gao@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1003,14 +1003,12 @@ bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t 
> value)
>          }
>          else
>          {
> -            if ( unlikely(vlapic_x2apic_mode(vlapic)) )
> -                return 0;
>              vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
>              pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
>          }

Especially with you not adding any code here, ...

> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -53,6 +53,9 @@
>      ((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_BASE)
>  #define vlapic_x2apic_mode(vlapic)                              \
>      ((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD)
> +#define vlapic_xapic_mode(vlapic)                               \
> +    (((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) && \
> +     !vlapic_x2apic_mode(vlapic))

... I think it is imperative that both macros are fully symmetric,
i.e. the enabled check should either be present in both or (less
desirable) absent.

I'm also not convinced checking the MSR bit for enabled state
is fully in line with how other code checks enabled state (see
vlapic_enabled() et al).

Jan


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