Re: [Xen-devel] [v4][PATCH 04/19] xen/passthrough: extend hypercall to support rdm reservation policy

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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