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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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).

> --- 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.

> @@ -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).

Jan