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

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

Thank you, Jan.

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.
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? :)

>> @@ -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...
> Jan
>
>
B.R.
Yu
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>