[PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[Juergen Gross]

In the past there have been spotted some shortcomings in the Xenstore
interface, which should be repaired. Those are in detail:

- Using driver domains for large number of domains needs per domain
  Xenstore quota [1]. The feedback sent was rather slim (one reply),
  but it was preferring a new set of wire commands.

- XSA-349 [2] has shown that the current definition of watches is not
  optimal, as it will trigger lots of events when a single one would
  suffice: for detecting new backend devices the backends in the Linux
  kernel are registering a watch for e.g. "/local/domain/0/backend"
  which will fire for ANY sub-node written below this node (on a test
  machine this added up to 91 watch events for only 3 devices).
  This can be limited dramatically by extending the XS_WATCH command
  to take another optional parameter specifying the depth of
  subdirectories to be considered for sending watch events ("0" would
  trigger a watch event only if the watched node itself being written).

- New features like above being added might make migration of guests
  between hosts with different Xenstore variants harder, so it should
  be possible to set the available feature set per domain. For socket
  connections it should be possible to read the available features.

- The special watches @introduceDomain and @releaseDomain are rather
  cumbersome to use, as they only tell you that SOME domain has been
  introduced/released. Any consumer of those watches needs to scan
  all domains on the host in order to find out the domid, causing
  significant pressure on the dominfo hypercall (imagine a system
  with 1000 domains running and one domain dying - there will be more
  than 1000 watch events triggered and 1000 xl daemons will try to
  find out whether "their" domain has died). Those watches should be
  enhanced to optionally be specific to a single domain and to let the
  event carry the related domid.

As some of those extensions will need to be considered in the Xenstore
migration stream, they should be defined in one go (in fact the 4th one
wouldn't need that, but it can easily be connected to the 2nd one).
As such extensions need to be flagged in the "features" in the ring
page anyway, it is fine to implement them independently.

Add the documentation of the new commands/features.

[1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
[2]: http://xenbits.xen.org/xsa/advisory-349.html

Changes in V2:
- added new patch 1
- remove feature bits for dom0-only features
- get-features without domid returns Xenstore supported features
- get/set-quota without domid for global quota access

Juergen Gross (4):
  tools/xenstore: modify feature bit specification in xenstore-ring.txt
  tools/xenstore: add documentation for new set/get-feature commands
  tools/xenstore: add documentation for new set/get-quota commands
  tools/xenstore: add documentation for extended watch command

 docs/misc/xenstore-ring.txt | 10 ++++----
 docs/misc/xenstore.txt      | 47 ++++++++++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.35.3

[PATCH v2 1/4] tools/xenstore: modify feature bit specification in xenstore-ring.txt

[Juergen Gross]

Instead of specifying the feature bits in xenstore-ring.txt as a mask
value use bit numbers. This will make the specification easier to read
when adding more features.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch (triggered by Julien Grall)
---
 docs/misc/xenstore-ring.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
index f3d6ca4264..2792d13530 100644
--- a/docs/misc/xenstore-ring.txt
+++ b/docs/misc/xenstore-ring.txt
@@ -62,12 +62,13 @@ the server feature bitmap. The server features are offered to the guest;
 it is up to the guest whether to use them or not. The guest should ignore
 any unknown feature bits.
 
-The following features are defined:
+The following features are defined (bit number 0 is equivalent to a mask
+value of 1):
 
-Mask    Description
+Bit     Description
 -----------------------------------------------------------------
-1       Ring reconnection (see the ring reconnection feature below)
-2       Connection error indicator (see connection error feature below)
+0       Ring reconnection (see the ring reconnection feature below)
+1       Connection error indicator (see connection error feature below)
 
 The "Connection state" field is used to request a ring close and reconnect.
 The "Connection state" field only contains valid data if the server has
-- 
2.35.3

[PATCH v2 2/4] tools/xenstore: add documentation for new set/get-feature commands

[Juergen Gross]

Add documentation for two new Xenstore wire commands SET_FEATURE and
GET_FEATURE used to set or query the Xenstore features visible in the
ring page of a given domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
Do we need support in the migration protocol for the features?
V2:
- remove feature bit (Julien Grall)
- GET_FEATURE without domid will return Xenstore supported features
  (triggered by Julien Grall)
---
 docs/misc/xenstore.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
index a3d3da0a5b..00f6969202 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -331,6 +331,20 @@ SET_TARGET		<domid>|<tdomid>|
 
 	xenstored prevents the use of SET_TARGET other than by dom0.
 
+GET_FEATURE		[<domid>|]		<value>|
+SET_FEATURE		<domid>|<value>|
+	Returns or sets the contents of the "feature" field located at
+	offset 2064 of the Xenstore ring page of the domain specified by
+	<domid>. <value> is a decimal number being a logical or of the
+	feature bits as defined in docs/misc/xenstore-ring.txt. Trying
+	to set a bit for a feature not being supported by the running
+	Xenstore will be denied. Providing no <domid> with the
+	GET_FEATURE command will return the features which are supported
+	by Xenstore.
+
+	xenstored prevents the use of GET_FEATURE and SET_FEATURE other
+	than by dom0.
+
 ---------- Miscellaneous ----------
 
 CONTROL			<command>|[<parameters>|]
-- 
2.35.3

[PATCH v2 3/4] tools/xenstore: add documentation for new set/get-quota commands

[Juergen Gross]

Add documentation for two new Xenstore wire commands SET_QUOTA and
GET_QUOTA used to set or query the global Xenstore quota or those of
a given domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
Note that it might be a good idea to add support to the Xenstore
migration protocol to transfer quota data (global and/or per domain).
V2:
- remove feature bit (Julien Grall)
- add possibility to access global quota and to query supported quotas
---
 docs/misc/xenstore.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
index 00f6969202..49b05e3c9a 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -345,6 +345,23 @@ SET_FEATURE		<domid>|<value>|
 	xenstored prevents the use of GET_FEATURE and SET_FEATURE other
 	than by dom0.
 
+GET_QUOTA		[[<domid>|]<quota>|]	<value>|
+SET_QUOTA		[<domid>|]<quota>|<value>|
+	Returns or sets a quota value for the domain being specified by
+	<domid>. Omitting <domid> will return or set the global quota
+	values, which are the default values for new domains. <quota> is
+	 one of "nodes", "watches", "transactions", "node-size",
+	"permissions", or any other implementation defined value. For
+	GET_QUOTA it is possible to omit the <quota> parameter together
+	with the <domid> parameter, which will return a single string of
+	all supported <quota> values separated by blanks. <value> is a
+	decimal number specifying the quota value, with "0" having the
+	special meaning of quota checks being disabled. The initial quota
+	settings for a domain are the global ones of Xenstore.
+
+	xenstored prevents the use of GET_QUOTA and SET_QUOTA other
+	than by dom0.
+
 ---------- Miscellaneous ----------
 
 CONTROL			<command>|[<parameters>|]
-- 
2.35.3

[PATCH v2 4/4] tools/xenstore: add documentation for extended watch command

[Juergen Gross]

Add documentation for an extension of the WATCH command used to limit
the scope of watched paths. Additionally it enables to receive more
information in the events related to special watches (@introduceDomain
or @releaseDomain).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
This will probably need an extension of the Xenstore migration
protocol, too.
---
 docs/misc/xenstore-ring.txt |  1 +
 docs/misc/xenstore.txt      | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
index 2792d13530..dbc7335e24 100644
--- a/docs/misc/xenstore-ring.txt
+++ b/docs/misc/xenstore-ring.txt
@@ -69,6 +69,7 @@ Bit     Description
 -----------------------------------------------------------------
 0       Ring reconnection (see the ring reconnection feature below)
 1       Connection error indicator (see connection error feature below)
+2       WATCH can take a third parameter limiting its scope
 
 The "Connection state" field is used to request a ring close and reconnect.
 The "Connection state" field only contains valid data if the server has
diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
index 49b05e3c9a..e2daf2eef8 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -187,7 +187,7 @@ SET_PERMS		<path>|<perm-as-string>|+?
 
 ---------- Watches ----------
 
-WATCH			<wpath>|<token>|?
+WATCH			<wpath>|<token>|[<depth>|]?
 	Adds a watch.
 
 	When a <path> is modified (including path creation, removal,
@@ -198,7 +198,11 @@ WATCH			<wpath>|<token>|?
 	matching watch results in a WATCH_EVENT message (see below).
 
 	The event's path matches the watch's <wpath> if it is an child
-	of <wpath>.
+	of <wpath>. This match can be limited by specifying <depth> (a
+	decimal value of 0 or larger): it denotes the directory levels
+	below <wpath> to consider for a match ("0" would not match for
+	a child of <wpath>, "1" would match only for a direct child,
+	etc.).
 
 	<wpath> can be a <path> to watch or @<wspecial>.  In the
 	latter case <wspecial> may have any syntax but it matches
@@ -209,7 +213,13 @@ WATCH			<wpath>|<token>|?
 				shutdown, and also on RELEASE
 				and domain destruction
 	<wspecial> events are sent to privileged callers or explicitly
-	via SET_PERMS enabled domains only.
+	via SET_PERMS enabled domains only. The semantics for a
+	specification of <depth> differ for generating <wspecial>
+	events: specifying "1" will report the related domid by using
+	@<wspecial>/<domid> for the reported path. Other <depth>
+	values are not supported.
+	For @releaseDomain it is possible to watch only for a specific
+	domain by specifying @releaseDomain/<domid> for the path.
 
 	When a watch is first set up it is triggered once straight
 	away, with <path> equal to <wpath>.  Watches may be triggered
-- 
2.35.3

RE: [PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[Henry Wang]

Hi,

It seems that this series [1] has been stale for a while with actions needed from
the maintainers (review needed). So sending this email as a gentle reminder.
Thanks!

[1] https://patchwork.kernel.org/project/xen-devel/list/?series=645480

Kind regards,
Henry

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Juergen Gross
> Subject: [PATCH v2 0/4] tools/xenstore: add some new features to the
> documentation
> 
> In the past there have been spotted some shortcomings in the Xenstore
> interface, which should be repaired. Those are in detail:
> 
> - Using driver domains for large number of domains needs per domain
>   Xenstore quota [1]. The feedback sent was rather slim (one reply),
>   but it was preferring a new set of wire commands.
> 
> - XSA-349 [2] has shown that the current definition of watches is not
>   optimal, as it will trigger lots of events when a single one would
>   suffice: for detecting new backend devices the backends in the Linux
>   kernel are registering a watch for e.g. "/local/domain/0/backend"
>   which will fire for ANY sub-node written below this node (on a test
>   machine this added up to 91 watch events for only 3 devices).
>   This can be limited dramatically by extending the XS_WATCH command
>   to take another optional parameter specifying the depth of
>   subdirectories to be considered for sending watch events ("0" would
>   trigger a watch event only if the watched node itself being written).
> 
> - New features like above being added might make migration of guests
>   between hosts with different Xenstore variants harder, so it should
>   be possible to set the available feature set per domain. For socket
>   connections it should be possible to read the available features.
> 
> - The special watches @introduceDomain and @releaseDomain are rather
>   cumbersome to use, as they only tell you that SOME domain has been
>   introduced/released. Any consumer of those watches needs to scan
>   all domains on the host in order to find out the domid, causing
>   significant pressure on the dominfo hypercall (imagine a system
>   with 1000 domains running and one domain dying - there will be more
>   than 1000 watch events triggered and 1000 xl daemons will try to
>   find out whether "their" domain has died). Those watches should be
>   enhanced to optionally be specific to a single domain and to let the
>   event carry the related domid.
> 
> As some of those extensions will need to be considered in the Xenstore
> migration stream, they should be defined in one go (in fact the 4th one
> wouldn't need that, but it can easily be connected to the 2nd one).
> As such extensions need to be flagged in the "features" in the ring
> page anyway, it is fine to implement them independently.
> 
> Add the documentation of the new commands/features.
> 
> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
> [2]: http://xenbits.xen.org/xsa/advisory-349.html
> 
> Changes in V2:
> - added new patch 1
> - remove feature bits for dom0-only features
> - get-features without domid returns Xenstore supported features
> - get/set-quota without domid for global quota access
> 
> Juergen Gross (4):
>   tools/xenstore: modify feature bit specification in xenstore-ring.txt
>   tools/xenstore: add documentation for new set/get-feature commands
>   tools/xenstore: add documentation for new set/get-quota commands
>   tools/xenstore: add documentation for extended watch command
> 
>  docs/misc/xenstore-ring.txt | 10 ++++----
>  docs/misc/xenstore.txt      | 47 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 50 insertions(+), 7 deletions(-)
> 
> --
> 2.35.3
> 

Re: [PATCH v2 1/4] tools/xenstore: modify feature bit specification in xenstore-ring.txt

[Julien Grall]

Hi Juergen,

On 27/05/2022 08:24, Juergen Gross wrote:
> Instead of specifying the feature bits in xenstore-ring.txt as a mask
> value use bit numbers. This will make the specification easier to read
> when adding more features.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/4] tools/xenstore: add documentation for new set/get-feature commands

[Julien Grall]

Hi Juergen,

On 27/05/2022 08:24, Juergen Gross wrote:
> Add documentation for two new Xenstore wire commands SET_FEATURE and
> GET_FEATURE used to set or query the Xenstore features visible in the
> ring page of a given domain.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> Do we need support in the migration protocol for the features?

I would say yes. You want to make sure that the client can be migrated 
without loosing features between two xenstored.

> V2:
> - remove feature bit (Julien Grall)
> - GET_FEATURE without domid will return Xenstore supported features
>    (triggered by Julien Grall)
> ---
>   docs/misc/xenstore.txt | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
> index a3d3da0a5b..00f6969202 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -331,6 +331,20 @@ SET_TARGET		<domid>|<tdomid>|
>   
>   	xenstored prevents the use of SET_TARGET other than by dom0.
>   
> +GET_FEATURE		[<domid>|]		<value>|
> +SET_FEATURE		<domid>|<value>|
> +	Returns or sets the contents of the "feature" field located at
> +	offset 2064 of the Xenstore ring page of the domain specified by
> +	<domid>. <value> is a decimal number being a logical or of the

In the context of migration, I am stilll a bit concerned that the 
features are stored in the ring because the guest could overwrite it.

I would expect the migration code to check that the GET_FEATURE <domid> 
is a subset of GET_FEATURE on the targeted Xenstored. So it can easily 
prevent a guest to migrate.

So I think this should be a shadow copy that will be returned instead of 
the contents of the "feature" field.

> +	feature bits as defined in docs/misc/xenstore-ring.txt. Trying
> +	to set a bit for a feature not being supported by the running
> +	Xenstore will be denied. Providing no <domid> with the
> +	GET_FEATURE command will return the features which are supported
> +	by Xenstore.

Do we want to allow modifying the features when the guest is running?

> +
> +	xenstored prevents the use of GET_FEATURE and SET_FEATURE other
> +	than by dom0.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/4] tools/xenstore: add documentation for new set/get-quota commands

[Julien Grall]

Hi Juergen,

On 27/05/2022 08:24, Juergen Gross wrote:
> Add documentation for two new Xenstore wire commands SET_QUOTA and
> GET_QUOTA used to set or query the global Xenstore quota or those of
> a given domain.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> Note that it might be a good idea to add support to the Xenstore
> migration protocol to transfer quota data (global and/or per domain).

I think this is needed because a user may have configured a domain with 
quotas above the default. After Live-Update, we would have a short 
window where the domain may not function properly.

I think it would be good to documentation the migration part in this 
patch. But if you want do it separately then:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 4/4] tools/xenstore: add documentation for extended watch command

[Julien Grall]

Hi Juergen,

On 27/05/2022 08:24, Juergen Gross wrote:
> Add documentation for an extension of the WATCH command used to limit
> the scope of watched paths. Additionally it enables to receive more
> information in the events related to special watches (@introduceDomain
> or @releaseDomain).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> This will probably need an extension of the Xenstore migration
> protocol, too.

That's going to be necessary in order to live-update xenstored.

I would prefer if this is dealt in this patch, but if you want to do it 
separately then:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/4] tools/xenstore: add documentation for new set/get-feature commands

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2649 bytes --]

On 23.06.22 20:27, Julien Grall wrote:
> Hi Juergen,
> 
> On 27/05/2022 08:24, Juergen Gross wrote:
>> Add documentation for two new Xenstore wire commands SET_FEATURE and
>> GET_FEATURE used to set or query the Xenstore features visible in the
>> ring page of a given domain.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> Do we need support in the migration protocol for the features?
> 
> I would say yes. You want to make sure that the client can be migrated without 
> loosing features between two xenstored.

Okay, will add that in V3.

> 
>> V2:
>> - remove feature bit (Julien Grall)
>> - GET_FEATURE without domid will return Xenstore supported features
>>    (triggered by Julien Grall)
>> ---
>>   docs/misc/xenstore.txt | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
>> index a3d3da0a5b..00f6969202 100644
>> --- a/docs/misc/xenstore.txt
>> +++ b/docs/misc/xenstore.txt
>> @@ -331,6 +331,20 @@ SET_TARGET        <domid>|<tdomid>|
>>       xenstored prevents the use of SET_TARGET other than by dom0.
>> +GET_FEATURE        [<domid>|]        <value>|
>> +SET_FEATURE        <domid>|<value>|
>> +    Returns or sets the contents of the "feature" field located at
>> +    offset 2064 of the Xenstore ring page of the domain specified by
>> +    <domid>. <value> is a decimal number being a logical or of the
> 
> In the context of migration, I am stilll a bit concerned that the features are 
> stored in the ring because the guest could overwrite it.
> 
> I would expect the migration code to check that the GET_FEATURE <domid> is a 
> subset of GET_FEATURE on the targeted Xenstored. So it can easily prevent a 
> guest to migrate.
> 
> So I think this should be a shadow copy that will be returned instead of the 
> contents of the "feature" field.

Of course. The value in the ring is meant only for the guest. Xenstore
will have the authoritative value in its private memory.

> 
>> +    feature bits as defined in docs/misc/xenstore-ring.txt. Trying
>> +    to set a bit for a feature not being supported by the running
>> +    Xenstore will be denied. Providing no <domid> with the
>> +    GET_FEATURE command will return the features which are supported
>> +    by Xenstore.
> 
> Do we want to allow modifying the features when the guest is running?

I think we can't remove features, but adding would be fine. For
simplicity it might be better to just deny a modification while the
guest is running.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 3/4] tools/xenstore: add documentation for new set/get-quota commands

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 979 bytes --]

