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.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
