Received-SPF: pass (google.com: domain of linux-kernel+bounces-6298-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 20 Dec 2023 10:15:40 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Sean Christopherson <seanjc@google.com>, Kevin Tian
	<kevin.tian@intel.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>, "olvaffe@gmail.com"
	<olvaffe@gmail.com>, Zhiyuan Lv <zhiyuan.lv@intel.com>, Zhenyu Z Wang
	<zhenyu.z.wang@intel.com>, Yongwei Ma <yongwei.ma@intel.com>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>, "wanpengli@tencent.com"
	<wanpengli@tencent.com>, "jmattson@google.com" <jmattson@google.com>,
	"joro@8bytes.org" <joro@8bytes.org>, "gurchetansingh@chromium.org"
	<gurchetansingh@chromium.org>, "kraxel@redhat.com" <kraxel@redhat.com>,
	"Yiwei Zhang" <zzyiwei@google.com>
Subject: Re: [RFC PATCH] KVM: Introduce KVM VIRTIO device
Message-ID: <ZYJOTLwreD+8l4gU@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20231214103520.7198-1-yan.y.zhao@intel.com>
 <BN9PR11MB5276BE04CBB6D07039086D658C93A@BN9PR11MB5276.namprd11.prod.outlook.com>
 <ZXzx1zXfZ6GV9TgI@google.com>
 <ZYEbhadnn6+clzX9@yzhao56-desk.sh.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZYEbhadnn6+clzX9@yzhao56-desk.sh.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?9ob6Uy95xtxFD/NsbpaLB3q74ZlQrEtvdI2wIihFieef03+L6BiPx1clxym1?=
 =?us-ascii?Q?i9+IDH+ezHc/x7DtvlU4HIaiYwr+UcviSo/DWBHYW9wnwFO0IODEYxgtQLtR?=
 =?us-ascii?Q?7XumH8gEkOV2xgauuH4ojtrjZtWBg04kpnuJRFCavStzc/4OJ4lhupvZYV37?=
 =?us-ascii?Q?XMdZrR/luT4GwrK48CBMSUhB8Qagt48M27OwKrhM5MXdLJn4RnM10SGroGtq?=
 =?us-ascii?Q?piQZy7S+xBKYDbmAoAQroAH3DGehy41d53Udq6sc2M7s2Qe7H9fd0zE1FT57?=
 =?us-ascii?Q?knO9WwI74u1rKvAvuFE4RB3G+EEPl1alXSy49mOb9ZozUO52GbYioKma5dcO?=
 =?us-ascii?Q?bqOvuOwEIBdFtQVx0F8fNgF2Pf0WBoxbFtyEFgYAKoqbOqYoLa/jx+EZdYd9?=
 =?us-ascii?Q?uMheFsLTFsR1gIZ0C3prPVrGhUd/aX3DzTy30Ous29qis3jYC7WqRiTuNuM8?=
 =?us-ascii?Q?KaMxCnpYylWavEEsAfOq16YXqSRdBGd+wQoeIous8qe8Ke8dHsrLiU42lqVu?=
 =?us-ascii?Q?/Ny3hQqDWwHIkf292d08ZhDEtpcS05nUjCeDfCUmU/nrtLCChm3fRiXUO0R4?=
 =?us-ascii?Q?oghrOwbl4IEta6s5ZjQLxi7ZD7cmaPyE55xU88Yl4wQ34c7jr6+fo6d4Fcs/?=
 =?us-ascii?Q?OBWCTlWcOXYml7SmoLksqSVwGYyIh6RdZ7q48dHS+sCvE2gzEYwnGs56W5vJ?=
 =?us-ascii?Q?Gj8f6kfRitPLakc3n/eTOGt4fArAAfXgnw7bFZmEa8KcqNC2Mtb5E+rmfe5g?=
 =?us-ascii?Q?nssB015qyiDHKMct6S7VXKoAv36KpSYPfsTKYsVVfKI6THk1kD8U9eeHHNVU?=
 =?us-ascii?Q?uhu7X5O+yvi4Q42qqHjalNU243ruUgpt25PjP/685SwZF8cZJx4rQVQ08SYM?=
 =?us-ascii?Q?AlVNkWdhrY4ADNXyZwcH/8D33/U44FgLZilPRNyvKiS17Cp7qrcL6XpSDdhI?=
 =?us-ascii?Q?025Ak6rDh45gmwAUlHdRXL7MeOZ3hDLhDzP1eJI7ckKrChU/tB5Jxhd7x9Ax?=
 =?us-ascii?Q?/A0NLj90dZQ5rM15FvbRxaqHl5RcYOXxdV68YNCsuP9fYjhTO+6wNKJNxC8p?=
 =?us-ascii?Q?ETB4u/Qx3Uvzm77F9vlcFane/2mvdCSknc/8lJOJm47GJAYYMQW3eJefv7Ol?=
 =?us-ascii?Q?a03IL+xqxPtc+xGWUjB08F2x8ZodH6hrvI7vbMgN2kF7tjs0gEPUjt1pIBdm?=
 =?us-ascii?Q?ek1nPlim0xuDyl68/SqkG8ODuDVaSg4c7w16n5VrgUv9Tpz8OR9WCku4tiIZ?=
 =?us-ascii?Q?iLpHvBK2MXp6JWLHFbKyUPDyU0EyvDh80IkdMIDvsFKK5EufA6hfRO7If/8U?=
 =?us-ascii?Q?ej7jcFfGTntG8cmg/mm0LMuSN2TdPHskjqiTiLlu0TBKNSHhhPaQ7PHiVD5w?=
 =?us-ascii?Q?jOWN0Fyv8xosYFv6U0Q0pzrlGplmglAA6EfZQfwQtW3yZd9fDt8F9Ne9R8zi?=
 =?us-ascii?Q?Ls+WOekQltV0RaIz99qyNhfoE5T5lQA6iuAVZxQ0Zfl1F84UH2qmDNOcmbZo?=
 =?us-ascii?Q?/nq9F7QRBiQ5h30z6/l0D5ZhtpYqIe5k2KoSVQ7PXIMcaTzlskIkdN+ABDHW?=
 =?us-ascii?Q?4eYeXSLh3xHZ4LiBPP9A7CbXFSKuhP6Gh1NEHkjt?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 177ed5d8-4e3d-4ca4-05bb-08dc0105a452
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2023 02:44:52.4767
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: FLJBZ6Sg7ohQg8JDBw8IvO/4F3dObpMORd+WjjXjr8Ec0lFpL/olhmZcXm6Q2CQ1SP5lfWlzqCaXcJvt0cWe7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4949
X-OriginatorOrg: intel.com

On Tue, Dec 19, 2023 at 12:26:45PM +0800, Yan Zhao wrote:
> On Mon, Dec 18, 2023 at 07:08:51AM -0800, Sean Christopherson wrote:
> > > > Implementation Consideration
> > > > ===
> > > > There is a previous series [1] from google to serve the same purpose to
> > > > let KVM be aware of virtio GPU's noncoherent DMA status. That series
> > > > requires a new memslot flag, and special memslots in user space.
> > > > 
> > > > We don't choose to use memslot flag to request honoring guest memory
> > > > type.
> > > 
> > > memslot flag has the potential to restrict the impact e.g. when using
> > > clflush-before-read in migration?
> > 
> > Yep, exactly.  E.g. if KVM needs to ensure coherency when freeing memory back to
> > the host kernel, then the memslot flag will allow for a much more targeted
> > operation.
> > 
> > > Of course the implication is to honor guest type only for the selected slot
> > > in KVM instead of applying to the entire guest memory as in previous series
> > > (which selects this way because vmx_get_mt_mask() is in perf-critical path
> > > hence not good to check memslot flag?)
> > 
> > Checking a memslot flag won't impact performance.  KVM already has the memslot
> > when creating SPTEs, e.g. the sole caller of vmx_get_mt_mask(), make_spte(), has
> > access to the memslot.
> > 
> > That isn't coincidental, KVM _must_ have the memslot to construct the SPTE, e.g.
> > to retrieve the associated PFN, update write-tracking for shadow pages, etc.
> > 
> Hi Sean,
> Do you prefer to introduce a memslot flag KVM_MEM_DMA or KVM_MEM_WC?
> For KVM_MEM_DMA, KVM needs to
> (a) search VMA for vma->vm_page_prot and convert it to page cache mode (with
>     pgprot2cachemode()? ), or
> (b) look up memtype of the PFN, by calling lookup_memtype(), similar to that in
>     pat_pfn_immune_to_uc_mtrr().
> 
> But pgprot2cachemode() and lookup_memtype() are not exported by x86 code now.
> 
> For KVM_MEM_WC, it requires user to ensure the memory is actually mapped
> to WC, right?
> 
> Then, vmx_get_mt_mask() just ignores guest PAT and programs host PAT as EPT type
> for the special memslot only, as below.
> Is this understanding correct?
> 
> static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> {
>         if (is_mmio)                                                                           
>                 return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;                          
>                                                                                                
>         if (gfn_in_dma_slot(vcpu->kvm, gfn)) {                                                 
>                 u8 type = MTRR_TYPE_WRCOMB;                                      
>                 //u8 type = pat_pfn_memtype(pfn);                                
>                 return (type << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;       
>         }                                                                                      
>                                                                                                
>         if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))                            
>                 return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;         
>                                                                                                
>         if (kvm_read_cr0_bits(vcpu, X86_CR0_CD)) {                                             
>                 if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_CD_NW_CLEARED))               
>                         return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;                      
>                 else                                                                           
>                         return (MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT) | 
>                                 VMX_EPT_IPAT_BIT;                                
>         }                                                                        
>                                                                                  
>         return kvm_mtrr_get_guest_memory_type(vcpu, gfn) << VMX_EPT_MT_EPTE_SHIFT;
> }
> 
> BTW, since the special memslot must be exposed to guest as virtio GPU BAR in
> order to prevent other guest drivers from access, I wonder if it's better to
> include some keyword like VIRTIO_GPU_BAR in memslot flag name.
Another choice is to add a memslot flag KVM_MEM_HONOR_GUEST_PAT, then user
(e.g. QEMU) does special treatment to this kind of memslots (e.g. skipping
reading/writing to them in general paths).

@@ -7589,26 +7589,29 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
        if (is_mmio)
                return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;

+       if (in_slot_honor_guest_pat(vcpu->kvm, gfn))
+               return kvm_mtrr_get_guest_memory_type(vcpu, gfn) << VMX_EPT_MT_EPTE_SHIFT;
+
        if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
                return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;

        if (kvm_read_cr0_bits(vcpu, X86_CR0_CD)) {
                if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_CD_NW_CLEARED))
                        return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;
                else
                        return (MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT) |
                                VMX_EPT_IPAT_BIT;
        }

        return kvm_mtrr_get_guest_memory_type(vcpu, gfn) << VMX_EPT_MT_EPTE_SHIFT;
 }