Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

["Durrant, Paul" <pdurrant@xxxxxxxxxx>]

> -----Original Message-----
> From: George Dunlap <george.dunlap@xxxxxxxxxx>
> Sent: 27 November 2019 16:34
> To: Jan Beulich <jbeulich@xxxxxxxx>; Durrant, Paul <pdurrant@xxxxxxxxxx>
> Cc: AndrewCooper <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>;
> Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Julien Grall
> <julien@xxxxxxx>; Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On 11/27/19 4:20 PM, Jan Beulich wrote:
> > On 27.11.2019 17:14,  Durrant, Paul  wrote:
> >>> From: Jan Beulich <jbeulich@xxxxxxxx>
> >>> Sent: 27 November 2019 15:56
> >>>
> >>> On 27.11.2019 15:37, Paul Durrant wrote:
> >>>> --- a/xen/arch/arm/setup.c
> >>>> +++ b/xen/arch/arm/setup.c
> >>>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
> >>> boot_phys_offset,
> >>>>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> >>>>          .max_evtchn_port = -1,
> >>>>          .max_grant_frames = gnttab_dom0_frames(),
> >>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
> >>>> +        .max_maptrack_frames = -1,
> >>>>      };
> >>>>      int rc;
> >>>>
> >>>> --- a/xen/arch/x86/setup.c
> >>>> +++ b/xen/arch/x86/setup.c
> >>>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
> >>> mbi_p)
> >>>>      struct xen_domctl_createdomain dom0_cfg = {
> >>>>          .flags = IS_ENABLED(CONFIG_TBOOT) ?
> XEN_DOMCTL_CDF_s3_integrity
> >>> : 0,
> >>>>          .max_evtchn_port = -1,
> >>>> -        .max_grant_frames = opt_max_grant_frames,
> >>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
> >>>> +        .max_grant_frames = -1,
> >>>> +        .max_maptrack_frames = -1,
> >>>>      };
> >>>
> >>> With these there's no need anymore for opt_max_maptrack_frames to
> >>> be non-static. Sadly Arm still wants opt_max_grant_frames
> >>> accessible in gnttab_dom0_frames().
> >>
> >> Yes, I was about to make them static until I saw what the ARM code did.
> >
> > But the one that Arm doesn't need should become static now.
> >
> >>>> --- a/xen/common/grant_table.c
> >>>> +++ b/xen/common/grant_table.c
> >>>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
> >>>>      return -ENOMEM;
> >>>>  }
> >>>>
> >>>> -int grant_table_init(struct domain *d, unsigned int
> max_grant_frames,
> >>>> -                     unsigned int max_maptrack_frames)
> >>>> +int grant_table_init(struct domain *d, int max_grant_frames,
> >>>> +                     int max_maptrack_frames)
> >>>>  {
> >>>>      struct grant_table *gt;
> >>>>      int ret = -ENOMEM;
> >>>>
> >>>> +    /* Default to maximum value if no value was specified */
> >>>> +    if ( max_grant_frames < 0 )
> >>>> +        max_grant_frames = opt_max_grant_frames;
> >>>> +    if ( max_maptrack_frames < 0 )
> >>>> +        max_maptrack_frames = opt_max_maptrack_frames;
> >>>> +
> >>>>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> >>>
> >>> I take it we don't expect people to specify 2^^31 or more
> >>> frames for either option. It looks like almost everything
> >>> here would cope, except for this very comparison. Nevertheless
> >>> I wonder whether you wouldn't better confine both values to
> >>> [0, INT_MAX] now, including when adjusted at runtime.
> >>
> >> I can certainly remove the 'U' from the definition of
> >> INITIAL_NR_GRANT_FRAMES,
> >
> > Oh, I didn't pay attention that is has a U on it - in this case
> > the comparison above is fine.
> >
> >> but do you want me to make opt_max_grant_frames and
> >> opt_max_maptrack_frames into signed ints and add signed parser
> >> code too?
> >
> > Definitely not. They should remain unsigned quantities, but their
> > values may need sanity checking now.
> >
> >> I also don't understand the 'adjusted at runtime' part.
> >
> > Well, for a command line drive value you could adjust an out of
> > bounds value in some __init function. But for runtime modifiable
> > settings you won't get away this easily.
> 
> TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
> long)(-1) or something, and explicitly compare to that.  That leaves
> open the possibility of having more sentinel values if we decided we
> wanted them.

I'm extremely confused now. What do you want me to compare and where?

I assume we're talking about the opt_XXX values. Am I supposed to stop >INT_MAX 
being assigned to them? Or should I define local unsigned values for 
max_maptrack/grant_frames and simply initialize them to the passed-in arg (if 
>= 0) or the opt_XXX value otherwise.

  Paul

> 
>  -George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel