[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 27.04.20 13:25, Paul Durrant wrote:
-----Original Message-----
From: Jürgen Groß <jgross@xxxxxxxx>
Sent: 27 April 2020 12:13
To: paul@xxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: 'Paul Durrant' <pdurrant@xxxxxxxxxx>
Subject: Re: [PATCH v2] docs/designs: re-work the xenstore migration document...

On 27.04.20 12:45, Paul Durrant wrote:
-----Original Message-----
From: Jürgen Groß <jgross@xxxxxxxx>
Sent: 27 April 2020 11:37
To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Paul Durrant <pdurrant@xxxxxxxxxx>
Subject: Re: [PATCH v2] docs/designs: re-work the xenstore migration document...

On 27.04.20 09:53, Paul Durrant wrote:
From: Paul Durrant <pdurrant@xxxxxxxxxx>

... to specify a separate migration stream that will also be suitable for
live update.

The original scope of the document was to support non-cooperative migration
of guests [1] but, since then, live update of xenstored has been brought into
scope. Thus it makes more sense to define a separate image format for
serializing xenstore state that is suitable for both purposes.

The document has been limited to specifying a new image format. The mechanism
for acquiring the image for live update or migration is not covered as that
is more appropriately dealt with by a patch to docs/misc/xenstore.txt. It is
also expected that, when the first implementation of live update or migration
making use of this specification is committed, that the document is moved from
docs/designs into docs/specs.

[1] See 

Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
Juergen Gross <jgross@xxxxxxxx>
Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
George Dunlap <george.dunlap@xxxxxxxxxx>
Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Jan Beulich <jbeulich@xxxxxxxx>
Julien Grall <julien@xxxxxxx>
Stefano Stabellini <sstabellini@xxxxxxxxxx>
Wei Liu <wl@xxxxxxx>

Mind adding CC: before those mail addresses in order to let git add
those to the recipients list?

D'oh... good spot.

    - Address comments from Juergen

Not all unfortunately. :-(



-Each WATCH_DATA record specifies a registered watch and is formatted as
+For live update the image format will contain a `CONNECTION_DATA` record for
+each connection to xenstore. For migration it will only contain a record for
+the domain being migrated.

-    0       1       2       3     octet
-| WATCH_DATA                    |
-| wpath length                  |
-| wpath data                    |
-| pad (0 to 3 octets)           |
+    0       1       2       3       4       5       6       7    octet
+| conn-id                       | pad                           |
+| conn-type     | conn-spec

I asked whether it wouldn't be better to drop the pad and move conn-type
and a 2-byte (unified) flag field at its position. This together ...

+| data-len                      | data
-| token length                  |
-| token data                    |
-| pad (0 to 3 octets)           |

-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

+| 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                                  |
+|             |                                                 |
+| `conn-spec` | See below                                       |
+|             |                                                 |
+| `data-len`  | The length (in octets) of any pending data not  |
+|             | yet written to the connection                   |
+|             |                                                 |
+| `data`      | Pending data (may be empty)                     |

+The format of `conn-spec` is dependent upon `conn-type`.


-Each TRANSACTION_DATA record specifies an open transaction and is formatted
-as follows:
+For `shared ring` connections it is as follows:

-    0       1       2       3     octet
-| TRANSACTION_DATA              |
-| tx_id                         |
+    0       1       2       3       4       5       6       7    octet
+                +-------+-------+-------+-------+-------+-------+
+                | domid         | tdomid        | flags         |
+| revtchn                       | levtchn                       |
+| mfn                                                           |

... with dropping levtchn (which isn't needed IMO) will make it much
easier to have a union in C (which needs to be aligned to 8 bytes
and have a length of a multiple of 8 bytes due to mfn).

So something like:

struct xs_state_connection {
       uint32_t conn_id;
       uint16_t conn_type;
       uint16_t flags;
#define XS_STATE_CONN_RELEASED    0x0002
#define XS_STATE_CONN_READONLY    0x0004
       union {
           struct {
               uint16_t domid;
               uint16_t tdomid;
               uint32_t evtchn;
               uint64_t mfn;
#define XS_STATE_MFN_INVALID    0xffffffffffffffffUL
           } ring;
           int32_t socket_fd;
       } spec;
       uint32_t data_out_len;
       uint8_t  data[];

The issue is making sure that the mfn is properly aligned. If I can drop the 
levtchn then this gets


-where tx_id is the non-zero identifier values of an open transaction.

-### Protocol Extension
+| 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 otherwise         |

DOMID_INVALID needs to be defined (or we need a reference where it is
coming from).

OK. It's in a public header... I'll reference it.

+|            |                                                  |
+| `flags`    | A bit-wise OR of:                                |
+|            | 0x0001: INTRODUCE has been issued                |

Just realized, I think we can drop those flags.

Reasoning: if INTRODUCE hasn't been issued, there can't be an active
connection to Xenstore for that domain, as Xenstore doesn't know about
the parameters to connect (especially the event channel is missing).

+|            | 0x0002: RELEASE has been issued                  |

And the same applies here: RELEASE will drop the connection to the
domain, so it can't appear in a connection record.

I think the presence of the RESUME command in xenstore.txt makes it non-obvious 
that we can forget about a domain once RELEASE has been called. The text there 
does say:

"It is not clear whether this is possible since one would
normally expect a domain not to be restarted after being shut
down without being destroyed in the meantime.  There are
currently no users of this request in xen-unstable."

So, perhaps this would be the time to remove RESUME from the spec?





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