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:52
> To: Durrant, Paul <pdurrant@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> 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:43 PM, Durrant, Paul wrote:
> >> -----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.
> 
> In this version of the patch, you change the domctl arguments from
> uint32_t to int32_t.  I would leave them uint32_t, and if (
> max_grant_frames == XENSOMETHING_MAX_DEFAULT ) max_grant_frames = opt_&c.
> 
> Then the only invalid value we have to worry about is checking for
> XENSOMETHING_MAX_DEFAULT.
> 
> This is a suggestion, and I wouldn't argue strongly if someone thought
> it was a bad idea, but it seems like the most straightforward option to
> me.

AFAICT the definition of that invalid value is going to be needed by both the 
grant table code and the user-space toolstack code so I guess the logical place 
for the definition would be a tools-only section of the public grant table 
header? TBH I prefer the idea of any negative value being default though.
As long as the xl/libxl parts don't allow a *specified* value > INT_MAX then 
that should be fine, although for the full story a custom parser for the 
command line values should also be added to ensure the same semantics there.

  Paul

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