* [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
@ 2021-01-15 14:30 Hubert Jasudowicz
2021-01-15 14:44 ` Roger Pau Monné
2021-01-15 14:44 ` Andrew Cooper
0 siblings, 2 replies; 5+ messages in thread
From: Hubert Jasudowicz @ 2021-01-15 14:30 UTC (permalink / raw)
To: xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, Michał Leszczyński
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
2021-01-15 14:30 [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements Hubert Jasudowicz
@ 2021-01-15 14:44 ` Roger Pau Monné
2021-01-19 16:57 ` Jan Beulich
2021-01-15 14:44 ` Andrew Cooper
1 sibling, 1 reply; 5+ messages in thread
From: Roger Pau Monné @ 2021-01-15 14:44 UTC (permalink / raw)
To: Hubert Jasudowicz
Cc: xen-devel, Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Wei Liu, Michał Leszczyński
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
2021-01-15 14:30 [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements Hubert Jasudowicz
2021-01-15 14:44 ` Roger Pau Monné
@ 2021-01-15 14:44 ` Andrew Cooper
1 sibling, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2021-01-15 14:44 UTC (permalink / raw)
To: Hubert Jasudowicz, xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Roger Pau Monné,
Wei Liu, Michał Leszczyński
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 )
> {
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
2021-01-15 14:44 ` Roger Pau Monné
@ 2021-01-19 16:57 ` Jan Beulich
2021-01-20 10:37 ` Hubert Jasudowicz
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2021-01-19 16:57 UTC (permalink / raw)
To: Roger Pau Monné, Hubert Jasudowicz
Cc: xen-devel, Jun Nakajima, Kevin Tian, Andrew Cooper, Wei Liu,
Michał Leszczyński
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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements
2021-01-19 16:57 ` Jan Beulich
@ 2021-01-20 10:37 ` Hubert Jasudowicz
0 siblings, 0 replies; 5+ messages in thread
From: Hubert Jasudowicz @ 2021-01-20 10:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Roger Pau Monné,
xen-devel, Jun Nakajima, Kevin Tian, Andrew Cooper, Wei Liu,
Michał Leszczyński
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-20 10:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15 14:30 [PATCH] x86/vmx: Remove IO bitmap from minimal VMX requirements Hubert Jasudowicz
2021-01-15 14:44 ` Roger Pau Monné
2021-01-19 16:57 ` Jan Beulich
2021-01-20 10:37 ` Hubert Jasudowicz
2021-01-15 14:44 ` Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).