* [PATCH v4 0/4] tools/xenstore: add some new features to the documentation
@ 2022-09-05 12:47 Juergen Gross
2022-09-05 12:47 ` [PATCH v4 1/4] tools/xenstore: minor fix of the migration stream doc Juergen Gross
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Juergen Gross @ 2022-09-05 12:47 UTC (permalink / raw)
To: xen-devel
Cc: Henry.Wang, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
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 V4:
- patch 2 removed, as already committed
- major rework of last patch
Changes in V3:
- patch 1 removed, as already committed
- new patch 1
- minor clarifications in patch 2
- new patch 5
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: minor fix of the migration stream doc
tools/xenstore: add documentation for new set/get-quota commands
tools/xenstore: add documentation for extended watch command
tools/xenstore: add migration stream extensions for new features
docs/designs/xenstore-migration.md | 166 +++++++++++++++++++++++++++--
docs/misc/xenstore-ring.txt | 1 +
docs/misc/xenstore.txt | 33 +++++-
3 files changed, 188 insertions(+), 12 deletions(-)
--
2.35.3
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 1/4] tools/xenstore: minor fix of the migration stream doc
2022-09-05 12:47 [PATCH v4 0/4] tools/xenstore: add some new features to the documentation Juergen Gross
@ 2022-09-05 12:47 ` Juergen Gross
2022-09-06 17:07 ` Julien Grall
2022-09-05 12:47 ` [PATCH v4 2/4] tools/xenstore: add documentation for new set/get-quota commands Juergen Gross
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2022-09-05 12:47 UTC (permalink / raw)
To: xen-devel
Cc: Henry.Wang, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Drop mentioning the non-existent read-only socket in the migration
stream description document.
The related record field was removed in commit 8868a0e3f674 ("docs:
update the xenstore migration stream documentation).
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
docs/designs/xenstore-migration.md | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
index b94af7fd7c..efa526f420 100644
--- a/docs/designs/xenstore-migration.md
+++ b/docs/designs/xenstore-migration.md
@@ -129,11 +129,9 @@ xenstored state that needs to be restored.
| `evtchn-fd` | The file descriptor used to communicate with |
| | the event channel driver |
-xenstored will resume in the original process context. Hence `rw-socket-fd` and
-`ro-socket-fd` simply specify the file descriptors of the sockets. Sockets
-are not always used, however, and so -1 will be used to denote an unused
-socket.
-
+xenstored will resume in the original process context. Hence `rw-socket-fd`
+simply specifies the file descriptor of the socket. Sockets are not always
+used, however, and so -1 will be used to denote an unused socket.
\pagebreak
--
2.35.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 2/4] tools/xenstore: add documentation for new set/get-quota commands
2022-09-05 12:47 [PATCH v4 0/4] tools/xenstore: add some new features to the documentation Juergen Gross
2022-09-05 12:47 ` [PATCH v4 1/4] tools/xenstore: minor fix of the migration stream doc Juergen Gross
@ 2022-09-05 12:47 ` Juergen Gross
2022-09-05 12:47 ` [PATCH v4 3/4] tools/xenstore: add documentation for extended watch command Juergen Gross
2022-09-05 12:47 ` [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features Juergen Gross
3 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2022-09-05 12:47 UTC (permalink / raw)
To: xen-devel
Cc: Henry.Wang, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Julien Grall
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>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
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 33d8fc87f7..73670d7907 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -348,6 +348,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 3/4] tools/xenstore: add documentation for extended watch command
2022-09-05 12:47 [PATCH v4 0/4] tools/xenstore: add some new features to the documentation Juergen Gross
2022-09-05 12:47 ` [PATCH v4 1/4] tools/xenstore: minor fix of the migration stream doc Juergen Gross
2022-09-05 12:47 ` [PATCH v4 2/4] tools/xenstore: add documentation for new set/get-quota commands Juergen Gross
@ 2022-09-05 12:47 ` Juergen Gross
2022-09-05 12:47 ` [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features Juergen Gross
3 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2022-09-05 12:47 UTC (permalink / raw)
To: xen-devel
Cc: Henry.Wang, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Julien Grall
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>
Reviewed-by: Julien Grall <jgrall@amazon.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 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 73670d7907..da47d7bb16 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features
2022-09-05 12:47 [PATCH v4 0/4] tools/xenstore: add some new features to the documentation Juergen Gross
` (2 preceding siblings ...)
2022-09-05 12:47 ` [PATCH v4 3/4] tools/xenstore: add documentation for extended watch command Juergen Gross
@ 2022-09-05 12:47 ` Juergen Gross
2022-09-06 17:27 ` Julien Grall
3 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2022-09-05 12:47 UTC (permalink / raw)
To: xen-devel
Cc: Henry.Wang, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Extend the definition of the Xenstore migration stream to cover new
features:
- per domain features
- extended watches (watch depth)
- per domain quota
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
V4:
- add new record types instead of modifying the existing ones
(Julien Grall)
---
docs/designs/xenstore-migration.md | 160 ++++++++++++++++++++++++++++-
1 file changed, 155 insertions(+), 5 deletions(-)
diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
index efa526f420..c70505c43a 100644
--- a/docs/designs/xenstore-migration.md
+++ b/docs/designs/xenstore-migration.md
@@ -43,7 +43,14 @@ the setting of the endianness bit.
|-----------|---------------------------------------------------|
| `ident` | 0x78656e73746f7265 ('xenstore' in ASCII) |
| | |
-| `version` | 0x00000001 (the version of the specification) |
+| `version` | The version of the specification, defined values: |
+| | 0x00000001: all fields and records without any |
+| | explicitly mentioned version |
+| | dependency are valid. |
+| | 0x00000002: all fields and records valid for |
+| | version 1 plus fields and records |
+| | explicitly stated to be supported in |
+| | version 2 are valid. |
| | |
| `flags` | 0 (LSB): Endianness: 0 = little, 1 = big |
| | |
@@ -77,7 +84,10 @@ NOTE: padding octets here and in all subsequent format specifications must be
| | 0x00000003: WATCH_DATA |
| | 0x00000004: TRANSACTION_DATA |
| | 0x00000005: NODE_DATA |
-| | 0x00000006 - 0xFFFFFFFF: reserved for future use |
+| | 0x00000006: GLOBAL_QUOTA_DATA |
+| | 0x00000007: DOMAIN_DATA |
+| | 0x00000008: WATCH_DATA_EXTENDED (version 2 and up) |
+| | 0x00000009 - 0xFFFFFFFF: reserved for future use |
| | |
| `len` | The length (in octets) of `body` |
| | |
@@ -129,6 +139,7 @@ xenstored state that needs to be restored.
| `evtchn-fd` | The file descriptor used to communicate with |
| | the event channel driver |
+
xenstored will resume in the original process context. Hence `rw-socket-fd`
simply specifies the file descriptor of the socket. Sockets are not always
used, however, and so -1 will be used to denote an unused socket.
@@ -241,9 +252,9 @@ the file descriptor of the socket connection.
### WATCH_DATA
-The image format will contain a `WATCH_DATA` record for each watch registered
-by a connection for which there is `CONNECTION_DATA` record previously present.
-
+The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
+record for each watch registered by a connection for which there is
+`CONNECTION_DATA` record previously present.
```
0 1 2 3 octet
@@ -406,6 +417,145 @@ A node permission specifier has the following format:
Note that perm1 defines the domain owning the node. See [4] for more
explanation of node permissions.
+\pagebreak
+
+### GLOBAL_QUOTA_DATA
+
+This record is only relevant for live update. It contains the global settings
+of xenstored quota.
+
+```
+ 0 1 2 3 octet
++-------+-------+-------+-------+
+| n-dom-quota | n-glob-quota |
++---------------+---------------+
+| quota-val 1 |
++-------------------------------+
+...
++-------------------------------+
+| quota-val N |
++-------------------------------+
+| quota-names
+...
+```
+
+
+| Field | Description |
+|----------------|----------------------------------------------|
+| `n-dom-quota` | Number of quota values which apply per |
+| | domain. |
+| | |
+| `n-glob-quota` | Number of quota values which apply globally |
+| | only. |
+| | |
+| `quota-val` | Quota values, first the ones applying per |
+| | domain, then the ones applying globally. A |
+| | value of 0 has the semantics of "unlimited". |
+| | |
+| `quota-names` | 0 delimited strings of the quota names in |
+| | the same sequence as the `quota-val` values. |
+
+
+Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
+and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
+recognized by the receiving side must be ignored.
+
+\pagebreak
+
+### DOMAIN_DATA
+
+This record is optional and can be present once for each domain.
+
+
+```
+ 0 1 2 3 octet
++-------+-------+-------+-------+
+| domain-id | n-quota |
++---------------+---------------+
+| features |
++-------------------------------+
+| quota-val 1 |
++-------------------------------+
+...
++-------------------------------+
+| quota-val N |
++-------------------------------+
+| quota-names
+...
+```
+
+
+| Field | Description |
+|----------------|----------------------------------------------|
+| `domain-id` | The domain-id of the domain this record |
+| | belongs to. |
+| | |
+| `n-quota` | Number of quota values. |
+| | |
+| `features` | Value of the feature field visible by the |
+| | guest at offset 2064 of the ring page. |
+| | Aligned to the next 4 octet boundary. |
+| | Only valid for version 2 and later. |
+| | |
+| `quota-val` | Quota values, a value of 0 has the semantics |
+| | "unlimited". |
+| | |
+| `quota-names` | 0 delimited strings of the quota names in |
+| | the same sequence as the `quota-val` values. |
+
+Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
+and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
+recognized by the receiving side must be ignored.
+
+\pagebreak
+
+### WATCH_DATA_EXTENDED
+
+The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
+record for each watch registered by a connection for which there is
+`CONNECTION_DATA` record previously present. The `WATCH_DATA_EXTENDED` record
+type is valid only in version 2 and later.
+
+```
+ 0 1 2 3 octet
++-------+-------+-------+-------+
+| conn-id |
++---------------+---------------+
+| wpath-len | token-len |
++---------------+---------------+
+| depth | |
++---------------+---------------+
+| wpath
+...
+| token
+...
+```
+
+
+| Field | Description |
+|-------------|-------------------------------------------------|
+| `conn-id` | The connection that issued the `WATCH` |
+| | operation [2] |
+| | |
+| `wpath-len` | The length (in octets) of `wpath` including the |
+| | NUL terminator |
+| | |
+| `token-len` | The length (in octets) of `token` including the |
+| | NUL terminator |
+| | |
+| `depth` | The number of directory levels below the |
+| | watched path to consider for a match. |
+| | A value of 0xffff is used for unlimited depth. |
+| | |
+| `wpath` | The watch path, as specified in the `WATCH` |
+| | operation |
+| | |
+| `token` | The watch identifier token, as specified in the |
+| | `WATCH` operation |
+
+\pagebreak
+
+
* * *
[1] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/designs/non-cooperative-migration.md
--
2.35.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/4] tools/xenstore: minor fix of the migration stream doc
2022-09-05 12:47 ` [PATCH v4 1/4] tools/xenstore: minor fix of the migration stream doc Juergen Gross
@ 2022-09-06 17:07 ` Julien Grall
0 siblings, 0 replies; 10+ messages in thread
From: Julien Grall @ 2022-09-06 17:07 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Henry.Wang, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini, Wei Liu
Hi Juergen,
On 05/09/2022 13:47, Juergen Gross wrote:
> Drop mentioning the non-existent read-only socket in the migration
> stream description document.
>
> The related record field was removed in commit 8868a0e3f674 ("docs:
> update the xenstore migration stream documentation).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
I think this also want to be backported so the documentation is accurate
on older release.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features
2022-09-05 12:47 ` [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features Juergen Gross
@ 2022-09-06 17:27 ` Julien Grall
2022-09-07 6:28 ` Juergen Gross
0 siblings, 1 reply; 10+ messages in thread
From: Julien Grall @ 2022-09-06 17:27 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Henry.Wang, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini, Wei Liu
Hi Juergen,
On 05/09/2022 13:47, Juergen Gross wrote:
> Extend the definition of the Xenstore migration stream to cover new
> features:
>
> - per domain features
> - extended watches (watch depth)
> - per domain quota
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3:
> - new patch
> V4:
> - add new record types instead of modifying the existing ones
> (Julien Grall)
> ---
> docs/designs/xenstore-migration.md | 160 ++++++++++++++++++++++++++++-
> 1 file changed, 155 insertions(+), 5 deletions(-)
>
> diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
> index efa526f420..c70505c43a 100644
> --- a/docs/designs/xenstore-migration.md
> +++ b/docs/designs/xenstore-migration.md
> @@ -43,7 +43,14 @@ the setting of the endianness bit.
> |-----------|---------------------------------------------------|
> | `ident` | 0x78656e73746f7265 ('xenstore' in ASCII) |
> | | |
> -| `version` | 0x00000001 (the version of the specification) |
> +| `version` | The version of the specification, defined values: |
> +| | 0x00000001: all fields and records without any |
> +| | explicitly mentioned version |
> +| | dependency are valid. |
> +| | 0x00000002: all fields and records valid for |
> +| | version 1 plus fields and records |
> +| | explicitly stated to be supported in |
> +| | version 2 are valid. |
I think it would be useful to outline in the commit message why the
version had to be bumped.
> | | |
> | `flags` | 0 (LSB): Endianness: 0 = little, 1 = big |
> | | |
> @@ -77,7 +84,10 @@ NOTE: padding octets here and in all subsequent format specifications must be
> | | 0x00000003: WATCH_DATA |
> | | 0x00000004: TRANSACTION_DATA |
> | | 0x00000005: NODE_DATA |
> -| | 0x00000006 - 0xFFFFFFFF: reserved for future use |
> +| | 0x00000006: GLOBAL_QUOTA_DATA |
> +| | 0x00000007: DOMAIN_DATA |
> +| | 0x00000008: WATCH_DATA_EXTENDED (version 2 and up) |
> +| | 0x00000009 - 0xFFFFFFFF: reserved for future use |
> | | |
> | `len` | The length (in octets) of `body` |
> | | |
> @@ -129,6 +139,7 @@ xenstored state that needs to be restored.
> | `evtchn-fd` | The file descriptor used to communicate with |
> | | the event channel driver |
>
> +
Spurious change?
> xenstored will resume in the original process context. Hence `rw-socket-fd`
> simply specifies the file descriptor of the socket. Sockets are not always
> used, however, and so -1 will be used to denote an unused socket.
> @@ -241,9 +252,9 @@ the file descriptor of the socket connection.
>
> ### WATCH_DATA
>
> -The image format will contain a `WATCH_DATA` record for each watch registered
> -by a connection for which there is `CONNECTION_DATA` record previously present.
> -
> +The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
> +record for each watch registered by a connection for which there is
> +`CONNECTION_DATA` record previously present.
>
> ```
> 0 1 2 3 octet
> @@ -406,6 +417,145 @@ A node permission specifier has the following format:
> Note that perm1 defines the domain owning the node. See [4] for more
> explanation of node permissions.
>
> +\pagebreak
> +
> +### GLOBAL_QUOTA_DATA
> +
> +This record is only relevant for live update. It contains the global settings
> +of xenstored quota.
> +
> +```
> + 0 1 2 3 octet
> ++-------+-------+-------+-------+
> +| n-dom-quota | n-glob-quota |
> ++---------------+---------------+
> +| quota-val 1 |
> ++-------------------------------+
> +...
> ++-------------------------------+
> +| quota-val N |
> ++-------------------------------+
> +| quota-names
> +...
> +```
> +
> +
> +| Field | Description |
> +|----------------|----------------------------------------------|
> +| `n-dom-quota` | Number of quota values which apply per |
> +| | domain. |
I would add "by default" or something similar to make clear that the
value in DOMAIN_DATA will override any quota set here. But see below
about 'n-dom-quota' and 'n-glob-quota'.
> +| | |
> +| `n-glob-quota` | Number of quota values which apply globally |
> +| | only. |
> +| | |
> +| `quota-val` | Quota values, first the ones applying per |
> +| | domain, then the ones applying globally. A |
> +| | value of 0 has the semantics of "unlimited". |
It is unclear to me why you need to make the distinction between "per
domain" and "globally". IOW shouldn't be the name of the quota already
indicates that?
> +| | |
> +| `quota-names` | 0 delimited strings of the quota names in |
> +| | the same sequence as the `quota-val` values. |
> +
> +
> +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
> +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
> +recognized by the receiving side must be ignored.
> +
> +\pagebreak
> +
> +### DOMAIN_DATA
> +
> +This record is optional and can be present once for each domain.
> +
> +
> +```
> + 0 1 2 3 octet
> ++-------+-------+-------+-------+
> +| domain-id | n-quota |
> ++---------------+---------------+
> +| features |
> ++-------------------------------+
> +| quota-val 1 |
> ++-------------------------------+
> +...
> ++-------------------------------+
> +| quota-val N |
> ++-------------------------------+
> +| quota-names
> +...
> +```
> +
> +
> +| Field | Description |
> +|----------------|----------------------------------------------|
> +| `domain-id` | The domain-id of the domain this record |
> +| | belongs to. |
> +| | |
> +| `n-quota` | Number of quota values. |
> +| | |
> +| `features` | Value of the feature field visible by the |
> +| | guest at offset 2064 of the ring page. |
> +| | Aligned to the next 4 octet boundary. |
Stale sentence?
> +| | Only valid for version 2 and later. |
Can you mention explicitly whether the field will unknown or 0 for
version 1?
> +| | |
> +| `quota-val` | Quota values, a value of 0 has the semantics |
> +| | "unlimited". |
> +| | |
> +| `quota-names` | 0 delimited strings of the quota names in |
> +| | the same sequence as the `quota-val` values. |
> +
> +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
> +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
> +recognized by the receiving side must be ignored.
> +
> +\pagebreak
> +
> +### WATCH_DATA_EXTENDED
NIT: I think it would be more logical if this is defined right next
after WATCH_DATA.
> +
> +The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
> +record for each watch registered by a connection for which there is
> +`CONNECTION_DATA` record previously present. The `WATCH_DATA_EXTENDED` record
> +type is valid only in version 2 and later.
> +
> +```
> + 0 1 2 3 octet
> ++-------+-------+-------+-------+
> +| conn-id |
> ++---------------+---------------+
> +| wpath-len | token-len |
> ++---------------+---------------+
> +| depth | |
> ++---------------+---------------+
It is not clear what would be the value of octet 2-3. Is it RES0 or UNKNOWN?
> +| wpath
> +...
> +| token
> +...
> +```
> +
> +
> +| Field | Description |
> +|-------------|-------------------------------------------------|
> +| `conn-id` | The connection that issued the `WATCH` |
> +| | operation [2] |
> +| | |
> +| `wpath-len` | The length (in octets) of `wpath` including the |
> +| | NUL terminator |
> +| | |
> +| `token-len` | The length (in octets) of `token` including the |
> +| | NUL terminator |
> +| | |
> +| `depth` | The number of directory levels below the |
> +| | watched path to consider for a match. |
> +| | A value of 0xffff is used for unlimited depth. |
> +| | |
> +| `wpath` | The watch path, as specified in the `WATCH` |
> +| | operation |
> +| | |
> +| `token` | The watch identifier token, as specified in the |
> +| | `WATCH` operation |
> +
> +\pagebreak
> +
> +
> * * *
>
> [1] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/designs/non-cooperative-migration.md
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features
2022-09-06 17:27 ` Julien Grall
@ 2022-09-07 6:28 ` Juergen Gross
2022-09-07 8:44 ` Julien Grall
0 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2022-09-07 6:28 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Henry.Wang, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini, Wei Liu
[-- Attachment #1.1.1: Type: text/plain, Size: 12638 bytes --]
On 06.09.22 19:27, Julien Grall wrote:
> Hi Juergen,
>
> On 05/09/2022 13:47, Juergen Gross wrote:
>> Extend the definition of the Xenstore migration stream to cover new
>> features:
>>
>> - per domain features
>> - extended watches (watch depth)
>> - per domain quota
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3:
>> - new patch
>> V4:
>> - add new record types instead of modifying the existing ones
>> (Julien Grall)
>> ---
>> docs/designs/xenstore-migration.md | 160 ++++++++++++++++++++++++++++-
>> 1 file changed, 155 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/designs/xenstore-migration.md
>> b/docs/designs/xenstore-migration.md
>> index efa526f420..c70505c43a 100644
>> --- a/docs/designs/xenstore-migration.md
>> +++ b/docs/designs/xenstore-migration.md
>> @@ -43,7 +43,14 @@ the setting of the endianness bit.
>> |-----------|---------------------------------------------------|
>> | `ident` | 0x78656e73746f7265 ('xenstore' in ASCII) |
>> | | |
>> -| `version` | 0x00000001 (the version of the specification) |
>> +| `version` | The version of the specification, defined values: |
>> +| | 0x00000001: all fields and records without any |
>> +| | explicitly mentioned version |
>> +| | dependency are valid. |
>> +| | 0x00000002: all fields and records valid for |
>> +| | version 1 plus fields and records |
>> +| | explicitly stated to be supported in |
>> +| | version 2 are valid. |
>
> I think it would be useful to outline in the commit message why the version had
> to be bumped.
Okay.
>
>> | | |
>> | `flags` | 0 (LSB): Endianness: 0 = little, 1 = big |
>> | | |
>> @@ -77,7 +84,10 @@ NOTE: padding octets here and in all subsequent format
>> specifications must be
>> | | 0x00000003: WATCH_DATA |
>> | | 0x00000004: TRANSACTION_DATA |
>> | | 0x00000005: NODE_DATA |
>> -| | 0x00000006 - 0xFFFFFFFF: reserved for future use |
>> +| | 0x00000006: GLOBAL_QUOTA_DATA |
>> +| | 0x00000007: DOMAIN_DATA |
>> +| | 0x00000008: WATCH_DATA_EXTENDED (version 2 and up) |
>> +| | 0x00000009 - 0xFFFFFFFF: reserved for future use |
>> | | |
>> | `len` | The length (in octets) of `body` |
>> | | |
>> @@ -129,6 +139,7 @@ xenstored state that needs to be restored.
>> | `evtchn-fd` | The file descriptor used to communicate with |
>> | | the event channel driver |
>> +
>
> Spurious change?
Yes.
>
>> xenstored will resume in the original process context. Hence `rw-socket-fd`
>> simply specifies the file descriptor of the socket. Sockets are not always
>> used, however, and so -1 will be used to denote an unused socket.
>> @@ -241,9 +252,9 @@ the file descriptor of the socket connection.
>> ### WATCH_DATA
>> -The image format will contain a `WATCH_DATA` record for each watch registered
>> -by a connection for which there is `CONNECTION_DATA` record previously present.
>> -
>> +The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
>> +record for each watch registered by a connection for which there is
>> +`CONNECTION_DATA` record previously present.
>> ```
>> 0 1 2 3 octet
>> @@ -406,6 +417,145 @@ A node permission specifier has the following format:
>> Note that perm1 defines the domain owning the node. See [4] for more
>> explanation of node permissions.
>> +\pagebreak
>> +
>> +### GLOBAL_QUOTA_DATA
>> +
>> +This record is only relevant for live update. It contains the global settings
>> +of xenstored quota.
>> +
>> +```
>> + 0 1 2 3 octet
>> ++-------+-------+-------+-------+
>> +| n-dom-quota | n-glob-quota |
>> ++---------------+---------------+
>> +| quota-val 1 |
>> ++-------------------------------+
>> +...
>> ++-------------------------------+
>> +| quota-val N |
>> ++-------------------------------+
>> +| quota-names
>> +...
>> +```
>> +
>> +
>> +| Field | Description |
>> +|----------------|----------------------------------------------|
>> +| `n-dom-quota` | Number of quota values which apply per |
>> +| | domain. |
>
> I would add "by default" or something similar to make clear that the value in
> DOMAIN_DATA will override any quota set here. But see below about 'n-dom-quota'
> and 'n-glob-quota'.
Okay.
>
>> +| | |
>> +| `n-glob-quota` | Number of quota values which apply globally |
>> +| | only. |
>> +| | |
>> +| `quota-val` | Quota values, first the ones applying per |
>> +| | domain, then the ones applying globally. A |
>> +| | value of 0 has the semantics of "unlimited". |
>
> It is unclear to me why you need to make the distinction between "per domain"
> and "globally". IOW shouldn't be the name of the quota already indicates that?
I think it could help in special cases.
Imagine Xenstore A knows about global quota g and domain quota d, while
Xenstore B doesn't know both. Initially I'm running Xenstore A on a
host, then I'm live-updating to B.
B gets the information that g is global, and d is per-domain, but has no
other idea what to do with the values of g and d. OTOH it knows that each
new domain should get d with the related value, so it can set d for each
newly created domain.
When B is either downgraded to A again, or a domain is migrated to another
host running A, B can add the quota information of d for all domains.
While this is nothing I'm planning to do in the near future, it might help
e.g. in cases with mixed C-xenstored and O-xenstored setups.
It doesn't cost really much, so I wanted to support this possibility in the
migration stream from the beginning.
>
>> +| | |
>> +| `quota-names` | 0 delimited strings of the quota names in |
>> +| | the same sequence as the `quota-val` values. |
>> +
>> +
>> +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
>> +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
>> +recognized by the receiving side must be ignored.
>> +
>> +\pagebreak
>> +
>> +### DOMAIN_DATA
>> +
>> +This record is optional and can be present once for each domain.
>> +
>> +
>> +```
>> + 0 1 2 3 octet
>> ++-------+-------+-------+-------+
>> +| domain-id | n-quota |
>> ++---------------+---------------+
>> +| features |
>> ++-------------------------------+
>> +| quota-val 1 |
>> ++-------------------------------+
>> +...
>> ++-------------------------------+
>> +| quota-val N |
>> ++-------------------------------+
>> +| quota-names
>> +...
>> +```
>> +
>> +
>> +| Field | Description |
>> +|----------------|----------------------------------------------|
>> +| `domain-id` | The domain-id of the domain this record |
>> +| | belongs to. |
>> +| | |
>> +| `n-quota` | Number of quota values. |
>> +| | |
>> +| `features` | Value of the feature field visible by the |
>> +| | guest at offset 2064 of the ring page. |
>> +| | Aligned to the next 4 octet boundary. |
>
> Stale sentence?
Oh yes, a survivor of V3.
>
>> +| | Only valid for version 2 and later. |
>
> Can you mention explicitly whether the field will unknown or 0 for version 1?
We have the general note "padding octets here and in all subsequent format
specifications must be written as zero". I think this should suffice.
>
>> +| | |
>> +| `quota-val` | Quota values, a value of 0 has the semantics |
>> +| | "unlimited". |
>> +| | |
>> +| `quota-names` | 0 delimited strings of the quota names in |
>> +| | the same sequence as the `quota-val` values. |
>> +
>> +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
>> +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
>> +recognized by the receiving side must be ignored.
>> +
>> +\pagebreak
>> +
>> +### WATCH_DATA_EXTENDED
>
> NIT: I think it would be more logical if this is defined right next after
> WATCH_DATA.
I was following the record type numbering, but I can move this record
description up if you like that better.
>
>> +
>> +The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
>> +record for each watch registered by a connection for which there is
>> +`CONNECTION_DATA` record previously present. The `WATCH_DATA_EXTENDED` record
>> +type is valid only in version 2 and later.
>> +
>> +```
>> + 0 1 2 3 octet
>> ++-------+-------+-------+-------+
>> +| conn-id |
>> ++---------------+---------------+
>> +| wpath-len | token-len |
>> ++---------------+---------------+
>> +| depth | |
>> ++---------------+---------------+
>
> It is not clear what would be the value of octet 2-3. Is it RES0 or UNKNOWN?
I don't understand the question. conn-id is a 4-byte item.
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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features
2022-09-07 6:28 ` Juergen Gross
@ 2022-09-07 8:44 ` Julien Grall
2022-09-07 9:35 ` Juergen Gross
0 siblings, 1 reply; 10+ messages in thread
From: Julien Grall @ 2022-09-07 8:44 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Henry.Wang, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini, Wei Liu
Hi Juergen,
On 07/09/2022 07:28, Juergen Gross wrote:
> On 06.09.22 19:27, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 05/09/2022 13:47, Juergen Gross wrote:
>>> Extend the definition of the Xenstore migration stream to cover new
>>> features:
>>>
>>> - per domain features
>>> - extended watches (watch depth)
>>> - per domain quota
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V3:
>>> - new patch
>>> V4:
>>> - add new record types instead of modifying the existing ones
>>> (Julien Grall)
>>> ---
>>> docs/designs/xenstore-migration.md | 160 ++++++++++++++++++++++++++++-
>>> 1 file changed, 155 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/designs/xenstore-migration.md
>>> b/docs/designs/xenstore-migration.md
>>> index efa526f420..c70505c43a 100644
>>> --- a/docs/designs/xenstore-migration.md
>>> +++ b/docs/designs/xenstore-migration.md
>>> @@ -43,7 +43,14 @@ the setting of the endianness bit.
>>> |-----------|---------------------------------------------------|
>>> | `ident` | 0x78656e73746f7265 ('xenstore' in ASCII) |
>>> | | |
>>> -| `version` | 0x00000001 (the version of the specification) |
>>> +| `version` | The version of the specification, defined values: |
>>> +| | 0x00000001: all fields and records without any |
>>> +| | explicitly mentioned version |
>>> +| | dependency are valid. |
>>> +| | 0x00000002: all fields and records valid for |
>>> +| | version 1 plus fields and records |
>>> +| | explicitly stated to be supported in |
>>> +| | version 2 are valid. |
>>
>> I think it would be useful to outline in the commit message why the
>> version had to be bumped.
>
> Okay.
>
>>
>>> | | |
>>> | `flags` | 0 (LSB): Endianness: 0 = little, 1 = big |
>>> | | |
>>> @@ -77,7 +84,10 @@ NOTE: padding octets here and in all subsequent
>>> format specifications must be
>>> | | 0x00000003: WATCH_DATA |
>>> | | 0x00000004: TRANSACTION_DATA |
>>> | | 0x00000005: NODE_DATA |
>>> -| | 0x00000006 - 0xFFFFFFFF: reserved for future use |
>>> +| | 0x00000006: GLOBAL_QUOTA_DATA |
>>> +| | 0x00000007: DOMAIN_DATA |
>>> +| | 0x00000008: WATCH_DATA_EXTENDED (version 2 and up) |
>>> +| | 0x00000009 - 0xFFFFFFFF: reserved for future use |
>>> | | |
>>> | `len` | The length (in octets) of `body` |
>>> | | |
>>> @@ -129,6 +139,7 @@ xenstored state that needs to be restored.
>>> | `evtchn-fd` | The file descriptor used to communicate with |
>>> | | the event channel driver |
>>> +
>>
>> Spurious change?
>
> Yes.
>
>>
>>> xenstored will resume in the original process context. Hence
>>> `rw-socket-fd`
>>> simply specifies the file descriptor of the socket. Sockets are not
>>> always
>>> used, however, and so -1 will be used to denote an unused socket.
>>> @@ -241,9 +252,9 @@ the file descriptor of the socket connection.
>>> ### WATCH_DATA
>>> -The image format will contain a `WATCH_DATA` record for each watch
>>> registered
>>> -by a connection for which there is `CONNECTION_DATA` record
>>> previously present.
>>> -
>>> +The image format will contain either a `WATCH_DATA` or a
>>> `WATCH_DATA_EXTENDED`
>>> +record for each watch registered by a connection for which there is
>>> +`CONNECTION_DATA` record previously present.
>>> ```
>>> 0 1 2 3 octet
>>> @@ -406,6 +417,145 @@ A node permission specifier has the following
>>> format:
>>> Note that perm1 defines the domain owning the node. See [4] for more
>>> explanation of node permissions.
>>> +\pagebreak
>>> +
>>> +### GLOBAL_QUOTA_DATA
>>> +
>>> +This record is only relevant for live update. It contains the global
>>> settings
>>> +of xenstored quota.
>>> +
>>> +```
>>> + 0 1 2 3 octet
>>> ++-------+-------+-------+-------+
>>> +| n-dom-quota | n-glob-quota |
>>> ++---------------+---------------+
>>> +| quota-val 1 |
>>> ++-------------------------------+
>>> +...
>>> ++-------------------------------+
>>> +| quota-val N |
>>> ++-------------------------------+
>>> +| quota-names
>>> +...
>>> +```
>>> +
>>> +
>>> +| Field | Description |
>>> +|----------------|----------------------------------------------|
>>> +| `n-dom-quota` | Number of quota values which apply per |
>>> +| | domain. |
>>
>> I would add "by default" or something similar to make clear that the
>> value in DOMAIN_DATA will override any quota set here. But see below
>> about 'n-dom-quota' and 'n-glob-quota'.
>
> Okay.
>
>>
>>> +| | |
>>> +| `n-glob-quota` | Number of quota values which apply globally |
>>> +| | only. |
>>> +| | |
>>> +| `quota-val` | Quota values, first the ones applying per |
>>> +| | domain, then the ones applying globally. A |
>>> +| | value of 0 has the semantics of "unlimited". |
>>
>> It is unclear to me why you need to make the distinction between "per
>> domain" and "globally". IOW shouldn't be the name of the quota already
>> indicates that?
>
> I think it could help in special cases.
>
> Imagine Xenstore A knows about global quota g and domain quota d, while
> Xenstore B doesn't know both. Initially I'm running Xenstore A on a
> host, then I'm live-updating to B.
>
> B gets the information that g is global, and d is per-domain, but has no
> other idea what to do with the values of g and d. OTOH it knows that each
> new domain should get d with the related value, so it can set d for each
> newly created domain.
>
> When B is either downgraded to A again, or a domain is migrated to another
> host running A, B can add the quota information of d for all domains.
>
> While this is nothing I'm planning to do in the near future, it might help
> e.g. in cases with mixed C-xenstored and O-xenstored setups.
>
> It doesn't cost really much, so I wanted to support this possibility in the
> migration stream from the beginning.
I can see the use-case. However...
>
>>
>>> +| | |
>>> +| `quota-names` | 0 delimited strings of the quota names in |
>>> +| | the same sequence as the `quota-val` values. |
>>> +
>>> +
>>> +Allowed quota names are those explicitly named in [2] for the
>>> `GET_QUOTA`
>>> +and `SET_QUOTA` commands, plus implementation specific ones. Quota
>>> names not
>>> +recognized by the receiving side must be ignored.
... this is seem to directly conflict with this sentence as to me
"ignore" means drop. What you want is for Xenstore to optionally preserve.
Also, I think what you wrote above would be helpful in the commit
message. It gives some insights for future reader on how the stream was
designed.
>>> +
>>> +\pagebreak
>>> +
>>> +### DOMAIN_DATA
>>> +
>>> +This record is optional and can be present once for each domain.
>>> +
>>> +
>>> +```
>>> + 0 1 2 3 octet
>>> ++-------+-------+-------+-------+
>>> +| domain-id | n-quota |
>>> ++---------------+---------------+
>>> +| features |
>>> ++-------------------------------+
>>> +| quota-val 1 |
>>> ++-------------------------------+
>>> +...
>>> ++-------------------------------+
>>> +| quota-val N |
>>> ++-------------------------------+
>>> +| quota-names
>>> +...
>>> +```
>>> +
>>> +
>>> +| Field | Description |
>>> +|----------------|----------------------------------------------|
>>> +| `domain-id` | The domain-id of the domain this record |
>>> +| | belongs to. |
>>> +| | |
>>> +| `n-quota` | Number of quota values. |
>>> +| | |
>>> +| `features` | Value of the feature field visible by the |
>>> +| | guest at offset 2064 of the ring page. |
>>> +| | Aligned to the next 4 octet boundary. |
>>
>> Stale sentence?
>
> Oh yes, a survivor of V3.
>
>>
>>> +| | Only valid for version 2 and later. |
>>
>> Can you mention explicitly whether the field will unknown or 0 for
>> version 1?
>
> We have the general note "padding octets here and in all subsequent format
> specifications must be written as zero". I think this should suffice.
I don't view this field as padding because it has a meaning. So I argue
it is not cover by the sentence.
Therefore I would add "Otherwise, the field is unknown/0" (pick the one
you prefer between 0 and unknown).
>
>>
>>> +| | |
>>> +| `quota-val` | Quota values, a value of 0 has the semantics |
>>> +| | "unlimited". |
>>> +| | |
>>> +| `quota-names` | 0 delimited strings of the quota names in |
>>> +| | the same sequence as the `quota-val` values. |
>>> +
>>> +Allowed quota names are those explicitly named in [2] for the
>>> `GET_QUOTA`
>>> +and `SET_QUOTA` commands, plus implementation specific ones. Quota
>>> names not
>>> +recognized by the receiving side must be ignored.
>>> +
>>> +\pagebreak
>>> +
>>> +### WATCH_DATA_EXTENDED
>>
>> NIT: I think it would be more logical if this is defined right next
>> after WATCH_DATA.
>
> I was following the record type numbering, but I can move this record
> description up if you like that better.
>
>>
>>> +
>>> +The image format will contain either a `WATCH_DATA` or a
>>> `WATCH_DATA_EXTENDED`
>>> +record for each watch registered by a connection for which there is
>>> +`CONNECTION_DATA` record previously present. The
>>> `WATCH_DATA_EXTENDED` record
>>> +type is valid only in version 2 and later.
>>> +
>>> +```
>>> + 0 1 2 3 octet
>>> ++-------+-------+-------+-------+
>>> +| conn-id |
>>> ++---------------+---------------+
>>> +| wpath-len | token-len |
>>> ++---------------+---------------+
>>> +| depth | |
>>> ++---------------+---------------+
>>
>> It is not clear what would be the value of octet 2-3. Is it RES0 or
>> UNKNOWN?
>
> I don't understand the question. conn-id is a 4-byte item.
I was referring to the blank after 'depth'. In other record, we use 'pad'.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features
2022-09-07 8:44 ` Julien Grall
@ 2022-09-07 9:35 ` Juergen Gross
0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2022-09-07 9:35 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Henry.Wang, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini, Wei Liu
[-- Attachment #1.1.1: Type: text/plain, Size: 6619 bytes --]
On 07.09.22 10:44, Julien Grall wrote:
> Hi Juergen,
>
> On 07/09/2022 07:28, Juergen Gross wrote:
>> On 06.09.22 19:27, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 05/09/2022 13:47, Juergen Gross wrote:
>>>> Extend the definition of the Xenstore migration stream to cover new
>>>> features:
>>>>
>>>> - per domain features
>>>> - extended watches (watch depth)
>>>> - per domain quota
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V3:
>>>> - new patch
>>>> V4:
>>>> - add new record types instead of modifying the existing ones
>>>> (Julien Grall)
>>>> ---
>>>> docs/designs/xenstore-migration.md | 160 ++++++++++++++++++++++++++++-
>>>> 1 file changed, 155 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/docs/designs/xenstore-migration.md
>>>> b/docs/designs/xenstore-migration.md
>>>> index efa526f420..c70505c43a 100644
>>>> --- a/docs/designs/xenstore-migration.md
>>>> +++ b/docs/designs/xenstore-migration.md
>>>> +| | |
>>>> +| `quota-names` | 0 delimited strings of the quota names in |
>>>> +| | the same sequence as the `quota-val` values. |
>>>> +
>>>> +
>>>> +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
>>>> +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
>>>> +recognized by the receiving side must be ignored.
>
> ... this is seem to directly conflict with this sentence as to me "ignore" means
> drop. What you want is for Xenstore to optionally preserve.
I'll rephrase that.
>
> Also, I think what you wrote above would be helpful in the commit message. It
> gives some insights for future reader on how the stream was designed.
Okay.
>
>>>> +
>>>> +\pagebreak
>>>> +
>>>> +### DOMAIN_DATA
>>>> +
>>>> +This record is optional and can be present once for each domain.
>>>> +
>>>> +
>>>> +```
>>>> + 0 1 2 3 octet
>>>> ++-------+-------+-------+-------+
>>>> +| domain-id | n-quota |
>>>> ++---------------+---------------+
>>>> +| features |
>>>> ++-------------------------------+
>>>> +| quota-val 1 |
>>>> ++-------------------------------+
>>>> +...
>>>> ++-------------------------------+
>>>> +| quota-val N |
>>>> ++-------------------------------+
>>>> +| quota-names
>>>> +...
>>>> +```
>>>> +
>>>> +
>>>> +| Field | Description |
>>>> +|----------------|----------------------------------------------|
>>>> +| `domain-id` | The domain-id of the domain this record |
>>>> +| | belongs to. |
>>>> +| | |
>>>> +| `n-quota` | Number of quota values. |
>>>> +| | |
>>>> +| `features` | Value of the feature field visible by the |
>>>> +| | guest at offset 2064 of the ring page. |
>>>> +| | Aligned to the next 4 octet boundary. |
>>>
>>> Stale sentence?
>>
>> Oh yes, a survivor of V3.
>>
>>>
>>>> +| | Only valid for version 2 and later. |
>>>
>>> Can you mention explicitly whether the field will unknown or 0 for version 1?
>>
>> We have the general note "padding octets here and in all subsequent format
>> specifications must be written as zero". I think this should suffice.
>
> I don't view this field as padding because it has a meaning. So I argue it is
> not cover by the sentence.
I'll rephrase that to: "padding octets or fields not valid in the used version
here and in all ..."
>
> Therefore I would add "Otherwise, the field is unknown/0" (pick the one you
> prefer between 0 and unknown).
>
>>
>>>
>>>> +| | |
>>>> +| `quota-val` | Quota values, a value of 0 has the semantics |
>>>> +| | "unlimited". |
>>>> +| | |
>>>> +| `quota-names` | 0 delimited strings of the quota names in |
>>>> +| | the same sequence as the `quota-val` values. |
>>>> +
>>>> +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
>>>> +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
>>>> +recognized by the receiving side must be ignored.
>>>> +
>>>> +\pagebreak
>>>> +
>>>> +### WATCH_DATA_EXTENDED
>>>
>>> NIT: I think it would be more logical if this is defined right next after
>>> WATCH_DATA.
>>
>> I was following the record type numbering, but I can move this record
>> description up if you like that better.
>>
>>>
>>>> +
>>>> +The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
>>>> +record for each watch registered by a connection for which there is
>>>> +`CONNECTION_DATA` record previously present. The `WATCH_DATA_EXTENDED` record
>>>> +type is valid only in version 2 and later.
>>>> +
>>>> +```
>>>> + 0 1 2 3 octet
>>>> ++-------+-------+-------+-------+
>>>> +| conn-id |
>>>> ++---------------+---------------+
>>>> +| wpath-len | token-len |
>>>> ++---------------+---------------+
>>>> +| depth | |
>>>> ++---------------+---------------+
>>>
>>> It is not clear what would be the value of octet 2-3. Is it RES0 or UNKNOWN?
>>
>> I don't understand the question. conn-id is a 4-byte item.
>
> I was referring to the blank after 'depth'. In other record, we use 'pad'.
Okay, will add "pad" to it.
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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-07 9:36 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-05 12:47 [PATCH v4 0/4] tools/xenstore: add some new features to the documentation Juergen Gross
2022-09-05 12:47 ` [PATCH v4 1/4] tools/xenstore: minor fix of the migration stream doc Juergen Gross
2022-09-06 17:07 ` Julien Grall
2022-09-05 12:47 ` [PATCH v4 2/4] tools/xenstore: add documentation for new set/get-quota commands Juergen Gross
2022-09-05 12:47 ` [PATCH v4 3/4] tools/xenstore: add documentation for extended watch command Juergen Gross
2022-09-05 12:47 ` [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features Juergen Gross
2022-09-06 17:27 ` Julien Grall
2022-09-07 6:28 ` Juergen Gross
2022-09-07 8:44 ` Julien Grall
2022-09-07 9:35 ` Juergen Gross
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.