Re: [Qemu-devel] [v3 2/5] Qemu-Xen-vTPM: Xen frontend driver infrastructure

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Thu, 29 Jan 2015, Xu, Quan wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: Tuesday, January 20, 2015 1:15 AM
> > To: Xu, Quan
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org;
> > stefano.stabellini@eu.citrix.com
> > Subject: Re: [v3 2/5] Qemu-Xen-vTPM: Xen frontend driver infrastructure
> > 
> > On Tue, 30 Dec 2014, Quan Xu wrote:
> > > This patch adds infrastructure for xen front drivers living in qemu,
> > > so drivers don't need to implement common stuff on their own.  It's
> > > mostly xenbus management stuff: some functions to access XenStore,
> > > setting up XenStore watches, callbacks on device discovery and state
> > > changes, and handle event channel between the virtual machines.
> > >
> > > Call xen_fe_register() function to register XenDevOps, and make sure,
> > > XenDevOps's flags is DEVOPS_FLAG_FE, which is flag bit to point out
> > > the XenDevOps is Xen frontend.
> > >
> > > --Changes in v3:
> > > -New xen_frontend.c file
> > > -Move xenbus_switch_state() to xen_frontend.c -Move xen_stubdom_be()
> > > to xenstore_fe_read_be_str() -Move *_stubdom_*() to *_fe_*()
> > >
> > > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > > ---
> > >  hw/xen/Makefile.objs         |   2 +-
> > >  hw/xen/xen_backend.c         |  11 +-
> > >  hw/xen/xen_frontend.c        | 323
> > +++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/xen/xen_backend.h |  14 ++
> > >  4 files changed, 348 insertions(+), 2 deletions(-)  create mode
> > > 100644 hw/xen/xen_frontend.c
> > >
> > > diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs index
> > > a0ca0aa..b0bb065 100644
> > > --- a/hw/xen/Makefile.objs
> > > +++ b/hw/xen/Makefile.objs
> > > @@ -1,5 +1,5 @@
> > >  # xen backend driver support
> > > -common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
> > > +common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
> > > +xen_frontend.o
> > >
> > >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> > >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o
> > > xen_pt_msi.o diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> > > index b2cb22b..ad6e324 100644
> > > --- a/hw/xen/xen_backend.c
> > > +++ b/hw/xen/xen_backend.c
> > > @@ -275,7 +275,7 @@ static struct XenDevice *xen_be_get_xendev(const
> > > char *type, int dom, int dev,
> > >  /*
> > >   * release xen backend device.
> > >   */
> > > -static struct XenDevice *xen_be_del_xendev(int dom, int dev)
> > > +struct XenDevice *xen_be_del_xendev(int dom, int dev)
> > >  {
> > >      struct XenDevice *xendev, *xnext;
> > >
> > > @@ -681,6 +681,10 @@ static void xenstore_update(void *unused)
> > >      if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) == 1) {
> > >          xenstore_update_fe(vec[XS_WATCH_PATH], (void*)ptr);
> > >      }
> > > +    if (sscanf(vec[XS_WATCH_TOKEN], "stub:%" PRIxPTR ":%d:%" PRIxPTR,
> > > +               &type, &dom, &ops) == 3) {
> > > +        xenstore_fe_update(vec[XS_WATCH_PATH], (void *)type, dom,
> > (void *)ops);
> > > +    }
> > 
> > Why "stub:"? Where is it coming from?
> > 
> > I think that the xenstore_update function should be moved to a new file:
> > we don't want xen_backend.c to be handling any frontend updates.
> > Maybe you could create a new file, hw/xen/xen_pvdev.c?
> > 
> 
> Stefano,
> 	I'd like to create hw/xen/xen_pvdev.c , include/hw/xen/ xen_pvdev.h and 
> include/hw/xen/xen_frontend.h
> 
> include/hw/xen/ xen_pvdev.h is for 
> struct XenDevOps{} and struct XenDevice {} definition and some other macro.
> 
> include/hw/xen/xen_frontend.h is for 
> xen frontend interface, such as xen_fe_register() .etc
> 
> hw/xen/xen_pvdev.c is for 
> some general parts of xen_backend/xen_frontend.
> It includes xenstore_update function xendevs management, such as xen_qtail_insert_xendev(..),
> xenstore_update(...) .etc.
> 
> do you agree with it?

It seems reasonable.