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

[PATCH 0/3] 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

Juergen Gross (3):
  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 |  3 +++
 docs/misc/xenstore.txt      | 40 ++++++++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/3] 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>
---
 docs/misc/xenstore-ring.txt |  1 +
 docs/misc/xenstore.txt      | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
index f91accb5b0..bd000f694e 100644
--- a/docs/misc/xenstore-ring.txt
+++ b/docs/misc/xenstore-ring.txt
@@ -68,6 +68,7 @@ Mask    Description
 -----------------------------------------------------------------
 1       Ring reconnection (see the ring reconnection feature below)
 2       Connection error indicator (see connection error feature below)
+4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
 
 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 ea3d8be177..31e3d53c52 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -332,6 +332,18 @@ 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.
+
+	xenstored prevents the use of GET_FEATURE and SET_FEATURE other
+	than by dom0.
+
 ---------- Miscellaneous ----------
 
 CONTROL			<command>|[<parameters>|]
-- 
2.34.1

[PATCH 2/3] 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 Xenstore quota of a given domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/xenstore-ring.txt |  1 +
 docs/misc/xenstore.txt      | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
index bd000f694e..0cb72a3e35 100644
--- a/docs/misc/xenstore-ring.txt
+++ b/docs/misc/xenstore-ring.txt
@@ -69,6 +69,7 @@ Mask    Description
 1       Ring reconnection (see the ring reconnection feature below)
 2       Connection error indicator (see connection error feature below)
 4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
+8       GET_QUOTA and SET_QUOTA Xenstore wire commands are available
 
 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 31e3d53c52..dd75a81328 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -344,6 +344,18 @@ 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>. <quota> is one of "nodes", "watches", "transactions",
+	"node-size" or "permissions". <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.34.1

[PATCH 3/3] 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>
---
 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 0cb72a3e35..eaa6d0a1a3 100644
--- a/docs/misc/xenstore-ring.txt
+++ b/docs/misc/xenstore-ring.txt
@@ -70,6 +70,7 @@ Mask    Description
 2       Connection error indicator (see connection error feature below)
 4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
 8       GET_QUOTA and SET_QUOTA Xenstore wire commands are available
+16      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 dd75a81328..f86c6d9757 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -188,7 +188,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,
@@ -199,7 +199,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
@@ -210,7 +214,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.34.1

Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Luca Fancellu]

> On 16 Mar 2022, at 16:10, Juergen Gross <jgross@suse.com> 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>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

> ---
> docs/misc/xenstore-ring.txt |  1 +
> docs/misc/xenstore.txt      | 12 ++++++++++++
> 2 files changed, 13 insertions(+)
> 
> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
> index f91accb5b0..bd000f694e 100644
> --- a/docs/misc/xenstore-ring.txt
> +++ b/docs/misc/xenstore-ring.txt
> @@ -68,6 +68,7 @@ Mask    Description
> -----------------------------------------------------------------
> 1       Ring reconnection (see the ring reconnection feature below)
> 2       Connection error indicator (see connection error feature below)
> +4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> 
> 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 ea3d8be177..31e3d53c52 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -332,6 +332,18 @@ 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.
> +
> +	xenstored prevents the use of GET_FEATURE and SET_FEATURE other
> +	than by dom0.
> +
> ---------- Miscellaneous ----------
> 
> CONTROL			<command>|[<parameters>|]
> -- 
> 2.34.1
> 
> 

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

[Luca Fancellu]

> On 16 Mar 2022, at 16:10, Juergen Gross <jgross@suse.com> wrote:
> 
> Add documentation for two new Xenstore wire commands SET_QUOTA and
> GET_QUOTA used to set or query the Xenstore quota of a given domain.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

