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: Tue, 07 Jul 2015 14:37:04 +0800 Message-ID: <559B7390.4070401@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> <559A9640.7050501@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559A9640.7050501@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 , Jan Beulich Cc: Kevin Tian , Keir Fraser , Ian Campbell , Andrew Cooper , Tim Deegan , "xen-devel@lists.xen.org" , Aravind Gopalakrishnan , Suravee Suthikulpanit , Yang Zhang , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org Just please go to review the new revision directly. Thanks Tiejun On 2015/7/6 22:52, Chen, Tiejun wrote: > 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 >> >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >