Re: [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps

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

On 2014/10/28 18:06, Jan Beulich wrote:
>>>> On 28.10.14 at 08:47, <tiejun.chen@intel.com> wrote:
>> On 2014/10/27 18:17, Jan Beulich wrote:
>>>>>> On 27.10.14 at 09:09, <tiejun.chen@intel.com> wrote:
>>>> On 2014/10/24 22:56, Jan Beulich wrote:
>>> _no matter_ what RMRRs a physical host has, it should not prevent
>>> the creation of guests (the worst that may result is that passing
>>> through certain devices doesn't work anymore, and even then the
>>> operator needs to be given a way of circumventing this if (s)he
>>> knows that the device won't access the range post-boot, or if it's
>>> being deemed acceptable for it to do so).
>>
>> As we know just legacy USB and GFX need these RMRR ranges.
>
> This is specified where?

In VT-D specification, I just see,

"The RMRR regions are expected to be used for legacy usages (such as 
USB, UMA Graphics, etc.) requiring reserved memory. Platform designers 
should avoid or limit use of reserved memory regions since these require 
system software to create holes in the DMA virtual address range 
available to system software and its driver."

>
>> Especially, I
>> believe just USB need << 1M space, so it may be possible to be placed
>> below 1M. But I think we can ask BIOS to reallocate them upwards like my
>> real platform,
>>
>> RMRR region: base_addr ab80a000 end_address ab81dfff
>>
>> I don't know what platform you're using, maybe its a legacy machine?
>
> A Westmere one.
>
>> But
>> anyway it should be feasible to update BIOS. And even we can ask BIOS do
>> this as a normal rule in the future.
>>
>> For GFX, oftentimes it need dozens of MB,
>>
>> RMRR region: base_addr ad000000 end_address af7fffff
>>
>> So it shouldn't be overlapped with <1M.
>
> These "I believe" and "shouldn't" are a real problem here: Please
> make claims only based on the specification, not on observations on
> a particular system. In the real world, you have to be prepared for
> implementations of a specification to be taking more liberties than
> allowed for; you should never assume people don't even make use
> off the full scope a specification provides for. I know I'm repeating
> myself, but again - remember you're changing the hypervisor here
> (and view hvmloader as an extension of the hypervisor inside the
> guest).

RMRR really is very troublesome.

The legacy usage of USB just cover ps2 emulation as I know. And as you 
see these address are different in different platforms so this mean 
they're not redistricted somewhere specific. And GFX need more space so 
its not possible to be placed under 1M.

So maybe I can drop patch #12, xen/vtd: re-enable USB device assignment, 
to leave USB out our scope. Or a little improvement is to check if its 
own range is below 1M.

>
>>>> I know this is ugly but as you know there's no any rule we can make good
>>>> use of this case. RMRR can start anywhere so We have to assume any
>>>> scenarios,
>>>>
>>>> 1. Just amid those remaining e820 entries.
>>>> 2. Already at the end.
>>>> 3. If coincide with one RAM range.
>>>> 4. If we're just aligned with start of one RAM range.
>>>> 5. If we're just aligned with end of one RAM range.
>>>> 6. If we're just in of one RAM range.
>>>> 7. If we're going last RAM:Hole range.
>>>>
>>>> So if you think we're handling correctly, maybe we can continue
>>>> optimizing this way once we have a better idea.
>>>
>>> I understand that there are various cases to be considered, but
>>> that's no different elsewhere. For example, look at
>>> xen/arch/x86/e820.c:e820_change_range_type() which gets
>>
>> I don't think this circumstance is same as our requirement.
>>
>> Here we are trying to insert different multiple entries that they have
>> different range.
>
> Inserting multiple entries can always be done by inserting one
> entry at a time. If that yields better (easier to understand and

Insert means you have to split one existing entry. Even it may overlap 
with two entries at the same time, and these two entries may not be 
continuous.

> maintain) code, and if the code path isn't a hot one, that should
> be the route to go.

I promise I can take a further look but it may be difficult to me.

>
>>>> With my patch:
>>>>
>>>> (d2)  f0000-fffff: Main BIOS
>>>> (d2) E820 table:
>>>> (d2)  [00]: 00000000:00000000 - 00000000:0009e000: RAM
>>>> (d2)  [01]: 00000000:0009e000 - 00000000:000a0000: RESERVED
>>>> (d2)  HOLE: 00000000:000a0000 - 00000000:000e0000
>>>> (d2)  [02]: 00000000:000e0000 - 00000000:00100000: RESERVED
>>>> (d2)  [03]: 00000000:00100000 - 00000000:ab80a000: RAM
>>>> (d2)  [04]: 00000000:ab80a000 - 00000000:ab81e000: RESERVED
>>>> (d2)  [05]: 00000000:ab81e000 - 00000000:ad000000: RAM
>>>> (d2)  [06]: 00000000:ad000000 - 00000000:af800000: RESERVED
>>>
>>> And this already answers what I asked above: You shouldn't be blindly
>>> hiding 40Mb from the guest.
>>
>> If we don't reserve these RMRR ranges, so guest may create 1:1 mapping.
>> Then it will affect a device usage in other VM, or a device usage may
>> corrupt these ranges in other VM.
>>
>> Yes, we really need a policy to do this. So please tell me what you expect.
>
> In the tool stack, don't even populate these holes with RAM. This
> will then lead to RAM getting populated further up at the upper end.

Shouldn't populate RAM still with guest_physmap_add_entry()? If yes, we 
already be there to mark them as p2m_access_n.

Thanks
Tiejun
>
> Jan
>