> ---
> docs/misc/xenstore-ring.txt |  1 +
> docs/misc/xenstore.txt      | 12 ++++++++++++
> 2 files changed, 13 insertions(+)
> 
> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
> index bd000f694e..0cb72a3e35 100644
> --- a/docs/misc/xenstore-ring.txt
> +++ b/docs/misc/xenstore-ring.txt
> @@ -69,6 +69,7 @@ Mask    Description
> 1       Ring reconnection (see the ring reconnection feature below)
> 2       Connection error indicator (see connection error feature below)
> 4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> +8       GET_QUOTA and SET_QUOTA Xenstore wire commands are available
> 
> 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 31e3d53c52..dd75a81328 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -344,6 +344,18 @@ 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>. <quota> is one of "nodes", "watches", "transactions",
> +	"node-size" or "permissions". <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.34.1
> 
> 

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

[Luca Fancellu]

> On 16 Mar 2022, at 16:10, Juergen Gross <jgross@suse.com> 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>
> ---
> 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 0cb72a3e35..eaa6d0a1a3 100644
> --- a/docs/misc/xenstore-ring.txt
> +++ b/docs/misc/xenstore-ring.txt
> @@ -70,6 +70,7 @@ Mask    Description
> 2       Connection error indicator (see connection error feature below)
> 4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> 8       GET_QUOTA and SET_QUOTA Xenstore wire commands are available
> +16      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 dd75a81328..f86c6d9757 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -188,7 +188,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,
> @@ -199,7 +199,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
> @@ -210,7 +214,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>

Typo: s/differ/differs/?

> +	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.34.1
> 
> 

I’m not an English native speaker so apologies if there is no mistake.

With that fixed (if it’s wrong):

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

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

[Juergen Gross]

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

On 16.03.22 19:16, Luca Fancellu wrote:
> 
> 
>> On 16 Mar 2022, at 16:10, Juergen Gross <jgross@suse.com> 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>
>> ---
>> 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 0cb72a3e35..eaa6d0a1a3 100644
>> --- a/docs/misc/xenstore-ring.txt
>> +++ b/docs/misc/xenstore-ring.txt
>> @@ -70,6 +70,7 @@ Mask    Description
>> 2       Connection error indicator (see connection error feature below)
>> 4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
>> 8       GET_QUOTA and SET_QUOTA Xenstore wire commands are available
>> +16      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 dd75a81328..f86c6d9757 100644
>> --- a/docs/misc/xenstore.txt
>> +++ b/docs/misc/xenstore.txt
>> @@ -188,7 +188,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,
>> @@ -199,7 +199,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
>> @@ -210,7 +214,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>
> 
> Typo: s/differ/differs/?

I think "semantics" is a plural word, but I've found some references
that it is often used with a singular verb. It seems both variants are
in use.

> I’m not an English native speaker so apologies if there is no mistake.

Me neither. Any advice from a native English speaker?


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 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Julien Grall]

Hi Juergen,

On 16/03/2022 16:10, 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>
> ---
>   docs/misc/xenstore-ring.txt |  1 +
>   docs/misc/xenstore.txt      | 12 ++++++++++++
>   2 files changed, 13 insertions(+)
> 
> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
> index f91accb5b0..bd000f694e 100644
> --- a/docs/misc/xenstore-ring.txt
> +++ b/docs/misc/xenstore-ring.txt
> @@ -68,6 +68,7 @@ Mask    Description

I find a bit odd we describe the feature in term of mask rather bit. 
This will get more difficult to read as we add more bits.

This is not new, so not change expected in this series.

>   -----------------------------------------------------------------
>   1       Ring reconnection (see the ring reconnection feature below)
>   2       Connection error indicator (see connection error feature below)
> +4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available

Below, you wrote the two commands are dom0 only. Furthermore, I would 
expect such comment return something like ENOSYS if they are not present.

So do we really need to add a feature bit for GET_FEATURE/SET_FEATURE?

>   
>   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 ea3d8be177..31e3d53c52 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -332,6 +332,18 @@ SET_TARGET		<domid>|<tdomid>|
>   
>   	xenstored prevents the use of SET_TARGET other than by dom0.
>   
> +GET_FEATURE		<domid>|		<value>|

Did you indented to add many spaces before <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.
How will the caller know which feature is supported? Also, what happen 
if a client decided to overwrite 'feature'? Could the result potentially 
prevent migration/liveupdate or else?

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Julien Grall]

