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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 25.07.18 at 13:49, <apop@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4467,6 +4467,30 @@ static int hvmop_get_param(
> >      return rc;
> >  }
> >
> > +/*
> > + * Find the struct vcpu given a dom_id and vcpu_id.
> > + * Return NULL if not found.
> > + */
> > +static struct vcpu *__get_vcpu(domid_t domain_id, uint32_t vcpu_id)
>
> No double leading underscores please, and avoid the use of fixed
> width types when you don't really need it (unsigned int is fine here).
>
> > +{
> > +    struct domain *dom;
>
> We commonly call this just d.
>
> > +    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.
>
> > @@ -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.
>
> > +            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).

Using #VE without an in-guest agent isn't really possible so this is
really just about designating the page from dom0 instead of doing it
with the in-guest agent. Something has to be present in the guest to
handle the new interrupts coming from #VE after all. I could image a
number of ways to coordinate that dom0 agent now setting this page
with the guest, my immediate guess is that it would be done through
the vm_event guest request channel. But it could also be done through
xenstore or even ssh.

Tamas

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