Re: [Xen-devel] PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 20:24, <dgdegra@xxxxxxxxxxxxx> wrote:
> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
> >>>>> On 22.06.16 at 16:13, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> >>>>>>> On 22.06.16 at 15:03, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> >>>>> I've finally found what was causing long standing issue of not working
> >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without
> >>>>> the other one in dom0). It looks to be this patch:
> >>>>>
> >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f
> >>>  
> >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
> >>>>>
> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
> >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with
> >>>>> EPERM in stubdomain.
> >>>>>
> >>>>> What would be the best solution for this? Allowing xc_domain_getinfo
> >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
> >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
> >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
> >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
> >>>>> implementing the logic from that commit solely in libxl?
> >>>>
> >>>> Once we fixed the quirky behavior of the current implementation
> >>>> (allowing information to be returned for other than the requested
> >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV.
> >>>
> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
> >>
> >> Which fix? I talked of one to be made.
> >>
> >>>> But let's ask Daniel explicitly. And in that context I then also wonder
> >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to
> >>>> the respective sysctl.
> >>>
> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h:
> >>>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
> >>>  - xsm_domctl (which enforce actual policy)
> >>>
> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be
> >>> useful to be able to just check if given domain is alive, without
> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
> >>> this useful also for any other inter-domain communication (for example
> >>> libxenvchan connection).
> >>>
> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
> >>> device-model domain is asking about its target domain, or calling domain
> >>> is xenstore domain/privileged domain. Right?
> >>
> >> Yes, that's what I think too.
> >>
> >>>  How to combine those
> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
> >>> usage of XSM_XS_PRIV)?
> > 
> > Changing the definition of XSM_XS_PRIV seems like the best solution, since
> > this is the only use.  I don't think it matters if the constant is renamed
> > to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used by a
> > caller, it could be removed and the actual logic placed in the switch
> > statement - that way it's clear this is a special case for getdomaininfo
> > instead of attempting to make this generic.
> > 
> > Either method works, and I agree allowing DM to invoke this domctl is both
> > useful and not going to introduce problems.  The getdomaininfo permission
> > will also need to be added to the device_model macro in xen.if.
> 
> What exactly this last sentence means I need to add I'm not sure
> about. Apart from that, how about the change below?
> 
> Jan
> 
> domctl: relax getdomaininfo permissions
> 
> Qemu needs access to this for the domain it controls, both due to it
> being used by xc_domain_memory_mapping() (which qemu calls) and the
> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().
> 
> This at once avoids a for_each_domain() loop when the ID of an
> existing domain gets passed in.
>
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?

Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
existing domain with ID >= passed one? Reading various comments in code
it looks to be used to domain enumeration. This patch changes this
behaviour.

> I further wonder why we have XSM_OTHER: The respective conversion into
> other XSM_* values in xsm/dummy.h could as well move into the callers,
> making intentions more obvious when looking at the actual code.
> 
> --- unstable.orig/xen/common/domctl.c
> +++ unstable/xen/common/domctl.c
> @@ -442,14 +442,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>      switch ( op->cmd )
>      {
>      case XEN_DOMCTL_createdomain:
> -    case XEN_DOMCTL_getdomaininfo:
>      case XEN_DOMCTL_test_assign_device:
>      case XEN_DOMCTL_gdbsx_guestmemio:
>          d = NULL;
>          break;
>      default:
>          d = rcu_lock_domain_by_id(op->domain);
> -        if ( d == NULL )
> +        if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
>              return -ESRCH;
>      }
>  
> @@ -863,14 +862,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>  
>      case XEN_DOMCTL_getdomaininfo:
>      {
> -        domid_t dom = op->domain;
> -
> -        rcu_read_lock(&domlist_read_lock);
> +        domid_t dom = DOMID_INVALID;
>  
> -        for_each_domain ( d )
> -            if ( d->domain_id >= dom )
> +        if ( !d )
> +        {
> +            ret = -EINVAL;
> +            if ( op->domain >= DOMID_FIRST_RESERVED )
>                  break;
>  
> +            rcu_read_lock(&domlist_read_lock);
> +
> +            dom = op->domain;
> +            for_each_domain ( d )
> +                if ( d->domain_id >= dom )
> +                    break;
> +        }
> +
>          ret = -ESRCH;
>          if ( d == NULL )
>              goto getdomaininfo_out;
> @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          copyback = 1;
>  
>      getdomaininfo_out:
> +        if ( dom == DOMID_INVALID )
> +            break;
> +
>          rcu_read_unlock(&domlist_read_lock);
>          d = NULL;
>          break;
> --- unstable.orig/xen/include/xsm/dummy.h
> +++ unstable/xen/include/xsm/dummy.h
> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>          return 0;
>      case XSM_TARGET:
>          if ( src == target )
> +        {
>              return 0;
> +    case XSM_XS_PRIV:
> +            if ( src->is_xenstore )
> +                return 0;
> +        }
>          /* fall through */
>      case XSM_DM_PRIV:
>          if ( target && src->target == target )
> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>          if ( src->is_privileged )
>              return 0;
>          return -EPERM;
> -    case XSM_XS_PRIV:
> -        if ( src->is_xenstore || src->is_privileged )
> -            return 0;
> -        return -EPERM;
>      default:
>          LINKER_BUG_ON(1);
>          return -EPERM;
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel