From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v4][PATCH 04/19] xen/passthrough: extend hypercall to support rdm reservation policy Date: Mon, 06 Jul 2015 22:52:48 +0800 Message-ID: <559A9640.7050501@intel.com> References: <1435053450-25131-1-git-send-email-tiejun.chen@intel.com> <1435053450-25131-5-git-send-email-tiejun.chen@intel.com> <5594FB07.6010100@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5594FB07.6010100@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: George Dunlap Cc: Kevin Tian , Keir Fraser , Jan Beulich , Andrew Cooper , Tim Deegan , "xen-devel@lists.xen.org" , Aravind Gopalakrishnan , Suravee Suthikulpanit , Yang Zhang , Stefano Stabellini , Ian Campbell List-Id: xen-devel@lists.xenproject.org Any comment to this? Thanks Tiejun On 2015/7/2 16:49, Chen, Tiejun wrote: >>> @@ -1898,7 +1899,13 @@ 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); >>> + /* >>> + * 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. >>> + */ >>> + ret = rmrr_identity_mapping(pdev->domain, 1, rmrr, >>> + XEN_DOMCTL_DEV_RDM_RELAXED); >> >> So two things. >> >> First, you assert that the value here won't matter, because the >> hardware domain is guaranteed never to have a conflict. >> >> Which is likely to be true almost all the time; but the question is, >> *if* something goes wrong, what should happen? >> >> For instance, suppose that someone accidentally introduces a bug in >> Xen that messes up or ignores reading a portion of the e820 map under >> certain circumstances. What should happen? > > Yes, you can image all possible cases. But if this kind of bug can come > true, I really very doubt if Xen can boot successfully. Because e820 is > a fundamental key to run OS, so this case is very easy to panic Xen, right? > > Anyway, I agree we should concern all corner cases. > >> >> If you set this to RELAXED, this clash will be silently ignored; which >> means that devices that need RMRR will simply malfunction in weird >> ways without any warning messages having been printed that might give > > No. We always post that messages regardless of relaxe or strict since > this massage just depends on one condition of that conflict exist. > >> someone a hint about what is going on. >> >> If you set this to STRICT, then this clash will print an error >> message, but as far as I can tell, the rest of the device assignment >> will continue as normal. (Please correct me if I've followed the code >> wrong.) > > Not all cases are like this behavior but here is true. > >> >> Since the device should be just as functional (or not functional) >> either way, but in the STRICT case should actually print an error >> message which someone might notice, it seems to me that STRICT is a >> better option for the hardware domain. >> > > Just see above. > >> Secondly, you assert in response to Kevin's question in v3 that this >> path is only reachable when assigning to the hardware domain. I think >> you at least need to update the comment here to indicate that's what >> you think; it's not at all obvious just from looking at the function > > What about this? > > PCI_DEVFN2(bdf) == devfn ) > { > /* > - * 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. > + * 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. > */ > ret = rmrr_identity_mapping(pdev->domain, 1, rmrr, > XEN_DOMCTL_DEV_RDM_RELAXED); > > >> that this is true. And if we do end up doing something besides >> STRICT, we should check to make sure that pdev->domain really *is* the >> hardware domain before acting like it is. >> >>> if ( ret ) >>> dprintk(XENLOG_ERR VTDPREFIX, "d%d: RMRR mapping >>> failed\n", >>> pdev->domain->domain_id); >>> @@ -1939,7 +1946,8 @@ static int intel_iommu_remove_device(u8 devfn, >>> struct pci_dev *pdev) >>> PCI_DEVFN2(bdf) != devfn ) >>> continue; >>> >>> - rmrr_identity_mapping(pdev->domain, 0, rmrr); >>> + rmrr_identity_mapping(pdev->domain, 0, rmrr, >>> + XEN_DOMCTL_DEV_RDM_RELAXED); >>> } >> >> Same here wrt STRICT. > > This is inside intel_iommu_remove_device() so actually any flag doesn't > take effect to rmrr_identity_mapping(). But I should add a comment like > this, > > + /* > + * Any flag is nothing to clear these mappings so here > + * its always safe to set XEN_DOMCTL_DEV_RDM_RELAXED. > + */ > > >> >> After those changes (a single RDM_RELAXED flag, passing STRICT in for >> the hardware domain) then I think this patch is in good shape. >> > > Based on my understanding to your concern, seems you always think in > case of "relax" we don't post any message, right? But now as I reply > above this is not correct so what's your further consideration? > > Anyway, I'm fine to change this. And after you suggested to keep one bit > just to indicate XEN_DOMCTL_DEV_RDM_RELAXED, we don't have that actual > XEN_DOMCTL_DEV_RDM_STRICT so I can just reset all associated flag as 0 > easily. > > Thanks > Tiejun > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >