From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy Date: Fri, 10 Jul 2015 14:26:19 +0100 Message-ID: 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" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436420047-25356-4-git-send-email-tiejun.chen@intel.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: Tiejun Chen 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 On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen wrote: > This patch extends the existing hypercall to support rdm reservation policy. > We return error or just throw out a warning message depending on whether > the policy is "strict" or "relaxed" when reserving RDM regions in pfn space. > Note in some special cases, e.g. add a device to hwdomain, and remove a > device from user domain, 'relaxed' is fine enough since this is always safe > to hwdomain. > > CC: Tim Deegan > CC: Keir Fraser > CC: Jan Beulich > CC: Andrew Cooper > CC: Suravee Suthikulpanit > CC: Aravind Gopalakrishnan > CC: Ian Campbell > CC: Stefano Stabellini > CC: Yang Zhang > CC: Kevin Tian > Signed-off-by: Tiejun Chen > --- > v6 ~ v7: > > * Nothing is changed. > > v5: > > * Just leave one bit XEN_DOMCTL_DEV_RDM_RELAXED as our flag, so > "0" means "strict" and "1" means "relaxed". Thanks for this; a few more comments... > @@ -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. > @@ -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." 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?) 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. -George