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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 24/02/2022 14:16, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> unless you have verified the sender and know the content is safe.
> 
> 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 ...
> 
>> -    if ( !cpu_has_vmx_virtualize_apic_accesses &&
>> +    if ( !v->domain->arch.hvm.assisted_xapic &&
>>            !virtualize_x2apic_mode )
>>           return;
> 
> ... here.
Previosuly we discussed setting v->domain->arch.hvm.assisted_xapic equal 
to only cpu_has_vmx_virtualize_x2apic_mode, that's why I have those 
additional checks as at least apic_reg_virt or virtual_intr_delivery are 
needed for the subsequent parts of this function. I might be 
misunderstanding your question.

Unless you mean that we should fallback to having 
v->domain->arch.hvm.assisted_xapic depend on those other features...?
> 
>> @@ -1124,9 +1125,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, 
>> uint32_t leaf,
>>            * and wrmsr in the guest will run without VMEXITs (see
>>            * vmx_vlapic_msr_changed()).
>>            */
>> -        if ( cpu_has_vmx_virtualize_x2apic_mode &&
>> -             cpu_has_vmx_apic_reg_virt &&
>> -             cpu_has_vmx_virtual_intr_delivery )
>> +        if ( cpu_has_vmx_apic_reg_virt && cpu_has_vmx_virtual_intr_delivery 
>> &&
>> +             v->domain->arch.hvm.assisted_x2apic )
>>               res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
> 
> While within the 80 cols limit, I think it would help readability if you
> kept it at one sub-condition per line.
Sure.

Thank you,

Jane.