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

[Hubert Jasudowicz <hubert.jasudowicz@xxxxxxx>]

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@xxxxxxx>
> >> ---
> >>  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