[PATCH] release kvmclock page on reset

[PATCH] release kvmclock page on reset

[Glauber Costa]

Up to know, we were relying on guest cooperation to turn off kvmclock.
I just realized that even though this is fine and nice, a more robust
method is to (also) turn it off on vcpu_reset on the hypervisor side.
This will protect us against reboots, and we don't expect the guest
to reset its cpu during normal operation anyway.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 arch/x86/kvm/x86.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bcc0efc..38b55b3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5878,6 +5878,11 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	vcpu->arch.apf.msr_val = 0;
 
+	if (vcpu->arch.time_page) {
+		kvm_release_page_dirty(vcpu->arch.time_page);
+		vcpu->arch.time_page = NULL;
+	}
+
 	kvm_clear_async_pf_completion_queue(vcpu);
 	kvm_async_pf_hash_reset(vcpu);
 	vcpu->arch.apf.halted = false;
-- 
1.7.2.3

Re: [PATCH] release kvmclock page on reset

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On 2011-01-28 20:48, Glauber Costa wrote:
> Up to know, we were relying on guest cooperation to turn off kvmclock.
> I just realized that even though this is fine and nice, a more robust
> method is to (also) turn it off on vcpu_reset on the hypervisor side.
> This will protect us against reboots, and we don't expect the guest
> to reset its cpu during normal operation anyway.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  arch/x86/kvm/x86.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bcc0efc..38b55b3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5878,6 +5878,11 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
>  	vcpu->arch.apf.msr_val = 0;
>  
> +	if (vcpu->arch.time_page) {
> +		kvm_release_page_dirty(vcpu->arch.time_page);
> +		vcpu->arch.time_page = NULL;
> +	}
> +

kvm_arch_vcpu_reset is only called on vcpu setup and when it receives a
sipi (provided in-kernel irqchip is in use). If you want this page to be
consistently reset on guest reboot, you have to trigger this from user
space. But I thought we are doing this already in qemu, don't we?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] release kvmclock page on reset

[Glauber Costa]

On Fri, 2011-01-28 at 22:09 +0100, Jan Kiszka wrote:
> On 2011-01-28 20:48, Glauber Costa wrote:
> > Up to know, we were relying on guest cooperation to turn off kvmclock.
> > I just realized that even though this is fine and nice, a more robust
> > method is to (also) turn it off on vcpu_reset on the hypervisor side.
> > This will protect us against reboots, and we don't expect the guest
> > to reset its cpu during normal operation anyway.
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  arch/x86/kvm/x86.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index bcc0efc..38b55b3 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5878,6 +5878,11 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
> >  	kvm_make_request(KVM_REQ_EVENT, vcpu);
> >  	vcpu->arch.apf.msr_val = 0;
> >  
> > +	if (vcpu->arch.time_page) {
> > +		kvm_release_page_dirty(vcpu->arch.time_page);
> > +		vcpu->arch.time_page = NULL;
> > +	}
> > +
> 
> kvm_arch_vcpu_reset is only called on vcpu setup and when it receives a
> sipi (provided in-kernel irqchip is in use). If you want this page to be
> consistently reset on guest reboot, you have to trigger this from user
> space. But I thought we are doing this already in qemu, don't we?

Humm, you might as well be right regarding reboots.
But in the end, it doesn't affect correctness here. If we're resetting
the vcpu, we should not let that kind of data live.

Re: [PATCH] release kvmclock page on reset

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On 2011-01-29 03:07, Glauber Costa wrote:
> On Fri, 2011-01-28 at 22:09 +0100, Jan Kiszka wrote:
>> On 2011-01-28 20:48, Glauber Costa wrote:
>>> Up to know, we were relying on guest cooperation to turn off kvmclock.
>>> I just realized that even though this is fine and nice, a more robust
>>> method is to (also) turn it off on vcpu_reset on the hypervisor side.
>>> This will protect us against reboots, and we don't expect the guest
>>> to reset its cpu during normal operation anyway.
>>>
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>> ---
>>>  arch/x86/kvm/x86.c |    5 +++++
>>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index bcc0efc..38b55b3 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -5878,6 +5878,11 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
>>>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
>>>  	vcpu->arch.apf.msr_val = 0;
>>>  
>>> +	if (vcpu->arch.time_page) {
>>> +		kvm_release_page_dirty(vcpu->arch.time_page);
>>> +		vcpu->arch.time_page = NULL;
>>> +	}
>>> +
>>
>> kvm_arch_vcpu_reset is only called on vcpu setup and when it receives a
>> sipi (provided in-kernel irqchip is in use). If you want this page to be
>> consistently reset on guest reboot, you have to trigger this from user
>> space. But I thought we are doing this already in qemu, don't we?
> 
> Humm, you might as well be right regarding reboots.
> But in the end, it doesn't affect correctness here. If we're resetting
> the vcpu, we should not let that kind of data live.
> 

Right, just checked that we reset other states like nmi_pending or
async_pf here as well. So doing the same for the time_page looks
appropriate.

But I think you should encapsulate the pattern above in a function and
substitute other occurrences at this chance. Also, the changelog should
clarify in which cases the code matters.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] release kvmclock page on reset

[Glauber Costa]

On Sat, 2011-01-29 at 09:56 +0100, Jan Kiszka wrote:
> On 2011-01-29 03:07, Glauber Costa wrote:
> > On Fri, 2011-01-28 at 22:09 +0100, Jan Kiszka wrote:
> >> On 2011-01-28 20:48, Glauber Costa wrote:
> >>> Up to know, we were relying on guest cooperation to turn off kvmclock.
> >>> I just realized that even though this is fine and nice, a more robust
> >>> method is to (also) turn it off on vcpu_reset on the hypervisor side.
> >>> This will protect us against reboots, and we don't expect the guest
> >>> to reset its cpu during normal operation anyway.
> >>>
> >>> Signed-off-by: Glauber Costa <glommer@redhat.com>
> >>> ---
> >>>  arch/x86/kvm/x86.c |    5 +++++
> >>>  1 files changed, 5 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >>> index bcc0efc..38b55b3 100644
> >>> --- a/arch/x86/kvm/x86.c
> >>> +++ b/arch/x86/kvm/x86.c
> >>> @@ -5878,6 +5878,11 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
> >>>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
> >>>  	vcpu->arch.apf.msr_val = 0;
> >>>  
> >>> +	if (vcpu->arch.time_page) {
> >>> +		kvm_release_page_dirty(vcpu->arch.time_page);
> >>> +		vcpu->arch.time_page = NULL;
> >>> +	}
> >>> +
> >>
> >> kvm_arch_vcpu_reset is only called on vcpu setup and when it receives a
> >> sipi (provided in-kernel irqchip is in use). If you want this page to be
> >> consistently reset on guest reboot, you have to trigger this from user
> >> space. But I thought we are doing this already in qemu, don't we?
> > 
> > Humm, you might as well be right regarding reboots.
> > But in the end, it doesn't affect correctness here. If we're resetting
> > the vcpu, we should not let that kind of data live.
> > 
> 
> Right, just checked that we reset other states like nmi_pending or
> async_pf here as well. So doing the same for the time_page looks
> appropriate.
> 
> But I think you should encapsulate the pattern above in a function and
> substitute other occurrences at this chance. Also, the changelog should
> clarify in which cases the code matters.

Fair. I will respin this today.

> Jan
>