[PATCH] public/gnttab: relax v2 recommendation

[PATCH] public/gnttab: relax v2 recommendation

[Jan Beulich]

With there being a way to disable v2 support, telling new guests to use
v2 exclusively is not a good suggestion.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
  */
 
 /*
- * Version 1 of the grant table entry structure is maintained purely
- * for backwards compatibility.  New guests should use version 2.
+ * Version 1 of the grant table entry structure is maintained largely for
+ * backwards compatibility.  New guests are recommended to support using
+ * version 2 to overcome version 1 limitations, but to be able to fall back
+ * to version 1.
  */
 #if __XEN_INTERFACE_VERSION__ < 0x0003020a
 #define grant_entry_v1 grant_entry

Re: [PATCH] public/gnttab: relax v2 recommendation

[Luca Fancellu]

> On 29 Apr 2021, at 14:10, Jan Beulich <jbeulich@suse.com> wrote:
> 
> With there being a way to disable v2 support, telling new guests to use
> v2 exclusively is not a good suggestion.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>  */
> 
> /*
> - * Version 1 of the grant table entry structure is maintained purely
> - * for backwards compatibility.  New guests should use version 2.
> + * Version 1 of the grant table entry structure is maintained largely for
> + * backwards compatibility.  New guests are recommended to support using
> + * version 2 to overcome version 1 limitations, but to be able to fall back
> + * to version 1.
>  */
> #if __XEN_INTERFACE_VERSION__ < 0x0003020a
> #define grant_entry_v1 grant_entry
> 

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

Re: [PATCH] public/gnttab: relax v2 recommendation

[Julien Grall]

Hi Jan,

