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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2014-09-18:
>>>> On 30.07.14 at 03:36, <tiejun.chen@xxxxxxxxx> 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 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 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"
> 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.
> 
> 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 argument
> not to allow (at least by default) passing through of devices
> associated with one or more RMRRs.

This is another issue that current RMRR handling logic is not right which I 
think we have discussed long time ago.
http://lists.xen.org/archives/html/xen-devel/2014-07/msg03249.html

Obviously, current RMRR handling logic is totally wrong. It is not a simple 
task to do it which I think Tiejun already spent about two months but we still 
have some divergences. From my point, this patch will mitigate this issue. At 
least, it doesn't make thing worse. Considering the Xen 4.5 release, we should 
let this patch in. For remain part, Tiejun will continue working on it to make 
the whole thing work well. Besides, since you have some misunderstood on our 
work style, why not give a chance to let us prove it?

> 
> Jan


Best regards,
Yang



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