Re: [Xen-devel] [PATCH v9 02/20] x86/VPMU: Manage VPMU_CONTEXT_SAVE flag in vpmu_save_force()

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 08/11/2014 09:28 AM, Jan Beulich wrote:
> > > > On 08.08.14 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > vpmu_load() may leave VPMU_CONTEXT_SAVE set after calling vpmu_save_force() if
> > the context is not loaded.
> To me this is ambiguous (and looking at the patch I can't really
> resolve it): Do you mean to say this can validly happen, or that it
> could have happened in error prior to the patch? In any event
> please alter the description to either clearly state what the wrong
> old behavior was, or what the correct new behavior is to be.
What I was trying to say (not particularly successfully, apparently) is 
that currently (i.e. prior to the patch) we may end up not clearing 
VPMU_CONTEXT_SAVE flag after calling vpmu_save_forced(). And that would 
be a bug, which this patch is fixing.
(It doesn't help that the diff doesn't show that the flag is cleared 
right after ->arch_vpmu_save() below)
-boris

> Jan
>
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > ---
> >   xen/arch/x86/hvm/vpmu.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> > index 63765fa..26e971b 100644
> > --- a/xen/arch/x86/hvm/vpmu.c
> > +++ b/xen/arch/x86/hvm/vpmu.c
> > @@ -130,6 +130,8 @@ static void vpmu_save_force(void *arg)
> >       if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
> >           return;
> >   
> > +    vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> > +
> >       if ( vpmu->arch_vpmu_ops )
> >           (void)vpmu->arch_vpmu_ops->arch_vpmu_save(v);
> >   
> > @@ -178,7 +180,6 @@ void vpmu_load(struct vcpu *v)
> >            */
> >           if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
> >           {
> > -            vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> >               on_selected_cpus(cpumask_of(vpmu->last_pcpu),
> >                                vpmu_save_force, (void *)v, 1);
> >               vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> > @@ -195,7 +196,6 @@ void vpmu_load(struct vcpu *v)
> >           vpmu = vcpu_vpmu(prev);
> >   
> >           /* Someone ran here before us */
> > -        vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> >           vpmu_save_force(prev);
> >           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> >   
> > --
> > 1.8.1.4

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel