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 11:18:24AM +0200, Marek Marczykowski-Górecki wrote:
> 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.

Hmm, looks like I've misread you patch. Reading again...

But now I see rcu_read_lock(&domlist_read_lock) is gets called only when
looping over domains, but rcu_read_unlock is called in any case. Is it
correct?

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