RE: [PATCH v9 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
> From: Jane Malalane
> Sent: Friday, April 1, 2022 6:47 PM
>
> Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
> XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xAPIC
> and
> x2APIC, on x86 hardware. This is so that xAPIC and x2APIC virtualization
> can subsequently be enabled on a per-domain basis.
> 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
> Signed-off-by: Jane Malalane
Reviewed-by: Kevin Tian
> ---
> CC: Wei Liu
> CC: Anthony PERARD
> CC: Juergen Gross
> CC: Andrew Cooper
> CC: George Dunlap
> CC: Jan Beulich
> CC: Julien Grall
> CC: Stefano Stabellini
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Jun Nakajima
> CC: Kevin Tian
> CC: "Roger Pau Monné"
>
> v9:
> * Move assisted_x{2}apic_available to vmx_vmcs_init() so they get
>declared at boot time, after vmx_secondary_exec_control is set
>
> v8:
> * Improve commit message
>
> v7:
> * Make sure assisted_x{2}apic_available evaluates to false, to ensure
>Xen builds, when !CONFIG_HVM
> * Fix coding style issues
>
> v6:
> * Limit abi check to x86
> * Fix coding style issue
>
> v5:
> * Have assisted_xapic_available solely depend on
>cpu_has_vmx_virtualize_apic_accesses and assisted_x2apic_available
>depend on cpu_has_vmx_virtualize_x2apic_mode and
>cpu_has_vmx_apic_reg_virt OR cpu_has_vmx_virtual_intr_delivery
>
> v4:
> * Fallback to the original v2/v1 conditions for setting
>assisted_xapic_available and assisted_x2apic_available so that in
>the future APIC virtualization can be exposed on AMD hardware
>since fine-graining of "AVIC" is not supported, i.e., AMD solely
>uses "AVIC Enable". This also means that sysctl mimics what's
>exposed in CPUID
>
> 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
>
> v2:
> * Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two
> * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
> * Set assisted_x{2}apic_available to be conditional upon "bsp" and
>annotate it with __ro_after_init
> * Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to
>_X86_ASSISTED_X{2}APIC
> * Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within
>sysctl.h
> * Fix padding introduced in struct xen_sysctl_physinfo and bump
>XEN_SYSCTL_INTERFACE_VERSION
> ---
> tools/golang/xenlight/helpers.gen.go | 4
> tools/golang/xenlight/types.gen.go | 2 ++
> tools/include/libxl.h| 7 +++
> tools/libs/light/libxl.c | 3 +++
> tools/libs/light/libxl_arch.h| 4
> tools/libs/light/libxl_arm.c | 5 +
> tools/libs/light/libxl_types.idl | 2 ++
> tools/libs/light/libxl_x86.c | 11 +++
> tools/ocaml/libs/xc/xenctrl.ml | 5 +
> tools/ocaml/libs/xc/xenctrl.mli | 5 +
> tools/ocaml/libs/xc/xenctrl_stubs.c | 15 +--
> tools/xl/xl_info.c | 6 --
> xen/arch/x86/hvm/hvm.c | 3 +++
> xen/arch/x86/hvm/vmx/vmcs.c | 6 ++
> xen/arch/x86/include/asm/hvm/hvm.h | 5 +
> xen/arch/x86/sysctl.c| 4
> xen/include/public/sysctl.h | 11 ++-
> 17 files changed, 93 insertions(+), 5 deletions(-)
>
> diff --git a/tools/golang/xenlight/helpers.gen.go
> b/tools/golang/xenlight/helpers.gen.go
> index b746ff1081..dd4e6c9f14 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace)
> x.CapVpmu = bool(xc.cap_vpmu)
> x.CapGnttabV1 = bool(xc.cap_gnttab_v1)
> x.CapGnttabV2 = bool(xc.cap_gnttab_v2)
> +x.CapAssistedXapic = bool(xc.cap_assisted_xapic)
> +x.CapAssistedX2Apic = b
Re: [PATCH v9 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On Thu, Apr 07, 2022 at 10:45:10AM +0200, Jan Beulich wrote:
> On 07.04.2022 10:28, Roger Pau Monné wrote:
> > On Fri, Apr 01, 2022 at 11:47:12AM +0100, Jane Malalane wrote:
> >> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> >> @@ -2145,6 +2145,12 @@ int __init vmx_vmcs_init(void)
> >>
> >> ret = _vmx_cpu_up(true);
> >>
> >> +/* 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);
> >
> > Setting assisted_x{2}apic_available should only be done !ret, or else
> > we might be reporting those capabilities when VMX is not usable, which
> > would be misleading IMO.
>
> Hmm, while I agree with the observation, wouldn't it be better if all
> the feature masks were cleared in case of failure, such that other
> code using the predicates wouldn't be mislead either? (That would
> likely want to be a separate, prereq change.)
Possibly, yes.
vmx_vmcs_init failing will lead to start_vmx failing and thus the
hvm_function table won't get setup, so I think we got away without
doing the cleaning because there where no code paths using it
anymore as HVM was disabled.
To not delay this series anymore it might be easier to just set
assisted_x{2}apic_available inside the !ret if where the keyhandler
also gets set.
Thanks, Roger.
Re: [PATCH v9 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On 07.04.2022 10:28, Roger Pau Monné wrote:
> On Fri, Apr 01, 2022 at 11:47:12AM +0100, Jane Malalane wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -2145,6 +2145,12 @@ int __init vmx_vmcs_init(void)
>>
>> ret = _vmx_cpu_up(true);
>>
>> +/* 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);
>
> Setting assisted_x{2}apic_available should only be done !ret, or else
> we might be reporting those capabilities when VMX is not usable, which
> would be misleading IMO.
Hmm, while I agree with the observation, wouldn't it be better if all
the feature masks were cleared in case of failure, such that other
code using the predicates wouldn't be mislead either? (That would
likely want to be a separate, prereq change.)
Jan
Re: [PATCH v9 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On Fri, Apr 01, 2022 at 11:47:12AM +0100, Jane Malalane wrote:
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 709a4191ef..e5dde9f8ce 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -117,6 +117,9 @@ static const char __initconst warning_hvm_fep[] =
> static bool_t __initdata opt_altp2m_enabled = 0;
> boolean_param("altp2m", opt_altp2m_enabled);
>
> +bool __ro_after_init assisted_xapic_available;
> +bool __ro_after_init assisted_x2apic_available;
> +
> static int cf_check cpu_callback(
> struct notifier_block *nfb, unsigned long action, void *hcpu)
> {
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 56fed2db03..53d97eaf13 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -2145,6 +2145,12 @@ int __init vmx_vmcs_init(void)
>
> ret = _vmx_cpu_up(true);
>
> +/* 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);
Setting assisted_x{2}apic_available should only be done !ret, or else
we might be reporting those capabilities when VMX is not usable, which
would be misleading IMO.
The rest LGTM, so with this taken care of:
Reviewed-by: Roger Pau Monné
Thanks, Roger.
Re: [PATCH v9 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On Fri, Apr 01, 2022 at 11:47:12AM +0100, 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. This is so that xAPIC and x2APIC virtualization > can subsequently be enabled on a per-domain basis. > 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 > Signed-off-by: Jane Malalane Reviewed-by: Anthony PERARD # tools -- Anthony PERARD
Re: [PATCH v9 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On 01.04.2022 12:47, 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. This is so that xAPIC and x2APIC virtualization > can subsequently be enabled on a per-domain basis. > 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 > Signed-off-by: Jane Malalane Reviewed-by: Jan Beulich # hypervisor
