Re: [PATCH v2 3/3] tools: introduce parameter max_wp_ram_ranges.

["Yu, Zhang" <yu.c.zhang@linux.intel.com>]

On 1/27/2016 6:27 PM, Jan Beulich wrote:
>>>> On 27.01.16 at 08:01, <yu.c.zhang@linux.intel.com> wrote:
>
>>
>> On 1/26/2016 7:00 PM, Jan Beulich wrote:
>>>>>> On 26.01.16 at 08:32, <yu.c.zhang@linux.intel.com> wrote:
>>>> On 1/22/2016 4:01 PM, Jan Beulich wrote:
>>>>>>>> On 22.01.16 at 04:20, <yu.c.zhang@linux.intel.com> wrote:
>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>> @@ -940,6 +940,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct
>>>>>> hvm_ioreq_server *s,
>>>>>>     {
>>>>>>         unsigned int i;
>>>>>>         int rc;
>>>>>> +    unsigned int max_wp_ram_ranges =
>>>>>> +        ( s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] > 0 ) ?
>>>>>> +        s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] :
>>>>>> +        MAX_NR_IO_RANGES;
>>>>>
>>>>> Besides this having stray blanks inside the parentheses it truncates
>>>>> the value from 64 to 32 bits and would benefit from using the gcc
>>>>> extension of omitting the middle operand of ?:. But even better
>>>>> would imo be if you avoided the local variable and ...
>>>>>
>>>> After second thought, how about we define a default value for this
>>>> parameter in libx.h, and initialize the parameter when creating the
>>>> domain with default value if it's not configured.
>>>
>>> No, I don't think the tool stack should be determining the default
>>> here (unless you want the default to be zero, and have zero
>>> indeed mean zero).
>>>
>> Thank you, Jan.
>> If we do not provide a default value in tool stack, the code above
>> should be kept, to initialize the local variable with either the one
>> set in the configuration file, or with MAX_NR_IO_RANGES. Is this OK?
>
> Well, not exactly: For one, the original comment (still present
> above) regarding truncation holds. And then another question is:
> Do you expect this resource type to be useful with its number of
> ranges limited to MAX_NR_IO_RANGES? I ask because if the
> answer is "no", having it default to zero might be as reasonable.
>

Thanks, Jan.

About the default value:
   You are right. :) For XenGT, MAX_NR_IO_RANGES may only work under
limited conditions. Having it default to zero means XenGT users must
manually configure this option. Since we have plans to push other XenGT
tool stack parameters(including a GVT-g flag), how about we set this
max_wp_ram_ranges to a default one when GVT-g flag is detected, and
till then, max_wp_ram_ranges is supposed to be configured explicitly for
XenGT?

About the truncation issue:
   I do not quite follow. Will this hurt if the value configured does
not exceed 4G? What about a type cast?

B.R.
Yu



> Jan
>
>