[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [v4][PATCH 04/19] xen/passthrough: extend hypercall to support rdm reservation policy
On Tue, Jun 23, 2015 at 10:57 AM, Tiejun Chen <tiejun.chen@xxxxxxxxx> 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 <tim@xxxxxxx> > CC: Keir Fraser <keir@xxxxxxx> > CC: Jan Beulich <jbeulich@xxxxxxxx> > CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx> > CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx> > CC: Ian Campbell <ian.campbell@xxxxxxxxxx> > CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx> > CC: Yang Zhang <yang.z.zhang@xxxxxxxxx> > CC: Kevin Tian <kevin.tian@xxxxxxxxx> > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx> OK, now that I've had a chance to go through the entire series, one more comment on this patch: > * Add code comments to describer why we fix to set a policy flag in some > cases like adding a device to hwdomain, and removing a device from user > domain. [snip] > @@ -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? 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 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.) 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. 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 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. After those changes (a single RDM_RELAXED flag, passing STRICT in for the hardware domain) then I think this patch is in good shape. -George _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |