Re: [PATCH v2 2/4] tools/xenstore: add documentation for new set/get-feature commands

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 24/06/2022 05:13, Juergen Gross wrote:
> On 23.06.22 20:27, Julien Grall wrote:
> > On 27/05/2022 08:24, 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 <jgross@xxxxxxxx>
> > > ---
> > > Do we need support in the migration protocol for the features?
> > I would say yes. You want to make sure that the client can be migrated 
> > without loosing features between two xenstored.
> > > V2:
> > > - remove feature bit (Julien Grall)
> > > - GET_FEATURE without domid will return Xenstore supported features
> > >    (triggered by Julien Grall)
> > > ---
> > >   docs/misc/xenstore.txt | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > >
> > > diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
> > > index a3d3da0a5b..00f6969202 100644
> > > --- a/docs/misc/xenstore.txt
> > > +++ b/docs/misc/xenstore.txt
> > > @@ -331,6 +331,20 @@ SET_TARGET        <domid>|<tdomid>|
> > >       xenstored prevents the use of SET_TARGET other than by dom0.
> > > +GET_FEATURE        [<domid>|]        <value>|
> > > +SET_FEATURE        <domid>|<value>|
> > > +    Returns or sets the contents of the "feature" field located at
> > > +    offset 2064 of the Xenstore ring page of the domain specified by
> > > +    <domid>. <value> is a decimal number being a logical or of the
> > In the context of migration, I am stilll a bit concerned that the 
> > features are stored in the ring because the guest could overwrite it.
> > I would expect the migration code to check that the GET_FEATURE 
> > <domid> is a subset of GET_FEATURE on the targeted Xenstored. So it 
> > can easily prevent a guest to migrate.
> > So I think this should be a shadow copy that will be returned instead 
> > of the contents of the "feature" field.
> Of course. The value in the ring is meant only for the guest. Xenstore
> will have the authoritative value in its private memory.
Good. I would suggest to clarify it in xenstore.txt because it suggests 
otherwise so far.
> > > +    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. Providing no <domid> with the
> > > +    GET_FEATURE command will return the features which are supported
> > > +    by Xenstore.
> > Do we want to allow modifying the features when the guest is running?
> I think we can't remove features, but adding would be fine. For
Agree with removing features, regarding adding I think we would need a 
mechanism to tell the client there is a new feature. It would require 
some work, so...
> simplicity it might be better to just deny a modification while the
> guest is running.
... I agree this is probably best for a first shot. This can be relaxed 
afterwards.
Cheers,

--
Julien Grall