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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall