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 03:12:47AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 10:57, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > 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.
> 
> No, it doesn't. It adjusts the behavior only for the DM case (which
> isn't supposed to get information on other than the target domain,
> i.e. in this one specific case the very domain ID needs to be passed
> in).

int xc_domain_getinfo(xc_interface *xch,
                      uint32_t first_domid,
                      unsigned int max_doms,
                      xc_dominfo_t *info)
{
    unsigned int nr_doms;
    uint32_t next_domid = first_domid;
    DECLARE_DOMCTL;
    int rc = 0;

    memset(info, 0, max_doms*sizeof(xc_dominfo_t));

    for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
    {   
        domctl.cmd = XEN_DOMCTL_getdomaininfo;
        domctl.domain = (domid_t)next_domid;
        if ( (rc = do_domctl(xch, &domctl)) < 0 )
            break;
        info->domid      = (uint16_t)domctl.domain;
(...)
        next_domid = (uint16_t)domctl.domain + 1;


Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next valid
domain.

> Also, how is this comment of yours related to the remark above?

Because this is why domid is needed in returned structure - to know about which
domain you've got the info.

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