From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 75A50C433E0 for ; Mon, 15 Jun 2020 11:18:15 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 50FBF20679 for ; Mon, 15 Jun 2020 11:18:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 50FBF20679 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jkn7r-000073-7z; Mon, 15 Jun 2020 11:18:03 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jkn7q-00006r-1t for xen-devel@lists.xenproject.org; Mon, 15 Jun 2020 11:18:02 +0000 X-Inumbo-ID: df640ee6-aef9-11ea-b7bb-bc764e2007e4 Received: from esa6.hc3370-68.iphmx.com (unknown [216.71.155.175]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id df640ee6-aef9-11ea-b7bb-bc764e2007e4; Mon, 15 Jun 2020 11:18:01 +0000 (UTC) Authentication-Results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: ebrzSCwXQMMPYhKsmDkX2cBqaAc8BOt+LK8mHaBgYLg1+kE74OArTI1YVd1Ev8cYk00DH5stRZ yoY/ALUQFMjRsmGFKNt26LIP0aXRVOq4J3vL/0La/fWy9tKyafS936+MLuB21H16MT80KdTAlU gSomtmfLABc4L0oIh1z9DvIjLKdHuvX9fbmZeNeUKcbeECf9AiGdjmUwyUC+gXY7o4EsP0NAiD dVFuG6KBdBX/FpQqKdf+Z4SDHOg8GUtCP/pv1IiK7mlTQNSdGQzm7/l4DgJGP4e/wNvrMzuE7S Dyg= X-SBRS: 2.7 X-MesageID: 20391563 X-Ironport-Server: esa6.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.73,514,1583211600"; d="scan'208";a="20391563" Date: Mon, 15 Jun 2020 13:17:49 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Grzegorz Uriasz Subject: Re: [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain Message-ID: <20200615111749.GC735@Air-de-Roger> References: <87d74a21bde95cfc7c53fad56bf8f0e47724953e.1592171394.git.gorbak25@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <87d74a21bde95cfc7c53fad56bf8f0e47724953e.1592171394.git.gorbak25@gmail.com> X-ClientProxiedBy: AMSPEX02CAS01.citrite.net (10.69.22.112) To AMSPEX02CL02.citrite.net (10.69.22.126) X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Wei Liu , jakub@bartmin.ski, Ian Jackson , marmarek@invisiblethingslab.com, Anthony PERARD , j.nowak26@student.uw.edu.pl, xen-devel@lists.xenproject.org, contact@puzio.waw.pl Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On Sun, Jun 14, 2020 at 10:12:01PM +0000, Grzegorz Uriasz wrote: > When qemu is running inside a linux based stubdomain, qemu does not > have the necessary permissions to map the ioports to the target domain. > Currently, libxl is granting permissions only for the VGA RAM memory region > and not passing the required ioports. This patch grants the required > permission for the necessary vga io ports. > > Signed-off-by: Grzegorz Uriasz > --- > tools/libxl/libxl_pci.c | 99 ++++++++++++++++++++++++++++++----------- > 1 file changed, 73 insertions(+), 26 deletions(-) > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > index 957ff5c8e9..436190f790 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -2441,17 +2441,75 @@ void libxl__device_pci_destroy_all(libxl__egc *egc, uint32_t domid, > } > } > > +static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t domid) { > + int ret, i; Nit: i can be unsigned int since it's only used as a loop counter. > + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT; > + uint64_t vga_iomem_npages = 0x20; unsigned int is fine to store the size. > + uint32_t stubdom_domid = libxl_get_stubdom_id(CTX, domid); > + uint64_t vga_ioport_start[] = {0x3B0, 0x3C0}; > + uint64_t vga_ioport_size[] = {0xC, 0x20}; For IO ports ranges/sizes you can safely use unsigned ints, those only go up to 65535, and also constify the array since it's read-only. Can you expand a bit on where those ranges are taken from? Why not pass the whole 0x03B0-0x03DF range? > + > + /* VGA RAM */ > + ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid, > + vga_iomem_start, vga_iomem_npages, 1); > + if (ret < 0) { > + LOGED(ERROR, domid, > + "failed to give stubdom%d access to iomem range " > + "%"PRIx64"-%"PRIx64" for VGA passthru", > + stubdom_domid, > + vga_iomem_start, (vga_iomem_start + (vga_iomem_npages << XC_PAGE_SHIFT) - 1)); > + return ret; I think it would be better to return a libxl error code here: ERROR_FAIL. You already log the error code, and libxl functions have their own set of error codes. > + } > + ret = xc_domain_iomem_permission(CTX->xch, domid, > + vga_iomem_start, vga_iomem_npages, 1); > + if (ret < 0) { > + LOGED(ERROR, domid, > + "failed to give dom%d access to iomem range " > + "%"PRIx64"-%"PRIx64" for VGA passthru", > + domid, vga_iomem_start, (vga_iomem_start + (vga_iomem_npages << XC_PAGE_SHIFT) - 1)); > + return ret; > + } > + > + /* VGA IOPORTS */ > + for (i = 0 ; i < 2 ; i++) { Please use ARRAY_SIZE(vga_ioport_start) here. And I would also assert that both vga_ioport_start and vga_ioport_size arrays have the same sizes before the loop. > + ret = xc_domain_ioport_permission(CTX->xch, stubdom_domid, > + vga_ioport_start[i], vga_ioport_size[i], 1); > + if (ret < 0) { > + LOGED(ERROR, domid, > + "failed to give stubdom%d access to ioport range " > + "%"PRIx64"-%"PRIx64" for VGA passthru", > + stubdom_domid, > + vga_ioport_start[i], (vga_ioport_start[i] + vga_ioport_size[i] - 1)); > + return ret; > + } > + ret = xc_domain_ioport_permission(CTX->xch, domid, > + vga_ioport_start[i], vga_ioport_size[i], 1); > + if (ret < 0) { > + LOGED(ERROR, domid, > + "failed to give dom%d access to ioport range " > + "%"PRIx64"-%"PRIx64" for VGA passthru", > + domid, vga_ioport_start[i], (vga_ioport_start[i] + vga_ioport_size[i] - 1)); > + return ret; > + } > + } > + > + return 0; > +} > + > +static int libxl__grant_igd_opregion_permission(libxl__gc *gc, const uint32_t domid) { > + return 0; > +} > + > int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid, > libxl_domain_config *const d_config) > { > - int i, ret; > + int i, ret = 0; > + bool vga_found = false, igd_found = false; > > if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru)) > return 0; > > - for (i = 0 ; i < d_config->num_pcidevs ; i++) { > - uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT; > - uint32_t stubdom_domid; > + for (i = 0 ; i < d_config->num_pcidevs && !igd_found ; i++) { > libxl_device_pci *pcidev = &d_config->pcidevs[i]; > unsigned long pci_device_class; > > @@ -2460,30 +2518,19 @@ int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid, > if (pci_device_class != 0x030000) /* VGA class */ > continue; > > - stubdom_domid = libxl_get_stubdom_id(CTX, domid); > - ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid, > - vga_iomem_start, 0x20, 1); > - if (ret < 0) { > - LOGED(ERROR, domid, > - "failed to give stubdom%d access to iomem range " > - "%"PRIx64"-%"PRIx64" for VGA passthru", > - stubdom_domid, > - vga_iomem_start, (vga_iomem_start + 0x20 - 1)); > - return ret; > - } > - ret = xc_domain_iomem_permission(CTX->xch, domid, > - vga_iomem_start, 0x20, 1); > - if (ret < 0) { > - LOGED(ERROR, domid, > - "failed to give dom%d access to iomem range " > - "%"PRIx64"-%"PRIx64" for VGA passthru", > - domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1)); > - return ret; > - } > - break; > + vga_found = true; > + if (pcidev->bus == 0 && pcidev->dev == 2 && pcidev->func == 0) > + igd_found = true; Could you check that the vendor is Intel also using sysfs_dev_get_vendor? Or else this could trigger on AMD boxes if they happen to have a VGA PCI device at 00:2.0. Thanks, Roger.