Re: [PATCH] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/6/23 15:32, Andrew Cooper wrote:
> On 06/02/2023 12:58 pm, Xenia Ragiadakou wrote:
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 270bc98195..6138dc0885 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -3011,6 +3011,10 @@ const struct hvm_function_table * __init start_vmx(void)
> >           setup_ept_dump();
> >       }
> >   
> > +    if ( cpu_has_vmx_virtualize_apic_accesses ||
> > +         cpu_has_vmx_virtualize_x2apic_mode )
> x2apic_mode is definitely wrong here, but I think apic_accesses is too.
Why?

> The top of vmx_vlapic_msr_changed() is buggy too.
>
> Right now, the hook is called unconditionally.  Given no adjustment in
> vmx_vlapic_msr_changed(), the new form (using an alternative) needs
> calling unconditionally too.
Ok, I will initialize it unconditionally when the vmx_function_table is 
defined.
> Naming wise, Linux is fairly bogus too.  This should be
> hvm_update_vlapic_mode(), but I suspect the hook will disappear in due
> course.
I can change the name. I gave the same name for cross reference purposes.

> > diff --git a/xen/arch/x86/include/asm/hvm/hvm.h 
> > b/xen/arch/x86/include/asm/hvm/hvm.h
> > index 80e4565bd2..b690e2924c 100644
> > --- a/xen/arch/x86/include/asm/hvm/hvm.h
> > +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> > @@ -786,6 +787,11 @@ static inline int hvm_pi_update_irte(const struct vcpu *v,
> >       return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
> >   }
> >   
> > +static inline void hvm_set_virtual_apic_mode(struct vcpu *v)
> > +{
> > +    alternative_vcall(hvm_funcs.set_virtual_apic_mode, v);
> This has to be something like:
>
> if ( hvm_funcs.set_virtual_apic_mode )
>      alternative_vcall(...)
>
> Otherwise, Xen will BUG() every time an SVM guest modifies MSR_APIC_BASE.
Yes, that's true. I will fix it.

> ~Andrew
--
Xenia