Re: [Xen-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@xxxxxxxxx]
> Sent: 13 March 2019 18:12
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; 
> marmarek@xxxxxxxxxxxxxxxxxxxxxx; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Anthony Perard 
> <anthony.perard@xxxxxxxxxx>; Paolo Bonzini
> <pbonzini@xxxxxxxxxx>; Richard Henderson <rth@xxxxxxxxxxx>; Eduardo Habkost 
> <ehabkost@xxxxxxxxxx>;
> Michael S. Tsirkin <mst@xxxxxxxxxx>; Marcel Apfelbaum 
> <marcel.apfelbaum@xxxxxxxxx>
> Subject: Re: [PATCH 2/6] xen: Move xenstore initialization to common location
> 
> On Wed, Mar 13, 2019 at 11:01 AM Paul Durrant <Paul.Durrant@xxxxxxxxxx> wrote:
> >
> > > -----Original Message-----
> > > From: Jason Andryuk [mailto:jandryuk@xxxxxxxxx]
> > > Sent: 11 March 2019 18:02
> > > To: qemu-devel@xxxxxxxxxx
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; marmarek@xxxxxxxxxxxxxxxxxxxxxx; 
> > > Jason Andryuk
> > > <jandryuk@xxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> > > Anthony Perard
> > > <anthony.perard@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 
> > > Paolo Bonzini
> > > <pbonzini@xxxxxxxxxx>; Richard Henderson <rth@xxxxxxxxxxx>; Eduardo 
> > > Habkost <ehabkost@xxxxxxxxxx>;
> > > Michael S. Tsirkin <mst@xxxxxxxxxx>; Marcel Apfelbaum 
> > > <marcel.apfelbaum@xxxxxxxxx>
> > > Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> > >
> > > For the xen stubdom case, we'll want xenstore initialized, but we'll
> > > want to skip the rest of xen_be_init.  Move the initialization to
> > > xen_hvm_init so we can conditionalize calling xen_be_init.
> > >
> > > xs_domain_open() is deprecated for xs_open(0), so make the replacement
> > > as well.
> >
> > Can you elaborate as to why you need to do this when the code at the top of 
> > xen_hvm_init() already
> opens xenstore for its own purposes, and AFAICT xenstore_update() is only 
> needed if QEMU is
> implementing a PV backend?
> >
> >
> 
> Hi, Paul.  Thanks for reviewing.
> 
> I think you are right, that this basically shouldn't be needed if PV
> backends are disabled.  This patch came out of OpenXT, where it is
> needed for some out-of-tree patches.  But that doesn't make it
> suitable for upstreaming.
> 
> However, while reviewing, it looks like the xen accelerator in
> hw/xen/xen-common.c:xen_init() registers xen_change_state_handler().
> xen_change_state_handler() uses the global xenstore handle and will
> exit(1) if NULL.

I see it yes. TBH signalling state via xenstore should go away as it is 
incompatible with deprivileging, and I think Anthony might have some patches 
for that? In the meantime I suggest just doing a local xs_open(0) in that 
function.

> I'm not sure how to get the XenIOState xenstore
> handle over to the accelerator's xen_init.

That would not be appropriate as the machine type may not be xenfv and hence 
xen_hvm_init() may not have been called.

  Paul

>  Outside of that, I think
> you are correct that xenstore_update doesn't need to be run when PV
> backends are disabled.
> 
> Thanks,
> Jason
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel