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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 5 Sep 2022, at 1:59 pm, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
> 
> 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

Please have a look once if this looks okay.

/*                                                                              
 * List of possible features for dom0less domUs                                 
 *                                                                              
 * DOM0LESS_ENHANCED_BASIC:     Notify the OS it is running on top of Xen. All 
the  
 *                                                              default 
features (excluding Xenstore) will be       
 *                                                              available. Note 
that an OS *must* not rely on the   
 *                                                              availability of 
Xen features if this is not set.    
 * DOM0LESS_XENSTORE:                   Xenstore will be enabled for the VM. 
This feature   
 *                                                              can't be 
enabled without the DOM0LESS_ENHANCED_BASIC.                            
 * 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.    
 */                                                                             
#define DOM0LESS_ENHANCED_BASIC     BIT(0, UL)                                  
#define DOM0LESS_XENSTORE                  BIT(1, UL)                           
       
#define DOM0LESS_ENHANCED                 (DOM0LESS_ENHANCED_BASIC  |  
DOM0LESS_XENSTORE)

Regards,
Rahul