Re: [PATCH v5 01/10] hw/virtio: Factorize virtio-mmio headers

[Sergio Lopez]

Philippe Mathieu-Daudé <address@hidden> writes:

> On 10/2/19 1:30 PM, Sergio Lopez wrote:
>> Put QOM and main struct definition in a separate header file, so it
>> can be accessed from other components.
>>
>> Signed-off-by: Sergio Lopez <address@hidden>
>> ---
>>   hw/virtio/virtio-mmio.c         | 48 +---------------------
>>   include/hw/virtio/virtio-mmio.h | 73 +++++++++++++++++++++++++++++++++
>>   2 files changed, 74 insertions(+), 47 deletions(-)
>>   create mode 100644 include/hw/virtio/virtio-mmio.h
>>
>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>> index 3d5ca0f667..94d934c44b 100644
>> --- a/hw/virtio/virtio-mmio.c
>> +++ b/hw/virtio/virtio-mmio.c
>> @@ -29,57 +29,11 @@
>>   #include "qemu/host-utils.h"
>>   #include "qemu/module.h"
>>   #include "sysemu/kvm.h"
>> -#include "hw/virtio/virtio-bus.h"
>> +#include "hw/virtio/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 2
>> -#define VIRT_VERSION_LEGACY 1
>> -#define VIRT_VENDOR 0x554D4551 /* 'QEMU' */
>> -
>> -typedef struct VirtIOMMIOQueue {
>> -    uint16_t num;
>> -    bool enabled;
>> -    uint32_t desc[2];
>> -    uint32_t avail[2];
>> -    uint32_t used[2];
>> -} VirtIOMMIOQueue;
>> -
>> -typedef struct {
>> -    /* Generic */
>> -    SysBusDevice parent_obj;
>> -    MemoryRegion iomem;
>> -    qemu_irq irq;
>> -    bool legacy;
>> -    /* 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;
>> -    /* Fields only used for non-legacy (v2) devices */
>> -    uint32_t guest_features[2];
>> -    VirtIOMMIOQueue vqs[VIRTIO_QUEUE_MAX];
>> -} VirtIOMMIOProxy;
>> -
>>   static bool virtio_mmio_ioeventfd_enabled(DeviceState *d)
>>   {
>>       return kvm_eventfds_enabled();
>> diff --git a/include/hw/virtio/virtio-mmio.h 
>> b/include/hw/virtio/virtio-mmio.h
>> new file mode 100644
>> index 0000000000..c8a6ef20de
>> --- /dev/null
>> +++ b/include/hw/virtio/virtio-mmio.h
>> @@ -0,0 +1,73 @@
>> +/*
>> + * 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
>
> I'd rather use HW_VIRTIO_MMIO_H

Looks like there isn't a consensus in this regard:

$ grep "ifndef" *
vhost-backend.h:#ifndef VHOST_BACKEND_H
vhost.h:#ifndef VHOST_H
vhost-scsi-common.h:#ifndef VHOST_SCSI_COMMON_H
vhost-scsi.h:#ifndef VHOST_SCSI_H
vhost-user-blk.h:#ifndef VHOST_USER_BLK_H
vhost-user.h:#ifndef HW_VIRTIO_VHOST_USER_H
vhost-user-scsi.h:#ifndef VHOST_USER_SCSI_H
vhost-vsock.h:#ifndef QEMU_VHOST_VSOCK_H
virtio-access.h:#ifndef QEMU_VIRTIO_ACCESS_H
virtio-balloon.h:#ifndef QEMU_VIRTIO_BALLOON_H
virtio-balloon.h.orig:#ifndef QEMU_VIRTIO_BALLOON_H
virtio-blk.h:#ifndef QEMU_VIRTIO_BLK_H
virtio-bus.h:#ifndef VIRTIO_BUS_H
virtio-crypto.h:#ifndef QEMU_VIRTIO_CRYPTO_H
virtio-gpu-bswap.h:#ifndef HW_VIRTIO_GPU_BSWAP_H
virtio-gpu.h:#ifndef HW_VIRTIO_GPU_H
virtio-gpu-pci.h:#ifndef HW_VIRTIO_GPU_PCI_H
virtio-gpu-pixman.h:#ifndef HW_VIRTIO_GPU_PIXMAN_H
virtio.h:#ifndef QEMU_VIRTIO_H
virtio-input.h:#ifndef QEMU_VIRTIO_INPUT_H
virtio-mmio.h:#ifndef QEMU_VIRTIO_MMIO_H
virtio-net.h:#ifndef QEMU_VIRTIO_NET_H
virtio-pmem.h:#ifndef HW_VIRTIO_PMEM_H
virtio-rng.h:#ifndef QEMU_VIRTIO_RNG_H
virtio-scsi.h:#ifndef QEMU_VIRTIO_SCSI_H
virtio-serial.h:#ifndef QEMU_VIRTIO_SERIAL_H

Do we have an actual policy written somewhere?

> Regardless:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
>
>> +
>> +#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 2
>> +#define VIRT_VERSION_LEGACY 1
>> +#define VIRT_VENDOR 0x554D4551 /* 'QEMU' */
>> +
>> +typedef struct VirtIOMMIOQueue {
>> +    uint16_t num;
>> +    bool enabled;
>> +    uint32_t desc[2];
>> +    uint32_t avail[2];
>> +    uint32_t used[2];
>> +} VirtIOMMIOQueue;
>> +
>> +typedef struct {
>> +    /* Generic */
>> +    SysBusDevice parent_obj;
>> +    MemoryRegion iomem;
>> +    qemu_irq irq;
>> +    bool legacy;
>> +    /* 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;
>> +    /* Fields only used for non-legacy (v2) devices */
>> +    uint32_t guest_features[2];
>> +    VirtIOMMIOQueue vqs[VIRTIO_QUEUE_MAX];
>> +} VirtIOMMIOProxy;
>> +
>> +#endif
>>

signature.asc
Description: PGP signature