Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

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

On 2014/7/30 17:23, Jan Beulich wrote:
>>>> On 30.07.14 at 10:59, <tiejun.chen@intel.com> wrote:
>> On 2014/7/30 16:36, Jan Beulich wrote:
>>>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>>> @@ -1867,8 +1867,14 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>
>>>>        while ( base_pfn < end_pfn )
>>>>        {
>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> -                                  IOMMUF_readable|IOMMUF_writable) )
>>>> +        if ( iommu_use_hap_pt(d) )
>>>> +        {
>>>> +            ASSERT(!iommu_passthrough || !is_hardware_domain(d));
>>>> +            if ( set_identity_p2m_entry(d, base_pfn) )
>>>> +                return -1;
>>>> +        }
>>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>>>                return -1;
>>>>            base_pfn++;
>>>>        }
>>>
>>> So I wonder how this plays together with
>>>
>>>       /* FIXME: Because USB RMRR conflicts with guest bios region,
>>>        * ignore USB RMRR temporarily.
>>>        */
>>>       seg = pdev->seg;
>>>       bus = pdev->bus;
>>>       if ( is_usb_device(seg, bus, pdev->devfn) )
>>>       {
>>>           ret = 0;
>>>           goto done;
>>>       }
>>>
>>> later in the same file (in intel_iommu_assign_device()). I.e. the
>>> improvement you claim won't be achieved for passed through USB
>>
>> I think we can remove this chunk of codes since these two patches
>> already check if they conflicts.
>
> Causing an apparent regression for anyone wanting to pass through
> a USB devices associated with an RMRR? I agree this is the more
> correct thing to do, but I'm not sure all our users would agree.
>

 From what those codes mean, it just return regardless whether they 
really conflict. And this is just a good assumption, so if I'm 
understanding this properly, actually our patches do this thing 
precisely because we further check if this assumption is true, then take 
necessary actions.

If you're fine to this point, I'd like to remove this with a separate 
patch. Or lets wait one day to look for more eyes.

Thanks
Tiejun