On 17/03/2022 11:07, Julien Grall wrote:
> Hi Juergen,
> 
> On 16/03/2022 16:10, 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>
>> ---
>>   docs/misc/xenstore-ring.txt |  1 +
>>   docs/misc/xenstore.txt      | 12 ++++++++++++
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
>> index f91accb5b0..bd000f694e 100644
>> --- a/docs/misc/xenstore-ring.txt
>> +++ b/docs/misc/xenstore-ring.txt
>> @@ -68,6 +68,7 @@ Mask    Description
> 
> I find a bit odd we describe the feature in term of mask rather bit. 
> This will get more difficult to read as we add more bits.
> 
> This is not new, so not change expected in this series.
> 
>>   -----------------------------------------------------------------
>>   1       Ring reconnection (see the ring reconnection feature below)
>>   2       Connection error indicator (see connection error feature below)
>> +4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> 
> Below, you wrote the two commands are dom0 only. Furthermore, I would 
> expect such comment return something like ENOSYS if they are not present.
> 
> So do we really need to add a feature bit for GET_FEATURE/SET_FEATURE?
> 
>>   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 ea3d8be177..31e3d53c52 100644
>> --- a/docs/misc/xenstore.txt
>> +++ b/docs/misc/xenstore.txt
>> @@ -332,6 +332,18 @@ SET_TARGET        <domid>|<tdomid>|
>>       xenstored prevents the use of SET_TARGET other than by dom0.
>> +GET_FEATURE        <domid>|        <value>|
> 
> Did you indented to add many spaces before <value>?

Please ignore this question. <value>  is part of the response. Sorry for 
the noise.

Cheers,

> 
>> +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.
> How will the caller know which feature is supported? Also, what happen 
> if a client decided to overwrite 'feature'? Could the result potentially 
> prevent migration/liveupdate or else?
> 
> Cheers,
> 

-- 
Julien Grall

Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Juergen Gross]

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

On 17.03.22 12:07, Julien Grall wrote:
> Hi Juergen,
> 
> On 16/03/2022 16:10, 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>
>> ---
>>   docs/misc/xenstore-ring.txt |  1 +
>>   docs/misc/xenstore.txt      | 12 ++++++++++++
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
>> index f91accb5b0..bd000f694e 100644
>> --- a/docs/misc/xenstore-ring.txt
>> +++ b/docs/misc/xenstore-ring.txt
>> @@ -68,6 +68,7 @@ Mask    Description
> 
> I find a bit odd we describe the feature in term of mask rather bit. This will 
> get more difficult to read as we add more bits.

Maybe this is in order to avoid big/little endian issues (which bit is
bit 0?)

> 
> This is not new, so not change expected in this series.
> 
>>   -----------------------------------------------------------------
>>   1       Ring reconnection (see the ring reconnection feature below)
>>   2       Connection error indicator (see connection error feature below)
>> +4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> 
> Below, you wrote the two commands are dom0 only. Furthermore, I would expect 
> such comment return something like ENOSYS if they are not present.
> 
> So do we really need to add a feature bit for GET_FEATURE/SET_FEATURE?

Good question. I'd be fine to drop it.

> 
>>   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 ea3d8be177..31e3d53c52 100644
>> --- a/docs/misc/xenstore.txt
>> +++ b/docs/misc/xenstore.txt
>> @@ -332,6 +332,18 @@ SET_TARGET        <domid>|<tdomid>|
>>       xenstored prevents the use of SET_TARGET other than by dom0.
>> +GET_FEATURE        <domid>|        <value>|
> 
> Did you indented to add many spaces before <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.
> How will the caller know which feature is supported? Also, what happen if a 
> client decided to overwrite 'feature'? Could the result potentially prevent 
> migration/liveupdate or else?

The caller could use "GET_FEATURE 0" to see the available features, assuming
that nobody would have changed dom0's features.

I'm not sure whether we should prevent dom0's features to be overwritten.


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 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Julien Grall]

Hi Juergen,

