Re: [PATCH 2/2] KVM: VMX: Use just one page for I/O permission bitmaps

[Quan Xu <quan.xu0@gmail.com>]

On 2017/12/08 01:06, Radim Krčmář wrote:
> 2017-12-07 10:33+0800, Quan Xu:
>>
>> On 2017/12/07 02:19, Jim Mattson wrote:
>>> On Wed, Dec 6, 2017 at 3:17 AM, Quan Xu <quan.xu0@gmail.com> wrote:
>>>> On 2017/12/06 05:26, Radim Krčmář wrote:
>>>>> 2017-12-01 10:21-0800, Jim Mattson:
>>>>>> Since we no longer allow any I/O ports to be passed through to the guest,
>>>>>> we can use the same page for I/O bitmap A and I/O bitmap B.
>>>>> I think we can disable the feature and save the second page as well:
>>>>>
>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>> index e25c55ea2eb7..80859a7cdf6d 100644
>>>>> --- a/arch/x86/kvm/vmx.c
>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>> @@ -3624,7 +3624,7 @@ static __init int setup_vmcs_config(struct
>>>>> vmcs_config *vmcs_conf)
>>>>>     #endif
>>>>>                 CPU_BASED_CR3_LOAD_EXITING |
>>>>>                 CPU_BASED_CR3_STORE_EXITING |
>>>>> -             CPU_BASED_USE_IO_BITMAPS |
>>>>> +             CPU_BASED_UNCOND_IO_EXITING |
>>>>>                 CPU_BASED_MOV_DR_EXITING |
>>>>>                 CPU_BASED_USE_TSC_OFFSETING |
>>>>>                 CPU_BASED_INVLPG_EXITING |
>>>>>
>>>> Jim / Radim,
>>>>
>>>>
>>>> As a logical processor uses these bitmaps if and only if the “use I/O
>>>> bitmaps” control is 1.
>>>>
>>>> Since we drop 'CPU_BASED_USE_IOBITMAPS' in vmcs configuration.. We could
>>>> also drop
>>>> 'IO_BITMAP_A'/'IO_BITMAP_B'/'vmx_io_bitmap_a'/'vmx_io_bitmap_b' for a
>>>> furthermore
>>>> cleanup as below:
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 04b4dbc..3e4f760 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -771,8 +771,6 @@ static struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu
>>>> *vcpu)
>>>>           FIELD(HOST_FS_SELECTOR, host_fs_selector),
>>>>           FIELD(HOST_GS_SELECTOR, host_gs_selector),
>>>>           FIELD(HOST_TR_SELECTOR, host_tr_selector),
>>>> -       FIELD64(IO_BITMAP_A, io_bitmap_a),
>>>> -       FIELD64(IO_BITMAP_B, io_bitmap_b),
>>> Stet.
>>>
>>>>           FIELD64(MSR_BITMAP, msr_bitmap),
>>>>           FIELD64(VM_EXIT_MSR_STORE_ADDR, vm_exit_msr_store_addr),
>>>>           FIELD64(VM_EXIT_MSR_LOAD_ADDR, vm_exit_msr_load_addr),
>>>> @@ -943,8 +941,6 @@ static bool nested_vmx_is_page_fault_vmexit(struct
>>>> vmcs12 *vmcs12,
>>>>    static DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock);
>>>>
>>>>    enum {
>>>> -       VMX_IO_BITMAP_A,
>>>> -       VMX_IO_BITMAP_B,
>>>>           VMX_MSR_BITMAP_LEGACY,
>>>>           VMX_MSR_BITMAP_LONGMODE,
>>>>           VMX_MSR_BITMAP_LEGACY_X2APIC_APICV,
>>>> @@ -958,8 +954,6 @@ enum {
>>>>
>>>>    static unsigned long *vmx_bitmap[VMX_BITMAP_NR];
>>>>
>>>> -#define vmx_io_bitmap_a (vmx_bitmap[VMX_IO_BITMAP_A])
>>>> -#define vmx_io_bitmap_b (vmx_bitmap[VMX_IO_BITMAP_B])
>>>>    #define vmx_msr_bitmap_legacy (vmx_bitmap[VMX_MSR_BITMAP_LEGACY])
>>>>    #define vmx_msr_bitmap_longmode (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE])
>>>>    #define vmx_msr_bitmap_legacy_x2apic_apicv
>>>> (vmx_bitmap[VMX_MSR_BITMAP_LEGACY_X2APIC_APICV])
>>>> @@ -3632,7 +3626,7 @@ static __init int setup_vmcs_config(struct vmcs_config
>>>> *vmcs_conf)
>>>>    #endif
>>>>                 CPU_BASED_CR3_LOAD_EXITING |
>>>>                 CPU_BASED_CR3_STORE_EXITING |
>>>> -             CPU_BASED_USE_IO_BITMAPS |
>>>> +             CPU_BASED_UNCOND_IO_EXITING |
>>> Is it safe to assume that all CPUs (both physical and virtual)
>>> currently running kvm will support this control bit?
>> Jim, I don't see some conditions to "Use I/O bitmaps" or "Unconditional I/O
>> exiting" in Intel SDM.
>> and in prepare_vmcs02(),
>>
>> ...
>>          /*
>>           * Merging of IO bitmap not currently supported.
>>           * Rather, exit every time.
>>           */
>>          exec_control &= ~CPU_BASED_USE_IO_BITMAPS;
>>          exec_control |= CPU_BASED_UNCOND_IO_EXITING;
>> ...
>>
>>
>> So I think it is safe for cpu and vcpu.
> I agree that it is safe.
>
>> as sdm said, """ "Unconditional I/O exiting" -- This control determines
>> whether executions of I/O
>> instructions (IN, INS/INSB/INSW/INSD, OUT, and OUTS/OUTSB/OUTSW/OUTSD) cause
>> VM exits.
>> "Use I/O bitmaps" -- For this control, “0” means “do not use I/O bitmaps”
>> and “1” means
>> “use I/O bitmaps.” """"
>>
>> I think it is safe for all CPU, if we clear the "Use I/O bitmaps". It
>> doesn't matter whether
>> the "Unconditional I/O exiting" is set or clear.. Of Course both bits are
>> clear, which may
>> make host/guest to the incorrect field.
       [...]
>>
>> is it?  Radim, could you help me double check it?
> I don't think it is correct to have neither,

   yes, I also mentioned that  this may make host/guest to the 
_incorrect_ field. :)
   but the fix is to set CPU_BASED_UNCOND_IO_EXITING and clear 
CPU_BASED_USE_IO_BITMAPS..

-             CPU_BASED_USE_IO_BITMAPS |
+             CPU_BASED_UNCOND_IO_EXITING |


also drop io_bitmap_a/io_bitmap_b that are not used at all(as 
CPU_BASED_USE_IO_BITMAPS is clear).

>   though, SMD describes them
> as an alternative:
>
> 33.3.2.1 PIC Virtualization
>    If the VMM is not supporting direct access to any I/O ports from a
>    guest, it can set the unconditional-I/O-exiting in the VM-execution
>    control field instead of activating I/O bitmaps.
>
> And a quick test shows that we get a VM entry failure if both of them
> are clear, although I can't find that explicitly in SDM.
I think.. if both of them are clear, guests/host share host's I/O 
resource, as mentioned, which may make host/guest to the _incorrect_ field..
CPU may have some check to prevent this happening.

Quan
Alibaba Cloud