1. Patchew
  2. Xen
  3. View series

[PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements

Hubert Jasudowicz posted 1 patch 3 years, 10 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/570e73b83c18cce4dbe4c70cc14b6df7c33f0502.1610720991.git.hubert.jasudowicz@cert.pl
xen/arch/x86/hvm/vmx/vmcs.c        | 8 +++++---
xen/include/asm-x86/hvm/vmx/vmcs.h | 2 ++
2 files changed, 7 insertions(+), 3 deletions(-)
[PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
Posted by Hubert Jasudowicz 3 years, 10 months ago 
This patch is a result of a downstream bug report[1]. Xen fails to
create a HVM domain while running under VMware Fusion 12.1.0 on
a modern Intel Core i9 CPU:

(XEN) VMX: CPU0 has insufficient CPU-Based Exec Control (b5b9fffe; requires 2299968c)
(XEN) VMX: failed to initialise.

It seems that Apple hypervisor API doesn't support this feature[2].

Move this bit from minimal required features to optional.

[1] https://github.com/CERT-Polska/drakvuf-sandbox/issues/418
[2] https://developer.apple.com/documentation/hypervisor/cpu_based_io_bitmaps

Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
---
 xen/arch/x86/hvm/vmx/vmcs.c        | 8 +++++---
 xen/include/asm-x86/hvm/vmx/vmcs.h | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 164535f8f0..bad4d6e206 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -276,10 +276,10 @@ static int vmx_init_vmcs_config(void)
            CPU_BASED_MONITOR_EXITING |
            CPU_BASED_MWAIT_EXITING |
            CPU_BASED_MOV_DR_EXITING |
-           CPU_BASED_ACTIVATE_IO_BITMAP |
            CPU_BASED_USE_TSC_OFFSETING |
            CPU_BASED_RDTSC_EXITING);
     opt = (CPU_BASED_ACTIVATE_MSR_BITMAP |
+           CPU_BASED_ACTIVATE_IO_BITMAP |
            CPU_BASED_TPR_SHADOW |
            CPU_BASED_MONITOR_TRAP_FLAG |
            CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
@@ -1168,8 +1168,10 @@ static int construct_vmcs(struct vcpu *v)
     }
 
     /* I/O access bitmap. */
-    __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
-    __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
+    if ( cpu_has_vmx_io_bitmap ) {
+        __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
+        __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
+    }
 
     if ( cpu_has_vmx_virtual_intr_delivery )
     {
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 906810592f..b00830a5b3 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -342,6 +342,8 @@ extern u64 vmx_ept_vpid_cap;
     (vmx_secondary_exec_control & SECONDARY_EXEC_XSAVES)
 #define cpu_has_vmx_tsc_scaling \
     (vmx_secondary_exec_control & SECONDARY_EXEC_TSC_SCALING)
+#define cpu_has_vmx_io_bitmap \
+    (vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_IO_BITMAP)
 
 #define VMCS_RID_TYPE_MASK              0x80000000
 
-- 
2.30.0
Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
Posted by Roger Pau Monné 3 years, 10 months ago 
On Fri, Jan 15, 2021 at 03:30:50PM +0100, Hubert Jasudowicz wrote:
> This patch is a result of a downstream bug report[1]. Xen fails to
> create a HVM domain while running under VMware Fusion 12.1.0 on
> a modern Intel Core i9 CPU:
> 
> (XEN) VMX: CPU0 has insufficient CPU-Based Exec Control (b5b9fffe; requires 2299968c)
> (XEN) VMX: failed to initialise.
> 
> It seems that Apple hypervisor API doesn't support this feature[2].
> 
> Move this bit from minimal required features to optional.
> 
> [1] https://github.com/CERT-Polska/drakvuf-sandbox/issues/418
> [2] https://developer.apple.com/documentation/hypervisor/cpu_based_io_bitmaps
> 
> Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c        | 8 +++++---
>  xen/include/asm-x86/hvm/vmx/vmcs.h | 2 ++
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 164535f8f0..bad4d6e206 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -276,10 +276,10 @@ static int vmx_init_vmcs_config(void)
>             CPU_BASED_MONITOR_EXITING |
>             CPU_BASED_MWAIT_EXITING |
>             CPU_BASED_MOV_DR_EXITING |
> -           CPU_BASED_ACTIVATE_IO_BITMAP |
>             CPU_BASED_USE_TSC_OFFSETING |
>             CPU_BASED_RDTSC_EXITING);
>      opt = (CPU_BASED_ACTIVATE_MSR_BITMAP |
> +           CPU_BASED_ACTIVATE_IO_BITMAP |
>             CPU_BASED_TPR_SHADOW |
>             CPU_BASED_MONITOR_TRAP_FLAG |
>             CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
> @@ -1168,8 +1168,10 @@ static int construct_vmcs(struct vcpu *v)
>      }
>  
>      /* I/O access bitmap. */
> -    __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
> -    __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
> +    if ( cpu_has_vmx_io_bitmap ) {
> +        __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
> +        __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
> +    }

Maybe I'm missing something, but don't you need to expand
EXIT_REASON_IO_INSTRUCTION in vmx_vmexit_handler when there's no IO
bitmap support so that all the emulation is bypassed and the IO port
access is replayed by Xen?

I think you don't strictly need to modify EXIT_REASON_IO_INSTRUCTION
and can use the existing g2m_ioport_list infrastructure to add the
allowed ports present on the bitmap and prevent them from being
forwarded to the IOREQ server.

Thanks, Roger.
Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
Posted by Jan Beulich 3 years, 10 months ago 
On 15.01.2021 15:44, Roger Pau Monné wrote:
> On Fri, Jan 15, 2021 at 03:30:50PM +0100, Hubert Jasudowicz wrote:
>> This patch is a result of a downstream bug report[1]. Xen fails to
>> create a HVM domain while running under VMware Fusion 12.1.0 on
>> a modern Intel Core i9 CPU:
>>
>> (XEN) VMX: CPU0 has insufficient CPU-Based Exec Control (b5b9fffe; requires 2299968c)
>> (XEN) VMX: failed to initialise.
>>
>> It seems that Apple hypervisor API doesn't support this feature[2].
>>
>> Move this bit from minimal required features to optional.
>>
>> [1] https://github.com/CERT-Polska/drakvuf-sandbox/issues/418
>> [2] https://developer.apple.com/documentation/hypervisor/cpu_based_io_bitmaps
>>
>> Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
>> ---
>>  xen/arch/x86/hvm/vmx/vmcs.c        | 8 +++++---
>>  xen/include/asm-x86/hvm/vmx/vmcs.h | 2 ++
>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 164535f8f0..bad4d6e206 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -276,10 +276,10 @@ static int vmx_init_vmcs_config(void)
>>             CPU_BASED_MONITOR_EXITING |
>>             CPU_BASED_MWAIT_EXITING |
>>             CPU_BASED_MOV_DR_EXITING |
>> -           CPU_BASED_ACTIVATE_IO_BITMAP |
>>             CPU_BASED_USE_TSC_OFFSETING |
>>             CPU_BASED_RDTSC_EXITING);
>>      opt = (CPU_BASED_ACTIVATE_MSR_BITMAP |
>> +           CPU_BASED_ACTIVATE_IO_BITMAP |
>>             CPU_BASED_TPR_SHADOW |
>>             CPU_BASED_MONITOR_TRAP_FLAG |
>>             CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
>> @@ -1168,8 +1168,10 @@ static int construct_vmcs(struct vcpu *v)
>>      }
>>  
>>      /* I/O access bitmap. */
>> -    __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
>> -    __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
>> +    if ( cpu_has_vmx_io_bitmap ) {
>> +        __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
>> +        __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
>> +    }
> 
> Maybe I'm missing something, but don't you need to expand
> EXIT_REASON_IO_INSTRUCTION in vmx_vmexit_handler when there's no IO
> bitmap support so that all the emulation is bypassed and the IO port
> access is replayed by Xen?

I think it's worse than this: I don't see us ever setting
"unconditional I/O exiting", which means guests would be allowed
access to all I/O ports. IOW I think that other bit needs setting
when I/O bitmaps can't be made use of.

Jan

> I think you don't strictly need to modify EXIT_REASON_IO_INSTRUCTION
> and can use the existing g2m_ioport_list infrastructure to add the
> allowed ports present on the bitmap and prevent them from being
> forwarded to the IOREQ server.
> 
> Thanks, Roger.
>
Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
Posted by Hubert Jasudowicz 3 years, 10 months ago 
On 2021-01-19, Jan Beulich wrote:
> On 15.01.2021 15:44, Roger Pau Monné wrote:
> > On Fri, Jan 15, 2021 at 03:30:50PM +0100, Hubert Jasudowicz wrote:
> >> This patch is a result of a downstream bug report[1]. Xen fails to
> >> create a HVM domain while running under VMware Fusion 12.1.0 on
> >> a modern Intel Core i9 CPU:
> >>
> >> (XEN) VMX: CPU0 has insufficient CPU-Based Exec Control (b5b9fffe; requires 2299968c)
> >> (XEN) VMX: failed to initialise.
> >>
> >> It seems that Apple hypervisor API doesn't support this feature[2].
> >>
> >> Move this bit from minimal required features to optional.
> >>
> >> [1] https://github.com/CERT-Polska/drakvuf-sandbox/issues/418
> >> [2] https://developer.apple.com/documentation/hypervisor/cpu_based_io_bitmaps
> >>
> >> Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> >> ---
> >>  xen/arch/x86/hvm/vmx/vmcs.c        | 8 +++++---
> >>  xen/include/asm-x86/hvm/vmx/vmcs.h | 2 ++
> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> >> index 164535f8f0..bad4d6e206 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> >> @@ -276,10 +276,10 @@ static int vmx_init_vmcs_config(void)
> >>             CPU_BASED_MONITOR_EXITING |
> >>             CPU_BASED_MWAIT_EXITING |
> >>             CPU_BASED_MOV_DR_EXITING |
> >> -           CPU_BASED_ACTIVATE_IO_BITMAP |
> >>             CPU_BASED_USE_TSC_OFFSETING |
> >>             CPU_BASED_RDTSC_EXITING);
> >>      opt = (CPU_BASED_ACTIVATE_MSR_BITMAP |
> >> +           CPU_BASED_ACTIVATE_IO_BITMAP |
> >>             CPU_BASED_TPR_SHADOW |
> >>             CPU_BASED_MONITOR_TRAP_FLAG |
> >>             CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
> >> @@ -1168,8 +1168,10 @@ static int construct_vmcs(struct vcpu *v)
> >>      }
> >>  
> >>      /* I/O access bitmap. */
> >> -    __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
> >> -    __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
> >> +    if ( cpu_has_vmx_io_bitmap ) {
> >> +        __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
> >> +        __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
> >> +    }
> > 
> > Maybe I'm missing something, but don't you need to expand
> > EXIT_REASON_IO_INSTRUCTION in vmx_vmexit_handler when there's no IO
> > bitmap support so that all the emulation is bypassed and the IO port
> > access is replayed by Xen?
> 
> I think it's worse than this: I don't see us ever setting
> "unconditional I/O exiting", which means guests would be allowed
> access to all I/O ports. IOW I think that other bit needs setting
> when I/O bitmaps can't be made use of.
> 