On 17/03/2022 11:19, Juergen Gross wrote:
> On 17.03.22 12:07, Julien Grall wrote:
>> On 16/03/2022 16:10, 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>
>>> ---
>>>   docs/misc/xenstore-ring.txt |  1 +
>>>   docs/misc/xenstore.txt      | 12 ++++++++++++
>>>   2 files changed, 13 insertions(+)
>>>
>>> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
>>> index f91accb5b0..bd000f694e 100644
>>> --- a/docs/misc/xenstore-ring.txt
>>> +++ b/docs/misc/xenstore-ring.txt
>>> @@ -68,6 +68,7 @@ Mask    Description
>>
>> I find a bit odd we describe the feature in term of mask rather bit. 
>> This will get more difficult to read as we add more bits.
> 
> Maybe this is in order to avoid big/little endian issues (which bit is
> bit 0?)

Both end have to talk the same endianess. Otherwise, one may read the 
wrong value.

So long they are using the same endianess, bit 0 is not going to be matter.

>>>   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 ea3d8be177..31e3d53c52 100644
>>> --- a/docs/misc/xenstore.txt
>>> +++ b/docs/misc/xenstore.txt
>>> @@ -332,6 +332,18 @@ SET_TARGET        <domid>|<tdomid>|
>>>       xenstored prevents the use of SET_TARGET other than by dom0.
>>> +GET_FEATURE        <domid>|        <value>|
>>
>> Did you indented to add many spaces before <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.
>> How will the caller know which feature is supported? Also, what happen 
>> if a client decided to overwrite 'feature'? Could the result 
>> potentially prevent migration/liveupdate or else?
> 
> The caller could use "GET_FEATURE 0" to see the available features, 
> assuming
> that nobody would have changed dom0's features.
> 
> I'm not sure whether we should prevent dom0's features to be overwritten.

I think it would be better to have a separate "domid" (maybe "server" or 
"global") to retrieve features supported by the server.

This would give us some flexibility to update dom0 features in the 
future if the needs arise (the first implementation may forbid it).

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Juergen Gross]

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

On 18.03.22 19:40, Julien Grall wrote:
> Hi Juergen,
> 
> On 17/03/2022 11:19, Juergen Gross wrote:
>> On 17.03.22 12:07, Julien Grall wrote:
>>> On 16/03/2022 16:10, 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>
>>>> ---
>>>>   docs/misc/xenstore-ring.txt |  1 +
>>>>   docs/misc/xenstore.txt      | 12 ++++++++++++
>>>>   2 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
>>>> index f91accb5b0..bd000f694e 100644
>>>> --- a/docs/misc/xenstore-ring.txt
>>>> +++ b/docs/misc/xenstore-ring.txt
>>>> @@ -68,6 +68,7 @@ Mask    Description
>>>
>>> I find a bit odd we describe the feature in term of mask rather bit. This 
>>> will get more difficult to read as we add more bits.
>>
>> Maybe this is in order to avoid big/little endian issues (which bit is
>> bit 0?)
> 
> Both end have to talk the same endianess. Otherwise, one may read the wrong value.
> 
> So long they are using the same endianess, bit 0 is not going to be matter.
> 
>>>>   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 ea3d8be177..31e3d53c52 100644
>>>> --- a/docs/misc/xenstore.txt
>>>> +++ b/docs/misc/xenstore.txt
>>>> @@ -332,6 +332,18 @@ SET_TARGET        <domid>|<tdomid>|
>>>>       xenstored prevents the use of SET_TARGET other than by dom0.
>>>> +GET_FEATURE        <domid>|        <value>|
>>>
>>> Did you indented to add many spaces before <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.
>>> How will the caller know which feature is supported? Also, what happen if a 
>>> client decided to overwrite 'feature'? Could the result potentially prevent 
>>> migration/liveupdate or else?
>>
>> The caller could use "GET_FEATURE 0" to see the available features, assuming
>> that nobody would have changed dom0's features.
>>
>> I'm not sure whether we should prevent dom0's features to be overwritten.
> 
> I think it would be better to have a separate "domid" (maybe "server" or 
> "global") to retrieve features supported by the server.
> 
> This would give us some flexibility to update dom0 features in the future if the 
> needs arise (the first implementation may forbid it).

Fine with me.


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 --]