* KVM: x86: do not retain disabled or invalid pvclock address
@ 2012-08-23 11:16 Marcelo Tosatti
2012-09-09 14:08 ` Avi Kivity
0 siblings, 1 reply; 3+ messages in thread
From: Marcelo Tosatti @ 2012-08-23 11:16 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm
In case an invalid or disabled gpa is written to the SYSTEM_TIME
MSR, do not retain its value. This is not documented behaviour,
nor should be supported.
Also clear it on system reset. Not doing so can hide bugs.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e00050c..ed4bfb7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1528,6 +1528,7 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
static void kvmclock_reset(struct kvm_vcpu *vcpu)
{
+ vcpu->arch.time = 0;
if (vcpu->arch.time_page) {
kvm_release_page_dirty(vcpu->arch.time_page);
vcpu->arch.time_page = NULL;
@@ -1632,8 +1633,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
/* we verify if the enable bit is set... */
- if (!(data & 1))
+ if (!(data & 1)) {
+ vcpu->arch.time = 0;
break;
+ }
/* ...but clean it before doing the actual write */
vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
@@ -1641,8 +1644,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
vcpu->arch.time_page =
gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
- if (is_error_page(vcpu->arch.time_page))
+ if (is_error_page(vcpu->arch.time_page)) {
vcpu->arch.time_page = NULL;
+ vcpu->arch.time = 0;
+ }
break;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: KVM: x86: do not retain disabled or invalid pvclock address
2012-08-23 11:16 KVM: x86: do not retain disabled or invalid pvclock address Marcelo Tosatti
@ 2012-09-09 14:08 ` Avi Kivity
2012-09-09 23:37 ` Marcelo Tosatti
0 siblings, 1 reply; 3+ messages in thread
From: Avi Kivity @ 2012-09-09 14:08 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm
On 08/23/2012 02:16 PM, Marcelo Tosatti wrote:
>
> In case an invalid or disabled gpa is written to the SYSTEM_TIME
> MSR, do not retain its value. This is not documented behaviour,
> nor should be supported.
>
> Also clear it on system reset. Not doing so can hide bugs.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e00050c..ed4bfb7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1528,6 +1528,7 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
>
> static void kvmclock_reset(struct kvm_vcpu *vcpu)
> {
> + vcpu->arch.time = 0;
> if (vcpu->arch.time_page) {
> kvm_release_page_dirty(vcpu->arch.time_page);
> vcpu->arch.time_page = NULL;
> @@ -1632,8 +1633,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>
> /* we verify if the enable bit is set... */
> - if (!(data & 1))
> + if (!(data & 1)) {
> + vcpu->arch.time = 0;
Should we not just assign data to vcpu->arch.time? That's how the real
MSRs work.
> break;
> + }
>
> /* ...but clean it before doing the actual write */
> vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
> @@ -1641,8 +1644,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> vcpu->arch.time_page =
> gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
>
> - if (is_error_page(vcpu->arch.time_page))
> + if (is_error_page(vcpu->arch.time_page)) {
> vcpu->arch.time_page = NULL;
> + vcpu->arch.time = 0;
> + }
>
Don't see why.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: KVM: x86: do not retain disabled or invalid pvclock address
2012-09-09 14:08 ` Avi Kivity
@ 2012-09-09 23:37 ` Marcelo Tosatti
0 siblings, 0 replies; 3+ messages in thread
From: Marcelo Tosatti @ 2012-09-09 23:37 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm
On Sun, Sep 09, 2012 at 05:08:31PM +0300, Avi Kivity wrote:
> On 08/23/2012 02:16 PM, Marcelo Tosatti wrote:
> >
> > In case an invalid or disabled gpa is written to the SYSTEM_TIME
> > MSR, do not retain its value. This is not documented behaviour,
> > nor should be supported.
> >
> > Also clear it on system reset. Not doing so can hide bugs.
> >
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e00050c..ed4bfb7 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1528,6 +1528,7 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
> >
> > static void kvmclock_reset(struct kvm_vcpu *vcpu)
> > {
> > + vcpu->arch.time = 0;
> > if (vcpu->arch.time_page) {
> > kvm_release_page_dirty(vcpu->arch.time_page);
> > vcpu->arch.time_page = NULL;
> > @@ -1632,8 +1633,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> > kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> >
> > /* we verify if the enable bit is set... */
> > - if (!(data & 1))
> > + if (!(data & 1)) {
> > + vcpu->arch.time = 0;
>
> Should we not just assign data to vcpu->arch.time? That's how the real
> MSRs work.
>
> > break;
> > + }
> >
> > /* ...but clean it before doing the actual write */
> > vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
> > @@ -1641,8 +1644,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> > vcpu->arch.time_page =
> > gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
> >
> > - if (is_error_page(vcpu->arch.time_page))
> > + if (is_error_page(vcpu->arch.time_page)) {
> > vcpu->arch.time_page = NULL;
> > + vcpu->arch.time = 0;
> > + }
> >
>
> Don't see why.
This would cause problems later with the new msr. But you're right,
there is nothing incorrect in the current behaviour.
Will keep these changes where they have more context and meaning.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-09-09 23:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-23 11:16 KVM: x86: do not retain disabled or invalid pvclock address Marcelo Tosatti
2012-09-09 14:08 ` Avi Kivity
2012-09-09 23:37 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox