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

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

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 27 November 2019 15:56
> To: Durrant, Paul <pdurrant@xxxxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <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>; 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 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.
 
> > --- 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, 
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? I also don't understand the 
'adjusted at runtime' part.

  Paul

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