Re: [Xen-devel] [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU

[Adrian Pop <apop@xxxxxxxxxxxxxxx>]

On Tue, Jul 31, 2018 at 05:53:00AM -0600, Jan Beulich wrote:
> > +    struct vcpu *v;
> > +
> > +    dom = rcu_lock_domain_by_id(domain_id);
> > +
> > +    for_each_vcpu( dom, v )
> > +    {
> > +        if ( vcpu_id == v->vcpu_id )
> > +        {
> > +            rcu_unlock_domain(dom);
> > +            return v;
> > +        }
> > +    }
> 
> for_each_vcpu() looks excessive here - all you need is a bounds
> check and an access into d->vcpus[]. Together with the fact
> that your caller has already identified and locked d I wonder
> whether this helper is needed in the first place.
 
All right.  I'll remove it altogether.

> > @@ -4576,26 +4599,32 @@ static int do_altp2m_op(
> >  
> >      case HVMOP_altp2m_vcpu_enable_notify:
> >      {
> > -        struct vcpu *curr = current;
> > +        struct vcpu *v;
> >          p2m_type_t p2mt;
> >  
> > -        if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
> > -             a.u.enable_notify.vcpu_id != curr->vcpu_id )
> > +        if ( a.u.enable_notify.pad )
> >          {
> >              rc = -EINVAL;
> >              break;
> >          }
> >  
> > -        if ( !gfn_eq(vcpu_altp2m(curr).veinfo_gfn, INVALID_GFN) ||
> > -             mfn_eq(get_gfn_query_unlocked(curr->domain,
> > +        v = __get_vcpu(a.domain, a.u.enable_notify.vcpu_id);
> > +        if ( !v )
> > +        {
> > +            rc = -EFAULT;
> 
> Hardly an appropriate error indicator for the condition.

I'll change it to -EINVAL.

> > +            break;
> > +        }
> > +
> > +        if ( !gfn_eq(vcpu_altp2m(v).veinfo_gfn, INVALID_GFN) ||
> > +             mfn_eq(get_gfn_query_unlocked(v->domain,
> >                      a.u.enable_notify.gfn, &p2mt), INVALID_MFN) )
> >          {
> >              rc = -EINVAL;
> >              break;
> >          }
> >  
> > -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> > -        altp2m_vcpu_update_vmfunc_ve(curr);
> > +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> > +        altp2m_vcpu_update_vmfunc_ve(v);
> 
> I'd like you to outline in the description how you mean an external
> agent to coordinate the use of this GFN with the guest (and in
> particular without in-guest agent).

I'll try to clarify this.

Thank you!

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel