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 11:18:24 +0200 Message-ID: <20160623091824.GH1593@mail-itl> References: <20160622130335.GA410@mail-itl> <576AB3B102000078000F7B53@prv-mh.provo.novell.com> <20160622141314.GD1593@mail-itl> <576AC98102000078000F7BDE@prv-mh.provo.novell.com> <576BBABD02000078000F7F14@prv-mh.provo.novell.com> <20160623085706.GG1593@mail-itl> <576BC42F02000078000F7F94@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0654769006359076397==" Return-path: In-Reply-To: <576BC42F02000078000F7F94@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Daniel De Graaf , xen-devel List-Id: xen-devel@lists.xenproject.org --===============0654769006359076397== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="T7HNq/m++EF3pl6p" Content-Disposition: inline --T7HNq/m++EF3pl6p Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: > >> >>> On 22.06.16 at 20:24, wrote: > >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: > >> >>>>> On 22.06.16 at 16:13, wrote: > >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >> >>>>>>> On 22.06.16 at 15:03, wrote: > >> >>>>> I've finally found what was causing long standing issue of not w= orking > >> >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - = without > >> >>>>> the other one in dom0). It looks to be this patch: > >> >>>>> > >> >>>=20 > > http://xenbits.xen.org/gitweb/?p=3Dxen.git;a=3Dcommitdiff;h=3Dc428c9f16= 2895cb3473f=20 > >> >>>>> ab26d23ffbf41a6f293d;hp=3Ddcccaf806a92eabb95929a67c344ac1e9ead62= 57 > >> >>>>> > >> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to che= ck if > >> >>>>> the target domain is auto-translated), but xc_domain_getinfo fai= ls with > >> >>>>> EPERM in stubdomain. > >> >>>>> > >> >>>>> What would be the best solution for this? Allowing xc_domain_get= info > >> >>>>> 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 hav= e 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, pos= sibly > >> >>>>> 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 exa= mple > >> >>> 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)? > >> >=20 > >> > 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 r= enamed > >> > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever us= ed 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 getdomain= info > >> > instead of attempting to make this generic. > >> >=20 > >> > Either method works, and I agree allowing DM to invoke this domctl i= s both > >> > useful and not going to introduce problems. The getdomaininfo permi= ssion > >> > will also need to be added to the device_model macro in xen.if. > >>=20 > >> What exactly this last sentence means I need to add I'm not sure > >> about. Apart from that, how about the change below? > >>=20 > >> Jan > >>=20 > >> domctl: relax getdomaininfo permissions > >>=20 > >> 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(). > >>=20 > >> This at once avoids a for_each_domain() loop when the ID of an > >> existing domain gets passed in. > >> > >> Reported-by: Marek Marczykowski-G=C3=B3recki > >> --- > >> 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? > >=20 > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first > > existing domain with ID >=3D passed one? Reading various comments in co= de > > it looks to be used to domain enumeration. This patch changes this > > behaviour. >=20 > 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 next val= id domain. > 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 w= hich domain you've got the info. --=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? --T7HNq/m++EF3pl6p Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXa6lhAAoJENuP0xzK19csw5wH/0tuxPQA9CdWonqTNW3V4pEY jtk1J4klYfbDt42TtVxec2AJj0qwjgmzwre3qaSAtX72g9deSEHrBs+hMziKY0xY 2e3VKk21LaVelpeapOKnDzWqlnWZ45zO6XoYPWiCSKL7xQjEuYAiTQ6LNWpOIdL0 a242Ldu2g5mkybNEMSBi5Bssgt0VDvGnU5HTLUcEhVZDY63x4LQir2KZpPUh2wtK JRZw/XbaYEQ1tWpFOdEf2WgRiYYsRXvwG2BnsZfjQKd6ZrKPWgGz9ZfhBAPy87pG xZwIWutL0Khs6+kM83r6QDzeTHZpB290ypQKDe8t2wmtE5qkCHu07CGYmrpn85A= =x16y -----END PGP SIGNATURE----- --T7HNq/m++EF3pl6p-- --===============0654769006359076397== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============0654769006359076397==--