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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.
>