Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Jul 9, 2015 at 6:33 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>
> ---
> v6 ~ v7:
>
> * Nothing is changed.
>
> v5:
>
> * Just leave one bit XEN_DOMCTL_DEV_RDM_RELAXED as our flag, so
>   "0" means "strict" and "1" means "relaxed".

Thanks for this; a few more comments...

> @@ -1577,9 +1578,15 @@ int iommu_do_pci_domctl(
>          seg = machine_sbdf >> 16;
>          bus = PCI_BUS(machine_sbdf);
>          devfn = PCI_DEVFN2(machine_sbdf);
> +        flag = domctl->u.assign_device.flag;
> +        if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )

This is not a blocker, but a stylistic comment: I would have inverted
the bitmask here, as that's conceptually what you're checking.  I
won't make this a blocker for going in.

> @@ -1898,7 +1899,14 @@ 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);
> +            /*
> +             * 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.
> +             */

Sorry I didn't give feedback on your comment before.  I don't find it
clear enough; I'd suggest something like this:

"iommu_add_device() is only called for the hardware domain (see
xen/drivers/passthrough/pci.c:pci_add_device()).  Since RMRRs are
always reserved in the e820 map for the hardware domain, there
shouldn't be a conflict."

I also said that if we went with anything other than STRICT that we'd
need to check to make sure that the domain really was the hardware
domain before proceeding, in case the assumption that pdev->domain ==
hardware_domain ever changed.  (Perhaps with an ASSERT -- Jan, what do
you think?)

Also, passing in RELAXED in locations where the flag is completely
ignored (such as when removing mappings) doesn't really make any
sense.

On the whole I think it would be better if you removed the RELAXED
flag for both removals and for hardware domains.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel