> -----Original Message----- > From: Jason Andryuk [mailto:[email protected]] > Sent: 13 March 2019 18:12 > To: Paul Durrant <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; Stefano > Stabellini <[email protected]>; Anthony Perard > <[email protected]>; Paolo Bonzini > <[email protected]>; Richard Henderson <[email protected]>; Eduardo Habkost > <[email protected]>; > Michael S. Tsirkin <[email protected]>; Marcel Apfelbaum > <[email protected]> > Subject: Re: [PATCH 2/6] xen: Move xenstore initialization to common location > > On Wed, Mar 13, 2019 at 11:01 AM Paul Durrant <[email protected]> wrote: > > > > > -----Original Message----- > > > From: Jason Andryuk [mailto:[email protected]] > > > Sent: 11 March 2019 18:02 > > > To: [email protected] > > > Cc: [email protected]; [email protected]; > > > Jason Andryuk > > > <[email protected]>; Stefano Stabellini <[email protected]>; > > > Anthony Perard > > > <[email protected]>; Paul Durrant <[email protected]>; > > > Paolo Bonzini > > > <[email protected]>; Richard Henderson <[email protected]>; Eduardo > > > Habkost <[email protected]>; > > > Michael S. Tsirkin <[email protected]>; Marcel Apfelbaum > > > <[email protected]> > > > 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
