From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device Date: Tue, 20 Jan 2015 14:37:31 +0000 Message-ID: <54BE682B.1040000@linaro.org> References: <1421159133-31526-1-git-send-email-julien.grall@linaro.org> <1421159133-31526-21-git-send-email-julien.grall@linaro.org> <54BE2F520200007800056D85@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YDZwT-0005mg-IL for xen-devel@lists.xenproject.org; Tue, 20 Jan 2015 14:38:06 +0000 Received: by mail-we0-f173.google.com with SMTP id q58so37755451wes.4 for ; Tue, 20 Jan 2015 06:37:59 -0800 (PST) In-Reply-To: <54BE2F520200007800056D85@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: Wei Liu , ian.campbell@citrix.com, tim@xen.org, Ian Jackson , stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 20/01/15 09:34, Jan Beulich wrote: >>>> On 13.01.15 at 15:25, wrote: >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -337,6 +337,13 @@ int iommu_do_domctl( >> ret = iommu_do_pci_domctl(domctl, d, u_domctl); >> #endif >> >> + if ( ret != -ENOSYS ) >> + return ret; >> + >> +#ifdef HAS_DEVICE_TREE >> + ret = iommu_do_dt_domctl(domctl, d, u_domctl); >> +#endif > > Please move the (inverted) if() into the #ifdef block (but also see > below regarding the specific error code used). What do you mean by the "inverted if()"? >> @@ -1533,13 +1534,19 @@ int iommu_do_pci_domctl( >> break; >> >> case XEN_DOMCTL_test_assign_device: >> - ret = xsm_test_assign_device(XSM_HOOK, domctl->u.assign_device.machine_sbdf); >> + ret = -ENOSYS; >> + if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI ) >> + break; > > I'm rather uncertain about the use of -ENOSYS here: The hypercall > isn't unimplemented after all. Provided you use consistent error > codes in the PCI and DT variants, there's no problem calling the > other variant if that specific error code got returned from the first > one - the second would then just return the same one again, even > if the first one failed on something other than the device type > check. As a result, I'd much prefer -ENODEV here. I will use -ENODEV on the next version. >> + >> + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; >> + >> + ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf); >> if ( ret ) >> break; >> >> - seg = domctl->u.assign_device.machine_sbdf >> 16; >> - bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff; >> - devfn = domctl->u.assign_device.machine_sbdf & 0xff; >> + seg = machine_sbdf >> 16; >> + bus = (machine_sbdf >> 8) & 0xff; >> + devfn = machine_sbdf & 0xff; > > If you fiddle with these, please make them use at least PCI_BUS() > and PCI_DEVFN2() (we don't have a matching macro for retrieving > the segment). Ok. > >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -475,12 +475,23 @@ typedef struct xen_domctl_sendtrigger xen_domctl_sendtrigger_t; >> DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t); >> >> >> -/* Assign PCI device to HVM guest. Sets up IOMMU structures. */ >> +/* Assign a device to a guest. Sets up IOMMU structures. */ >> /* XEN_DOMCTL_assign_device */ >> /* XEN_DOMCTL_test_assign_device */ >> /* XEN_DOMCTL_deassign_device */ >> +#define XEN_DOMCTL_DEV_PCI 0 >> +#define XEN_DOMCTL_DEV_DT 1 >> struct xen_domctl_assign_device { >> - uint32_t machine_sbdf; /* machine PCI ID of assigned device */ >> + uint32_t dev; /* XEN_DOMCTL_DEV_* */ >> + union { >> + struct { >> + uint32_t machine_sbdf; /* machine PCI ID of assigned device */ >> + } pci; >> + struct { >> + uint32_t size; /* Length of the path */ >> + XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node */ >> + } dt; >> + } u; >> }; > > An incompatible change like this requires bumping > XEN_DOMCTL_INTERFACE_VERSION when not already done during > the current release cycle. The XEN_DOMCTL_INTERFACE_VERSION will be bumped in patch #1 of this series [1]. Regards, [1] https://patches.linaro.org/43014/ -- Julien Grall