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: Thu, 02 Jul 2015 16:49:11 +0800 Message-ID: <5594FB07.6010100@intel.com> References: <1435053450-25131-1-git-send-email-tiejun.chen@intel.com> <1435053450-25131-5-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 >> @@ -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