Re: [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Thu, Sep 05, 2019 at 10:15:15AM +0800, Peter Xu wrote:
> On Wed, Sep 04, 2019 at 10:26:58AM -0700, Sean Christopherson wrote:
> > On Thu, Aug 15, 2019 at 06:34:56PM +0800, Peter Xu wrote:
> > > Tracing the ID helps to pair vmenters and vmexits for guests with
> > > multiple vCPUs.
> > > 
> > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  arch/x86/kvm/trace.h | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > > index b5c831e79094..c682f3f7f998 100644
> > > --- a/arch/x86/kvm/trace.h
> > > +++ b/arch/x86/kvm/trace.h
> > > @@ -232,17 +232,20 @@ TRACE_EVENT(kvm_exit,
> > >  		__field(	u32,	        isa             )
> > >  		__field(	u64,	        info1           )
> > >  		__field(	u64,	        info2           )
> > > +		__field(	int,	        vcpu_id         )
> > 
> > It doesn't actually affect anything, but vcpu_id is stored and printed as
> > an 'unsigned int' everywhere else in the trace code.  Stylistically I like
> > that approach even though struct kvm_vcpu holds it as a signed int.
> 
> True.  I can switch to unsigned int to get aligned with the rest if
> there's other comment to address.  Though from codebase-wise I would
> even prefer signed because it gives us a chance to set an invalid vcpu
> id (-1) where necessary, or notice something severly wrong when <-1.

I agree that a signed int makes sense for flows like kvm_get_vcpu_by_id(),
where the incoming id isn't sanitized.  But for struct kvm_vcpu, a negative 
vcpu_id is simply impossible, e.g. vcpu_id is set to a postive (and capped)
value as part of the core vcpu initialization and is never changed.  I
prefer storing an unsigned val in the tracing as it communicates that
vcpu_id is always valid.  I suspect struct kvm_vcpu uses a signed int
purely to avoid having to constantly cast it to an int.

> After all it should far cover our usage (IIUC max vcpu id should be
> 512 cross archs).