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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> 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.  
 
/*                                                                              
 * 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.    
 */                                                                             
#define DOM0LESS_XENSTORE       BIT(0, UL)                                      
#define DOM0LESS_ENHANCED       BIT(1,UL)                                       
#define DOM0LESS_ENHANCED_FULL  (DOM0LESS_XENSTORE | DOM0LESS_ENHANCED)

Regards,
Rahul