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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2022 16:48, Roger Pau Monné wrote:
> 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?

Not necessarily all of them, but on a case by case basis. But _where_
a control bit would appear (if supported) would be well known up front.
This wouldn't be very different from VMX'es "supports the 1-setting of"
information provided via MSRs. The hypervisor would indicate which of
the bits can be controlled on a per-guest basis.

Jan