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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2022 17:01, 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.
> 
> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Nit: Please try to keep tags in chronological order.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,12 @@ static int vmx_init_vmcs_config(bool bsp)
>              MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>      }
>  
> +    /* Check whether hardware supports accelerated xapic and x2apic. */
> +    assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> +    assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode &&
> +                                cpu_has_vmx_apic_reg_virt &&
> +                                cpu_has_vmx_virtual_intr_delivery;

Imo this would better live in .init.text, which would guarantee that
hot-onlined CPUs later cannot corrupt the original setting. IOW at the
very least the setting of the variables wants to be conditional upon
"bsp", but ideally this would move to e.g. vmx_vmcs_init().

> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -756,6 +756,10 @@ static inline void pv_inject_sw_interrupt(unsigned int 
> vector)
>                        : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
>                                                : PV64_VM_ASSIST_MASK)
>  
> +extern bool assisted_xapic_available;
> +
> +extern bool assisted_x2apic_available;

May I suggest that you omit the intermediate blank line, just like
you did below?

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -69,6 +69,9 @@ struct l3_cache_info {
>      unsigned long size;
>  };
>  
> +bool __read_mostly assisted_xapic_available;
> +bool __read_mostly assisted_x2apic_available;

With the above this could then be __ro_after_init.

> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -326,6 +326,10 @@ struct xen_arch_domainconfig {
>  
>  /* GPE0 bit set during CPU hotplug */
>  #define XEN_ACPI_GPE0_CPUHP_BIT      2
> +
> +#define XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_XAPIC  (1u << 0)
> +#define XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X2APIC (1u << 1)
> +
>  #endif

I have to admit I'm not convinced it is a good idea to put these here,
far away and disconnected from ...

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -120,6 +120,7 @@ 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_ARCH_ */

... this. See e.g. XEN_SYSCTL_get_cpu_levelling_caps which is entirely
x86-specific yet still fully contained in sysctl.h. Furthermore I
think you want "X86" somewhere in the names of the two #define-s.
While xAPIC and x2APIC are sufficiently clearly x86 terms, future
further bits may not as obviously be, yet we will want to have the
names be consistent. Perhaps best to replace "ARCH" by "X86"?

>      uint64_aligned_t total_pages;
>      uint64_aligned_t free_pages;
>      uint64_aligned_t scrub_pages;

With these subsequent fields you're introducing a padding hole.
Please make that explicit (including the setting of it to zero if
that doesn't happen implicitly already). And changing the layout of a
struct here also requires that XEN_SYSCTL_INTERFACE_VERSION be bumped,
at least as long as it hasn't yet in the current dev cycle.

Jan