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

["Yu, Zhang" <yu.c.zhang@xxxxxxxxxxxxxxx>]

On 1/26/2016 7:00 PM, Jan Beulich wrote:
> > > > 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).
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

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