From mboxrd@z Thu Jan 1 00:00:00 1970 From: Quan Xu Subject: Re: [PATCH 2/2] KVM: VMX: Use just one page for I/O permission bitmaps Date: Fri, 8 Dec 2017 10:04:31 +0800 Message-ID: References: <20171201182110.7143-1-jmattson@google.com> <20171201182110.7143-2-jmattson@google.com> <20171205212639.GD20099@flask> <2b8eff6f-4b35-ce3a-c716-eb8fb7461eb3@gmail.com> <07d648fb-fa42-9e74-66cc-d371f604bf59@gmail.com> <20171207170629.GD29615@flask> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Jim Mattson , kvm list , P J P To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: Received: from mail-ot0-f193.google.com ([74.125.82.193]:36527 "EHLO mail-ot0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751020AbdLHCEj (ORCPT ); Thu, 7 Dec 2017 21:04:39 -0500 Received: by mail-ot0-f193.google.com with SMTP id d5so8081146oti.3 for ; Thu, 07 Dec 2017 18:04:39 -0800 (PST) In-Reply-To: <20171207170629.GD29615@flask> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: 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 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