[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





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".

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.