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/31 14:21, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Friday, October 31, 2014 1:41 PM
>>
>> On 2014/10/30 17:20, Jan Beulich wrote:
>>>>>> On 30.10.14 at 04:11, <tiejun.chen@intel.com> wrote:
>>>> On 2014/10/29 17:15, Jan Beulich wrote:
>>>>>>>> On 29.10.14 at 08:43, <tiejun.chen@intel.com> wrote:
>>>>>> 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."
>>>>>
>>>>> Nice that you quote it, but did you also read it properly? There's this
>>>>> little "etc" following the explicit naming of USB and UMA...
>>>>
>>>> Yes. But this already clarify RMRR "used for legacy usage" and "avoid or
>>>> limit use of reserved memory regions", so RMRR would be gone finally. So
>>>> I mean it may be acceptable to assume something based our known info.
>>>
>>> No. Making assumption on observed broken behavior is okay (to work
>>> around it), but making assumptions for not (yet) observed correct
>>> behavior to be absent should never be done.
>>>
>>>>>>> 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.
>>>>>
>>>>> Marking them with p2m_access_n is not the same as not populating
>>>>> the regions in the first place. Again - hiding multiple megabytes of
>>>>> memory (and who knows if it can't grow into the gigabyte range) is
>>>>> just not acceptable. Even for just a few pages I wouldn't be really
>>>>
>>>> I don't think so. If you're considering a VM, this case should be same
>>>> under native circumstance. And in native case, all RMRR ranges are
>>>> marked as reserved in e820 table.
>>>
>>> That would only be a valid comparison if all devices associated with
>>> RMRRs would also be passed to that particular VM. But since you're
>>> doing the E820 adjustment to all VMs (no matter whether they would
>>> ever get _any_ device passed through to them) this is not comparing
>>> like entities.
>>>
>>> Thinking about this some more, this odd universal hole punching in
>>> the E820 is very likely to end up causing problems. Hence I think
>>> this really should be optional behavior, with pass through of devices
>>> associated with RMRRs failing if not used. (This ought to include
>>> punching holes for _just_ the devices passed through to a guest
>>> upon creation when the option is not enabled.)
>>
>> Yeah, we had a similar discussion internal to add a parameter to force
>> reserving RMRR. In this case we can't create a VM if these ranges
>> conflict with anything. So what about this idea?
>>
>
> Adding a new parameter (e.g. 'check-passthrough') looks the right
> approach. When the parameter is on, RMRR check/hole punch is

Could we just extend a sub option as one subset of 'pci' parameter? like 
pci = ["00:02.0@passthrough"] or something else, I think it should make 
sense.

> activated at VM creation. Otherwise we just keep existing behavior.

Yes, VM creation should be failed from any overlap. If without this 
option, just make device assignment failed in case of conflict, but VM 
can still step forward.

>
> If user configures device pass-through at creation time, this parameter
> will be set by default. If user wants the VM capable of device hot-plug,
> an explicit parameter can be added in the config file to enforce RMRR
> check at creation time.

When we check if any RMRR exists, we should post this recommended 
message while parsing ACPI for RMRR.

Thanks
Tiejun

>
> Thanks
> Kevin
>
>