|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value
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. By the way the comment is wrong and correspond to what should be ENHANCED_FULL here ENHANCED would be the base without Xenstore. Bertrand > > Cheers, > > -- > Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |