Re: [PATCH v4] docs/designs: re-work the xenstore migration document...

[Julien Grall <julien@xen.org>]

Hi Paul,

On 27/04/2020 16:50, Paul Durrant wrote:
> diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
> index 6ab351e8fe..51d8b85171 100644
> --- a/docs/designs/xenstore-migration.md
> +++ b/docs/designs/xenstore-migration.md
> @@ -3,254 +3,400 @@
>   ## Background
>   
>   The design for *Non-Cooperative Migration of Guests*[1] explains that extra
> -save records are required in the migrations stream to allow a guest running
> -PV drivers to be migrated without its co-operation. Moreover the save
> -records must include details of registered xenstore watches as well as
> -content; information that cannot currently be recovered from `xenstored`,
> -and hence some extension to the xenstore protocol[2] will also be required.
> -
> -The *libxenlight Domain Image Format* specification[3] already defines a
> -record type `EMULATOR_XENSTORE_DATA` but this is not suitable for
> -transferring xenstore data pertaining to the domain directly as it is
> -specified such that keys are relative to the path
> -`/local/domain/$dm_domid/device-model/$domid`. Thus it is necessary to
> -define at least one new save record type.
> +save records are required in the migrations stream to allow a guest running PV
> +drivers to be migrated without its co-operation. Moreover the save records must
> +include details of registered xenstore watches as well ascontent; information

s/ascontent/as content/

[...]

> +### END
> +
> +The end record marks the end of the image, and is the final record
> +in the stream.
>   
>   ```
> -    0       1       2       3     octet
> -+-------+-------+-------+-------+
> -| NODE_DATA                     |
> -+-------------------------------+
> -| path length                   |
> -+-------------------------------+
> -| path data                     |
> -...
> -| pad (0 to 3 octets)           |
> -+-------------------------------+
> -| perm count (N)                |
> -+-------------------------------+
> -| perm0                         |
> -+-------------------------------+
> -...
> -+-------------------------------+
> -| permN                         |
> -+-------------------------------+
> -| value length                  |
> -+-------------------------------+
> -| value data                    |
> -...
> -| pad (0 to 3 octets)           |
> -+-------------------------------+
> +    0       1       2       3       4       5       6       7    octet
> ++-------+-------+-------+-------+-------+-------+-------+-------+
>   ```
>   
> -where perm0..N are formatted as follows:
>   
> +The end record contains no fields; its body length is 0.
> +
> +\pagebreak
> +
> +### GLOBAL_DATA
> +
> +This record is only relevant for live update. It contains details of global
> +xenstored state that needs to be restored.

Reading this paragraph, it sounds like GLOBAL_DATA should always be 
present in the liveupdate stream. However ...

[...]

> -path length and value length are specified in octets (excluding the NUL
> -terminator of the path). perm should be one of the ASCII values `w`, `r`,
> -`b` or `n` as described in [2]. All pad values should be 0.
> -All paths should be absolute (i.e. start with `/`) and as described in
> -[2].
> +| Field          | Description                                  |
> +|----------------|----------------------------------------------|
> +| `rw-socket-fd` | The file descriptor of the socket accepting  |
> +|                | read-write connections                       |
> +|                |                                              |
> +| `ro-socket-fd` | The file descriptor of the socket accepting  |
> +|                | read-only connections                        |
>   
> +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 may not always be available in XenStored. So should we 
reserve a value for "not in-use socket"?

[...]

> -wpath length and token length are specified in octets (excluding the NUL
> -terminator). The wpath should be as described for the `WATCH` operation in
> -[2]. The token is an arbitrary string of octets not containing any NUL
> -values.
>   
> +| Field       | Description                                     |
> +|-------------|-------------------------------------------------|
> +| `conn-id`   | A non-zero number used to identify this         |
> +|             | connection in subsequent connection-specific    |
> +|             | records                                         |
> +|             |                                                 |
> +| `conn-type` | 0x0000: shared ring                             |
> +|             | 0x0001: socket                                  |

I would write "0x0002 - 0xFFFF: reserved for future use" to match the 
rest of the specification.

[...]

> -where tx_id is the non-zero identifier values of an open transaction.
>   
> +| Field     | Description                                       |
> +|-----------|---------------------------------------------------|
> +| `domid`   | The domain-id that owns the shared page           |
> +|           |                                                   |
> +| `tdomid`  | The domain-id that `domid` acts on behalf of if   |
> +|           | it has been subject to an SET_TARGET              |
> +|           | operation [2] or DOMID_INVALID [3] otherwise      |
> +|           |                                                   |
> +| `flags`   | Must be zero                                      |
> +|           |                                                   |
> +| `evtchn`  | The port number of the interdomain channel used   |
> +|           | by `domid` to communicate with xenstored          |
> +|           |                                                   |
> +| `mfn`     | The MFN of the shared page for a live update or   |
> +|           | ~0 (i.e. all bits set) otherwise                  |
>   
> -### Protocol Extension
> +Since the ABI guarantees that entry 1 in `domid`'s grant table will always
> +contain the GFN of the shared page, so for a live update `mfn` can be used to
> +give confidence that `domid` has not been re-cycled during the update.

I am confused, there is no way a XenStored running in an Arm domain can 
map the MFN of the shared page. So how is this going to work out?

[...]

> -START_DOMAIN_TRANSACTION    <domid>|<transid>|
> +    0       1       2       3    octet
> ++-------+-------+-------+-------+
> +| conn-id                       |
> ++-------------------------------+
> +| tx-id                         |
> ++---------------+---------------+
> +| path-len      | value-len     |
> ++---------------+---------------+
> +| access        | perm-count    |
> ++---------------+---------------+
> +| perm1                         |
> ++-------------------------------+
> +...
> ++-------------------------------+
> +| permN                         |
> ++---------------+---------------+
> +| path
> +...
> +| value
> +...
> +```
> +
> +
> +| Field        | Description                                    |
> +|--------------|------------------------------------------------|
> +| `conn-id`    | If this value is non-zero then this record     |
> +|              | related to a pending transaction               |

If conn-id is 0, how do you know the owner of the node?

> +|              |                                                |
> +| `tx-id`      | This value should be ignored if `conn-id` is   |
> +|              | zero. Otherwise it specifies the id of the     |
> +|              | pending transaction                            |
> +|              |                                                |
> +| `path-len`   | The length (in octets) of `path` including the |
> +|              | NUL terminator                                 |
> +|              |                                                |
> +| `value-len`  | The length (in octets) of `value` (which will  |
> +|              | be zero for a deleted node)                    |
> +|              |                                                |
> +| `access`     | This value should be ignored if this record    |
> +|              | does not relate to a pending transaction,      |
> +|              | otherwise it specifies the accesses made to    |
> +|              | the node and hence is a bitwise OR of:         |
> +|              |                                                |
> +|              | 0x0001: read                                   |
> +|              | 0x0002: written                                |
> +|              |                                                |
> +|              | The value will be zero for a deleted node      |
> +|              |                                                |
> +| `perm-count` | The number (N) of node permission specifiers   |
> +|              | (which will be 0 for a node deleted in a       |
> +|              | pending transaction)                           |
> +|              |                                                |
> +| `perm1..N`   | A list of zero or more node permission         |
> +|              | specifiers (see below)                         |

This is a bit odd to start at one. Does it mean perm0 exists and not 
preserved?

> +|              |                                                |
> +| `path`       | The absolute path of the node                  |
> +|              |                                                |
> +| `value`      | The node value (which may be empty or contain  |
> +|              | NUL octets)                                    |

Cheers,

-- 
Julien Grall