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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 24/02/2022 17:04, Jan Beulich wrote:
> On 24.02.2022 17:59, Jane Malalane wrote:
>> 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.
> 
> My expectation would have been that assisted_x2apic_available is assigned
> what is (in context above) assigned to virtualize_x2apic_mode (in patch 1).
> Anything deviating from this needs, I think, explaining there.

Oh, sorry, I kept thinking you meant cpu_has_... instead of the local 
variable and it was just all confusing me. Would it help if I didn't use 
a local variable at all, or changed the name then. This is really just a 
check that is done before executing the code below in the function.

Thanks,

Jane.