This part of the MTRR patches was dropped by Xiao. Bring SVM on feature
parity with VMX, and then do guest MTRR virtualization for both VMX and SVM.
The IPAT bit of VMX extended page tables is emulated by mangling the guest
PAT value.
I do not have any AMD machines that support an IOMMU, so I would like
some help testing these patches. Thanks,
Paolo
v1->v2: AMD IOMMUs do have snooping control [Joerg]
New patch 1
Jan Kiszka (1):
KVM: SVM: Sync g_pat with guest-written PAT value
Paolo Bonzini (3):
KVM: count number of assigned devices
KVM: SVM: use NPT page attributes
KVM: x86: apply guest MTRR virtualization on host reserved pages
arch/x86/include/asm/kvm_host.h | 3 ++-
arch/x86/kvm/iommu.c | 2 ++
arch/x86/kvm/svm.c | 54 +++++++++++++++++++++++++++++++++++++----
arch/x86/kvm/vmx.c | 11 +++------
arch/x86/kvm/x86.c | 18 ++++++++++++++
include/linux/kvm_host.h | 18 +++++++++++++-
virt/kvm/vfio.c | 5 ++++
7 files changed, 96 insertions(+), 15 deletions(-)
--
1.8.3.1
If there are no assigned devices, the guest PAT are not providing
any useful information and can be overridden to writeback; VMX
always does this because it has the "IPAT" bit in its extended
page table entries, but SVM does not have anything similar.
Hook into VFIO and legacy device assignment so that they
provide this information to KVM.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 3 ++-
arch/x86/kvm/iommu.c | 2 ++
arch/x86/kvm/x86.c | 18 ++++++++++++++++++
include/linux/kvm_host.h | 18 +++++++++++++++++-
virt/kvm/vfio.c | 5 +++++
5 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 2a7f5d782c33..7fe26ba9082e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -602,8 +602,9 @@ struct kvm_arch {
struct list_head assigned_dev_head;
struct iommu_domain *iommu_domain;
bool iommu_noncoherent;
-#define __KVM_HAVE_ARCH_NONCOHERENT_DMA
+#define __KVM_HAVE_ARCH_VFIO_HOOKS
atomic_t noncoherent_dma_count;
+ atomic_t assigned_device_count;
struct kvm_pic *vpic;
struct kvm_ioapic *vioapic;
struct kvm_pit *vpit;
diff --git a/arch/x86/kvm/iommu.c b/arch/x86/kvm/iommu.c
index 7dbced309ddb..5c520ebf6343 100644
--- a/arch/x86/kvm/iommu.c
+++ b/arch/x86/kvm/iommu.c
@@ -200,6 +200,7 @@ int kvm_assign_device(struct kvm *kvm, struct pci_dev *pdev)
goto out_unmap;
}
+ kvm_arch_start_assignment(kvm);
pci_set_dev_assigned(pdev);
dev_info(&pdev->dev, "kvm assign device\n");
@@ -224,6 +225,7 @@ int kvm_deassign_device(struct kvm *kvm, struct pci_dev *pdev)
iommu_detach_device(domain, &pdev->dev);
pci_clear_dev_assigned(pdev);
+ kvm_arch_end_assignment(kvm);
dev_info(&pdev->dev, "kvm deassign device\n");
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bbaf44e8f0d3..6e455875d2e4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8218,6 +8218,24 @@ bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu)
kvm_x86_ops->interrupt_allowed(vcpu);
}
+void kvm_arch_start_assignment(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.assigned_device_count);
+}
+EXPORT_SYMBOL_GPL(kvm_arch_start_assignment);
+
+void kvm_arch_end_assignment(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.assigned_device_count);
+}
+EXPORT_SYMBOL_GPL(kvm_arch_end_assignment);
+
+bool kvm_arch_has_assigned_device(struct kvm *kvm)
+{
+ return atomic_read(&kvm->arch.assigned_device_count);
+}
+EXPORT_SYMBOL_GPL(kvm_arch_has_assigned_device);
+
void kvm_arch_register_noncoherent_dma(struct kvm *kvm)
{
atomic_inc(&kvm->arch.noncoherent_dma_count);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 9564fd78c547..56146fdb1ff0 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -716,10 +716,13 @@ static inline void kvm_arch_free_vm(struct kvm *kvm)
}
#endif
-#ifdef __KVM_HAVE_ARCH_NONCOHERENT_DMA
+#ifdef __KVM_HAVE_ARCH_VFIO_HOOKS
void kvm_arch_register_noncoherent_dma(struct kvm *kvm);
void kvm_arch_unregister_noncoherent_dma(struct kvm *kvm);
bool kvm_arch_has_noncoherent_dma(struct kvm *kvm);
+void kvm_arch_start_assignment(struct kvm *kvm);
+void kvm_arch_end_assignment(struct kvm *kvm);
+bool kvm_arch_has_assigned_device(struct kvm *kvm);
#else
static inline void kvm_arch_register_noncoherent_dma(struct kvm *kvm)
{
@@ -733,6 +736,19 @@ static inline bool kvm_arch_has_noncoherent_dma(struct kvm *kvm)
{
return false;
}
+
+static inline void kvm_arch_start_assignment(struct kvm *kvm)
+{
+}
+
+static inline void kvm_arch_end_assignment(struct kvm *kvm)
+{
+}
+
+static inline bool kvm_arch_has_assigned_device(struct kvm *kvm)
+{
+ return false;
+}
#endif
static inline wait_queue_head_t *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 620e37f741b8..1dd087da6f31 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -155,6 +155,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
list_add_tail(&kvg->node, &kv->group_list);
kvg->vfio_group = vfio_group;
+ kvm_arch_start_assignment(dev->kvm);
+
mutex_unlock(&kv->lock);
kvm_vfio_update_coherency(dev);
@@ -190,6 +192,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
break;
}
+ kvm_arch_end_assignment(dev->kvm);
+
mutex_unlock(&kv->lock);
kvm_vfio_group_put_external_user(vfio_group);
@@ -239,6 +243,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
kvm_vfio_group_put_external_user(kvg->vfio_group);
list_del(&kvg->node);
kfree(kvg);
+ kvm_arch_end_assignment(dev->kvm);
}
kvm_vfio_update_coherency(dev);
--
1.8.3.1
Right now, NPT page attributes are not used, and the final page
attribute depends solely on gPAT (which however is not synced
correctly), the guest MTRRs and the guest page attributes.
However, we can do better by mimicking what is done for VMX.
In the absence of PCI passthrough, the guest PAT can be ignored
and the page attributes can be just WB. If passthrough is being
used, instead, keep respecting the guest PAT, and emulate the guest
MTRRs through the PAT field of the nested page tables.
The only snag is that WP memory cannot be emulated correctly,
because Linux's default PAT setting only includes the other types.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 42 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 602b974a60a6..0f125c1860ec 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1085,6 +1085,47 @@ static u64 svm_compute_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc)
return target_tsc - tsc;
}
+static void svm_set_guest_pat(struct vcpu_svm *svm, u64 *g_pat)
+{
+ struct kvm_vcpu *vcpu = &svm->vcpu;
+
+ /* Unlike Intel, AMD takes the guest's CR0.CD into account.
+ *
+ * AMD does not have IPAT. To emulate it for the case of guests
+ * with no assigned devices, just set everything to WB. If guests
+ * have assigned devices, however, we cannot force WB for RAM
+ * pages only, so use the guest IPAT as passed.
+ */
+ if (!kvm_arch_has_assigned_device(vcpu->kvm))
+ *g_pat = 0x0606060606060606;
+ else
+ *g_pat = vcpu->arch.pat;
+}
+
+static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
+{
+ u8 cache;
+
+ /*
+ * 1. MMIO: always map as UC
+ * 2. No passthrough: always map as WB, and force guest PAT to WB as well
+ * 3. Passthrough: can't guarantee the result, try to trust guest.
+ */
+ if (is_mmio)
+ return _PAGE_NOCACHE;
+
+ if (!kvm_arch_has_assigned_device(vcpu->kvm))
+ return 0;
+
+ cache = kvm_mtrr_get_guest_memory_type(vcpu, gfn);
+
+ /* Linux's host PAT value does not support WP. */
+ if (cache == _PAGE_CACHE_MODE_WP)
+ cache = _PAGE_CACHE_MODE_UC_MINUS;
+
+ return cachemode2protval(cache);
+}
+
static void init_vmcb(struct vcpu_svm *svm, bool init_event)
{
struct vmcb_control_area *control = &svm->vmcb->control;
@@ -1180,6 +1221,7 @@ static void init_vmcb(struct vcpu_svm *svm, bool init_event)
clr_cr_intercept(svm, INTERCEPT_CR3_READ);
clr_cr_intercept(svm, INTERCEPT_CR3_WRITE);
save->g_pat = svm->vcpu.arch.pat;
+ svm_set_guest_pat(svm, &save->g_pat);
save->cr3 = 0;
save->cr4 = 0;
}
@@ -4088,11 +4130,6 @@ static bool svm_has_high_real_mode_segbase(void)
return true;
}
-static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
-{
- return 0;
-}
-
static void svm_cpuid_update(struct kvm_vcpu *vcpu)
{
}
--
1.8.3.1
From: Jan Kiszka <[email protected]>
When hardware supports the g_pat VMCB field, we can use it for emulating
the PAT configuration that the guest configures by writing to the
corresponding MSR.
Signed-off-by: Jan Kiszka <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0f125c1860ec..728ac43fae16 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3296,6 +3296,16 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
case MSR_VM_IGNNE:
vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
break;
+ case MSR_IA32_CR_PAT:
+ if (npt_enabled) {
+ if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
+ return 1;
+ vcpu->arch.pat = data;
+ svm_set_guest_pat(svm, &svm->vmcb->save.g_pat);
+ mark_dirty(svm->vmcb, VMCB_NPT);
+ break;
+ }
+ /* fall through */
default:
return kvm_set_msr_common(vcpu, msr);
}
--
1.8.3.1
Currently guest MTRR is avoided if kvm_is_reserved_pfn returns true.
However, the guest could prefer a different page type than UC for
such pages. A good example is that pass-throughed VGA frame buffer is
not always UC as host expected.
This patch enables full use of virtual guest MTRRs.
Suggested-by: Xiao Guangrong <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm.c | 7 ++-----
arch/x86/kvm/vmx.c | 11 +++--------
2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 728ac43fae16..43f3a2a3479a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1107,14 +1107,11 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
u8 cache;
/*
- * 1. MMIO: always map as UC
+ * 1. MMIO: trust guest MTRR, so same as item 3.
* 2. No passthrough: always map as WB, and force guest PAT to WB as well
* 3. Passthrough: can't guarantee the result, try to trust guest.
*/
- if (is_mmio)
- return _PAGE_NOCACHE;
-
- if (!kvm_arch_has_assigned_device(vcpu->kvm))
+ if (!is_mmio && !kvm_arch_has_assigned_device(vcpu->kvm))
return 0;
cache = kvm_mtrr_get_guest_memory_type(vcpu, gfn);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e856dd566f4c..5b4e9384717a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8632,22 +8632,17 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
u64 ipat = 0;
/* For VT-d and EPT combination
- * 1. MMIO: always map as UC
+ * 1. MMIO: guest may want to apply WC, trust it.
* 2. EPT with VT-d:
* a. VT-d without snooping control feature: can't guarantee the
- * result, try to trust guest.
+ * result, try to trust guest. So the same as item 1.
* b. VT-d with snooping control feature: snooping control feature of
* VT-d engine can guarantee the cache correctness. Just set it
* to WB to keep consistent with host. So the same as item 3.
* 3. EPT without VT-d: always map as WB and set IPAT=1 to keep
* consistent with host MTRR
*/
- if (is_mmio) {
- cache = MTRR_TYPE_UNCACHABLE;
- goto exit;
- }
-
- if (!kvm_arch_has_noncoherent_dma(vcpu->kvm)) {
+ if (!is_mmio && !kvm_arch_has_noncoherent_dma(vcpu->kvm)) {
ipat = VMX_EPT_IPAT_BIT;
cache = MTRR_TYPE_WRBACK;
goto exit;
--
1.8.3.1
Hi Paolo,
On Tue, Jul 07, 2015 at 03:45:35PM +0200, Paolo Bonzini wrote:
> I do not have any AMD machines that support an IOMMU, so I would like
> some help testing these patches. Thanks,
What kind of testing do you want? Booting a guest with a device attached
is probably not sufficient, right?
Joerg
On 07/07/2015 16:06, Joerg Roedel wrote:
> > I do not have any AMD machines that support an IOMMU, so I would like
> > some help testing these patches. Thanks,
>
> What kind of testing do you want? Booting a guest with a device attached
> is probably not sufficient, right?
The guest should not take ages to boot, which is typical of messed-up
MTRR/PAT. And if you add some actual usage of the attached device, it
actually is sufficient. Even a simple ping test, or typing through a
USB keyboard on a passed-through controller, is enough.
Paolo
On Tue, Jul 07, 2015 at 04:09:07PM +0200, Paolo Bonzini wrote:
> The guest should not take ages to boot, which is typical of messed-up
> MTRR/PAT. And if you add some actual usage of the attached device, it
> actually is sufficient. Even a simple ping test, or typing through a
> USB keyboard on a passed-through controller, is enough.
Okay, I do some testing on this tomorrow and let you know.
Joerg
On Tue, 2015-07-07 at 15:45 +0200, Paolo Bonzini wrote:
> If there are no assigned devices, the guest PAT are not providing
> any useful information and can be overridden to writeback; VMX
> always does this because it has the "IPAT" bit in its extended
> page table entries, but SVM does not have anything similar.
> Hook into VFIO and legacy device assignment so that they
> provide this information to KVM.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 3 ++-
> arch/x86/kvm/iommu.c | 2 ++
> arch/x86/kvm/x86.c | 18 ++++++++++++++++++
> include/linux/kvm_host.h | 18 +++++++++++++++++-
> virt/kvm/vfio.c | 5 +++++
> 5 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 2a7f5d782c33..7fe26ba9082e 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -602,8 +602,9 @@ struct kvm_arch {
> struct list_head assigned_dev_head;
> struct iommu_domain *iommu_domain;
> bool iommu_noncoherent;
> -#define __KVM_HAVE_ARCH_NONCOHERENT_DMA
> +#define __KVM_HAVE_ARCH_VFIO_HOOKS
Do we really want to tie these two things together under something
that's not strictly a "vfio" option? Legacy assignment also makes use
of these, as shown in this patch, but even if we consider that temporary
until legacy assignment is removed, I can imagine platforms that might
care about one but not the other. I don't really see the harm in using
a separate #define, perhaps __KVM_HAVE_ARCH_ASSIGNED_DEVICE. Thanks,
Alex
> atomic_t noncoherent_dma_count;
> + atomic_t assigned_device_count;
> struct kvm_pic *vpic;
> struct kvm_ioapic *vioapic;
> struct kvm_pit *vpit;
> diff --git a/arch/x86/kvm/iommu.c b/arch/x86/kvm/iommu.c
> index 7dbced309ddb..5c520ebf6343 100644
> --- a/arch/x86/kvm/iommu.c
> +++ b/arch/x86/kvm/iommu.c
> @@ -200,6 +200,7 @@ int kvm_assign_device(struct kvm *kvm, struct pci_dev *pdev)
> goto out_unmap;
> }
>
> + kvm_arch_start_assignment(kvm);
> pci_set_dev_assigned(pdev);
>
> dev_info(&pdev->dev, "kvm assign device\n");
> @@ -224,6 +225,7 @@ int kvm_deassign_device(struct kvm *kvm, struct pci_dev *pdev)
> iommu_detach_device(domain, &pdev->dev);
>
> pci_clear_dev_assigned(pdev);
> + kvm_arch_end_assignment(kvm);
>
> dev_info(&pdev->dev, "kvm deassign device\n");
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bbaf44e8f0d3..6e455875d2e4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8218,6 +8218,24 @@ bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu)
> kvm_x86_ops->interrupt_allowed(vcpu);
> }
>
> +void kvm_arch_start_assignment(struct kvm *kvm)
> +{
> + atomic_inc(&kvm->arch.assigned_device_count);
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_start_assignment);
> +
> +void kvm_arch_end_assignment(struct kvm *kvm)
> +{
> + atomic_dec(&kvm->arch.assigned_device_count);
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_end_assignment);
> +
> +bool kvm_arch_has_assigned_device(struct kvm *kvm)
> +{
> + return atomic_read(&kvm->arch.assigned_device_count);
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_has_assigned_device);
> +
> void kvm_arch_register_noncoherent_dma(struct kvm *kvm)
> {
> atomic_inc(&kvm->arch.noncoherent_dma_count);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 9564fd78c547..56146fdb1ff0 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -716,10 +716,13 @@ static inline void kvm_arch_free_vm(struct kvm *kvm)
> }
> #endif
>
> -#ifdef __KVM_HAVE_ARCH_NONCOHERENT_DMA
> +#ifdef __KVM_HAVE_ARCH_VFIO_HOOKS
> void kvm_arch_register_noncoherent_dma(struct kvm *kvm);
> void kvm_arch_unregister_noncoherent_dma(struct kvm *kvm);
> bool kvm_arch_has_noncoherent_dma(struct kvm *kvm);
> +void kvm_arch_start_assignment(struct kvm *kvm);
> +void kvm_arch_end_assignment(struct kvm *kvm);
> +bool kvm_arch_has_assigned_device(struct kvm *kvm);
> #else
> static inline void kvm_arch_register_noncoherent_dma(struct kvm *kvm)
> {
> @@ -733,6 +736,19 @@ static inline bool kvm_arch_has_noncoherent_dma(struct kvm *kvm)
> {
> return false;
> }
> +
> +static inline void kvm_arch_start_assignment(struct kvm *kvm)
> +{
> +}
> +
> +static inline void kvm_arch_end_assignment(struct kvm *kvm)
> +{
> +}
> +
> +static inline bool kvm_arch_has_assigned_device(struct kvm *kvm)
> +{
> + return false;
> +}
> #endif
>
> static inline wait_queue_head_t *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 620e37f741b8..1dd087da6f31 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -155,6 +155,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
> list_add_tail(&kvg->node, &kv->group_list);
> kvg->vfio_group = vfio_group;
>
> + kvm_arch_start_assignment(dev->kvm);
> +
> mutex_unlock(&kv->lock);
>
> kvm_vfio_update_coherency(dev);
> @@ -190,6 +192,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
> break;
> }
>
> + kvm_arch_end_assignment(dev->kvm);
> +
> mutex_unlock(&kv->lock);
>
> kvm_vfio_group_put_external_user(vfio_group);
> @@ -239,6 +243,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
> kvm_vfio_group_put_external_user(kvg->vfio_group);
> list_del(&kvg->node);
> kfree(kvg);
> + kvm_arch_end_assignment(dev->kvm);
> }
>
> kvm_vfio_update_coherency(dev);
On 07/07/2015 17:22, Alex Williamson wrote:
>> > -#define __KVM_HAVE_ARCH_NONCOHERENT_DMA
>> > +#define __KVM_HAVE_ARCH_VFIO_HOOKS
> Do we really want to tie these two things together under something
> that's not strictly a "vfio" option? Legacy assignment also makes use
> of these, as shown in this patch, but even if we consider that temporary
> until legacy assignment is removed, I can imagine platforms that might
> care about one but not the other. I don't really see the harm in using
> a separate #define, perhaps __KVM_HAVE_ARCH_ASSIGNED_DEVICE. Thanks,
Sure, that's okay.
Paolo
On 07/07/2015 09:45 PM, Paolo Bonzini wrote:
> Right now, NPT page attributes are not used, and the final page
> attribute depends solely on gPAT (which however is not synced
> correctly), the guest MTRRs and the guest page attributes.
>
> However, we can do better by mimicking what is done for VMX.
> In the absence of PCI passthrough, the guest PAT can be ignored
> and the page attributes can be just WB. If passthrough is being
> used, instead, keep respecting the guest PAT, and emulate the guest
> MTRRs through the PAT field of the nested page tables.
>
> The only snag is that WP memory cannot be emulated correctly,
> because Linux's default PAT setting only includes the other types.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/svm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 602b974a60a6..0f125c1860ec 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1085,6 +1085,47 @@ static u64 svm_compute_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc)
> return target_tsc - tsc;
> }
>
> +static void svm_set_guest_pat(struct vcpu_svm *svm, u64 *g_pat)
> +{
> + struct kvm_vcpu *vcpu = &svm->vcpu;
> +
> + /* Unlike Intel, AMD takes the guest's CR0.CD into account.
I noticed this code in svm_set_cr0():
if (!(vcpu->kvm->arch.disabled_quirks & KVM_QUIRK_CD_NW_CLEARED))
cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
gCR0.CD is hidden to CPU if KVM_QUIRK_CD_NW_CLEARED is not set and looks like
it is the normal case after grepping Qemu code.
> + *
> + * AMD does not have IPAT. To emulate it for the case of guests
> + * with no assigned devices, just set everything to WB. If guests
> + * have assigned devices, however, we cannot force WB for RAM
> + * pages only, so use the guest IPAT as passed.
> + */
> + if (!kvm_arch_has_assigned_device(vcpu->kvm))
> + *g_pat = 0x0606060606060606;
> + else
> + *g_pat = vcpu->arch.pat;
> +}
> +
> +static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> +{
> + u8 cache;
> +
> + /*
> + * 1. MMIO: always map as UC
> + * 2. No passthrough: always map as WB, and force guest PAT to WB as well
> + * 3. Passthrough: can't guarantee the result, try to trust guest.
> + */
> + if (is_mmio)
> + return _PAGE_NOCACHE;
> +
> + if (!kvm_arch_has_assigned_device(vcpu->kvm))
> + return 0;
> +
> + cache = kvm_mtrr_get_guest_memory_type(vcpu, gfn);
> +
@cache return from kvm_mtrr_get_guest_memory_type is MTRR_TYPE_*
which is different with _PAGE_CACHE_MODE_*. The latter is pure SW
usage, e.g:
_PAGE_CACHE_MODE_WB = 0 and #define MTRR_TYPE_WRBACK 6
On 08/07/2015 07:59, Xiao Guangrong wrote:
>
>
> On 07/07/2015 09:45 PM, Paolo Bonzini wrote:
>> Right now, NPT page attributes are not used, and the final page
>> attribute depends solely on gPAT (which however is not synced
>> correctly), the guest MTRRs and the guest page attributes.
>>
>> However, we can do better by mimicking what is done for VMX.
>> In the absence of PCI passthrough, the guest PAT can be ignored
>> and the page attributes can be just WB. If passthrough is being
>> used, instead, keep respecting the guest PAT, and emulate the guest
>> MTRRs through the PAT field of the nested page tables.
>>
>> The only snag is that WP memory cannot be emulated correctly,
>> because Linux's default PAT setting only includes the other types.
>>
>> Signed-off-by: Paolo Bonzini <[email protected]>
>> ---
>> arch/x86/kvm/svm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 42 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 602b974a60a6..0f125c1860ec 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1085,6 +1085,47 @@ static u64 svm_compute_tsc_offset(struct
>> kvm_vcpu *vcpu, u64 target_tsc)
>> return target_tsc - tsc;
>> }
>>
>> +static void svm_set_guest_pat(struct vcpu_svm *svm, u64 *g_pat)
>> +{
>> + struct kvm_vcpu *vcpu = &svm->vcpu;
>> +
>> + /* Unlike Intel, AMD takes the guest's CR0.CD into account.
>
> I noticed this code in svm_set_cr0():
>
> if (!(vcpu->kvm->arch.disabled_quirks & KVM_QUIRK_CD_NW_CLEARED))
> cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
>
> gCR0.CD is hidden to CPU if KVM_QUIRK_CD_NW_CLEARED is not set and looks
> like
> it is the normal case after grepping Qemu code.
>
>> + *
>> + * AMD does not have IPAT. To emulate it for the case of guests
>> + * with no assigned devices, just set everything to WB. If guests
>> + * have assigned devices, however, we cannot force WB for RAM
>> + * pages only, so use the guest IPAT as passed.
>> + */
>> + if (!kvm_arch_has_assigned_device(vcpu->kvm))
>> + *g_pat = 0x0606060606060606;
>> + else
>> + *g_pat = vcpu->arch.pat;
>> +}
>> +
>> +static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool
>> is_mmio)
>> +{
>> + u8 cache;
>> +
>> + /*
>> + * 1. MMIO: always map as UC
>> + * 2. No passthrough: always map as WB, and force guest PAT to WB
>> as well
>> + * 3. Passthrough: can't guarantee the result, try to trust guest.
>> + */
>> + if (is_mmio)
>> + return _PAGE_NOCACHE;
>> +
>> + if (!kvm_arch_has_assigned_device(vcpu->kvm))
>> + return 0;
>> +
>> + cache = kvm_mtrr_get_guest_memory_type(vcpu, gfn);
>> +
>
> @cache return from kvm_mtrr_get_guest_memory_type is MTRR_TYPE_*
> which is different with _PAGE_CACHE_MODE_*. The latter is pure SW
> usage, e.g:
> _PAGE_CACHE_MODE_WB = 0 and #define MTRR_TYPE_WRBACK 6
Oops, you're right. In fact my first version was correct, then I
changed it to use cachemode2protval and screwed up.
Paolo
On 07/08/2015 07:19 PM, Paolo Bonzini wrote:
>
>
> On 08/07/2015 07:59, Xiao Guangrong wrote:
>>
>>
>> On 07/07/2015 09:45 PM, Paolo Bonzini wrote:
>>> Right now, NPT page attributes are not used, and the final page
>>> attribute depends solely on gPAT (which however is not synced
>>> correctly), the guest MTRRs and the guest page attributes.
>>>
>>> However, we can do better by mimicking what is done for VMX.
>>> In the absence of PCI passthrough, the guest PAT can be ignored
>>> and the page attributes can be just WB. If passthrough is being
>>> used, instead, keep respecting the guest PAT, and emulate the guest
>>> MTRRs through the PAT field of the nested page tables.
>>>
>>> The only snag is that WP memory cannot be emulated correctly,
>>> because Linux's default PAT setting only includes the other types.
>>>
>>> Signed-off-by: Paolo Bonzini <[email protected]>
>>> ---
>>> arch/x86/kvm/svm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 42 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index 602b974a60a6..0f125c1860ec 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -1085,6 +1085,47 @@ static u64 svm_compute_tsc_offset(struct
>>> kvm_vcpu *vcpu, u64 target_tsc)
>>> return target_tsc - tsc;
>>> }
>>>
>>> +static void svm_set_guest_pat(struct vcpu_svm *svm, u64 *g_pat)
>>> +{
>>> + struct kvm_vcpu *vcpu = &svm->vcpu;
>>> +
>>> + /* Unlike Intel, AMD takes the guest's CR0.CD into account.
>>
>> I noticed this code in svm_set_cr0():
>>
>> if (!(vcpu->kvm->arch.disabled_quirks & KVM_QUIRK_CD_NW_CLEARED))
>> cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
>>
>> gCR0.CD is hidden to CPU if KVM_QUIRK_CD_NW_CLEARED is not set and looks
>> like
>> it is the normal case after grepping Qemu code.
>>
How about this one? I still do not know how SVM properly emulates CR0.CD? :(
On 09/07/2015 04:30, Xiao Guangrong wrote:
>>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>> index 602b974a60a6..0f125c1860ec 100644
>>>> --- a/arch/x86/kvm/svm.c
>>>> +++ b/arch/x86/kvm/svm.c
>>>> @@ -1085,6 +1085,47 @@ static u64 svm_compute_tsc_offset(struct
>>>> kvm_vcpu *vcpu, u64 target_tsc)
>>>> return target_tsc - tsc;
>>>> }
>>>>
>>>> +static void svm_set_guest_pat(struct vcpu_svm *svm, u64 *g_pat)
>>>> +{
>>>> + struct kvm_vcpu *vcpu = &svm->vcpu;
>>>> +
>>>> + /* Unlike Intel, AMD takes the guest's CR0.CD into account.
>>>
>>> I noticed this code in svm_set_cr0():
>>>
>>> if (!(vcpu->kvm->arch.disabled_quirks & KVM_QUIRK_CD_NW_CLEARED))
>>> cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
>>>
>>> gCR0.CD is hidden to CPU if KVM_QUIRK_CD_NW_CLEARED is not set and looks
>>> like it is the normal case after grepping Qemu code.
Hi Xiao,
yes, this is correct. QEMU still does not have support for disabling
"quirks", so gCR0.CD is currently hidden on SVM. I would like to
include this series in 4.2, while for 4.3 I will disable the quirk above
altogether (it is superseded by the way PAT is forced to all-WB).
In any case, this is just a KVM limitation. gCR0.CD is taken into
account by the processor when computing the effective memory type. It
uses all of these:
- the guest page tables PAT/PCD/PWT
- the guest page tables CR3.PCD/CR3.PWD
- the nested page tables PAT/PCD/PWT
- the guest page tables CR3.PCD/CR3.PWD
- the host MTRRs
- gCR0.CD
- hCR0.CD
Paolo
> How about this one? I still do not know how SVM properly emulates
> CR0.CD? :(
> --
> 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
>
On 07/09/2015 11:18 PM, Paolo Bonzini wrote:
>
>
> On 09/07/2015 04:30, Xiao Guangrong wrote:
>>>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>>> index 602b974a60a6..0f125c1860ec 100644
>>>>> --- a/arch/x86/kvm/svm.c
>>>>> +++ b/arch/x86/kvm/svm.c
>>>>> @@ -1085,6 +1085,47 @@ static u64 svm_compute_tsc_offset(struct
>>>>> kvm_vcpu *vcpu, u64 target_tsc)
>>>>> return target_tsc - tsc;
>>>>> }
>>>>>
>>>>> +static void svm_set_guest_pat(struct vcpu_svm *svm, u64 *g_pat)
>>>>> +{
>>>>> + struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>> +
>>>>> + /* Unlike Intel, AMD takes the guest's CR0.CD into account.
>>>>
>>>> I noticed this code in svm_set_cr0():
>>>>
>>>> if (!(vcpu->kvm->arch.disabled_quirks & KVM_QUIRK_CD_NW_CLEARED))
>>>> cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
>>>>
>>>> gCR0.CD is hidden to CPU if KVM_QUIRK_CD_NW_CLEARED is not set and looks
>>>> like it is the normal case after grepping Qemu code.
>
> Hi Xiao,
>
> yes, this is correct. QEMU still does not have support for disabling
> "quirks", so gCR0.CD is currently hidden on SVM. I would like to
> include this series in 4.2, while for 4.3 I will disable the quirk above
> altogether (it is superseded by the way PAT is forced to all-WB).
>
That plan sounds good to me.
You will drop disabled_quirks completely or just enable it in Qemu? :)
On 10/07/2015 03:19, Xiao Guangrong wrote:
>> yes, this is correct. QEMU still does not have support for disabling
>> "quirks", so gCR0.CD is currently hidden on SVM. I would like to
>> include this series in 4.2, while for 4.3 I will disable the quirk above
>> altogether (it is superseded by the way PAT is forced to all-WB).
>
> That plan sounds good to me.
>
> You will drop disabled_quirks completely or just enable it in Qemu? :)
I will drop this quirk completely. Other quirks (well, there's just
one) will remain.
Paolo
On 07/10/2015 06:47 PM, Paolo Bonzini wrote:
>
>
> On 10/07/2015 03:19, Xiao Guangrong wrote:
>>> yes, this is correct. QEMU still does not have support for disabling
>>> "quirks", so gCR0.CD is currently hidden on SVM. I would like to
>>> include this series in 4.2, while for 4.3 I will disable the quirk above
>>> altogether (it is superseded by the way PAT is forced to all-WB).
>>
>> That plan sounds good to me.
>>
>> You will drop disabled_quirks completely or just enable it in Qemu? :)
>
> I will drop this quirk completely. Other quirks (well, there's just
> one) will remain.
Make senses.
The whole patchset looks good to me!
Reviewed-by: Xiao Guangrong <[email protected]>
On 07/07/2015 06:45 AM, Paolo Bonzini wrote:
> Right now, NPT page attributes are not used, and the final page
> attribute depends solely on gPAT (which however is not synced
> correctly), the guest MTRRs and the guest page attributes.
>
> However, we can do better by mimicking what is done for VMX.
> In the absence of PCI passthrough, the guest PAT can be ignored
> and the page attributes can be just WB. If passthrough is being
> used, instead, keep respecting the guest PAT, and emulate the guest
> MTRRs through the PAT field of the nested page tables.
>
> The only snag is that WP memory cannot be emulated correctly,
> because Linux's default PAT setting only includes the other types.
>
As of quite recently, Linux understands that the PAT has eight slots,
and we can probably give you WP. Do you want it?
--Andy
On 17/07/2015 02:35, Andy Lutomirski wrote:
>
>> Right now, NPT page attributes are not used, and the final page
>> attribute depends solely on gPAT (which however is not synced
>> correctly), the guest MTRRs and the guest page attributes.
>>
>> However, we can do better by mimicking what is done for VMX.
>> In the absence of PCI passthrough, the guest PAT can be ignored
>> and the page attributes can be just WB. If passthrough is being
>> used, instead, keep respecting the guest PAT, and emulate the guest
>> MTRRs through the PAT field of the nested page tables.
>>
>> The only snag is that WP memory cannot be emulated correctly,
>> because Linux's default PAT setting only includes the other types.
>>
>
> As of quite recently, Linux understands that the PAT has eight slots,
> and we can probably give you WP. Do you want it?
It doesn't really matter, it's mostly used in the firmware.
Paolo