Hi Julien, > 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. /* * 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. */ #define DOM0LESS_XENSTORE BIT(0, UL) #define DOM0LESS_ENHANCED BIT(1,UL) #define DOM0LESS_ENHANCED_FULL (DOM0LESS_XENSTORE | DOM0LESS_ENHANCED)
Regards, Rahul
