@@ -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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
