|
[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 Stefano, > On 5 Sep 2022, at 23:41, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote: > > On Mon, 5 Sep 2022, Rahul Singh wrote: >>> On 5 Sep 2022, at 1:59 pm, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> >>> wrote: >>> >>> Hi Julien, >>> >>>> On 5 Sep 2022, at 13:08, Julien Grall <julien@xxxxxxx> wrote: >>>> >>>> >>>> >>>> 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". >>> >>> 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 >> >> Please have a look once if this looks okay. >> >> /* >> >> * List of possible features for dom0less domUs >> >> * >> >> * DOM0LESS_ENHANCED_BASIC: Notify the OS it is running on top of Xen. All >> the >> * default >> features (excluding Xenstore) will be >> * available. Note >> that an OS *must* not rely on the >> * availability of >> Xen features if this is not set. >> * DOM0LESS_XENSTORE: Xenstore will be enabled for the VM. >> This feature >> * can't be >> enabled without the DOM0LESS_ENHANCED_BASIC. >> * 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_ENHANCED_BASIC BIT(0, UL) >> >> #define DOM0LESS_XENSTORE BIT(1, UL) >> >> #define DOM0LESS_ENHANCED (DOM0LESS_ENHANCED_BASIC | >> DOM0LESS_XENSTORE) > > Let me have a chance to propose a naming scheme as well :-) > > I agree with Julien: I prefer this proposal compared to the earlier one > by Bertrand and Rahul because I think it is a lot clearer and "ENHANCED" > should mean everything. Also, it makes it easier from a compatibility > perspective because it matches the current definition. > > But I also agree with Bertrand that "BASIC" doesn't sound nice. I think > we should keep "DOM0LESS_ENHANCED" and "DOM0LESS_XENSTORE" as suggested > here, but replace "DOM0LESS_ENHANCED_BASIC" with something better. Some > ideas: > > - DOM0LESS_ENHANCED_LIMITED > - DOM0LESS_ENHANCED_MINI Personally I do not find those more clear then BASIC > - DOM0LESS_ENHANCED_NO_XS This has the problem to be true now but would need renaming if we introduce a definition for an other bit. > - DOM0LESS_ENHANCED_GNT_EVTCHN I would vote for this one as it explicitly state what is in so the bitset system is even more meaningful. > > Any of these are better than BASIC from my point of view. Now I am off > to get the green paint for my shed. Have fun ;-) Cheers Bertrand
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |