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

2022-03-31 Thread Jane Malalane
On 23/03/2022 11:30, Roger Pau Monné wrote:
> On Wed, Mar 16, 2022 at 09:13:14AM +, Jane Malalane wrote:
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index e1e1fa14e6..77ce0b2121 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>   MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>   }
>>   
>> +/* Check whether hardware supports accelerated xapic and x2apic. */
>> +if ( bsp )
>> +{
>> +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);
>> +}
> 
> I'm afraid using cpu_has_vmx_* is not correct here. The
> vmx_secondary_exec_control variable hasn't been set here, so you will
> need to move those checks to the end of the function, after
> vmx_secondary_exec_control has been set.

Sorry I missed that. As Jan previously suggested, maybe I could also 
just move this to vmx_vmcs_init() and thus drop the "if ( bsp )" ?

Thank you,

Jane.

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

2022-03-23 Thread Roger Pau Monné
On Wed, Mar 16, 2022 at 09:13:14AM +, Jane Malalane wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index e1e1fa14e6..77ce0b2121 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>  MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>  }
>  
> +/* Check whether hardware supports accelerated xapic and x2apic. */
> +if ( bsp )
> +{
> +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);
> +}

I'm afraid using cpu_has_vmx_* is not correct here. The
vmx_secondary_exec_control variable hasn't been set here, so you will
need to move those checks to the end of the function, after
vmx_secondary_exec_control has been set.

Thanks, Roger.



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

2022-03-16 Thread Tian, Kevin
> From: Jane Malalane
> Sent: Wednesday, March 16, 2022 5:13 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 

btw the r-b from Roger is lost...

> ---
> 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é" 
> 
> 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  |  9 +
>  xen/arch/x86/include/asm/hvm/hvm.h   |  5 +
>  xen/arch/x86/sysctl.c|  4 
>  xen/include/public/sysctl.h  | 11 ++-
>  17 files changed, 96 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 = bool(xc.cap_assisted_x2apic)
> 
>   return nil}
> 
> @@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVm