| Message ID | 570e73b83c18cce4dbe4c70cc14b6df7c33f0502.1610720991.git.hubert.jasudowicz@cert.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vmx: Remove IO bitmap from minimal VMX requirements | expand |
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.
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 ) > { >
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. >
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
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
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(-)