Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

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

On 2014/7/24 19:35, Tim Deegan wrote:
> At 19:25 +0800 on 24 Jul (1406226315), Chen, Tiejun wrote:
>> On 2014/7/24 19:11, Tim Deegan wrote:
>>> At 19:00 +0800 on 24 Jul (1406224818), Tiejun Chen wrote:
>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>> cases like some GFX drivers it still need to access RMRR.
>>>>
>>>> Here we will create those RMRR mappings even in shared EPT case.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>
>>> For the interaction with p2m code:
>>>
>>> Acked-by: Tim Deegn <tim@xen.org>
>>
>> Thanks a lot.
>>
>>>
>>> though I am still worried about what happens if this overwrites an
>>> existing mapping.
>>>
>>
>> As I understand RMRR should be specific. Its unlikely created for
>> different mapping since this should be fixed in BIOS phase. And this is
>> why that function is named as rmrr_identity_mapping().
>
> But the BIOS only sets up _Xen's_ memory map.  The toolstack sets up
> the _VM's_ memory map, and unless it can avoid the RMRRs of all
> devices in the system (and add them to guest E820s) it risks a clash
> here.

RMRR seems be used barely. Here in my case, just GFX passthrough needs 
this since some windows GFX drivers may access those stolen memory now.

>
> If you can hot-plug one of these devices into a running guest, then
> there's no way to be safe, as the guest might have been booted on
> another system and migrated.

I guess the original RMRR implementation don't consider live migration 
so current RMRR shouldn't support live migration, right?

So this should be anther story we need to address.

>
> So I think it would be prudent to at least try to detect a clash here
> and return an error.

Or do you already have some better ways to do this? I'd like to code 
them in another thread.

Tiejun

>
> (BTW, I think this patch can go in as-is, and the address-space clash
> be fixed up separately.)
>
> Tim.
>
>