From: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>,
Jan Beulich <JBeulich@suse.com>,
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 16:59:43 +0200 [thread overview]
Message-ID: <20160623145943.GF410@mail-itl> (raw)
In-Reply-To: <4b6d4a3a-fa46-bee9-ec38-3c7b2fe34a7b@citrix.com>
[-- Attachment #1.1: Type: text/plain, Size: 6627 bytes --]
On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote:
> On 23/06/16 14:25, Marek Marczykowski-Górecki wrote:
> > On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
> >>>>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> wrote:
> >>> 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@invisiblethingslab.com> wrote:
> >>>>>> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >>>>>>> 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?
> >> How that? There is this third hunk:
> > Ok, after drawing a flowchart of the control in this function after your
> > change, on a piece of paper, this case looks fine. But depending on how
> > the domain was found (explicit loop or rcu_lock_domain_by_id), different
> > locks are held, which makes it harder to follow what is going on.
> >
> > Crazy idea: how about making the code easy/easier to read instead of
> > obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is
> > convolved enough. How about this version (2 patches):
> >
> > xen: move domain lookup for getdomaininfo to the same
> >
> > XEN_DOMCTL_getdomaininfo have different semantics than most of others
> > domctls - it returns information about first valid domain with ID >=
> > argument. But that's no excuse for having the lookup done in a different
> > place, which made handling different corner cases unnecessary complex.
> > Move the lookup to the first switch clause. And adjust locking to be the
> > same as for other cases.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>
> FWIW, I prefer this solution to the issue.
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with a few style
> nits.
Fixed patch according to your comments:
xen: move domain lookup for getdomaininfo to the same
XEN_DOMCTL_getdomaininfo have different semantics than most of others
domctls - it returns information about first valid domain with ID >=
argument. But that's no excuse for having the lookup code in a different
place, which made handling different corner cases unnecessary complex.
Move the lookup to the first switch clause. And adjust locking to be the
same as for other cases.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
xen/common/domctl.c | 44 +++++++++++++++++++++++---------------------
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e43904e..41de3e8 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
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;
+
+ case XEN_DOMCTL_getdomaininfo:
+ d = rcu_lock_domain_by_id(op->domain);
+
+ if ( d == NULL )
+ {
+ /* Search for the next available domain. */
+ rcu_read_lock(&domlist_read_lock);
+
+ for_each_domain ( d )
+ if ( d->domain_id >= op->domain )
+ {
+ rcu_lock_domain(d);
+ break;
+ }
+
+ rcu_read_unlock(&domlist_read_lock);
+ if ( d == NULL )
+ return -ESRCH;
+ }
+ break;
+
default:
d = rcu_lock_domain_by_id(op->domain);
if ( d == NULL )
@@ -862,33 +883,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
break;
case XEN_DOMCTL_getdomaininfo:
- {
- domid_t dom = op->domain;
-
- rcu_read_lock(&domlist_read_lock);
-
- for_each_domain ( d )
- if ( d->domain_id >= dom )
- break;
-
- ret = -ESRCH;
- if ( d == NULL )
- goto getdomaininfo_out;
-
ret = xsm_getdomaininfo(XSM_HOOK, d);
if ( ret )
- goto getdomaininfo_out;
+ break;
getdomaininfo(d, &op->u.getdomaininfo);
-
op->domain = op->u.getdomaininfo.domain;
copyback = 1;
-
- getdomaininfo_out:
- rcu_read_unlock(&domlist_read_lock);
- d = NULL;
break;
- }
case XEN_DOMCTL_getvcpucontext:
{
--
2.5.5
--
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-06-23 14:59 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
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 [this message]
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=20160623145943.GF410@mail-itl \
--to=marmarek@invisiblethingslab.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=dgdegra@tycho.nsa.gov \
--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).