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

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

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?

>> About this local variable, we keep it, and ...
>>
>>>> @@ -962,7 +966,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct
>> hvm_ioreq_server *s,
>>>>            if ( !s->range[i] )
>>>>                goto fail;
>>>>
>>>> -        rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
>>>> +        if ( i == HVMOP_IO_RANGE_WP_MEM )
>>>> +            rangeset_limit(s->range[i], max_wp_ram_ranges);
>>>> +        else
>>>> +            rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
>>>
>>> ... did the entire computation here, using ?: for the second argument
>>> of the function invocation.
>>>
>> ... replace the if/else pair with sth. like:
>>           rangeset_limit(s->range[i],
>>                          ((i == HVMOP_IO_RANGE_WP_MEM)?
>>                           max_wp_ram_ranges:
>>                           MAX_NR_IO_RANGES));
>> This 'max_wp_ram_ranges' has no particular usages, but the string
>> "s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES]"
>> is too lengthy, and can easily break the 80 column limitation. :)
>> Does this approach sounds OK? :)
>
> Seems better than the original, so okay.
>
>>>> @@ -6009,6 +6016,7 @@ static int hvm_allow_set_param(struct domain *d,
>>>>        case HVM_PARAM_IOREQ_SERVER_PFN:
>>>>        case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>>>>        case HVM_PARAM_ALTP2M:
>>>> +    case HVM_PARAM_MAX_WP_RAM_RANGES:
>>>>            if ( value != 0 && a->value != value )
>>>>                rc = -EEXIST;
>>>>            break;
>>>
>>> Is there a particular reason you want this limit to be unchangeable
>>> after having got set once?
>>>
>> Well, not exactly. :)
>> I added this limit because by now we do not have any approach to
>> change the max range numbers inside ioreq server during run-time.
>> I can add another patch to introduce an xl command, which can change
>> it dynamically. But I doubt the necessity of this new command and
>> am also wonder if this new command would cause more confusion for
>> the user...
>
> And I didn't say you need to expose this to the user. All I asked
> was whether you really mean the value to be a set-once one. If
> yes, the code above is fine. If no, the code above should be
> changed, but there's then still no need to expose a way to
> "manually" adjust the value until a need for such arises.
>

I see. The constraint is not necessary. And I'll remove this code. :)

> Jan
>
>

B.R.
Yu