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

[Jürgen Groß <jgross@xxxxxxxx>]

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 
> > > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/designs/non-cooperative-migration.md
> > >
> > > 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.
>
> > > v2:
> > >    - Address comments from Juergen
> > Not all unfortunately. :-(
> OK.
>
> > > +### CONNECTION_DATA
> > >
> > > -Each WATCH_DATA record specifies a registered watch and is formatted as
> > > -follows:
> > > +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
> > > -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                                  |
> > > +|             |                                                 |
> > > +| `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)                     |
> > >
> > > -**TRANSACTION_DATA**
> > > +The format of `conn-spec` is dependent upon `conn-type`.
> > >
> > > +\pagebreak
> > >
> > > -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;
> > #define XS_STATE_CONN_TYPE_RING   0
> > #define XS_STATE_CONN_TYPE_SOCKET 1
> >       uint16_t flags;
> > #define XS_STATE_CONN_INTRODUCED  0x0001
> > #define XS_STATE_CONN_RELEASED    0x0002
> > #define XS_STATE_CONN_READONLY    0x0004
> >       union {
> >           struct {
> >               uint16_t domid;
> >               uint16_t tdomid;
> > #define XS_STATE_DOMID_INVALID  0xffffU
> >               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 easier.
>
> > >    ```
> > >
> > > -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.


Juergen