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

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall