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

["Chen, Tiejun" <tiejun.chen@intel.com>]

Any comment to this?

Thanks
Tiejun

On 2015/7/2 16:49, Chen, Tiejun wrote:
>>> @@ -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@lists.xen.org
> http://lists.xen.org/xen-devel
>
>