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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.01.16 at 08:32, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> On 1/22/2016 4:01 PM, Jan Beulich wrote:
>>>>> On 22.01.16 at 04:20, <yu.c.zhang@xxxxxxxxxxxxxxx> 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).

> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel