On 11/27/19 4:43 PM, Durrant, Paul wrote: >> -----Original Message----- >> From: George Dunlap <george.dun...@citrix.com> >> Sent: 27 November 2019 16:34 >> To: Jan Beulich <jbeul...@suse.com>; Durrant, Paul <pdurr...@amazon.com> >> Cc: AndrewCooper <andrew.coop...@citrix.com>; Anthony PERARD >> <anthony.per...@citrix.com>; Roger Pau Monné <roger....@citrix.com>; >> Volodymyr Babchuk <volodymyr_babc...@epam.com>; George Dunlap >> <george.dun...@eu.citrix.com>; Ian Jackson <ian.jack...@eu.citrix.com>; >> Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>; Stefano >> Stabellini <sstabell...@kernel.org>; xen-devel@lists.xenproject.org; >> Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Julien Grall >> <jul...@xen.org>; Wei Liu <w...@xen.org> >> 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 <jbeul...@suse.com> >>>>> 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. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel