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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/06/16 09:32, 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?

This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy
semantics, which go out of their way to make it easy to get wrong.

It is important that you always check the returned domid, as it may not
be the domain you asked for.  In particular, if a domain you are looking
after dies, the adjacent domain's information will be returnned.

Also, noone has yet addressed the issue that, strictly speaking,
xen_domctl_getdomaininfo_t is versioned by both the DOMCTL and the
SYSCTL interface version.  This in particular makes things interesting
for valgrind support.

~Andrew

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