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

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

On 2015/5/19 19:00, Wei Liu wrote:
> On Tue, May 19, 2015 at 06:50:11PM +0800, Chen, Tiejun wrote:
>> On 2015/5/19 17:42, Wei Liu wrote:
>
> [...]
>
>>>>>>>>> want you want?
>>>>>>>>
>>>>>>>> We have a little bit of complexity here,
>>>>>>>>
>>>>>>>> "Default per-device RDM policy is 'force', while default global RDM policy
>>>>>>>> is 'try'. When both policies are specified on a given region, 'force' is
>>>>>>>> always preferred."
>>>>>>>>
>>>>>>>
>>>>>>> This is going to be done in actual code anyway.
>>>>>>>
>>>>>>> This type is used both in global and per-device setting, so I envisage
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> this to have an invalid value to start with. Appropriate default values
>>>>>>
>>>>>> Sounds I should set this,
>>>>>>
>>>>>> +libxl_rdm_reserve_flag = Enumeration("rdm_reserve_flag", [
>>>>>> +    (-1, "invalid"),
>>>>>> +    (0, "strict"),
>>>>>> +    (1, "relaxed"),
>>>>>> +    ], init_val = "LIBXL_RDM_RESERVE_FLAG_INVALID")
>>>>>> +
>>>>>>
>>>>>
>>>
>>> Yet another question about this feature. The current setup suggests that
>>> we must choose a policy, either "strict" or "relaxed", i.e. there is no
>>> way to disable this feature, as in there is no "none" policy to skip
>>> checking rdm conflict.
>>>
>>> AIUI this feature is more like a bug fix to existing problem, so we
>>> always want to enable it. And the default relaxed policy makes sure we
>>> don't break guest that was working before this feature. Do I understand
>>> this correctly?
>>>
>>> If we risk breaking existing guests, we might want to consider adding a
>>> "none" (name subject to improvement) policy to just skip RDM all
>>> together.
>>
>> We have this definition,
>>
>> +libxl_rdm_reserve_type = Enumeration("rdm_reserve_type", [
>> +    (0, "none"),
>> +    (1, "host"),
>> +    ])
>>
>> If we set 'type=none', this means we would do nothing actually since we
>> don't expose any rdms to guest. This behavior will ensue we go back the
>> existing scenario without our patch.
>>
>
> But this only works with global configuration and individual
> configuration in PCI spec trumps this, right?

You're right.

>
> Think about how an old configuration migrated to newer version of Xen
> should work. For example, I don't have rdm= but have pci=['xxxx']. Do we
> need to make sure this still work? I guess the answer is if it already

Definitely.

> works before RDM it should continue to work as there is really no
> conflict before. In this case whether  we enable RDM or not doesn't make
> a difference to a guest that's already working before. Am I right?

I think we can set the default 'type' to NONE,

libxl__rdm_setdefault()
{
     b_info->rdm.type = LIBXL_RDM_RESERVE_TYPE_NONE;

and then,

libxl__domain_device_construct_rdm()
{
     ...
     /* Might not expose rdm. */
     if (type == LIBXL_RDM_RESERVE_TYPE_NONE)
	return 0;

This means we don't expose any rdm so we would never concern any policy 
anymore.


Thanks
Tiejun

>
>>>
>>>>> Yes, and then don't forget to set the value to appropriate value in the
>>>>> _setdefault functions for different types.
>>>>
>
> [...]
>
>>> Spaces.
>>>
>>>>>>>>>> +    if ( NULL == (buf2 = ptr = strdup(str)) )
>>>>>>>>>> +        return ERROR_NOMEM;
>>>>>>>>>> +
>>>>>>>>>> +    for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
>>>>>>>>>> +        switch(state) {
>>>>
>>>> I thought initially you let me to follow that previous "if" :)
>>>>
>>>
>>> Just be consistent with other part of the source code.
>>
>> I just refer to that existing xlu_pci_parse_bdf()...
>>
>
> Sorry I didn't mean to blame you for something that's not your fault.
>
>> Anyway I guess you mean I should do something like this,
>>
>>      if (NULL == (buf2 = ptr = strdup(str)))
>>          return ERROR_NOMEM;
>>
>>      for (tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
>>          switch(state) {
>>          case STATE_TYPE:
>>              if (*ptr == '=') {
>> 	    ...
>>
>
> Fair enough. I prefer consistency.
>
> Wei.
>
>> Thanks
>> Tiejun
>>
>
>