Received-SPF: pass (google.com: domain of linux-kernel+bounces-4698-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 19 Dec 2023 12:26:45 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: 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: <ZYEbhadnn6+clzX9@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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZXzx1zXfZ6GV9TgI@google.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?p3ddz74dNOIIQqMsmF6VCBARE/L5/h34f4PqQDrTZPFxQCUcHLS1aHyCNPhU?=
 =?us-ascii?Q?8S695JLQ8nxhB9IJNZa7Lmo3glIgNtOjSN6rLwwp9M3cJJmHzthtZrIY/SF5?=
 =?us-ascii?Q?T0xXvoJaq7hvG8trLV00G5O5kZYIF+FSWlpDY41a7ITC9YDihDJnwPJ5zGi/?=
 =?us-ascii?Q?bW9HK0F+xm+in+c3fQH8fummXkKKE8ZKmTQyF7a/F2gqS6h8FUktm8ZI0t4W?=
 =?us-ascii?Q?wSwj5KTT/HEX7ZyEkZOtrvF/pSlPuMdbL+W5agjV8vyjV8DY5GIcGC363qlR?=
 =?us-ascii?Q?m/I5BNctt870qzKcZ8pdOimj1xYNqcsBXnzH6s1cbjwo5vwG91WymPYdOeQX?=
 =?us-ascii?Q?le32PzooL9d/6XYBQUZ1m9v5ODUfEZKGuQRj9ho4A3TY8/S/W30vSWBDJQ3C?=
 =?us-ascii?Q?lXWJuv75uUUZfJi0wor5aAOGs7MkRi2ztcZEMyCr+ZzOgLVENmQimeP93JKw?=
 =?us-ascii?Q?daxHaw3XyEoIF4NFduSAZL3LY2obZKWikSO8RqhyZK/IU2acWGfgapJp6A9L?=
 =?us-ascii?Q?vF8hOjE1F2GQIEsfzW5faBtEIwHoCbYiSvt6AOuTbUCJ+TkhVboKUw+hye4r?=
 =?us-ascii?Q?IWDghHJHWtwzX2NNyXstTF7DZRRCZpnBLvBVIWw9UV55kYq0L3BFvZDrddyU?=
 =?us-ascii?Q?MD1hEQcWraZ7t/XT1BSVkD9ygd0+/hpzb28iPFIOqDa1SOrBRk9D0AZYR496?=
 =?us-ascii?Q?hYnFhB/C1InW093HG2tk/3mewS8U6Y1feoqq+kTLU+6LoOGrbNo6YIBtiQOg?=
 =?us-ascii?Q?HfS6RAlPgoh3+WYHTsNRt7tKAeNa1pYEb9o6NFT3FpvINnuOAyUIeKGz4pi7?=
 =?us-ascii?Q?ScZfHg87Y0Ldo1+yzy0C6m4eMViK/6JbjbVu/9nYNOyAA/szch1YJcQMjhHa?=
 =?us-ascii?Q?f2UV6sL8Oqijg8sLJhLpX13jtRZ2WrL9q/Svx/NHNyyxCRjWqpC1ek/ZLwlO?=
 =?us-ascii?Q?DdvPd+fNxpkhkEB/6uWehK4i876QexGg4QR/zEciU7+c8v4cJBGaqbjvbQh6?=
 =?us-ascii?Q?4+tzM+7GGq8d7/aoInVMi/CSsSVCLq9sIdQzy83vrAXYTZQUMVS1NtLag0GT?=
 =?us-ascii?Q?w7Qjp6iV6/AEcsSBq3t6ICHmvH7CQrAkry+SOVT1TwZgQspA9F+hW3kIMh0t?=
 =?us-ascii?Q?o3LSYW4BLU3ZgPtmqDGcA+wAJA3QIbweH8lmJ7HpJ+4vYQ/lOAhjvWFgPeEU?=
 =?us-ascii?Q?rp+yZtQfO8jTgnPzWdzUF8yFlEiYfChRMZr7599c10sqW/HgvaziGlnTIzE/?=
 =?us-ascii?Q?yXYjRkn3vL7lDDesP+tNSbR3KpPp1kJXyPm4+SfC7N3hrl6ZcRPyNXTBVham?=
 =?us-ascii?Q?RuqYiHUk6vcZ2vL+h2u14JICeJG52WkL9sE27SGPbuscQ5Iui3SPcCFdgNoO?=
 =?us-ascii?Q?iH9PMv/b+oxg8P7cCMHvgFW2WH37W6EfOlCZn67IdU+3h/FiZKHshh3+INVI?=
 =?us-ascii?Q?BPaIqkL8QxuACiuScrq351q+tvNGnkjl+EXM1wQxkXM70hwTHwdcmVmSbD4m?=
 =?us-ascii?Q?SCUWq53v6hb9VP1oVwpEJNAN/HQTWcLIEMLogGfxD6TWKSRYYy0as+F8GInR?=
 =?us-ascii?Q?rCjUYctN+6PFeBkxcXNV7UTy7tkHwtoj4wrPwmvo?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b06b8127-9199-4107-628a-08dc004ec9af
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Dec 2023 04:55:57.2225
 (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: mzq1o5SSFUKJSkUSRTZKRFv3v9CPjh2ZULZpqoxzcdwoGVYFoapQ8UbvHMkUYyN8wMhHKl5qcXlQZrHd3Kke4g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4742
X-OriginatorOrg: intel.com

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.