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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.16 at 11:18, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> 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.

Please consider the case of max_doms == 1, which is the only one
of interest here (and the only one getting tweaked).

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

I'm sorry, but no - this does not explain the duplication. Just look
at the code you quoted: It uses domctl.domain, but completely
ignores domctl.u.getdomaininfo.domain.

Jan

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