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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 11/27/19 4:43 PM, Durrant, Paul wrote:
>> -----Original Message-----
>> From: George Dunlap <george.dunlap@xxxxxxxxxx>
>> Sent: 27 November 2019 16:34
>> To: Jan Beulich <jbeulich@xxxxxxxx>; Durrant, Paul <pdurrant@xxxxxxxxxx>
>> Cc: AndrewCooper <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>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
>> 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 11/27/19 4:20 PM, Jan Beulich wrote:
>>> 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.
>>
>> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel