KVM: Enable snooping control for supported hardware
PATCH 1/1 KVM: Enable snooping control for supported hardware
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.
Snooping control can guarantee the cache correctness of accessing the DMA engine of VT-d.
--- 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(-)
arch/x86/include/asm/kvm_host.h +2 -0
@@ -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;
arch/x86/kvm/mmu.c +20 -3
@@ -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 ();
virt/kvm/iommu.c +24 -3
@@ -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; + } +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,
Signature · 1 line
1.5.4.5
Sheng,
Quoted 5 lines · Sheng Yang
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.
Quoted 48 lines
Snooping control can guarantee the cache correctness of accessing the DMA engine of VT-d.
Signed-off-by Sheng Yang <[email protected]>--- 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(-)arch/x86/include/asm/kvm_host.h +2
-0@@ -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;arch/x86/kvm/mmu.c +13
-3@@ -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 )) {
arch/x86/kvm/mmu.c static int set_spte(struct kvm_vcpu *vcpu, u64 *shadow_pte, · line 1739
Quoted 10 lines
arch/x86/kvm/mmu.c +7
-0+ 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();Quoted 47 lines
virt/kvm/iommu.c +10
-3@@ -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 ))
virt/kvm/iommu.c int kvm_assign_device(struct kvm *kvm, · line 126
Quoted 10 lines
virt/kvm/iommu.c +10
-0+ 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?
Quoted 20 lines
virt/kvm/iommu.c +4
-0+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,Signature · 6 lines
1.5.4.5 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · On Saturday 25 April 2009 06:37:17 Marcelo Tosatti wrote:
Sheng,
Quoted 6 lines · Sheng Yang
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.
Quoted 3 lines
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?
(sorry, resend, HTML is enabled by mistake.)
Yes. In fact it's usually usable, though we still have to deal with some exceptions.
Quoted 4 lines
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?
With snooping control, we set it to be WB, as the same of host, for IOMMU engine would deal with the cache issue if it used a DMA address. So direct accessing to the memory also won't cause trouble.
Guest can set any memory as UC or WC, but we think the only purpose for this is DMA/MMIO. MMIO address can be tell, and we ignored the setting to DMA(that's means WB, and explained above), for it would handled by IOMMU engine with snooping control feature.
Quoted 63 lines
More comments below.
Quoted 56 lines
Snooping control can guarantee the cache correctness of accessing the DMA engine of VT-d.
Signed-off-by Sheng Yang <[email protected]>---
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(-)(quoted diff) +0
-0
b/arch/x86/include/asm/kvm_host.hindex 3fc4623..d2da40f 100644arch/x86/include/asm/kvm_host.h +2
-0@@ -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;arch/x86/kvm/mmu.c +20
-3@@ -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 Quoted 2 lines
mt_mask=MTRR_TYPE_UNCACHABLE<<kvm_x86_ops->get_mt_mask_shift();Can you move this to subarch code?
Quoted 8 lines
Perhaps replace
get_mt_mask_shiftwithu64 (*get_spte_mt_mask)(bool
is_iommu_page);And then just do
if (
mt_mask)
spte |=kvm_x86_ops->get_spte_mt_mask();
Yeah. And sorry, I now remembered that you mentioned it before...
Quoted 45 lines
Quoted 45 lines
virt/kvm/iommu.c +6
-2@@ -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);virt/kvm/iommu.c +1
-1@@ -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; }
Quoted 16 lines
virt/kvm/iommu.c int kvm_assign_device(struct kvm *kvm, · line 126
Quoted 13 lines
virt/kvm/iommu.c +13
-0+ 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?
This point is for the device assigned (should still in initialized stage, but maybe also hot-plug which can happen at any time). Before assign, we don't know which devices would for KVM, and then don't know which engine they're hooked, so also don't know if the engine support snooping control...
Signature · 2 lines
regards Yang, Sheng
Quoted 21 lines
Quoted 21 lines
virt/kvm/iommu.c +4
-1+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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html