Re: [Qemu-devel] [PATCH v3 1/4] hw/virtio: Factorize virtio-mmio headers

[Michael S. Tsirkin]

On Thu, Jul 25, 2019 at 10:46:00AM +0100, Liam Merwick wrote:
> On 02/07/2019 13:11, Sergio Lopez wrote:
> > Put QOM and main struct definition in a separate header file, so it
> > can be accesed from other components.
> 
> typo: accesed -> accessed
> 
> > 
> > This is needed for the microvm machine type implementation.
> > 
> > Signed-off-by: Sergio Lopez <address@hidden>
> 
> One nit below, either way
> 
> Reviewed-by: Liam Merwick <address@hidden>
> 
> > ---
> >   hw/virtio/virtio-mmio.c | 35 +-----------------------
> >   hw/virtio/virtio-mmio.h | 60 +++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 61 insertions(+), 34 deletions(-)
> >   create mode 100644 hw/virtio/virtio-mmio.h
> > 
> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > index 97b7f35496..87c7fe4d8d 100644
> > --- a/hw/virtio/virtio-mmio.c
> > +++ b/hw/virtio/virtio-mmio.c
> > @@ -26,44 +26,11 @@
> >   #include "qemu/host-utils.h"
> >   #include "qemu/module.h"
> >   #include "sysemu/kvm.h"
> > -#include "hw/virtio/virtio-bus.h"
> > +#include "virtio-mmio.h"
> 
> 
> Virtually all the other includes of virtio-xxx.h files in hw/virtio use the
> full path - e.g. "hw/virtio/virtio-mmio.h" - maybe do the same to be
> consistent.

That's for headers under include/.
Local ones are ok with a short name.


> 
> >   #include "qemu/error-report.h"
> >   #include "qemu/log.h"
> >   #include "trace.h"
> > -/* QOM macros */
> > -/* virtio-mmio-bus */
> > -#define TYPE_VIRTIO_MMIO_BUS "virtio-mmio-bus"
> > -#define VIRTIO_MMIO_BUS(obj) \
> > -        OBJECT_CHECK(VirtioBusState, (obj), TYPE_VIRTIO_MMIO_BUS)
> > -#define VIRTIO_MMIO_BUS_GET_CLASS(obj) \
> > -        OBJECT_GET_CLASS(VirtioBusClass, (obj), TYPE_VIRTIO_MMIO_BUS)
> > -#define VIRTIO_MMIO_BUS_CLASS(klass) \
> > -        OBJECT_CLASS_CHECK(VirtioBusClass, (klass), TYPE_VIRTIO_MMIO_BUS)
> > -
> > -/* virtio-mmio */
> > -#define TYPE_VIRTIO_MMIO "virtio-mmio"
> > -#define VIRTIO_MMIO(obj) \
> > -        OBJECT_CHECK(VirtIOMMIOProxy, (obj), TYPE_VIRTIO_MMIO)
> > -
> > -#define VIRT_MAGIC 0x74726976 /* 'virt' */
> > -#define VIRT_VERSION 1
> > -#define VIRT_VENDOR 0x554D4551 /* 'QEMU' */
> > -
> > -typedef struct {
> > -    /* Generic */
> > -    SysBusDevice parent_obj;
> > -    MemoryRegion iomem;
> > -    qemu_irq irq;
> > -    /* Guest accessible state needing migration and reset */
> > -    uint32_t host_features_sel;
> > -    uint32_t guest_features_sel;
> > -    uint32_t guest_page_shift;
> > -    /* virtio-bus */
> > -    VirtioBusState bus;
> > -    bool format_transport_address;
> > -} VirtIOMMIOProxy;
> > -
> >   static bool virtio_mmio_ioeventfd_enabled(DeviceState *d)
> >   {
> >       return kvm_eventfds_enabled();
> > diff --git a/hw/virtio/virtio-mmio.h b/hw/virtio/virtio-mmio.h
> > new file mode 100644
> > index 0000000000..2f3973f8c7
> > --- /dev/null
> > +++ b/hw/virtio/virtio-mmio.h
> > @@ -0,0 +1,60 @@
> > +/*
> > + * Virtio MMIO bindings
> > + *
> > + * Copyright (c) 2011 Linaro Limited
> > + *
> > + * Author:
> > + *  Peter Maydell <address@hidden>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef QEMU_VIRTIO_MMIO_H
> > +#define QEMU_VIRTIO_MMIO_H
> > +
> > +#include "hw/virtio/virtio-bus.h"
> > +
> > +/* QOM macros */
> > +/* virtio-mmio-bus */
> > +#define TYPE_VIRTIO_MMIO_BUS "virtio-mmio-bus"
> > +#define VIRTIO_MMIO_BUS(obj) \
> > +        OBJECT_CHECK(VirtioBusState, (obj), TYPE_VIRTIO_MMIO_BUS)
> > +#define VIRTIO_MMIO_BUS_GET_CLASS(obj) \
> > +        OBJECT_GET_CLASS(VirtioBusClass, (obj), TYPE_VIRTIO_MMIO_BUS)
> > +#define VIRTIO_MMIO_BUS_CLASS(klass) \
> > +        OBJECT_CLASS_CHECK(VirtioBusClass, (klass), TYPE_VIRTIO_MMIO_BUS)
> > +
> > +/* virtio-mmio */
> > +#define TYPE_VIRTIO_MMIO "virtio-mmio"
> > +#define VIRTIO_MMIO(obj) \
> > +        OBJECT_CHECK(VirtIOMMIOProxy, (obj), TYPE_VIRTIO_MMIO)
> > +
> > +#define VIRT_MAGIC 0x74726976 /* 'virt' */
> > +#define VIRT_VERSION 1
> > +#define VIRT_VENDOR 0x554D4551 /* 'QEMU' */
> > +
> > +typedef struct {
> > +    /* Generic */
> > +    SysBusDevice parent_obj;
> > +    MemoryRegion iomem;
> > +    qemu_irq irq;
> > +    /* Guest accessible state needing migration and reset */
> > +    uint32_t host_features_sel;
> > +    uint32_t guest_features_sel;
> > +    uint32_t guest_page_shift;
> > +    /* virtio-bus */
> > +    VirtioBusState bus;
> > +    bool format_transport_address;
> > +} VirtIOMMIOProxy;


I'm repeating myself, but still: if you insist on virtio mmio, please
implement virtio 1 and use that with microvm. We can't keep carrying
legacy interface into every new machine type.

> > +
> > +#endif
> >