Re: [RFC][v2][PATCH 01/14] tools: introduce some new parameters to set rdm policy

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

On 2015/6/7 19:06, Wei Liu wrote:
> On Wed, Jun 03, 2015 at 09:35:16AM +0800, Chen, Tiejun wrote:
> [...]
>>>
>>>> +reserved regions explicitly. And using "host" to include all reserved regions
>>>> +reported on this platform which is good to handle hotplug scenario. In the
>>>> +future this parameter may be further extended to allow specifying random
>>>> +regions, e.g. even those belonging to another platform as a preparation
>>>
>>> Extending how? What's your envisaged syntax for those random regions?
>>
>> We didn't go into details while discussing that design. Maybe we can do
>> something like this,
>>
>> rdm="type=host,reserve=strict,rdm_add=size[KMG][@offset[KMG]],size[KMG][@offset[KMG]],..."
>>
>
> This limits the extra regions to type host and strict policy. If that's
> what you want then it's fine.

The policy still can be changed with "reserve=". But whatever, I mean 
this current format is easy to extend :) You know, the hotplug is really 
a complicated case, we really need to consider more next.

Thanks
Tiejun

>
>>> Should you want to reserve more, an array is more useful. Could you
>>
>> Yeah.
>>
>>> provide some examples?
>>
>> But we may have alternative approach to this when I noticed some guys are
>> trying to delivery some patches about setting rmrr region by xen
>> commandline. So I also would like to check this likelihood when we can step
>> forward.
>>
>
> Makes sense.
>
>>>
>>>> +for live migration with passthrough devices.
>>>> +
>>>> +"none" means we have nothing to do all reserved regions and ignore all policies,
>>>> +so guest work as before.
>
> [...]
>
>>>> diff --git a/docs/misc/vtd.txt b/docs/misc/vtd.txt
>>>> index 9af0e99..7d63c47 100644
>>>> --- a/docs/misc/vtd.txt
>>>> +++ b/docs/misc/vtd.txt
>>>> @@ -111,6 +111,30 @@ in the config file:
>>>>   To override for a specific device:
>>>>   	pci = [ '01:00.0,msitranslate=0', '03:00.0' ]
>>>>
>>>> +RDM, 'reserved device memory', for PCI Device Passthrough
>>>> +---------------------------------------------------------
>>>> +
>>>> +There are some devices the BIOS controls, for e.g. USB devices to perform
>>>> +PS2 emulation. The regions of memory used for these devices are marked
>>>> +reserved in the e820 map. When we turn on DMA translation, DMA to those
>>>> +regions will fail. Hence BIOS uses RMRR to specify these regions along with
>>>> +devices that need to access these regions. OS is expected to setup
>>>> +identity mappings for these regions for these devices to access these regions.
>>>> +
>>>> +While creating a VM we should reserve them in advance, and avoid any conflicts.
>>>> +So we introduce user configurable parameters to specify RDM resource and
>>>> +according policies,
>>>> +
>>>> +To enable this globally, add "rdm" in the config file:
>>>> +
>>>> +    rdm = "type=host, reserve=relaxed"   (default policy is "relaxed")
>>>> +
>>>> +Or just for a specific device:
>>>> +
>>>> +    pci = [ '01:00.0,rdm_reserve=relaxed', '03:00.0,rdm_reserve=strict' ]
>>>> +
>>>> +For all the options available to RDM, see xl.cfg(5).
>>>> +
>>>>
>>>>   Caveat on Conventional PCI Device Passthrough
>>>>   ---------------------------------------------
>>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>>> index f0da7dc..d649ead 100644
>>>> --- a/tools/libxl/libxl_create.c
>>>> +++ b/tools/libxl/libxl_create.c
>>>> @@ -100,6 +100,12 @@ static int sched_params_valid(libxl__gc *gc,
>>>>       return 1;
>>>>   }
>>>>
>>>> +void libxl__rdm_setdefault(libxl__gc *gc, libxl_domain_build_info *b_info)
>>>> +{
>>>> +    b_info->rdm.type = LIBXL_RDM_RESERVE_TYPE_NONE;
>>
>> Based on our previous discussion, I will initial this firstly,
>>
>> +libxl_rdm_reserve_type = Enumeration("rdm_reserve_type", [
>> +    (0, "none"),
>> +    (1, "host"),
>> +    ], init_val = "LIBXL_RDM_RESERVE_TYPE_NONE")
>> +
>>
>> and then, I would remove this line since right now we just own two options,
>> "none" or "host". And both they're fine.
>>
>>>> +    b_info->rdm.reserve = LIBXL_RDM_RESERVE_FLAG_RELAXED;
>>>
>>> No, not like this. You set everything back to none and relaxed even if
>>> it is set before this point.
>>>
>>> It should be
>>>      if (xxx == DEFAULT_SENTINEL_VALUE)
>>>          xxx = THE_DEFAULT_YOU_WANT;
>>>
>>> Have a look at libxl__device_nic_setdefault etc to get an idea
>>> how it works. Don't hesitate to ask if I'm not clear enough.
>>
>> But indeed, here we should set rdm.reserve as you said,
>>
>> +void libxl__rdm_setdefault(libxl__gc *gc, libxl_domain_build_info *b_info)
>> +{
>> +    if (b_info->rdm.reserve == LIBXL_RDM_RESERVE_FLAG_INVALID)
>> +        b_info->rdm.reserve = LIBXL_RDM_RESERVE_FLAG_RELAXED;
>> +}
>> +
>>
>
> Yes. This is fine.
>
> Wei.
>