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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

Jan



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