Re: [PATCH] KVM: Enable snooping control for supported hardware

[Marcelo Tosatti <mtosatti@redhat.com>]

Sheng,

On Thu, Apr 23, 2009 at 04:43:02PM +0800, Sheng Yang wrote:
> Memory aliases with different memory type is a problem for guest. For the guest
> without assigned device, the memory type of guest memory would always been the
> same as host(WB); but for the assigned device, some part of memory may be used
> as DMA and then set to uncacheable memory type(UC/WC), which would be a conflict of
> host memory type then be a potential issue.

So the issue arises when the host attempts to access the DMA memory
(which is typed as uncached by the guest) with its WB typing (MTRR).
That would mean that host accesses of such memory are potentially stale?

Because "24.3.2 Creating and Using Cached Translation Information"
mentions that with EPT enabled the guest will only use translations that
are EPTP-tagged, therefore with the correct (UC) typing, which you set
in the EPT pagetable?

More comments below.

> Snooping control can guarantee the cache correctness of accessing the DMA engine
> of VT-d.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h |    2 ++
>  arch/x86/kvm/mmu.c              |   23 ++++++++++++++++++++---
>  virt/kvm/iommu.c                |   27 ++++++++++++++++++++++++---
>  3 files changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3fc4623..d2da40f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -397,6 +397,8 @@ struct kvm_arch{
>  	struct list_head assigned_dev_head;
>  	struct list_head oos_global_pages;
>  	struct iommu_domain *iommu_domain;
> +#define KVM_IOMMU_CACHE_COHERENCY	0x1
> +	int iommu_flags;
>  	struct kvm_pic *vpic;
>  	struct kvm_ioapic *vioapic;
>  	struct kvm_pit *vpit;
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 409d08e..1e63a87 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1713,10 +1713,27 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *shadow_pte,
>  	if (largepage)
>  		spte |= PT_PAGE_SIZE_MASK;
>  	if (mt_mask) {
> +		/* For VT-d and EPT combination
> +		 * 1. MMIO: always map as UC
> +		 * 2. EPT without VT-d: always map as WB and set IGMT=1 to
> +		 *    keep consistent with host MTRR
> +		 * 3. EPT with VT-d:
> +		 *   a. VT-d with snooping control feature: keep consistent
> +		 *	with host MTRR can guarantee the correctness
> +		 *   b. VT-d without snooping control feature: can't
> +		 *      guarantee the result, try to trust guest.
> +		 */
>  		if (!kvm_is_mmio_pfn(pfn)) {
> -			mt_mask = get_memory_type(vcpu, gfn) <<
> -				kvm_x86_ops->get_mt_mask_shift();
> -			mt_mask |= VMX_EPT_IGMT_BIT;
> +			if (vcpu->kvm->arch.iommu_domain &&
> +			    !(vcpu->kvm->arch.iommu_flags &
> +				    KVM_IOMMU_CACHE_COHERENCY)) {
> +				mt_mask = get_memory_type(vcpu, gfn) <<
> +					  kvm_x86_ops->get_mt_mask_shift();
> +			} else {
> +				mt_mask = MTRR_TYPE_WRBACK <<
> +					kvm_x86_ops->get_mt_mask_shift();
> +				mt_mask |= VMX_EPT_IGMT_BIT;
> +			}
>  		} else
>  			mt_mask = MTRR_TYPE_UNCACHABLE <<
>  				kvm_x86_ops->get_mt_mask_shift();

Can you move this to subarch code? 

Perhaps replace get_mt_mask_shift with 

    u64 (*get_spte_mt_mask)(bool is_iommu_page);

And then just do

    if (mt_mask)
        spte |= kvm_x86_ops->get_spte_mt_mask();


> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
> index 4c40375..1514758 100644
> --- a/virt/kvm/iommu.c
> +++ b/virt/kvm/iommu.c
> @@ -39,11 +39,16 @@ int kvm_iommu_map_pages(struct kvm *kvm,
>  	pfn_t pfn;
>  	int i, r = 0;
>  	struct iommu_domain *domain = kvm->arch.iommu_domain;
> +	int flags;
>  
>  	/* check if iommu exists and in use */
>  	if (!domain)
>  		return 0;
>  
> +	flags = IOMMU_READ | IOMMU_WRITE;
> +	if (kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY)
> +		flags |= IOMMU_CACHE;
> +
>  	for (i = 0; i < npages; i++) {
>  		/* check if already mapped */
>  		if (iommu_iova_to_phys(domain, gfn_to_gpa(gfn)))
> @@ -53,8 +58,7 @@ int kvm_iommu_map_pages(struct kvm *kvm,
>  		r = iommu_map_range(domain,
>  				    gfn_to_gpa(gfn),
>  				    pfn_to_hpa(pfn),
> -				    PAGE_SIZE,
> -				    IOMMU_READ | IOMMU_WRITE);
> +				    PAGE_SIZE, flags);
>  		if (r) {
>  			printk(KERN_ERR "kvm_iommu_map_address:"
>  			       "iommu failed to map pfn=%lx\n", pfn);
> @@ -88,7 +92,7 @@ int kvm_assign_device(struct kvm *kvm,
>  {
>  	struct pci_dev *pdev = NULL;
>  	struct iommu_domain *domain = kvm->arch.iommu_domain;
> -	int r;
> +	int r, last_flags;
>  
>  	/* check if iommu exists and in use */
>  	if (!domain)
> @@ -107,12 +111,29 @@ int kvm_assign_device(struct kvm *kvm,
>  		return r;
>  	}
>  
> +	last_flags = kvm->arch.iommu_flags;
> +	if (iommu_domain_has_cap(kvm->arch.iommu_domain,
> +				 IOMMU_CAP_CACHE_COHERENCY))
> +		kvm->arch.iommu_flags |= KVM_IOMMU_CACHE_COHERENCY;
> +
> +	/* Check if need to update IOMMU page table for guest memory */
> +	if ((last_flags ^ kvm->arch.iommu_flags) ==
> +			KVM_IOMMU_CACHE_COHERENCY) {
> +		kvm_iommu_unmap_memslots(kvm);
> +		r = kvm_iommu_map_memslots(kvm);
> +		if (r)
> +			goto out_unmap;
> +	}

You really need to check for a change? How can the IOMMU cache coherency
capability change while a guest is operational?

> +
>  	printk(KERN_DEBUG "assign device: host bdf = %x:%x:%x\n",
>  		assigned_dev->host_busnr,
>  		PCI_SLOT(assigned_dev->host_devfn),
>  		PCI_FUNC(assigned_dev->host_devfn));
>  
>  	return 0;
> +out_unmap:
> +	kvm_iommu_unmap_memslots(kvm);
> +	return r;
>  }
>  
>  int kvm_deassign_device(struct kvm *kvm,
> -- 
> 1.5.4.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html