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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 5 Sep 2022, Rahul Singh wrote:
> > 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)

Let me have a chance to propose a naming scheme as well :-)

I agree with Julien: I prefer this proposal compared to the earlier one
by Bertrand and Rahul because I think it is a lot clearer and "ENHANCED"
should mean everything. Also, it makes it easier from a compatibility
perspective because it matches the current definition.

But I also agree with Bertrand that "BASIC" doesn't sound nice. I think
we should keep "DOM0LESS_ENHANCED" and "DOM0LESS_XENSTORE" as suggested
here, but replace "DOM0LESS_ENHANCED_BASIC" with something better. Some
ideas:

- DOM0LESS_ENHANCED_LIMITED
- DOM0LESS_ENHANCED_MINI
- DOM0LESS_ENHANCED_NO_XS
- DOM0LESS_ENHANCED_GNT_EVTCHN

Any of these are better than BASIC from my point of view. Now I am off
to get the green paint for my shed.