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 <[email protected]>
>> Signed-off-by: Jane Malalane <[email protected]>
>> ---
>> 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.
