Re: [PATCH v3 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 28, 2022 at 02:14:26PM +0100, Jan Beulich wrote:
> On 28.02.2022 12:20, Roger Pau Monné wrote:
> > On Thu, Feb 24, 2022 at 03:16:08PM +0100, Jan Beulich wrote:
> >> On 18.02.2022 18:29, Jane Malalane wrote:
> >>> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >>> @@ -3333,15 +3333,15 @@ static void vmx_install_vlapic_mapping(struct 
> >>> vcpu *v)
> >>>  
> >>>  void vmx_vlapic_msr_changed(struct vcpu *v)
> >>>  {
> >>> -    int virtualize_x2apic_mode;
> >>> +    bool virtualize_x2apic_mode;
> >>>      struct vlapic *vlapic = vcpu_vlapic(v);
> >>>      unsigned int msr;
> >>>  
> >>>      virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
> >>>                                  cpu_has_vmx_virtual_intr_delivery) &&
> >>> -                               cpu_has_vmx_virtualize_x2apic_mode );
> >>> +                               v->domain->arch.hvm.assisted_x2apic );
> >>
> >> Following from my comment on patch 1, I'd expect this to become a simple
> >> assignment of v->domain->arch.hvm.assisted_x2apic (at which point the
> >> local variable could go away), just like ...
> > 
> > I think we want to keep assisted_x{2}apic mapped to
> > SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES and
> > SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE respectively, so that in the
> > future we could add further controls for
> > SECONDARY_EXEC_APIC_REGISTER_VIRT and interrupt delivery.
> 
> If we want to be able to control more (most?) VMX sub-features, it
> would seem to me as if this would better be modeled accordingly
> right away. At that point there would likely need to be VMX and SVM
> specific controls rather than general HVM ones.

I would have to check the AMD interface for hardware APIC
virtualization support, I'm not sure how different the control values
are there.

> Plus then it might
> make sense to match bit assignments in our interface with that in
> the VT-x spec.

That could work for things in secondary_exec_control, but posted
interrupts are controlled in pin based exec control, so we would need
to expose two different fields? Not sure it's worth the extra effort
to match bit positions with the spec (or maybe I'm not understanding
this correctly).

Are you suggesting a (VMX) generic interface where the hypervisor
exposes the raw vmx_secondary_exec_control and possibly
vmx_pin_based_exec_control and let the toolstack play with it, setting
in the VMCS what it gets back from the toolstack?

That would imply quite a rework of the code in order to detect enabled
features based on domain specific VMX fields (instead of using the
global vmx_{secondary,pin_based}_exec_control variables)

Thanks, Roger.