Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 5 Sep 2022, at 13:08, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 05/09/2022 12:54, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 5 Sep 2022, at 12:43, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> 
>>> 
>>> On 05/09/2022 12:12, Rahul Singh wrote:
>>>> Hi Julien,
>>> 
>>> Hi Rahul,
>>> 
>>>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xxxxxxx> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 02/09/2022 16:54, Rahul Singh wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Rahul,
>>>>> 
>>>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xxxxxxx> wrote:
>>>>>>> 
>>>>>>> Hi Bertrand,
>>>>>>> 
>>>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xxxxxxx> wrote:
>>>>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = 
>>>>>>>>> false. I think it would be clearer if ``dom0less_enhanced`` is turned 
>>>>>>>>> to an enum with 3 values:
>>>>>>>>> - None
>>>>>>>>> - NOXENSTORE/BASIC
>>>>>>>>> - FULLY_ENHANCED
>>>>>>>>> 
>>>>>>>>> If we want to be future proof, I would use a field 'flags' where 
>>>>>>>>> non-zero means enhanced. Each bit would indicate which features of 
>>>>>>>>> Xen is exposed.
>>>>>>>> I think that could be a good solution if we do it this way:
>>>>>>>> - define a dom0less feature field and have defines like the following:
>>>>>>>> #define DOM0LESS_GNTTAB
>>>>>>>> #define DOM0LESS_EVENTCHN
>>>>>>>> #define DOM0LESS_XENSTORE >
>>>>>>>> - define dom0less enhanced as the right combination:
>>>>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| 
>>>>>>>> DOM0LESS_XENSTORE)
>>>>>>> 
>>>>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead 
>>>>>>> of defining a bit for gnttab and evtchn. This will avoid the question 
>>>>>>> of why we are introducing bits for both features but not the 
>>>>>>> hypercall...
>>>>>>> 
>>>>>>> As this is an internal interface, it would be easier to modify 
>>>>>>> afterwards.
>>>>>> How about this?
>>>>>> /*
>>>>>>  * List of possible features for dom0less domUs
>>>>>>  *
>>>>>>  * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>>>>>  *                                                          evtchn, will 
>>>>>> be enabled for the VM.
>>>>> 
>>>>> Technically, the guest can already use the grant-table and evtchn 
>>>>> interfaces. This also reads quite odd to me because "including" doesn't 
>>>>> tell what's not enabled. So one could assume Xenstored is also enabled. 
>>>>> In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more 
>>>>> confusing.
>>>>> 
>>>>> So I would suggest the following wording:
>>>>> 
>>>>> "Notify the OS it is running on top of Xen. All the default features but 
>>>>> Xenstore will be available. Note that an OS *must* not rely on the 
>>>>> availability of Xen features if this is not set.
>>>>> "
>>>>> 
>>>>> The wording can be updated once we properly disable event channel/grant 
>>>>> table when the flag is not set.
>>>>> 
>>>>>>  * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
>>>>> 
>>>>> I would make clear this can't be used without the first one.
>>>>> 
>>>>>>  * DOM0LESS_ENHANCED:              Xen PV interfaces, including 
>>>>>> grant-table xenstore >   *                                               
>>>>>>            and
>>>>> evtchn, will be enabled for the VM.
>>>>> 
>>>>> See above about "PV interfaces". So I would suggest to reword to:
>>>>> 
>>>>> "Notify the OS it is running on top of Xen. All the default features 
>>>>> (including Xenstore) will be available".
>>>>> 
>>>>>>  */
>>>>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>>>>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
>>>>> 
>>>>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE 
>>>>> as bit 0 and 1 set.
>>>>> 
>>>>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | 
>>>>>> DOM0LESS_XENSTORE)
>>>>  Bertrand and I discussed this again we came to the conclusion that 
>>>> DOM0LESS_ENHANCED_BASIC is not
>>>> the suitable name as this makes the code unclear and does not correspond 
>>>> to DT settings. We propose this
>>>> please let me know your thoughts.
>>> 
>>> To me the default of "enhanced" should be all Xen features. Anything else 
>>> should be consider as reduced/basic/minimum. Hence why I still think we 
>>> need to add it in the name even if this is not what we expose in the DT. In 
>>> fact...
>>>>  /*
>>>>  * List of possible features for dom0less domUs
>>>>  *
>>>>  * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM. 
>>>> This feature
>>>>  *                                                 can't be enabled 
>>>> without the DOM0LESS_ENHANCED.
>>>>  * DOM0LESS_ENHANCED:              Notify the OS it is running on top of 
>>>> Xen. All the
>>>>  *                                                         default 
>>>> features (including Xenstore) will be
>>>>  *                                                         available. Note 
>>>> that an OS *must* not rely on the
>>>>  *                                                         availability of 
>>>> Xen features if this is not set.
>>> 
>>> ... what you wrote here match what I wrote above. So it is not clear to me 
>>> what's the point of having a flag DOM0LESS_XENSTORE.
>> When we looked at the code with the solution using BASIC, it was really not 
>> easy to understand.
> 
> I don't quite understand how this is different from ENHANCED, ENHANCED_FULL. 
> In fact, without looking at the documentation, they mean exactly the same...
> 
> The difference between "BASIC" and "ENHANCED" is clear. You know that in one 
> case, you would get less than the other.
> 
>> By the way the comment is wrong and correspond to what should be 
>> ENHANCED_FULL here
>> ENHANCED would be the base without Xenstore.
> 
> Thanks for the confirmation. I am afraid, I am strongly against the 
> terminology you proposed (see above why).
> 
> I think BASIC (or similar name) is better. But I am open to suggestion so 
> long it is not "DOM0LESS_ENHANCED" vs "DOM0LESS_ENHANCED_FULL".

I do not agree but I think this is only internal and could easily be modified 
one day if we have more use-cases.
So let’s go for BASIC and unblock this before the feature freeze.

Bertrand

> 
> As an aside, I think it is more logical if you define DOM0LESS_XENSTORE as 
> bit 1. But that's NIT at this point. What matters is the naming.
> 
> Cheers,
> 
> -- 
> Julien Grall