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

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

On 2014/9/18 17:09, 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 gave this a try on the one box I have which exposes RMRRs
> (since those are for USB devices I also used your patch to drop
> the USB special casing as done in your later patch series, and I
> further had to fiddle with vtd_ept_page_compatible() in order to
> get page table sharing to actually work on that box [I'll send the
> resulting patch later]) - with the result that passing through an
> affected USB controller (as expected) doesn't work anymore. Which

With or without these two patches, USB always can be passed through in 
my platform. Note I'm using ubuntu as a Guest OS.

> raises the question why the two patches alone would work at all.
> Could you please share information on the address ranges specified
> by the RMRR(s) in your case? I simply wonder whether things just

(XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
(XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address 
ab818fff
(XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
(XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address 
af7fffff


> happen to work for you on the particular system(s) you're testing
> on, as I'd generally expect an address space collision to be possible
> for any RMRR.
>
> I think you understand the consequences: If the series here has no
> way of reliably working without the other one, "iommu=no-sharept"

This already is our known way but we need to support the PT in both 
shared and non-shared cases.

> is going to be the solution for you, at once being one more argument
> towards dropping page table sharing altogether. The one argument
> in favor of the two patches here would be that they at least detect
> the collision now, thus forcing people to suppress page table sharing.

Sorry this sort of estimate is out of the scope I can answer properly. 
Maybe Yang or Kevin can do follow this if possible.

>
> But what's worse, I can't see how the non-sharing case is being
> handled correctly either (independent of the series here):
> rmrr_identity_mapping() blindly overwrites what may already be
> in the page tables, breaking consistency with the CPU-side P2M
> (iiuc this is a problem even for PV, including Dom0). Plus there's
> nothing being done to prevent subsequent overwriting of these
> 1:1 entries by "normal" P2M manipulations. All in all another

I'm not sure this as well.

Yang and Kevin,

What are your comments about this point?

> argument not to allow (at least by default) passing through of
> devices associated with one or more RMRRs.

Here I have to wait Kevin and Yang's comments since they're familiar 
with these internals than me :), then I can try to figure out 
appropriate ways to fix these arguments if they really exist.

Thanks
Tiejun

>
> Jan
>
>
>