| Message ID | 5535368B.9060408@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-04-20 19:25+0200, Jan Kiszka: > 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 <jan.kiszka@siemens.com> > --- > > Changes in v2: > - add mark_dirty as found missing by Radim Thanks. Reviewed-by: Radim Kr?má? <rkrcmar@redhat.com> -- 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
On 20/04/2015 19:25, Jan Kiszka wrote: > 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 <jan.kiszka@siemens.com> I'm not sure about this. The problem is that, unlike Intel, AMD has no way for the host to force its PAT value and ignore the guest's. I'm worried about potential performance problems in the guest. This is not as bad as on ARM, because the guest cannot disable the cache snooping protocol and thus cache coherency is guaranteed (see tables 7-10 and 15-20 in the AMD docs), but still I think I'd prefer having some knob (module parameter) to enable/disable gPAT. It's okay to make it enabled by default. Paolo > --- > > Changes in v2: > - add mark_dirty as found missing by Radim > > 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 ce741b8..68fdddc 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -3245,6 +3245,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; > + svm->vmcb->save.g_pat = data; > + mark_dirty(svm->vmcb, VMCB_NPT); > + vcpu->arch.pat = data; > + break; > + } > + /* fall through */ > default: > return kvm_set_msr_common(vcpu, msr); > } -- 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
On 2015-04-21 13:09, Paolo Bonzini wrote: > > > On 20/04/2015 19:25, Jan Kiszka wrote: >> 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 <jan.kiszka@siemens.com> > > I'm not sure about this. The problem is that, unlike Intel, AMD has no > way for the host to force its PAT value and ignore the guest's. I'm > worried about potential performance problems in the guest. I think the guest needs to get what it requests - see my remark in http://thread.gmane.org/gmane.comp.emulators.kvm.devel/135271. > > This is not as bad as on ARM, because the guest cannot disable the cache You mean AMD, I guess. > snooping protocol and thus cache coherency is guaranteed (see tables > 7-10 and 15-20 in the AMD docs), but still I think I'd prefer having > some knob (module parameter) to enable/disable gPAT. It's okay to make > it enabled by default. I still don't get the scenario where we want to override the guest settings. Maybe you can help out - would be valuable for the reasoning in code or commit logs as well. Jan
On 21/04/2015 13:25, Jan Kiszka wrote: > On 2015-04-21 13:09, Paolo Bonzini wrote: >> >> >> On 20/04/2015 19:25, Jan Kiszka wrote: >>> 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 <jan.kiszka@siemens.com> >> >> I'm not sure about this. The problem is that, unlike Intel, AMD has no >> way for the host to force its PAT value and ignore the guest's. I'm >> worried about potential performance problems in the guest. > > I think the guest needs to get what it requests - see my remark in > http://thread.gmane.org/gmane.comp.emulators.kvm.devel/135271. > >> >> This is not as bad as on ARM, because the guest cannot disable the cache > > You mean AMD, I guess. No, I meant ARM. :) On ARM the guest can even disable the cache snooping protocol, making things particularly messy when QEMU accesses the processor caches and the guest doesn't. At least on AMD you cannot do this. >> snooping protocol and thus cache coherency is guaranteed (see tables >> 7-10 and 15-20 in the AMD docs), but still I think I'd prefer having >> some knob (module parameter) to enable/disable gPAT. It's okay to make >> it enabled by default. > > I still don't get the scenario where we want to override the guest > settings. Maybe you can help out - would be valuable for the reasoning > in code or commit logs as well. Basically it's an optimization. The guest can set the UC memory type on PCI BARs that are actually backed by RAM in QEMU, and then accesses to these BARs will be unnecessarily slow. It would be particularly bad if, for example, access to ivshmem were slowed down because the guest PAT says the memory is uncacheable. Paolo -- 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
On 2015-04-21 13:32, Paolo Bonzini wrote: > Basically it's an optimization. The guest can set the UC memory type on > PCI BARs that are actually backed by RAM in QEMU, and then accesses to > these BARs will be unnecessarily slow. It would be particularly bad if, > for example, access to ivshmem were slowed down because the guest PAT > says the memory is uncacheable. ivshmem is pv anyway - why shouldn't the guest driver take this room for optimization into account and ask for a cached mapping? Is that that only use case? Jan
On 21/04/2015 13:56, Jan Kiszka wrote: > > Basically it's an optimization. The guest can set the UC memory type on > > PCI BARs that are actually backed by RAM in QEMU, and then accesses to > > these BARs will be unnecessarily slow. It would be particularly bad if, > > for example, access to ivshmem were slowed down because the guest PAT > > says the memory is uncacheable. > > ivshmem is pv anyway - why shouldn't the guest driver take this room for > optimization into account and ask for a cached mapping? > > Is that that only use case? I guess a frame buffer would be affected as well, though probably the guest would set it to WC so it's less bad. Paolo -- 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
2015-04-21 13:09+0200, Paolo Bonzini: > > > On 20/04/2015 19:25, Jan Kiszka wrote: > > 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 <jan.kiszka@siemens.com> > > I'm not sure about this. The problem is that, unlike Intel, AMD has no > way for the host to force its PAT value and ignore the guest's. I'm > worried about potential performance problems in the guest. We already set g_pat to 0x0007040600070406ULL in init_vmcb(). This patch uses caching that the guest expects, which might improve performance as well. I think it's a step in right direction even if we somehow optimize cache coherent cases later. -- 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
On 2015-04-21 14:21, Radim Kr?má? wrote: > 2015-04-21 13:09+0200, Paolo Bonzini: >> >> >> On 20/04/2015 19:25, Jan Kiszka wrote: >>> 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 <jan.kiszka@siemens.com> >> >> I'm not sure about this. The problem is that, unlike Intel, AMD has no >> way for the host to force its PAT value and ignore the guest's. I'm >> worried about potential performance problems in the guest. > > We already set g_pat to 0x0007040600070406ULL in init_vmcb(). > This patch uses caching that the guest expects, which might improve > performance as well. I think it's a step in right direction even if we > somehow optimize cache coherent cases later. This topic is still open - and the patch still applies. Jan
On 2015-05-24 17:28, Jan Kiszka wrote: > On 2015-04-21 14:21, Radim Kr?má? wrote: >> 2015-04-21 13:09+0200, Paolo Bonzini: >>> >>> >>> On 20/04/2015 19:25, Jan Kiszka wrote: >>>> 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 <jan.kiszka@siemens.com> >>> >>> I'm not sure about this. The problem is that, unlike Intel, AMD has no >>> way for the host to force its PAT value and ignore the guest's. I'm >>> worried about potential performance problems in the guest. >> >> We already set g_pat to 0x0007040600070406ULL in init_vmcb(). >> This patch uses caching that the guest expects, which might improve >> performance as well. I think it's a step in right direction even if we >> somehow optimize cache coherent cases later. > > This topic is still open - and the patch still applies. Just rebased by kvm patch queue for some new entries - in this old one is still there. What can we do about it? Jan
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index ce741b8..68fdddc 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -3245,6 +3245,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; + svm->vmcb->save.g_pat = data; + mark_dirty(svm->vmcb, VMCB_NPT); + vcpu->arch.pat = data; + break; + } + /* fall through */ default: return kvm_set_msr_common(vcpu, msr); }
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 <jan.kiszka@siemens.com> --- Changes in v2: - add mark_dirty as found missing by Radim arch/x86/kvm/svm.c | 10 ++++++++++ 1 file changed, 10 insertions(+)