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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 24/02/2022 14:08, 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:
>> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and
>> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic
>> and x2apic, on x86 hardware.
>> No such features are currently implemented on AMD hardware.
>>
>> For that purpose, also add an arch-specific "capabilities" parameter
>> to struct xen_sysctl_physinfo.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
>> ---
>> v3:
>>   * Define XEN_SYSCTL_PHYSCAP_ARCH_MAX for ABI checking and actually
>>     set arch_capbilities, via a call to c_bitmap_to_ocaml_list()
>>   * Have assisted_x2apic_available only depend on
>>     cpu_has_vmx_virtualize_x2apic_mode
> 
> I understand this was the result from previous discussion, but this
> needs justifying in the description. Not the least because it differs
> from when XEN_HVM_CPUID_X2APIC_VIRT would be set as well as from what
> vmx_vlapic_msr_changed() does. The difference between those two is
> probably intended (judging from a comment there), but the further
> difference to what you add isn't obvious.

Okay, I will make that explicit.

> Which raises another thought: If that hypervisor leaf was part of the
> HVM feature set, the tool stack could be able to obtain the wanted
> information without altering sysctl (assuming the conditions to set
> the respective bits were the same). And I would view it as generally
> reasonable for there to be a way for tool stacks to know what
> hypervisor leaves guests are going to get to see (at the maximum and
> by default).

Like the "cpuid" xtf test allows us to?
Makes sense to me. I'm happy to take that up after.

> 
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -35,7 +35,7 @@
>>   #include "domctl.h"
>>   #include "physdev.h"
>>   
>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015
>>   
>>   /*
>>    * Read console content from Xen buffer ring.
>> @@ -111,6 +111,13 @@ struct xen_sysctl_tbuf_op {
>>   /* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
>>   #define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2
>>   
>> +/* The platform supports x{2}apic hardware assisted emulation. */
>> +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC  (1u << 0)
>> +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC (1u << 1)
>> +
>> +/* Max XEN_SYSCTL_PHYSCAP_X86{ARM}__* constant. Used for ABI checking. */
>> +#define XEN_SYSCTL_PHYSCAP_ARCH_MAX XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC
> 
> Doesn't this then need to be a per-arch constant? The ABIs would differ
> unless we required that every bit may only be used for a single purpose.
> IOW it would want to be named XEN_SYSCTL_PHYSCAP_X86_MAX.

Okay.

> 
>> @@ -120,6 +127,8 @@ struct xen_sysctl_physinfo {
>>       uint32_t max_node_id; /* Largest possible node ID on this host */
>>       uint32_t cpu_khz;
>>       uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */
>> +    uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_X86{ARM}_??? */
>> +    uint32_t pad; /* Must be zero. */
> 
> If this was an input field (or could potentially become one), the
> comment would be applicable. But the whole struct is OUT-only, so
> either omit the comment or use e.g. "will" or better "reserved" (as
> people shouldn't make themselves dependent on the field being zero).

Will ommit.

Thank you,

Jane.