Hi Julien,
> On 5 Sep 2022, at 1:59 pm, Bertrand Marquis <bertrand.marq...@arm.com> wrote:
>
> 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
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)
Regards,
Rahul
