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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2017/12/07 02:19, Jim Mattson wrote:
On Wed, Dec 6, 2017 at 3:17 AM, Quan Xu <quan.xu0@xxxxxxxxx> 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.

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?


Quan
Alibaba Cloud

               CPU_BASED_MOV_DR_EXITING |
               CPU_BASED_USE_TSC_OFFSETING |
               CPU_BASED_INVLPG_EXITING |
@@ -5445,10 +5439,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
  #endif
         int i;

-       /* I/O */
-       vmcs_write64(IO_BITMAP_A, __pa(vmx_io_bitmap_a));
-       vmcs_write64(IO_BITMAP_B, __pa(vmx_io_bitmap_b));
-
         if (enable_shadow_vmcs) {
                 vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap));
                 vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap));
@@ -6751,13 +6741,9 @@ static __init int hardware_setup(void)
                         goto out;
         }

-       vmx_io_bitmap_b = (unsigned long *)__get_free_page(GFP_KERNEL);
         memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE);
         memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE);

-       memset(vmx_io_bitmap_a, 0xff, PAGE_SIZE);
-       memset(vmx_io_bitmap_b, 0xff, PAGE_SIZE);
-
         memset(vmx_msr_bitmap_legacy, 0xff, PAGE_SIZE);
         memset(vmx_msr_bitmap_longmode, 0xff, PAGE_SIZE);





Quan
Alibaba Cloud




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux