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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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