On 29/04/2021 14:10, Jan Beulich wrote:
> With there being a way to disable v2 support, telling new guests to use
> v2 exclusively is not a good suggestion.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>    */
>   
>   /*
> - * Version 1 of the grant table entry structure is maintained purely
> - * for backwards compatibility.  New guests should use version 2.
> + * Version 1 of the grant table entry structure is maintained largely for
> + * backwards compatibility.  New guests are recommended to support using
> + * version 2 to overcome version 1 limitations, but to be able to fall back
> + * to version 1.

v2 is not supported on Arm and I don't see it coming anytime soon. 
AFAIK, Linux will also not use grant table v2 unless the guest has a 
address space larger than 44 (?) bits.

I can't remember why Linux decided to not use it everywhere, but this is 
a sign that v2 is not always desired.

So I think it would be better to recommend new guest to use v1 unless 
they hit the limitations (to be details).

Cheers,

-- 
Julien Grall

Re: [PATCH] public/gnttab: relax v2 recommendation

[Jan Beulich]

On 30.04.2021 10:19, Julien Grall wrote:
> On 29/04/2021 14:10, Jan Beulich wrote:
>> With there being a way to disable v2 support, telling new guests to use
>> v2 exclusively is not a good suggestion.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/include/public/grant_table.h
>> +++ b/xen/include/public/grant_table.h
>> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>>    */
>>   
>>   /*
>> - * Version 1 of the grant table entry structure is maintained purely
>> - * for backwards compatibility.  New guests should use version 2.
>> + * Version 1 of the grant table entry structure is maintained largely for
>> + * backwards compatibility.  New guests are recommended to support using
>> + * version 2 to overcome version 1 limitations, but to be able to fall back
>> + * to version 1.
> 
> v2 is not supported on Arm and I don't see it coming anytime soon. 
> AFAIK, Linux will also not use grant table v2 unless the guest has a 
> address space larger than 44 (?) bits.

Yes, as soon as there are frame numbers not representable in 32 bits.

> I can't remember why Linux decided to not use it everywhere, but this is 
> a sign that v2 is not always desired.
> 
> So I think it would be better to recommend new guest to use v1 unless 
> they hit the limitations (to be details).

IOW you'd prefer s/be able to fall back/default/? I'd be fine that way.

Jan

Re: [PATCH] public/gnttab: relax v2 recommendation

[Julien Grall]

Hi Jan,

On 30/04/2021 09:36, Jan Beulich wrote:
> On 30.04.2021 10:19, Julien Grall wrote:
>> On 29/04/2021 14:10, Jan Beulich wrote:
>>> With there being a way to disable v2 support, telling new guests to use
>>> v2 exclusively is not a good suggestion.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/include/public/grant_table.h
>>> +++ b/xen/include/public/grant_table.h
>>> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>>>     */
>>>    
>>>    /*
>>> - * Version 1 of the grant table entry structure is maintained purely
>>> - * for backwards compatibility.  New guests should use version 2.
>>> + * Version 1 of the grant table entry structure is maintained largely for
>>> + * backwards compatibility.  New guests are recommended to support using
>>> + * version 2 to overcome version 1 limitations, but to be able to fall back
>>> + * to version 1.
>>
>> v2 is not supported on Arm and I don't see it coming anytime soon.
>> AFAIK, Linux will also not use grant table v2 unless the guest has a
>> address space larger than 44 (?) bits.
> 
> Yes, as soon as there are frame numbers not representable in 32 bits.
> 
>> I can't remember why Linux decided to not use it everywhere, but this is
>> a sign that v2 is not always desired.
>>
>> So I think it would be better to recommend new guest to use v1 unless
>> they hit the limitations (to be details).
> 
> IOW you'd prefer s/be able to fall back/default/? I'd be fine that way

Yes. We would also need to document the limitations as they don't seem 
to be (clearly?) written down in the headers.

Cheers,

-- 
Julien Grall

Re: [PATCH] public/gnttab: relax v2 recommendation

[Jan Beulich]

On 05.05.2021 10:12, Julien Grall wrote:
> Hi Jan,
> 
> On 30/04/2021 09:36, Jan Beulich wrote:
>> On 30.04.2021 10:19, Julien Grall wrote:
>>> On 29/04/2021 14:10, Jan Beulich wrote:
>>>> With there being a way to disable v2 support, telling new guests to use
>>>> v2 exclusively is not a good suggestion.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/include/public/grant_table.h
>>>> +++ b/xen/include/public/grant_table.h
>>>> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>>>>     */
>>>>    
>>>>    /*
>>>> - * Version 1 of the grant table entry structure is maintained purely
>>>> - * for backwards compatibility.  New guests should use version 2.
>>>> + * Version 1 of the grant table entry structure is maintained largely for
>>>> + * backwards compatibility.  New guests are recommended to support using
>>>> + * version 2 to overcome version 1 limitations, but to be able to fall back
>>>> + * to version 1.
>>>
>>> v2 is not supported on Arm and I don't see it coming anytime soon.
>>> AFAIK, Linux will also not use grant table v2 unless the guest has a
>>> address space larger than 44 (?) bits.
>>
>> Yes, as soon as there are frame numbers not representable in 32 bits.
>>
>>> I can't remember why Linux decided to not use it everywhere, but this is
>>> a sign that v2 is not always desired.
>>>
>>> So I think it would be better to recommend new guest to use v1 unless
>>> they hit the limitations (to be details).
>>
>> IOW you'd prefer s/be able to fall back/default/? I'd be fine that way
> 
> Yes.

Okay, I've changed that part, but ...

> We would also need to document the limitations as they don't seem 
> to be (clearly?) written down in the headers.

... I'm struggling to see where (and perhaps even why) this would be
needed. The v1 and v2 grant table entry formats are all there. I'm
inclined to consider this an orthogonal addition to make by whoever
thinks such an addition is needed in the first place.

Jan

Re: [PATCH] public/gnttab: relax v2 recommendation

[Julien Grall]

Hi Jan,

On 05/05/2021 09:24, Jan Beulich wrote:
> On 05.05.2021 10:12, Julien Grall wrote:
>> Hi Jan,
>>
>> On 30/04/2021 09:36, Jan Beulich wrote:
>>> On 30.04.2021 10:19, Julien Grall wrote:
>>>> On 29/04/2021 14:10, Jan Beulich wrote:
>>>>> With there being a way to disable v2 support, telling new guests to use
>>>>> v2 exclusively is not a good suggestion.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> --- a/xen/include/public/grant_table.h
>>>>> +++ b/xen/include/public/grant_table.h
>>>>> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>>>>>      */
>>>>>     
>>>>>     /*
>>>>> - * Version 1 of the grant table entry structure is maintained purely
>>>>> - * for backwards compatibility.  New guests should use version 2.
>>>>> + * Version 1 of the grant table entry structure is maintained largely for
>>>>> + * backwards compatibility.  New guests are recommended to support using
>>>>> + * version 2 to overcome version 1 limitations, but to be able to fall back
>>>>> + * to version 1.
>>>>
>>>> v2 is not supported on Arm and I don't see it coming anytime soon.
>>>> AFAIK, Linux will also not use grant table v2 unless the guest has a
>>>> address space larger than 44 (?) bits.
>>>
>>> Yes, as soon as there are frame numbers not representable in 32 bits.
>>>
>>>> I can't remember why Linux decided to not use it everywhere, but this is
>>>> a sign that v2 is not always desired.
>>>>
>>>> So I think it would be better to recommend new guest to use v1 unless
>>>> they hit the limitations (to be details).
>>>
>>> IOW you'd prefer s/be able to fall back/default/? I'd be fine that way
>>
>> Yes.
> 
> Okay, I've changed that part, but ...
> 
>> We would also need to document the limitations as they don't seem
>> to be (clearly?) written down in the headers.
> 
> ... I'm struggling to see where (and perhaps even why) this would be
> needed. The v1 and v2 grant table entry formats are all there. I'm
> inclined to consider this an orthogonal addition to make by whoever
> thinks such an addition is needed in the first place.

The current comment is not mentionning about limitations but instead say 
"new OS should use v2". Your proposal is to say "default to v1 but use 
v2 if you hit limitations".

As Xen developper, I am aware of a single limitation (the 44 bits). But 
here you suggest there are multiple ones. I could probably figure out 
the others if I dig into the code...

Now imagine, you are an OS developper new to Xen. I don't think this is 
fair to say "there are limitations but I will not tell you directly. 
Instead you should try to infer them from the definitions". There is a 
chance, he/she may have missed some of the limitations and therefore the 
decision to switch between v1 and v2 would be done incorrectly.

In addition to that, it also means she/he may end up to implement the 
two versions when implementing v1 may just be sufficient (custom OSes 
may never need 44 bits worth of address space).

This is not a very friendly way to work on Xen. FAOD, I am not saying 
that the other headers are perfect... Instead, I am saying we ought to 
improve new wording to make the project a bit more welcoming.

Cheers,

-- 
Julien Grall

Re: [PATCH] public/gnttab: relax v2 recommendation

[Jan Beulich]

On 05.05.2021 10:51, Julien Grall wrote:
> Hi Jan,
> 
> On 05/05/2021 09:24, Jan Beulich wrote:
>> On 05.05.2021 10:12, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 30/04/2021 09:36, Jan Beulich wrote:
>>>> On 30.04.2021 10:19, Julien Grall wrote:
>>>>> On 29/04/2021 14:10, Jan Beulich wrote:
>>>>>> With there being a way to disable v2 support, telling new guests to use
>>>>>> v2 exclusively is not a good suggestion.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>
>>>>>> --- a/xen/include/public/grant_table.h
>>>>>> +++ b/xen/include/public/grant_table.h
>>>>>> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>>>>>>      */
>>>>>>     
>>>>>>     /*
>>>>>> - * Version 1 of the grant table entry structure is maintained purely
>>>>>> - * for backwards compatibility.  New guests should use version 2.
>>>>>> + * Version 1 of the grant table entry structure is maintained largely for
>>>>>> + * backwards compatibility.  New guests are recommended to support using
>>>>>> + * version 2 to overcome version 1 limitations, but to be able to fall back
>>>>>> + * to version 1.
>>>>>
>>>>> v2 is not supported on Arm and I don't see it coming anytime soon.
>>>>> AFAIK, Linux will also not use grant table v2 unless the guest has a
>>>>> address space larger than 44 (?) bits.
>>>>
>>>> Yes, as soon as there are frame numbers not representable in 32 bits.
>>>>
>>>>> I can't remember why Linux decided to not use it everywhere, but this is
>>>>> a sign that v2 is not always desired.
>>>>>
>>>>> So I think it would be better to recommend new guest to use v1 unless
>>>>> they hit the limitations (to be details).
>>>>
>>>> IOW you'd prefer s/be able to fall back/default/? I'd be fine that way
>>>
>>> Yes.
>>
>> Okay, I've changed that part, but ...
>>
>>> We would also need to document the limitations as they don't seem
>>> to be (clearly?) written down in the headers.
>>
>> ... I'm struggling to see where (and perhaps even why) this would be
>> needed. The v1 and v2 grant table entry formats are all there. I'm
>> inclined to consider this an orthogonal addition to make by whoever
>> thinks such an addition is needed in the first place.
> 
> The current comment is not mentionning about limitations but instead say 
> "new OS should use v2". Your proposal is to say "default to v1 but use 
> v2 if you hit limitations".

I've intentionally not said "if you hit limitations".

> As Xen developper, I am aware of a single limitation (the 44 bits). But 
> here you suggest there are multiple ones. I could probably figure out 
> the others if I dig into the code...
> 
> Now imagine, you are an OS developper new to Xen. I don't think this is 
> fair to say "there are limitations but I will not tell you directly. 
> Instead you should try to infer them from the definitions". There is a 
> chance, he/she may have missed some of the limitations and therefore the 
> decision to switch between v1 and v2 would be done incorrectly.
> 
> In addition to that, it also means she/he may end up to implement the 
> two versions when implementing v1 may just be sufficient (custom OSes 
> may never need 44 bits worth of address space).

This limitation is based on guest attributes only for HVM. For PV,
host properties matter, so PV guests strictly should be able to
fall back to v2, or they are liable to end up not working on large
hosts.

The other limitation is more a performance one - v2 separates grant
attributes from grant status.

> This is not a very friendly way to work on Xen. FAOD, I am not saying 
> that the other headers are perfect... Instead, I am saying we ought to 
> improve new wording to make the project a bit more welcoming.

I don't think the public header is the place to go into such lengths,
especially when all the information is already there. Textually
describing the same aspects should be done elsewhere imo. I'm of the
firm opinion that the patch as is represents an improvement. There's
no suggestion anywhere that things couldn't be further improved, as
is the case about always.

Since I created this patch only because my request to correct the
statement led to me being asked to provide the suggested new text,
may I suggest that you pick up this patch or create one from scratch
to accommodate all your wishes, if you believe this extra information
really belongs there? All I'm after is to correct a statement that's
actively misleading.

Jan

Re: [PATCH] public/gnttab: relax v2 recommendation

[Julien Grall]

Hi Jan,

On 05/05/2021 11:57, Jan Beulich wrote:
> On 05.05.2021 10:51, Julien Grall wrote:
>> On 05/05/2021 09:24, Jan Beulich wrote:
>>> On 05.05.2021 10:12, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 30/04/2021 09:36, Jan Beulich wrote:
>>>>> On 30.04.2021 10:19, Julien Grall wrote:
>>>>>> On 29/04/2021 14:10, Jan Beulich wrote:
>>>>>>> With there being a way to disable v2 support, telling new guests to use
>>>>>>> v2 exclusively is not a good suggestion.
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>
>>>>>>> --- a/xen/include/public/grant_table.h
>>>>>>> +++ b/xen/include/public/grant_table.h
>>>>>>> @@ -121,8 +121,10 @@ typedef uint32_t grant_ref_t;
>>>>>>>       */
>>>>>>>      
>>>>>>>      /*
>>>>>>> - * Version 1 of the grant table entry structure is maintained purely
>>>>>>> - * for backwards compatibility.  New guests should use version 2.
>>>>>>> + * Version 1 of the grant table entry structure is maintained largely for
>>>>>>> + * backwards compatibility.  New guests are recommended to support using
>>>>>>> + * version 2 to overcome version 1 limitations, but to be able to fall back
>>>>>>> + * to version 1.
>>>>>>
>>>>>> v2 is not supported on Arm and I don't see it coming anytime soon.
>>>>>> AFAIK, Linux will also not use grant table v2 unless the guest has a
>>>>>> address space larger than 44 (?) bits.
>>>>>
>>>>> Yes, as soon as there are frame numbers not representable in 32 bits.
>>>>>
>>>>>> I can't remember why Linux decided to not use it everywhere, but this is
>>>>>> a sign that v2 is not always desired.
>>>>>>
>>>>>> So I think it would be better to recommend new guest to use v1 unless
>>>>>> they hit the limitations (to be details).
>>>>>
>>>>> IOW you'd prefer s/be able to fall back/default/? I'd be fine that way
>>>>
>>>> Yes.
>>>
>>> Okay, I've changed that part, but ...
>>>
>>>> We would also need to document the limitations as they don't seem
>>>> to be (clearly?) written down in the headers.
>>>
>>> ... I'm struggling to see where (and perhaps even why) this would be
>>> needed. The v1 and v2 grant table entry formats are all there. I'm
>>> inclined to consider this an orthogonal addition to make by whoever
>>> thinks such an addition is needed in the first place.
>>
>> The current comment is not mentionning about limitations but instead say
>> "new OS should use v2". Your proposal is to say "default to v1 but use
>> v2 if you hit limitations".
> 
> I've intentionally not said "if you hit limitations".

This doesn't really change the point I made. :)

>> This is not a very friendly way to work on Xen. FAOD, I am not saying
>> that the other headers are perfect... Instead, I am saying we ought to
>> improve new wording to make the project a bit more welcoming.
> 
> I don't think the public header is the place to go into such lengths,
> especially when all the information is already there. Textually
> describing the same aspects should be done elsewhere imo.

The goal of comments is to document anything that cannot be easily 
inferred. This is the case of the limitations you mention but don't 
describe.

  I'm of the
> firm opinion that the patch as is represents an improvement. 

I haven't suggested that patch wasn't improvement. However, I think it 
can easily be improved further.

> There's
> no suggestion anywhere that things couldn't be further improved, as
> is the case about always.
> 
> Since I created this patch only because my request to correct the
> statement led to me being asked to provide the suggested new text,
> may I suggest that you pick up this patch or create one from scratch
> to accommodate all your wishes, if you believe this extra information
> really belongs there? All I'm after is to correct a statement that's
> actively misleading.

I am a bit confused with this answer. Are you saying you are not willing 
to write it but if someone else does you will accept it?

Cheers,

-- 
Julien Grall

Re: [PATCH] public/gnttab: relax v2 recommendation

[Jan Beulich]

On 05.05.2021 13:50, Julien Grall wrote:
> On 05/05/2021 11:57, Jan Beulich wrote:
>> Since I created this patch only because my request to correct the
>> statement led to me being asked to provide the suggested new text,
>> may I suggest that you pick up this patch or create one from scratch
>> to accommodate all your wishes, if you believe this extra information
>> really belongs there? All I'm after is to correct a statement that's
>> actively misleading.
> 
> I am a bit confused with this answer. Are you saying you are not willing 
> to write it but if someone else does you will accept it?

Along these lines, yes. One problem is if I start enumerating the
limitations, arguing could easily start whether I cover them all,
some are not worthwhile to mention, etc. Plus, as said, I'm not
convinced this is the right place for the information. In such a
case I can accept someone else thinking differently and making
such a change, but I'd like to be convinced of changes I'm doing
myself.

Jan