Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands
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 --- 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 || xenstored prevents the use of SET_TARGET other than by dom0. +GET_FEATURE | | Did you indented to add many spaces before ? +SET_FEATURE || + Returns or sets the contents of the "feature" field located at + offset 2064 of the Xenstore ring page of the domain specified by + . 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 OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands
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 --- 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 || xenstored prevents the use of SET_TARGET other than by dom0. +GET_FEATURE | | Did you indented to add many spaces before ? +SET_FEATURE || + Returns or sets the contents of the "feature" field located at + offset 2064 of the Xenstore ring page of the domain specified by + . 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
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 --- 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 || xenstored prevents the use of SET_TARGET other than by dom0. +GET_FEATURE | | Did you indented to add many spaces before ? +SET_FEATURE || + Returns or sets the contents of the "feature" field located at + offset 2064 of the Xenstore ring page of the domain specified by + . 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 OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands
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 --- 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 || xenstored prevents the use of SET_TARGET other than by dom0. +GET_FEATURE | | Did you indented to add many spaces before ? Please ignore this question. is part of the response. Sorry for the noise. Cheers, +SET_FEATURE || + Returns or sets the contents of the "feature" field located at + offset 2064 of the Xenstore ring page of the domain specified by + . 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
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 --- 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 @@ MaskDescription 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 || xenstored prevents the use of SET_TARGET other than by dom0. +GET_FEATURE | | Did you indented to add many spaces before ? +SET_FEATURE|| + Returns or sets the contents of the "feature" field located at + offset 2064 of the Xenstore ring page of the domain specified by + . 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
> On 16 Mar 2022, at 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 Reviewed-by: Luca Fancellu 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 @@ MaskDescription > - > 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 || > > xenstored prevents the use of SET_TARGET other than by dom0. > > +GET_FEATURE || > +SET_FEATURE || > + Returns or sets the contents of the "feature" field located at > + offset 2064 of the Xenstore ring page of the domain specified by > + . 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 |[|] > -- > 2.34.1 > >
