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

[Jane Malalane <Jane.Malalane@citrix.com>]

On 25/02/2022 13:13, Anthony PERARD wrote:
> On Fri, Feb 18, 2022 at 05:29:43PM +0000, Jane Malalane wrote:
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index 333ffad38d..1c83cae711 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -535,6 +535,13 @@
>>   #define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1
>>   
>>   /*
>> + * LIBXL_HAVE_ASSISTED_APIC indicates that libxl_domain_build_info has
>> + * assisted_x{2}apic fields, for enabling hardware assisted virtualization for
> 
> Could you spell out both "assisted_xapic and assisted_x2apic" as that
> would allow for grep to find both string.
Will do (for both cases).
> 
>> + * x{2}apic per domain.
>> + */
>> +#define LIBXL_HAVE_ASSISTED_APIC 1
>> +
>> +/*
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 39fdca1b49..ba5b8f433f 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -1384,8 +1384,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>>       }
>>   }
>>   
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> -                                              libxl_domain_build_info *b_info)
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> +                                             libxl_domain_build_info *b_info,
>> +                                             const libxl_physinfo *physinfo)
>>   {
>>       /* ACPI is disabled by default */
>>       libxl_defbool_setdefault(&b_info->acpi, false);
>> @@ -1399,6 +1400,8 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> 
> There is another return in this function, which want to return 0 rather
> than void.
> 
>>       memset(&b_info->u, '\0', sizeof(b_info->u));
>>       b_info->type = LIBXL_DOMAIN_TYPE_INVALID;
>>       libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH);
>> +
>> +    return 0;
>>   }
>>   
>>   int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index e0a06ecfe3..c377d13b19 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -819,11 +827,27 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>>   {
>>   }
>>   
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> -                                              libxl_domain_build_info *b_info)
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> +                                             libxl_domain_build_info *b_info,
>> +                                             const libxl_physinfo *physinfo)
>>   {
>>       libxl_defbool_setdefault(&b_info->acpi, true);
>>       libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
>> +
>> +    if (b_info->type != LIBXL_DOMAIN_TYPE_PV) {
>> +        libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
>> +                             physinfo->cap_assisted_xapic);
>> +        libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic,
>> +                             physinfo->cap_assisted_x2apic);
>> +    }
>> +
>> +    else if (!libxl_defbool_is_default(b_info->arch_x86.assisted_xapic) ||
> 
> This "else" needs to be on the same line as the "}" 2 lines above.
Okay.

Thank you,

Jane.