On 11/27/19 4:20 PM, Jan Beulich wrote: > On 27.11.2019 17:14, Durrant, Paul wrote: >>> From: Jan Beulich <[email protected]> >>> 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. -George _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
