Hi Julien, > On 5 Sep 2022, at 13:08, Julien Grall <jul...@xen.org> wrote: > > > > On 05/09/2022 12:54, Bertrand Marquis wrote: >> Hi Julien, >>> On 5 Sep 2022, at 12:43, Julien Grall <jul...@xen.org> wrote: >>> >>> >>> >>> On 05/09/2022 12:12, Rahul Singh wrote: >>>> Hi Julien, >>> >>> Hi Rahul, >>> >>>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <jul...@xen.org> wrote: >>>>> >>>>> >>>>> >>>>> On 02/09/2022 16:54, Rahul Singh wrote: >>>>>> Hi Julien, >>>>> >>>>> Hi Rahul, >>>>> >>>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <jul...@xen.org> wrote: >>>>>>> >>>>>>> Hi Bertrand, >>>>>>> >>>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote: >>>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <jul...@xen.org> 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 > > 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
