RE: [PATCH v7 09/10] xen/vmtrace: support for VM forks

["Lengyel, Tamas" <tamas.lengyel@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, January 26, 2021 9:22 AM
> To: Cooper, Andrew <andrew.cooper3@xxxxxxxxxx>
> Cc: Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>; Roger Pau Monné
> <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Nakajima, Jun
> <jun.nakajima@xxxxxxxxx>; Tian, Kevin <kevin.tian@xxxxxxxxx>; Michał
> Leszczyński <michal.leszczynski@xxxxxxx>; Tamas K Lengyel
> <tamas@xxxxxxxxxxxxx>; Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v7 09/10] xen/vmtrace: support for VM forks
> 
> On 21.01.2021 22:27, Andrew Cooper wrote:
> > From: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> >
> > Implement vmtrace_reset_pt function. Properly set IPT state for VM
> > forks.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> albeit it strikes me as
> somewhat odd that ...
> 
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2408,6 +2408,16 @@ static int vmtrace_output_position(struct vcpu
> *v, uint64_t *pos)
> >      return v->arch.hvm.vmx.ipt_active;  }
> >
> > +static int vmtrace_reset(struct vcpu *v) {
> > +    if ( !v->arch.hvm.vmx.ipt_active )
> > +        return -EINVAL;
> > +
> > +    v->arch.msrs->rtit.output_offset = 0;
> > +    v->arch.msrs->rtit.status = 0;
> > +    return 0;
> > +}
> 
> ... the function/hook return non-void, yet ...
> 
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1632,6 +1632,8 @@ static int copy_vcpu_settings(struct domain *cd,
> const struct domain *d)
> >              copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn);
> >          }
> >
> > +        hvm_vmtrace_reset(cd_vcpu);
> 
> ... the only caller doesn't care.

Noted. For now the function could just return void since if IPT is not enabled 
we don't care from a fork reset perspective. But perhaps in the future we want 
to return that information to the toolstack. Haven't decided yet whether that's 
important or not, for now things are OK as-is.

Tamas