| Message ID | 20190702121106.28374-2-slp@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the microvm machine type | expand |
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 <slp@redhat.com> One nit below, either way Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > --- > 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. > #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 <peter.maydell@linaro.org> > + * > + * 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; > + > +#endif >
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 <slp@redhat.com> > > One nit below, either way > > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > > > --- > > 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 <peter.maydell@linaro.org> > > + * > > + * 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 > >
On Thu, 25 Jul 2019 at 10:58, Michael S. Tsirkin <mst@redhat.com> wrote: > > 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 <slp@redhat.com> > > > > One nit below, either way > > > > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > > > > > --- > > > 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. Yes, but we should put this one into include/ as that fits with our usual arrangement of where we put the headers for devices. > 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. Agreed (but we've had this discussion on another thread, as you say). thanks -- PMM
On 25/07/19 11:58, Michael S. Tsirkin wrote: > 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. I'd give Sergio the benefit of doubt, since so far he's addressed many other review comments---just, one at a time. :) Paolo
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" #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 <peter.maydell@linaro.org> + * + * 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; + +#endif
Put QOM and main struct definition in a separate header file, so it can be accesed from other components. This is needed for the microvm machine type implementation. Signed-off-by: Sergio Lopez <slp@redhat.com> --- 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