From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy Date: Mon, 13 Jul 2015 14:47:22 +0800 Message-ID: <55A35EFA.9030304@intel.com> References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-4-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Kevin Tian , Keir Fraser , Suravee Suthikulpanit , Andrew Cooper , Tim Deegan , "xen-devel@lists.xen.org" , Aravind Gopalakrishnan , Jan Beulich , Yang Zhang , Stefano Stabellini , Ian Campbell List-Id: xen-devel@lists.xenproject.org > Thanks for this; a few more comments... > Thanks for your time. >> @@ -1577,9 +1578,15 @@ int iommu_do_pci_domctl( >> seg = machine_sbdf >> 16; >> bus = PCI_BUS(machine_sbdf); >> devfn = PCI_DEVFN2(machine_sbdf); >> + flag = domctl->u.assign_device.flag; >> + if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED ) > > This is not a blocker, but a stylistic comment: I would have inverted > the bitmask here, as that's conceptually what you're checking. I > won't make this a blocker for going in. What about this? diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 6e23fc6..17a4206 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1579,7 +1579,7 @@ int iommu_do_pci_domctl( bus = PCI_BUS(machine_sbdf); devfn = PCI_DEVFN2(machine_sbdf); flag = domctl->u.assign_device.flag; - if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED ) + if ( flag & ~XEN_DOMCTL_DEV_RDM_MASK ) { ret = -EINVAL; break; diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index bca25c9..07549a4 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -480,6 +480,7 @@ struct xen_domctl_assign_device { } u; /* IN */ #define XEN_DOMCTL_DEV_RDM_RELAXED 1 +#define XEN_DOMCTL_DEV_RDM_MASK 0x1 uint32_t flag; /* flag of assigned device */ }; typedef struct xen_domctl_assign_device xen_domctl_assign_device_t; > >> @@ -1898,7 +1899,14 @@ static int intel_iommu_add_device(u8 devfn, struct pci_dev *pdev) >> PCI_BUS(bdf) == pdev->bus && >> PCI_DEVFN2(bdf) == devfn ) >> { >> - ret = rmrr_identity_mapping(pdev->domain, 1, rmrr); >> + /* >> + * Here means we're add a device to the hardware domain >> + * so actually RMRR is always reserved on e820 so either >> + * of flag is fine for hardware domain and here we'd like >> + * to pass XEN_DOMCTL_DEV_RDM_RELAXED. >> + */ > > Sorry I didn't give feedback on your comment before. I don't find it > clear enough; I'd suggest something like this: > > "iommu_add_device() is only called for the hardware domain (see > xen/drivers/passthrough/pci.c:pci_add_device()). Since RMRRs are > always reserved in the e820 map for the hardware domain, there > shouldn't be a conflict." Loos good and thanks. > > I also said that if we went with anything other than STRICT that we'd > need to check to make sure that the domain really was the hardware > domain before proceeding, in case the assumption that pdev->domain == > hardware_domain ever changed. (Perhaps with an ASSERT -- Jan, what do > you think?) Sounds reasonable. > > Also, passing in RELAXED in locations where the flag is completely > ignored (such as when removing mappings) doesn't really make any > sense. > > On the whole I think it would be better if you removed the RELAXED > flag for both removals and for hardware domains. > Just as I said in another email I agreed your STRICT way. Thanks Tiejun