Re: [PATCH v6 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 09/03/2022 10:36, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 05:31:17PM +0000, Jane Malalane wrote:
>> Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
>> XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xapic
>> and x2apic, on x86 hardware.
>> No such features are currently implemented on AMD hardware.
>>
>> HW assisted xAPIC virtualization will be reported if HW, at the
>> minimum, supports virtualize_apic_accesses as this feature alone means
>> that an access to the APIC page will cause an APIC-access VM exit. An
>> APIC-access VM exit provides a VMM with information about the access
>> causing the VM exit, unlike a regular EPT fault, thus simplifying some
>> internal handling.
>>
>> HW assisted x2APIC virtualization will be reported if HW supports
>> virtualize_x2apic_mode and, at least, either apic_reg_virt or
>> virtual_intr_delivery. This also means that
>> sysctl follows the conditionals in vmx_vlapic_msr_changed().
>>
>> For that purpose, also add an arch-specific "capabilities" parameter
>> to struct xen_sysctl_physinfo.
>>
>> Note that this interface is intended to be compatible with AMD so that
>> AVIC support can be introduced in a future patch. Unlike Intel that
>> has multiple controls for APIC Virtualization, AMD has one global
>> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
>> control cannot be done on a common interface.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
> 
> Overall LGTM, just one question and one nit.
> 
>> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
>> b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> index 5b4fe72c8d..7e9c32ad1b 100644
>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> @@ -712,7 +712,7 @@ CAMLprim value stub_xc_send_debug_keys(value xch, value 
>> keys)
>>   CAMLprim value stub_xc_physinfo(value xch)
>>   {
>>      CAMLparam1(xch);
>> -    CAMLlocal2(physinfo, cap_list);
>> +    CAMLlocal3(physinfo, cap_list, arch_cap_list);
>>      xc_physinfo_t c_physinfo;
>>      int r;
>>   
>> @@ -731,7 +731,7 @@ CAMLprim value stub_xc_physinfo(value xch)
>>              /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_MAX max */
>>              (c_physinfo.capabilities);
>>   
>> -    physinfo = caml_alloc_tuple(10);
>> +    physinfo = caml_alloc_tuple(11);
>>      Store_field(physinfo, 0, Val_int(c_physinfo.threads_per_core));
>>      Store_field(physinfo, 1, Val_int(c_physinfo.cores_per_socket));
>>      Store_field(physinfo, 2, Val_int(c_physinfo.nr_cpus));
>> @@ -743,6 +743,17 @@ CAMLprim value stub_xc_physinfo(value xch)
>>      Store_field(physinfo, 8, cap_list);
>>      Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1));
>>   
>> +#if defined(__i386__) || defined(__x86_64__)
>> +    /*
>> +     * arch_capabilities: physinfo_arch_cap_flag list;
>> +     */
>> +    arch_cap_list = c_bitmap_to_ocaml_list
>> +            /* ! physinfo_arch_cap_flag CAP_ none */
>> +            /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_X86_MAX max */
>> +            (c_physinfo.arch_capabilities);
>> +    Store_field(physinfo, 10, arch_cap_list);
>> +#endif
> 
> Have you tried to build this on Arm? I wonder whether the compiler
> will complain about arch_cap_list being unused there?
Built it - no error.

Thank you,

Jane.