Sure, I'll fix it and get back to you with a v2.

Hubert Jasudowicz
Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
Posted by Andrew Cooper 3 years, 10 months ago 
On 15/01/2021 14:30, Hubert Jasudowicz wrote:
> This patch is a result of a downstream bug report[1]. Xen fails to
> create a HVM domain while running under VMware Fusion 12.1.0 on
> a modern Intel Core i9 CPU:
>
> (XEN) VMX: CPU0 has insufficient CPU-Based Exec Control (b5b9fffe; requires 2299968c)
> (XEN) VMX: failed to initialise.
>
> It seems that Apple hypervisor API doesn't support this feature[2].
>
> Move this bit from minimal required features to optional.
>
> [1] https://github.com/CERT-Polska/drakvuf-sandbox/issues/418
> [2] https://developer.apple.com/documentation/hypervisor/cpu_based_io_bitmaps
>
> Signed-off-by: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>

For others reviewing, this was my suggestion to fix it.

The IO port bitmap is only used as a performance optimisation for legacy
BIOS code using port 0x80/0xed for IO delays, which isn't a good enough
reason for the feature to be mandatory.

Nested virt like this is primarily used for ease of development.  The
VMExit IO path should DTRT, even for a PVH dom0.

> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 164535f8f0..bad4d6e206 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1168,8 +1168,10 @@ static int construct_vmcs(struct vcpu *v)
>      }
>  
>      /* I/O access bitmap. */
> -    __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
> -    __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
> +    if ( cpu_has_vmx_io_bitmap ) {

Brace on newline.  Can be fixed on commit - no need to resend just for this.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

> +        __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm.io_bitmap));
> +        __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm.io_bitmap) + PAGE_SIZE);
> +    }
>  
>      if ( cpu_has_vmx_virtual_intr_delivery )
>      {
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.