From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= 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 Message-ID: <20160623145943.GF410@mail-itl> References: <576AC98102000078000F7BDE@prv-mh.provo.novell.com> <576BBABD02000078000F7F14@prv-mh.provo.novell.com> <20160623085706.GG1593@mail-itl> <576BC42F02000078000F7F94@prv-mh.provo.novell.com> <20160623091824.GH1593@mail-itl> <20160623092353.GI1593@mail-itl> <576BCC2602000078000F7FC9@prv-mh.provo.novell.com> <20160623132551.GE410@mail-itl> <4b6d4a3a-fa46-bee9-ec38-3c7b2fe34a7b@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5344767843569514601==" Return-path: In-Reply-To: <4b6d4a3a-fa46-bee9-ec38-3c7b2fe34a7b@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Andrew Cooper Cc: Daniel De Graaf , Jan Beulich , xen-devel List-Id: xen-devel@lists.xenproject.org --===============5344767843569514601== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="tmoQ0UElFV5VgXgH" Content-Disposition: inline --tmoQ0UElFV5VgXgH Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote: > On 23/06/16 14:25, Marek Marczykowski-G=C3=B3recki wrote: > > On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote: > >>>>> On 23.06.16 at 11:23, wrote: > >>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-G=C3=B3r= ecki wrote: > >>>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: > >>>>>>>> On 23.06.16 at 10:57, 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 >=3D 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 =3D first_domid; > >>>> DECLARE_DOMCTL; > >>>> int rc =3D 0; > >>>> > >>>> memset(info, 0, max_doms*sizeof(xc_dominfo_t)); > >>>> > >>>> for ( nr_doms =3D 0; nr_doms < max_doms; nr_doms++ ) > >>>> { =20 > >>>> domctl.cmd =3D XEN_DOMCTL_getdomaininfo; > >>>> domctl.domain =3D (domid_t)next_domid; > >>>> if ( (rc =3D do_domctl(xch, &domctl)) < 0 ) > >>>> break; > >>>> info->domid =3D (uint16_t)domctl.domain; > >>>> (...) > >>>> next_domid =3D (uint16_t)domctl.domain + 1; > >>>> > >>>> > >>>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning n= ext=20 > >>> 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 w= hen > >>> 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 >=3D > > 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=C3=B3recki >=20 > FWIW, I prefer this solution to the issue. >=20 > Reviewed-by: Andrew Cooper , 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 >=3D 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=C3=B3recki --- 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 =3D NULL; break; + + case XEN_DOMCTL_getdomaininfo: + d =3D rcu_lock_domain_by_id(op->domain); + + if ( d =3D=3D NULL ) + { + /* Search for the next available domain. */ + rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + if ( d->domain_id >=3D op->domain ) + { + rcu_lock_domain(d); + break; + } + + rcu_read_unlock(&domlist_read_lock); + if ( d =3D=3D NULL ) + return -ESRCH; + } + break; + default: d =3D rcu_lock_domain_by_id(op->domain); if ( d =3D=3D NULL ) @@ -862,33 +883,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u= _domctl) break; =20 case XEN_DOMCTL_getdomaininfo: - { - domid_t dom =3D op->domain; - - rcu_read_lock(&domlist_read_lock); - - for_each_domain ( d ) - if ( d->domain_id >=3D dom ) - break; - - ret =3D -ESRCH; - if ( d =3D=3D NULL ) - goto getdomaininfo_out; - ret =3D xsm_getdomaininfo(XSM_HOOK, d); if ( ret ) - goto getdomaininfo_out; + break; =20 getdomaininfo(d, &op->u.getdomaininfo); - op->domain =3D op->u.getdomaininfo.domain; copyback =3D 1; - - getdomaininfo_out: - rcu_read_unlock(&domlist_read_lock); - d =3D NULL; break; - } =20 case XEN_DOMCTL_getvcpucontext: { --=20 2.5.5 --=20 Best Regards, Marek Marczykowski-G=C3=B3recki 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? --tmoQ0UElFV5VgXgH Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXa/lgAAoJENuP0xzK19csLJkH/j7O9R/RSDwrrgpx1z59bRYO Fn98Z4Xq3enZ5EMe1ucdqbIIg9VHSZkqe1BZSVoQra6tS7iVX1oai8o9RVvevktO YPE8kE622P7kJW84MTU0At2cZTTPrfikWITKPYnBlwK4BudfJg2vJcihkyOhmb+d I0IIWgYc2PPmhgkKxLGwph/KeXuDDjSfbcSzR98jkTaRXKie5/il4e15ulKHb2q9 x72bXwCO72mgT5DPlEaSJQTYvXJaWCIyIvxC73FsTpUpm0ctTp38E1pfnPgneA1t oWvtVVK4jtRsIjplIiq+86HMKx+Oaf9m8C2YoS+lsp9+DM3QzDwhPvfPzu+drlo= =ime0 -----END PGP SIGNATURE----- --tmoQ0UElFV5VgXgH-- --===============5344767843569514601== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============5344767843569514601==--