From: "Jan Beulich" <JBeulich@suse.com>
To: marmarek@invisiblethingslab.com, Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: xen-devel <xen-devel@lists.xen.org>
Subject: Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"
Date: Thu, 23 Jun 2016 02:32:29 -0600 [thread overview]
Message-ID: <576BBABD02000078000F7F14@prv-mh.provo.novell.com> (raw)
In-Reply-To: <b7646251-42f1-10a0-18ec-5d799b72c4c7@tycho.nsa.gov>
>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
> On 06/22/2016 11:23 AM, Jan Beulich wrote:
>>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
>>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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@invisiblethingslab.com>
---
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?
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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-06-23 8:32 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-22 13:03 PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" Marek Marczykowski-Górecki
2016-06-22 13:50 ` Jan Beulich
2016-06-22 14:13 ` Marek Marczykowski-Górecki
2016-06-22 15:23 ` Jan Beulich
2016-06-22 18:24 ` Daniel De Graaf
2016-06-23 8:32 ` Jan Beulich [this message]
2016-06-23 8:39 ` Jan Beulich
2016-06-23 14:33 ` Daniel De Graaf
2016-06-23 8:57 ` Marek Marczykowski-Górecki
2016-06-23 9:12 ` Jan Beulich
2016-06-23 9:18 ` Marek Marczykowski-Górecki
2016-06-23 9:23 ` Marek Marczykowski-Górecki
2016-06-23 9:46 ` Jan Beulich
2016-06-23 13:25 ` Marek Marczykowski-Górecki
2016-06-23 14:12 ` Andrew Cooper
2016-06-23 14:59 ` Marek Marczykowski-Górecki
2016-06-23 15:01 ` Andrew Cooper
2016-06-23 14:45 ` Jan Beulich
2016-06-23 15:00 ` Daniel De Graaf
2016-06-23 15:22 ` Marek Marczykowski-Górecki
2016-06-23 15:30 ` Daniel De Graaf
2016-06-23 15:37 ` Jan Beulich
2016-06-23 15:45 ` Marek Marczykowski-Górecki
2016-06-23 15:49 ` Marek Marczykowski-Górecki
2016-06-23 16:02 ` Jan Beulich
2016-06-23 9:45 ` Jan Beulich
2016-06-23 9:44 ` Andrew Cooper
2016-06-23 9:50 ` Jan Beulich
2016-06-23 14:15 ` Andrew Cooper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=576BBABD02000078000F7F14@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=dgdegra@tycho.nsa.gov \
--cc=marmarek@invisiblethingslab.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).