On 23.06.22 20:34, Julien Grall wrote:
> Hi Juergen,
> 
> On 27/05/2022 08:24, Juergen Gross wrote:
>> Add documentation for two new Xenstore wire commands SET_QUOTA and
>> GET_QUOTA used to set or query the global Xenstore quota or those of
>> a given domain.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> Note that it might be a good idea to add support to the Xenstore
>> migration protocol to transfer quota data (global and/or per domain).
> 
> I think this is needed because a user may have configured a domain with quotas 
> above the default. After Live-Update, we would have a short window where the 
> domain may not function properly.
> 
> I think it would be good to documentation the migration part in this patch. But 
> if you want do it separately then:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks.

I think I will add another patch documenting all of the needed migration
stream additions.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 2/4] tools/xenstore: add documentation for new set/get-feature commands

[Julien Grall]

Hi Juergen,

On 24/06/2022 05:13, Juergen Gross wrote:
> On 23.06.22 20:27, Julien Grall wrote:
>> On 27/05/2022 08:24, Juergen Gross wrote:
>>> Add documentation for two new Xenstore wire commands SET_FEATURE and
>>> GET_FEATURE used to set or query the Xenstore features visible in the
>>> ring page of a given domain.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> Do we need support in the migration protocol for the features?
>>
>> I would say yes. You want to make sure that the client can be migrated 
>> without loosing features between two xenstored.
>>
>>> V2:
>>> - remove feature bit (Julien Grall)
>>> - GET_FEATURE without domid will return Xenstore supported features
>>>    (triggered by Julien Grall)
>>> ---
>>>   docs/misc/xenstore.txt | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
>>> index a3d3da0a5b..00f6969202 100644
>>> --- a/docs/misc/xenstore.txt
>>> +++ b/docs/misc/xenstore.txt
>>> @@ -331,6 +331,20 @@ SET_TARGET        <domid>|<tdomid>|
>>>       xenstored prevents the use of SET_TARGET other than by dom0.
>>> +GET_FEATURE        [<domid>|]        <value>|
>>> +SET_FEATURE        <domid>|<value>|
>>> +    Returns or sets the contents of the "feature" field located at
>>> +    offset 2064 of the Xenstore ring page of the domain specified by
>>> +    <domid>. <value> is a decimal number being a logical or of the
>>
>> In the context of migration, I am stilll a bit concerned that the 
>> features are stored in the ring because the guest could overwrite it.
>>
>> I would expect the migration code to check that the GET_FEATURE 
>> <domid> is a subset of GET_FEATURE on the targeted Xenstored. So it 
>> can easily prevent a guest to migrate.
>>
>> So I think this should be a shadow copy that will be returned instead 
>> of the contents of the "feature" field.
> 
> Of course. The value in the ring is meant only for the guest. Xenstore
> will have the authoritative value in its private memory.

