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

[Julien Grall <julien@xxxxxxx>]

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".
As an aside, I think it is more logical if you define DOM0LESS_XENSTORE 
as bit 1. But that's NIT at this point. What matters is the naming.
Cheers,

--
Julien Grall