From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arianna Avanzini Subject: Re: [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory Date: Sat, 10 May 2014 03:10:24 +0200 Message-ID: <536D7C80.6020504@gmail.com> References: <1399305254-3695-1-git-send-email-avanzini.arianna@gmail.com> <1399305254-3695-11-git-send-email-avanzini.arianna@gmail.com> <5368C22B020000780000F3C9@mail.emea.novell.com> Reply-To: Arianna Avanzini Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5368C22B020000780000F3C9@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Ian.Campbell@eu.citrix.com, paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, julien.grall@citrix.com, etrudeau@broadcom.com, tim@xen.org, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org On 05/06/2014 11:06 AM, Jan Beulich wrote: >>>> On 05.05.14 at 17:54, wrote: >> tools/libxl/libxl_create.c | 17 ++++++++++++++++ >> tools/libxl/libxl_pci.c | 26 +++++++++-------------- >> xen/common/domctl.c | 51 +++++++++++++++++----------------------------- > > First of all - is it (from a functionality pov) really necessary to mix > tools and hypervisor changes here. > I think it's not. Thank you for pointing that out, I will split the patch according to your suggestion. >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -1149,6 +1149,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, >> libxl__spawn_stub_dm(egc, &dcs->dmss); >> else >> libxl__spawn_local_dm(egc, &dcs->dmss.dm); >> + >> + /* >> + * If VGA passthru is enabled by domain config, be sure that the >> + * domain can access VGA-related iomem regions. >> + */ >> + if (d_config->b_info.u.hvm.gfx_passthru.val) { >> + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT; >> + ret = xc_domain_iomem_permission(CTX->xch, domid, >> + vga_iomem_start, 0x20, 1); >> + if (ret < 0) { >> + LOGE(ERROR, >> + "failed to give dom%d access to iomem range " >> + "%"PRIx64"-%"PRIx64" for VGA passthru", >> + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1)); >> + goto error_out; >> + } >> + } > > While you added some text to the description regarding this change, > it still remains unclear why this is really needed, since no other code > is being removed in its place. So my minimum expectation here would > be for you to point out which code this replaces/duplicates, and why > the original needs to remain in place. > Right, the commit description is still very confused, thank you for pointing that out. QEMU seems to rely only on the memory_mapping domctl to map VGA-related memory areas in case gfx passthru is enabled, if I'm seeing things correctly. The xc_domain_memory_mapping() function is invoked from the register_vga_regions() function (defined in hw/pt-graphics.c). The latter function seems to be executed upon registration of a physical device (register_real_device() -> pt_register_regions() in hw/pass-through.c), and to be actually executed only if gfx passthru is enabled for the device (if it is not, the function seems to immediately return without performing any mapping operation of I/O memory or ports). Since this patch aims at separating the functions of the memory_mapping and iomem_permission domctls, memory_mapping does not implicitly grants permission to mapped I/O-memory ranges; having QEMU invoking just the memory_mapping domctl would lead to a failure. This hunk was supposed to allow to the domain access to the necessary VGA-related memory ranges in case gfx passthru is enabled by domain configuration. > Furthermore (I'm not sure if the same mistake is being done > elsewhere, you may not be the original one to blame for this) I think > it is a mistake to mix up VGA and graphics pass-through: When you > want a graphics card (usually the primary one) to behave as VGA, it > needs the legacy VGA memory and I/O port ranges to be under its > control. Any other (usually secondary) card would not need this, as > can be easily seen by the otherwise resulting conflict if you pass > through multiple graphics cards to distinct domains. > I see, thank you for the clear explanation. Unfortunately I just relied on the QEMU code, which seems to execute the function (and map those memory ranges) for all devices, provided that gfx passthru is enabled and there is a VGA controller. I most probably overlooked something. >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -803,18 +803,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) >> { >> unsigned long mfn = op->u.iomem_permission.first_mfn; >> unsigned long nr_mfns = op->u.iomem_permission.nr_mfns; >> + unsigned long mfn_end = mfn + nr_mfns - 1; >> int allow = op->u.iomem_permission.allow_access; >> >> ret = -EINVAL; >> - if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */ >> + if ( mfn_end < mfn ) /* wrap? */ >> break; >> >> - if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) ) >> - ret = -EPERM; >> - else if ( allow ) >> - ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1); >> + ret = -EPERM; >> + if ( !iomem_access_permitted(current->domain, mfn, mfn_end) || >> + xsm_iomem_permission(XSM_HOOK, d, mfn, mfn_end, allow) ) >> + break; >> + >> + if ( allow ) >> + ret = iomem_permit_access(d, mfn, mfn_end); >> else > > I'd prefer this to remain an if/else-if sequence, like done in > http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg03988.html. > Perhaps that patch (once I get to submit v2) could serve as a > prereq for this change of yours? > Certainly, thank you for the pointer. >> @@ -838,7 +842,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) >> break; >> >> ret = -EPERM; >> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ) >> + if ( !iomem_access_permitted(d, mfn, mfn_end) ) > > I'm not sure if we shouldn't rather be conservative here and check > both domains' permissions. > Sure, thank you for the suggestion. > And now that I reached the end of the patch I'm still missing the > similar adjustments for I/O port handling, while I think I saw you > claim somewhere that in this version you did deal with that. > The previous version of the patch gave access permission to I/O ports (in do_pci_add()) only to paravirtualized domains. This version of the patch executes also for HVM domains all the code that was previously executed only for PV domains, including the invocation of ioport_permission. As Ian Campbell suggested (and hoping I understood his suggestion correctly), it does: if (hvm) device_model_related_code(); previously_pv_specific_code(); > Jan > -- /* * Arianna Avanzini * avanzini.arianna@gmail.com * 73628@studenti.unimore.it */