Good. I would suggest to clarify it in xenstore.txt because it suggests 
otherwise so far.

>>> +    feature bits as defined in docs/misc/xenstore-ring.txt. Trying
>>> +    to set a bit for a feature not being supported by the running
>>> +    Xenstore will be denied. Providing no <domid> with the
>>> +    GET_FEATURE command will return the features which are supported
>>> +    by Xenstore.
>>
>> Do we want to allow modifying the features when the guest is running?
> 
> I think we can't remove features, but adding would be fine. For

Agree with removing features, regarding adding I think we would need a 
mechanism to tell the client there is a new feature. It would require 
some work, so...

> simplicity it might be better to just deny a modification while the
> guest is running.

... I agree this is probably best for a first shot. This can be relaxed 
afterwards.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[Jan Beulich]

On 27.05.2022 09:24, Juergen Gross wrote:
> In the past there have been spotted some shortcomings in the Xenstore
> interface, which should be repaired. Those are in detail:
> 
> - Using driver domains for large number of domains needs per domain
>   Xenstore quota [1]. The feedback sent was rather slim (one reply),
>   but it was preferring a new set of wire commands.
> 
> - XSA-349 [2] has shown that the current definition of watches is not
>   optimal, as it will trigger lots of events when a single one would
>   suffice: for detecting new backend devices the backends in the Linux
>   kernel are registering a watch for e.g. "/local/domain/0/backend"
>   which will fire for ANY sub-node written below this node (on a test
>   machine this added up to 91 watch events for only 3 devices).
>   This can be limited dramatically by extending the XS_WATCH command
>   to take another optional parameter specifying the depth of
>   subdirectories to be considered for sending watch events ("0" would
>   trigger a watch event only if the watched node itself being written).
> 
> - New features like above being added might make migration of guests
>   between hosts with different Xenstore variants harder, so it should
>   be possible to set the available feature set per domain. For socket
>   connections it should be possible to read the available features.
> 
> - The special watches @introduceDomain and @releaseDomain are rather
>   cumbersome to use, as they only tell you that SOME domain has been
>   introduced/released. Any consumer of those watches needs to scan
>   all domains on the host in order to find out the domid, causing
>   significant pressure on the dominfo hypercall (imagine a system
>   with 1000 domains running and one domain dying - there will be more
>   than 1000 watch events triggered and 1000 xl daemons will try to
>   find out whether "their" domain has died). Those watches should be
>   enhanced to optionally be specific to a single domain and to let the
>   event carry the related domid.
> 
> As some of those extensions will need to be considered in the Xenstore
> migration stream, they should be defined in one go (in fact the 4th one
> wouldn't need that, but it can easily be connected to the 2nd one).
> As such extensions need to be flagged in the "features" in the ring
> page anyway, it is fine to implement them independently.
> 
> Add the documentation of the new commands/features.
> 
> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
> [2]: http://xenbits.xen.org/xsa/advisory-349.html
> 
> Changes in V2:
> - added new patch 1
> - remove feature bits for dom0-only features
> - get-features without domid returns Xenstore supported features
> - get/set-quota without domid for global quota access
> 
> Juergen Gross (4):
>   tools/xenstore: modify feature bit specification in xenstore-ring.txt
>   tools/xenstore: add documentation for new set/get-feature commands
>   tools/xenstore: add documentation for new set/get-quota commands
>   tools/xenstore: add documentation for extended watch command

Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
seeing there had been R-b with no other follow-up (leaving aside the v2)
in a long time. Please advise if I should revert the commits. I'm sorry
for the confusion. (I also wonder why the R-b weren't carried over to v2.)

Jan

Re: [PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[Julien Grall]

Hi Jan,

On 18/07/2022 17:12, Jan Beulich wrote:
> On 27.05.2022 09:24, Juergen Gross wrote:
>> In the past there have been spotted some shortcomings in the Xenstore
>> interface, which should be repaired. Those are in detail:
>>
>> - Using driver domains for large number of domains needs per domain
>>    Xenstore quota [1]. The feedback sent was rather slim (one reply),
>>    but it was preferring a new set of wire commands.
>>
>> - XSA-349 [2] has shown that the current definition of watches is not
>>    optimal, as it will trigger lots of events when a single one would
>>    suffice: for detecting new backend devices the backends in the Linux
>>    kernel are registering a watch for e.g. "/local/domain/0/backend"
>>    which will fire for ANY sub-node written below this node (on a test
>>    machine this added up to 91 watch events for only 3 devices).
>>    This can be limited dramatically by extending the XS_WATCH command
>>    to take another optional parameter specifying the depth of
>>    subdirectories to be considered for sending watch events ("0" would
>>    trigger a watch event only if the watched node itself being written).
>>
>> - New features like above being added might make migration of guests
>>    between hosts with different Xenstore variants harder, so it should
>>    be possible to set the available feature set per domain. For socket
>>    connections it should be possible to read the available features.
>>
>> - The special watches @introduceDomain and @releaseDomain are rather
>>    cumbersome to use, as they only tell you that SOME domain has been
>>    introduced/released. Any consumer of those watches needs to scan
>>    all domains on the host in order to find out the domid, causing
>>    significant pressure on the dominfo hypercall (imagine a system
>>    with 1000 domains running and one domain dying - there will be more
>>    than 1000 watch events triggered and 1000 xl daemons will try to
>>    find out whether "their" domain has died). Those watches should be
>>    enhanced to optionally be specific to a single domain and to let the
>>    event carry the related domid.
>>
>> As some of those extensions will need to be considered in the Xenstore
>> migration stream, they should be defined in one go (in fact the 4th one
>> wouldn't need that, but it can easily be connected to the 2nd one).
>> As such extensions need to be flagged in the "features" in the ring
>> page anyway, it is fine to implement them independently.
>>
>> Add the documentation of the new commands/features.
>>
>> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
>> [2]: http://xenbits.xen.org/xsa/advisory-349.html
>>
>> Changes in V2:
>> - added new patch 1
>> - remove feature bits for dom0-only features
>> - get-features without domid returns Xenstore supported features
>> - get/set-quota without domid for global quota access
>>
>> Juergen Gross (4):
>>    tools/xenstore: modify feature bit specification in xenstore-ring.txt
>>    tools/xenstore: add documentation for new set/get-feature commands
>>    tools/xenstore: add documentation for new set/get-quota commands
>>    tools/xenstore: add documentation for extended watch command
> 
> Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
> seeing there had been R-b with no other follow-up (leaving aside the v2)
> in a long time. Please advise if I should revert the commits. I'm sorry
> for the confusion. (I also wonder why the R-b weren't carried over to v2.)

patch #1 is a new patch. The patch #2, #3, #4 have been reworded and the 
overall interaction is different. So I don't think the reviewed-by 
should have been carried.

I had some concerns in v1 which were addressed in v2. I have reviewed v2 
a while ago. From my perspective, patch #1, #3, #4 are ready to go. 
Patch #2 needs a respin and we also need to clarify the integration with 
migration/live-update.

As you committed, I would be OK if this is addressed in a follow-up 
series. But this *must* be addressed by the time 4.17 is released 
because otherwise we will commit ourself to a broken interface. @Henry, 
please add this in the blocker list.

Cheers,

-- 
Julien Grall

RE: [PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2 0/4] tools/xenstore: add some new features to the
> documentation
> 
> Hi Jan,
> 
> On 18/07/2022 17:12, Jan Beulich wrote:
> > On 27.05.2022 09:24, Juergen Gross wrote:
> >>
> >> Changes in V2:
> >> - added new patch 1
> >> - remove feature bits for dom0-only features
> >> - get-features without domid returns Xenstore supported features
> >> - get/set-quota without domid for global quota access
> >>
> >> Juergen Gross (4):
> >>    tools/xenstore: modify feature bit specification in xenstore-ring.txt
> >>    tools/xenstore: add documentation for new set/get-feature commands
> >>    tools/xenstore: add documentation for new set/get-quota commands
> >>    tools/xenstore: add documentation for extended watch command
> >
> > Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
> > seeing there had been R-b with no other follow-up (leaving aside the v2)
> > in a long time. Please advise if I should revert the commits. I'm sorry
> > for the confusion. (I also wonder why the R-b weren't carried over to v2.)
> 
> patch #1 is a new patch. The patch #2, #3, #4 have been reworded and the
> overall interaction is different. So I don't think the reviewed-by
> should have been carried.
> 
> I had some concerns in v1 which were addressed in v2. I have reviewed v2
> a while ago. From my perspective, patch #1, #3, #4 are ready to go.
> Patch #2 needs a respin and we also need to clarify the integration with
> migration/live-update.
> 
> As you committed, I would be OK if this is addressed in a follow-up
> series. But this *must* be addressed by the time 4.17 is released
> because otherwise we will commit ourself to a broken interface. @Henry,
> please add this in the blocker list.

Thank you very much for this information. I've added this in blocker list.
I will keep that in mind and send (proper) reminders during the timeline
of the 4.17 release process.

Kind regards,
Henry 

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[Jan Beulich]

On 18.07.2022 18:28, Julien Grall wrote:
> On 18/07/2022 17:12, Jan Beulich wrote:
>> On 27.05.2022 09:24, Juergen Gross wrote:
>>> In the past there have been spotted some shortcomings in the Xenstore
>>> interface, which should be repaired. Those are in detail:
>>>
>>> - Using driver domains for large number of domains needs per domain
>>>    Xenstore quota [1]. The feedback sent was rather slim (one reply),
>>>    but it was preferring a new set of wire commands.
>>>
>>> - XSA-349 [2] has shown that the current definition of watches is not
>>>    optimal, as it will trigger lots of events when a single one would
>>>    suffice: for detecting new backend devices the backends in the Linux
>>>    kernel are registering a watch for e.g. "/local/domain/0/backend"
>>>    which will fire for ANY sub-node written below this node (on a test
>>>    machine this added up to 91 watch events for only 3 devices).
>>>    This can be limited dramatically by extending the XS_WATCH command
>>>    to take another optional parameter specifying the depth of
>>>    subdirectories to be considered for sending watch events ("0" would
>>>    trigger a watch event only if the watched node itself being written).
>>>
>>> - New features like above being added might make migration of guests
>>>    between hosts with different Xenstore variants harder, so it should
>>>    be possible to set the available feature set per domain. For socket
>>>    connections it should be possible to read the available features.
>>>
>>> - The special watches @introduceDomain and @releaseDomain are rather
>>>    cumbersome to use, as they only tell you that SOME domain has been
>>>    introduced/released. Any consumer of those watches needs to scan
>>>    all domains on the host in order to find out the domid, causing
>>>    significant pressure on the dominfo hypercall (imagine a system
>>>    with 1000 domains running and one domain dying - there will be more
>>>    than 1000 watch events triggered and 1000 xl daemons will try to
>>>    find out whether "their" domain has died). Those watches should be
>>>    enhanced to optionally be specific to a single domain and to let the
>>>    event carry the related domid.
>>>
>>> As some of those extensions will need to be considered in the Xenstore
>>> migration stream, they should be defined in one go (in fact the 4th one
>>> wouldn't need that, but it can easily be connected to the 2nd one).
>>> As such extensions need to be flagged in the "features" in the ring
>>> page anyway, it is fine to implement them independently.
>>>
>>> Add the documentation of the new commands/features.
>>>
>>> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
>>> [2]: http://xenbits.xen.org/xsa/advisory-349.html
>>>
>>> Changes in V2:
>>> - added new patch 1
>>> - remove feature bits for dom0-only features
>>> - get-features without domid returns Xenstore supported features
>>> - get/set-quota without domid for global quota access
>>>
>>> Juergen Gross (4):
>>>    tools/xenstore: modify feature bit specification in xenstore-ring.txt
>>>    tools/xenstore: add documentation for new set/get-feature commands
>>>    tools/xenstore: add documentation for new set/get-quota commands
>>>    tools/xenstore: add documentation for extended watch command
>>
>> Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
>> seeing there had been R-b with no other follow-up (leaving aside the v2)
>> in a long time. Please advise if I should revert the commits. I'm sorry
>> for the confusion. (I also wonder why the R-b weren't carried over to v2.)
> 
> patch #1 is a new patch. The patch #2, #3, #4 have been reworded and the 
> overall interaction is different. So I don't think the reviewed-by 
> should have been carried.
> 
> I had some concerns in v1 which were addressed in v2. I have reviewed v2 
> a while ago. From my perspective, patch #1, #3, #4 are ready to go. 
> Patch #2 needs a respin and we also need to clarify the integration with 
> migration/live-update.
> 
> As you committed, I would be OK if this is addressed in a follow-up 
> series. But this *must* be addressed by the time 4.17 is released 
> because otherwise we will commit ourself to a broken interface. @Henry, 
> please add this in the blocker list.

If you hadn't answered, I would have reverted these right away this
morning, in particular to remove the (now wrong) feature bit part
(patches 2 and 3 have dropped their feature bit additions in v2).
If you nevertheless think an incremental update is going to be okay,
I'll leave things alone. But personally I think this mistake of mine
would better be corrected immediately.

Jan

Re: [PATCH v2 0/4] tools/xenstore: add some new features to the documentation

[Julien Grall]

Hi Jan,

On 19/07/2022 06:58, Jan Beulich wrote:
> On 18.07.2022 18:28, Julien Grall wrote:
>> On 18/07/2022 17:12, Jan Beulich wrote:
>>> On 27.05.2022 09:24, Juergen Gross wrote:
>> As you committed, I would be OK if this is addressed in a follow-up
>> series. But this *must* be addressed by the time 4.17 is released
>> because otherwise we will commit ourself to a broken interface. @Henry,
>> please add this in the blocker list.
> 
> If you hadn't answered, I would have reverted these right away this
> morning, in particular to remove the (now wrong) feature bit part
> (patches 2 and 3 have dropped their feature bit additions in v2).
> If you nevertheless think an incremental update is going to be okay,
> I'll leave things alone. But personally I think this mistake of mine
> would better be corrected immediately.

I wasn't arguing against a revert and it looks like Juergen is away for 
the next 2 weeks. So if you prefer to correct the mistake now, then 
please revert it.

Cheers,

-- 
Julien Grall