[Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

[Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

[Yoni Bettan]

    - this is a simple example of how to write a pci device that supports
      portio, mmio, irq and dma

Signed-off-by: Yoni Bettan <ybettan@redhat.com>
---
 hw/pci/Makefile.objs |   1 +
 hw/pci/pci_example.c | 309 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 310 insertions(+)
 create mode 100644 hw/pci/pci_example.c

diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
index 9f905e6344..e684b72f90 100644
--- a/hw/pci/Makefile.objs
+++ b/hw/pci/Makefile.objs
@@ -4,6 +4,7 @@ common-obj-$(CONFIG_PCI) += shpc.o
 common-obj-$(CONFIG_PCI) += slotid_cap.o
 common-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
 common-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
+common-obj-$(CONFIG_PCI) += pci_example.o
 
 common-obj-$(call lnot,$(CONFIG_PCI)) += pci-stub.o
 common-obj-$(CONFIG_ALL) += pci-stub.o
diff --git a/hw/pci/pci_example.c b/hw/pci/pci_example.c
new file mode 100644
index 0000000000..326c9b7fa1
--- /dev/null
+++ b/hw/pci/pci_example.c
@@ -0,0 +1,309 @@
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+
+#define TYPE_PCI_EXAMPLE "pci-example"
+
+#define PCI_EXAMPLE(obj)  \
+    OBJECT_CHECK(PCIExampleDevState, (obj), TYPE_PCI_EXAMPLE)
+
+
+#define ERROR -1
+#define BLOCK_SIZE 64
+#define EXAMPLE_MMIO_SIZE BLOCK_SIZE
+#define EXAMPLE_PIO_SIZE BLOCK_SIZE
+#define DMA_BUFF_SIZE 4096
+
+/*---------------------------------------------------------------------------*/
+/*                                 PCI Struct                                */
+/*---------------------------------------------------------------------------*/
+
+typedef struct PCIExampleDevState {
+    /*< private >*/
+    PCIDevice parent_obj;
+    /*< public >*/
+
+    /* memory region */
+    MemoryRegion portio;
+    MemoryRegion mmio;
+    MemoryRegion irqio;
+    MemoryRegion dmaio;
+
+    /* data registers */
+    uint64_t memData, ioData, dmaPhisicalBase;
+
+    qemu_irq irq;
+    /* for the driver to determine if this device caused the interrupt */
+    uint64_t threwIrq;
+
+} PCIExampleDevState;
+
+
+/*---------------------------------------------------------------------------*/
+/*                         Read/Write functions                              */
+/*---------------------------------------------------------------------------*/
+
+/* do nothing because the mmio read is done from DMA buffer */
+static uint64_t pci_example_mmio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return ERROR;
+}
+
+static void
+pci_example_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
+    PCIDevice *pciDev = (PCIDevice *)opaque;
+
+    if (size != 1) {
+        return;
+    }
+
+    /* compute the result */
+    pms->memData = val * 2;
+
+    /* write the result directly to phisical memory */
+    cpu_physical_memory_write(pms->dmaPhisicalBase, &pms->memData,
+            DMA_BUFF_SIZE);
+
+    /* raise an IRQ to notify DMA has finished  */
+    pms->threwIrq = 1;
+    pci_irq_assert(pciDev);
+}
+
+static uint64_t pci_example_pio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
+
+    if (size != 1) {
+        return ERROR;
+    }
+
+    return pms->ioData;
+}
+
+static void
+pci_example_pio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
+    PCIDevice *pciDev = (PCIDevice *)opaque;
+
+    if (size != 1) {
+        return;
+    }
+
+    pms->ioData = val * 2;
+    pms->threwIrq = 1;
+    pci_irq_assert(pciDev);
+}
+
+static uint64_t pci_example_irqio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
+
+    if (size != 1) {
+        return ERROR;
+    }
+
+    return pms->threwIrq;
+}
+
+static void
+pci_example_irqio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
+    PCIDevice *pciDev = (PCIDevice *)opaque;
+
+    if (size != 1) {
+        return;
+    }
+
+    /* give the ability to assert IRQ , we will use it only to deassert IRQ */
+    if (val) {
+        pci_irq_assert(pciDev);
+        pms->threwIrq = 1;
+    } else {
+        pms->threwIrq = 0;
+        pci_irq_deassert(pciDev);
+    }
+}
+
+/* do nothing because physical DMA buffer addres is onlyt set and don't need to
+ * be red */
+static uint64_t
+pci_example_dma_base_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return ERROR;
+}
+
+static void
+pci_example_dma_base_write(void *opaque, hwaddr addr, uint64_t val,
+        unsigned size)
+{
+    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
+
+    if (size != 4) {
+        return;
+    }
+
+    /* notify the device about the physical address of the DMA buffer that the
+     * driver has allocated */
+    switch (addr) {
+        /* lower bytes */
+        case(0):
+            pms->dmaPhisicalBase &= 0xffffffff00000000;
+            break;
+
+        /* upper bytes */
+        case(4):
+            val <<= 32;
+            pms->dmaPhisicalBase &= 0x00000000ffffffff;
+            break;
+    }
+
+    pms->dmaPhisicalBase |= val;
+}
+
+/*---------------------------------------------------------------------------*/
+/*                             PCI region ops                                */
+/*---------------------------------------------------------------------------*/
+
+/* callback called when memory region representing the MMIO space is accessed */
+static const MemoryRegionOps pci_example_mmio_ops = {
+    .read = pci_example_mmio_read,
+    .write = pci_example_mmio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+/* callback called when memory region representing the PIO space is accessed */
+static const MemoryRegionOps pci_example_pio_ops = {
+    .read = pci_example_pio_read,
+    .write = pci_example_pio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+/* callback called when memory region representing the IRQ space is accessed */
+static const MemoryRegionOps pci_example_irqio_ops = {
+    .read = pci_example_irqio_read,
+    .write = pci_example_irqio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+/* callback called when memory region representing the DMA space is accessed */
+static const MemoryRegionOps pci_example_dma_ops = {
+    .read = pci_example_dma_base_read,
+    .write = pci_example_dma_base_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+/*---------------------------------------------------------------------------*/
+/*                             PCI functions                                 */
+/*---------------------------------------------------------------------------*/
+
+/* this is called when lunching the vm with "-device <device name>" */
+static void pci_example_realize(PCIDevice *pciDev, Error **errp)
+{
+   PCIExampleDevState *d = PCI_EXAMPLE(pciDev);
+   uint8_t *pciCond = pciDev->config;
+
+   d->threwIrq = 0;
+
+   /* initiallise the memory region of the CPU to the device */
+   memory_region_init_io(&d->mmio, OBJECT(d), &pci_example_mmio_ops, d,
+           "pci-example-mmio", EXAMPLE_MMIO_SIZE);
+
+   memory_region_init_io(&d->portio, OBJECT(d), &pci_example_pio_ops, d,
+           "pci-example-portio", EXAMPLE_PIO_SIZE);
+
+   memory_region_init_io(&d->irqio, OBJECT(d), &pci_example_irqio_ops, d,
+           "pci-example-irqio", EXAMPLE_PIO_SIZE);
+
+   memory_region_init_io(&d->dmaio, OBJECT(d), &pci_example_dma_ops, d,
+           "pci-example-dma-base", EXAMPLE_MMIO_SIZE);
+
+   /* alocate BARs */
+   pci_register_bar(pciDev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
+   pci_register_bar(pciDev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
+   pci_register_bar(pciDev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->irqio);
+   pci_register_bar(pciDev, 3, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->dmaio);
+
+   /* give interrupt support.
+    * a pci device has 4 pin for interrupt, here we use pin A */
+   pci_config_set_interrupt_pin(pciCond, 1);
+}
+
+
+/* the destructor of pci_example_realize() */
+static void pci_example_uninit(PCIDevice *dev)
+{
+    /* unregister BARs and other stuff */
+}
+
+
+/* class constructor */
+static void pci_example_class_init(ObjectClass *klass, void *data)
+{
+   /* sort of dynamic cast */
+   DeviceClass *dc = DEVICE_CLASS(klass);
+   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+   k->realize = pci_example_realize;
+   k->exit = pci_example_uninit;
+
+   /* some regular IDs in HEXA */
+   k->vendor_id = PCI_VENDOR_ID_REDHAT;
+   k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
+   k->class_id = PCI_CLASS_OTHERS;
+
+   /* set the device bitmap category */
+   set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+
+   k->revision = 0x00;
+   dc->desc = "PCI Example Device";
+}
+
+/*---------------------------------------------------------------------------*/
+/*                            QEMU overhead                                  */
+/*---------------------------------------------------------------------------*/
+
+
+/* Contains all the informations of the device we are creating.
+ * class_init will be called when we are defining our device. */
+static const TypeInfo pci_example_info = {
+    .name           = TYPE_PCI_EXAMPLE,
+    .parent         = TYPE_PCI_DEVICE,
+    .instance_size  = sizeof(PCIExampleDevState),
+    .class_init     = pci_example_class_init,
+    .interfaces     = (InterfaceInfo[]) {
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { },
+    },
+};
+
+
+/* function called before the qemu main it will define our device */
+static void pci_example_register_types(void)
+{
+    type_register_static(&pci_example_info);
+}
+
+/* make qemu register our device at qemu-booting */
+type_init(pci_example_register_types)
+
+
+
-- 
2.17.1

Re: [Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

[Yoni Bettan]

On 8/28/18 10:24 AM, Yoni Bettan wrote:
>      - this is a simple example of how to write a pci device that supports
>        portio, mmio, irq and dma

As the commit message miss a little bit of information I will add it 
here and update
the commit message in the next Version.

The main goal is to add 2 example devices to be used as a template or a 
guideline for
contributors when they wish to create a new device.
Those devices may be compiled by default with qemu, to ensure they won't 
break, but not have to
be linked, so performance won't decrease.
Another reason for such devices is to document "the right way" to write 
a new device in qemu.

  * PCI device:
      o its driver is located at
        https://github.com/ybettan/QemuDeviceDrivers.git
  * virtIO device:
      o its driver isn't written yet but will be added to the same
        Github repository written above.
      o when this device will be done the next step is to insert it into
        the virtio specification.


Both devices can be found at https://github.com/ybettan/qemu.git under 
'pci' and 'virtio' branches.

In addition I am writing a blog to give a logical overview of the virtio 
protocol and a step-by-step
guide to write a new virtio device.
This blog can be found at https://howtovms.wordpress.com. 
<https://howtovms.wordpress.com>
>
> Signed-off-by: Yoni Bettan <ybettan@redhat.com>
> ---
>   hw/pci/Makefile.objs |   1 +
>   hw/pci/pci_example.c | 309 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 310 insertions(+)
>   create mode 100644 hw/pci/pci_example.c
>
> diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
> index 9f905e6344..e684b72f90 100644
> --- a/hw/pci/Makefile.objs
> +++ b/hw/pci/Makefile.objs
> @@ -4,6 +4,7 @@ common-obj-$(CONFIG_PCI) += shpc.o
>   common-obj-$(CONFIG_PCI) += slotid_cap.o
>   common-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
>   common-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
> +common-obj-$(CONFIG_PCI) += pci_example.o
>   
>   common-obj-$(call lnot,$(CONFIG_PCI)) += pci-stub.o
>   common-obj-$(CONFIG_ALL) += pci-stub.o
> diff --git a/hw/pci/pci_example.c b/hw/pci/pci_example.c
> new file mode 100644
> index 0000000000..326c9b7fa1
> --- /dev/null
> +++ b/hw/pci/pci_example.c
> @@ -0,0 +1,309 @@
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +
> +#define TYPE_PCI_EXAMPLE "pci-example"
> +
> +#define PCI_EXAMPLE(obj)  \
> +    OBJECT_CHECK(PCIExampleDevState, (obj), TYPE_PCI_EXAMPLE)
> +
> +
> +#define ERROR -1
> +#define BLOCK_SIZE 64
> +#define EXAMPLE_MMIO_SIZE BLOCK_SIZE
> +#define EXAMPLE_PIO_SIZE BLOCK_SIZE
> +#define DMA_BUFF_SIZE 4096
> +
> +/*---------------------------------------------------------------------------*/
> +/*                                 PCI Struct                                */
> +/*---------------------------------------------------------------------------*/
> +
> +typedef struct PCIExampleDevState {
> +    /*< private >*/
> +    PCIDevice parent_obj;
> +    /*< public >*/
> +
> +    /* memory region */
> +    MemoryRegion portio;
> +    MemoryRegion mmio;
> +    MemoryRegion irqio;
> +    MemoryRegion dmaio;
> +
> +    /* data registers */
> +    uint64_t memData, ioData, dmaPhisicalBase;
> +
> +    qemu_irq irq;
> +    /* for the driver to determine if this device caused the interrupt */
> +    uint64_t threwIrq;
> +
> +} PCIExampleDevState;
> +
> +
> +/*---------------------------------------------------------------------------*/
> +/*                         Read/Write functions                              */
> +/*---------------------------------------------------------------------------*/
> +
> +/* do nothing because the mmio read is done from DMA buffer */
> +static uint64_t pci_example_mmio_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return ERROR;
> +}
> +
> +static void
> +pci_example_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +    PCIDevice *pciDev = (PCIDevice *)opaque;
> +
> +    if (size != 1) {
> +        return;
> +    }
> +
> +    /* compute the result */
> +    pms->memData = val * 2;
> +
> +    /* write the result directly to phisical memory */
> +    cpu_physical_memory_write(pms->dmaPhisicalBase, &pms->memData,
> +            DMA_BUFF_SIZE);
> +
> +    /* raise an IRQ to notify DMA has finished  */
> +    pms->threwIrq = 1;
> +    pci_irq_assert(pciDev);
> +}
> +
> +static uint64_t pci_example_pio_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +
> +    if (size != 1) {
> +        return ERROR;
> +    }
> +
> +    return pms->ioData;
> +}
> +
> +static void
> +pci_example_pio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +    PCIDevice *pciDev = (PCIDevice *)opaque;
> +
> +    if (size != 1) {
> +        return;
> +    }
> +
> +    pms->ioData = val * 2;
> +    pms->threwIrq = 1;
> +    pci_irq_assert(pciDev);
> +}
> +
> +static uint64_t pci_example_irqio_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +
> +    if (size != 1) {
> +        return ERROR;
> +    }
> +
> +    return pms->threwIrq;
> +}
> +
> +static void
> +pci_example_irqio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +    PCIDevice *pciDev = (PCIDevice *)opaque;
> +
> +    if (size != 1) {
> +        return;
> +    }
> +
> +    /* give the ability to assert IRQ , we will use it only to deassert IRQ */
> +    if (val) {
> +        pci_irq_assert(pciDev);
> +        pms->threwIrq = 1;
> +    } else {
> +        pms->threwIrq = 0;
> +        pci_irq_deassert(pciDev);
> +    }
> +}
> +
> +/* do nothing because physical DMA buffer addres is onlyt set and don't need to
> + * be red */
> +static uint64_t
> +pci_example_dma_base_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return ERROR;
> +}
> +
> +static void
> +pci_example_dma_base_write(void *opaque, hwaddr addr, uint64_t val,
> +        unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +
> +    if (size != 4) {
> +        return;
> +    }
> +
> +    /* notify the device about the physical address of the DMA buffer that the
> +     * driver has allocated */
> +    switch (addr) {
> +        /* lower bytes */
> +        case(0):
> +            pms->dmaPhisicalBase &= 0xffffffff00000000;
> +            break;
> +
> +        /* upper bytes */
> +        case(4):
> +            val <<= 32;
> +            pms->dmaPhisicalBase &= 0x00000000ffffffff;
> +            break;
> +    }
> +
> +    pms->dmaPhisicalBase |= val;
> +}
> +
> +/*---------------------------------------------------------------------------*/
> +/*                             PCI region ops                                */
> +/*---------------------------------------------------------------------------*/
> +
> +/* callback called when memory region representing the MMIO space is accessed */
> +static const MemoryRegionOps pci_example_mmio_ops = {
> +    .read = pci_example_mmio_read,
> +    .write = pci_example_mmio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +/* callback called when memory region representing the PIO space is accessed */
> +static const MemoryRegionOps pci_example_pio_ops = {
> +    .read = pci_example_pio_read,
> +    .write = pci_example_pio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +/* callback called when memory region representing the IRQ space is accessed */
> +static const MemoryRegionOps pci_example_irqio_ops = {
> +    .read = pci_example_irqio_read,
> +    .write = pci_example_irqio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +/* callback called when memory region representing the DMA space is accessed */
> +static const MemoryRegionOps pci_example_dma_ops = {
> +    .read = pci_example_dma_base_read,
> +    .write = pci_example_dma_base_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +/*---------------------------------------------------------------------------*/
> +/*                             PCI functions                                 */
> +/*---------------------------------------------------------------------------*/
> +
> +/* this is called when lunching the vm with "-device <device name>" */
> +static void pci_example_realize(PCIDevice *pciDev, Error **errp)
> +{
> +   PCIExampleDevState *d = PCI_EXAMPLE(pciDev);
> +   uint8_t *pciCond = pciDev->config;
> +
> +   d->threwIrq = 0;
> +
> +   /* initiallise the memory region of the CPU to the device */
> +   memory_region_init_io(&d->mmio, OBJECT(d), &pci_example_mmio_ops, d,
> +           "pci-example-mmio", EXAMPLE_MMIO_SIZE);
> +
> +   memory_region_init_io(&d->portio, OBJECT(d), &pci_example_pio_ops, d,
> +           "pci-example-portio", EXAMPLE_PIO_SIZE);
> +
> +   memory_region_init_io(&d->irqio, OBJECT(d), &pci_example_irqio_ops, d,
> +           "pci-example-irqio", EXAMPLE_PIO_SIZE);
> +
> +   memory_region_init_io(&d->dmaio, OBJECT(d), &pci_example_dma_ops, d,
> +           "pci-example-dma-base", EXAMPLE_MMIO_SIZE);
> +
> +   /* alocate BARs */
> +   pci_register_bar(pciDev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> +   pci_register_bar(pciDev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> +   pci_register_bar(pciDev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->irqio);
> +   pci_register_bar(pciDev, 3, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->dmaio);
> +
> +   /* give interrupt support.
> +    * a pci device has 4 pin for interrupt, here we use pin A */
> +   pci_config_set_interrupt_pin(pciCond, 1);
> +}
> +
> +
> +/* the destructor of pci_example_realize() */
> +static void pci_example_uninit(PCIDevice *dev)
> +{
> +    /* unregister BARs and other stuff */
> +}
> +
> +
> +/* class constructor */
> +static void pci_example_class_init(ObjectClass *klass, void *data)
> +{
> +   /* sort of dynamic cast */
> +   DeviceClass *dc = DEVICE_CLASS(klass);
> +   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +   k->realize = pci_example_realize;
> +   k->exit = pci_example_uninit;
> +
> +   /* some regular IDs in HEXA */
> +   k->vendor_id = PCI_VENDOR_ID_REDHAT;
> +   k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
> +   k->class_id = PCI_CLASS_OTHERS;
> +
> +   /* set the device bitmap category */
> +   set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> +   k->revision = 0x00;
> +   dc->desc = "PCI Example Device";
> +}
> +
> +/*---------------------------------------------------------------------------*/
> +/*                            QEMU overhead                                  */
> +/*---------------------------------------------------------------------------*/
> +
> +
> +/* Contains all the informations of the device we are creating.
> + * class_init will be called when we are defining our device. */
> +static const TypeInfo pci_example_info = {
> +    .name           = TYPE_PCI_EXAMPLE,
> +    .parent         = TYPE_PCI_DEVICE,
> +    .instance_size  = sizeof(PCIExampleDevState),
> +    .class_init     = pci_example_class_init,
> +    .interfaces     = (InterfaceInfo[]) {
> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +        { },
> +    },
> +};
> +
> +
> +/* function called before the qemu main it will define our device */
> +static void pci_example_register_types(void)
> +{
> +    type_register_static(&pci_example_info);
> +}
> +
> +/* make qemu register our device at qemu-booting */
> +type_init(pci_example_register_types)
> +
> +
> +

Re: [Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

[Cornelia Huck]

On Tue, 28 Aug 2018 10:24:26 +0300
Yoni Bettan <ybettan@redhat.com> wrote:

>     - this is a simple example of how to write a pci device that supports
>       portio, mmio, irq and dma

Do you also plan to add example code for MSI(-X)?

[Not just asking because s390 only supports MSI-X. Honestly :)]

> 
> Signed-off-by: Yoni Bettan <ybettan@redhat.com>
> ---
>  hw/pci/Makefile.objs |   1 +
>  hw/pci/pci_example.c | 309 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 310 insertions(+)
>  create mode 100644 hw/pci/pci_example.c
> 

(...)

> diff --git a/hw/pci/pci_example.c b/hw/pci/pci_example.c
> new file mode 100644
> index 0000000000..326c9b7fa1
> --- /dev/null
> +++ b/hw/pci/pci_example.c
> @@ -0,0 +1,309 @@
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +
> +#define TYPE_PCI_EXAMPLE "pci-example"
> +
> +#define PCI_EXAMPLE(obj)  \
> +    OBJECT_CHECK(PCIExampleDevState, (obj), TYPE_PCI_EXAMPLE)
> +
> +
> +#define ERROR -1

It's probably not a good idea to obfuscate the return code. Either
return -1, or a proper error code.

> +#define BLOCK_SIZE 64
> +#define EXAMPLE_MMIO_SIZE BLOCK_SIZE
> +#define EXAMPLE_PIO_SIZE BLOCK_SIZE
> +#define DMA_BUFF_SIZE 4096
> +
> +/*---------------------------------------------------------------------------*/
> +/*                                 PCI Struct                                */
> +/*---------------------------------------------------------------------------*/
> +
> +typedef struct PCIExampleDevState {
> +    /*< private >*/
> +    PCIDevice parent_obj;
> +    /*< public >*/
> +
> +    /* memory region */
> +    MemoryRegion portio;
> +    MemoryRegion mmio;
> +    MemoryRegion irqio;
> +    MemoryRegion dmaio;
> +
> +    /* data registers */
> +    uint64_t memData, ioData, dmaPhisicalBase;

I think we want this to be mem_data, io_data, and dma_physical_base
instead.

> +
> +    qemu_irq irq;
> +    /* for the driver to determine if this device caused the interrupt */
> +    uint64_t threwIrq;

Same here (threw_irq).

> +
> +} PCIExampleDevState;
> +
> +
> +/*---------------------------------------------------------------------------*/
> +/*                         Read/Write functions                              */
> +/*---------------------------------------------------------------------------*/
> +
> +/* do nothing because the mmio read is done from DMA buffer */
> +static uint64_t pci_example_mmio_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return ERROR;
> +}
> +
> +static void
> +pci_example_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)

I think our preferred style is to keep return value and function name
on the same line and add line breaks in the parameter list, if needed.
(Some more occurrences further down.)

> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;

peds? ped_state? (I'm not sure pms <-> PCIExampleDevState is obvious;
I'm not sure either whether my suggestions are better :)

> +    PCIDevice *pciDev = (PCIDevice *)opaque;

pci_dev is better, I think. Also, I'd probably refer to pms->parent_obj
(maybe call that one pci_device instead?) instead of casting.

> +
> +    if (size != 1) {
> +        return;
> +    }
> +
> +    /* compute the result */
> +    pms->memData = val * 2;
> +
> +    /* write the result directly to phisical memory */
> +    cpu_physical_memory_write(pms->dmaPhisicalBase, &pms->memData,
> +            DMA_BUFF_SIZE);

Indentation seems off.

> +
> +    /* raise an IRQ to notify DMA has finished  */
> +    pms->threwIrq = 1;
> +    pci_irq_assert(pciDev);
> +}

(...)

> +/* do nothing because physical DMA buffer addres is onlyt set and don't need to
> + * be red */

s/addres/address/
s/onlyt/only/
s/don't/doesn't/
s/red/read/

Also, we prefer the

/*
 * comment style
 */

if it can't fit on one line.

(...)

> +/*---------------------------------------------------------------------------*/
> +/*                             PCI functions                                 */
> +/*---------------------------------------------------------------------------*/
> +
> +/* this is called when lunching the vm with "-device <device name>" */

s/lunching/launching/

Although that's simply the normal realization process, which could also
be triggered by hotplug.

> +static void pci_example_realize(PCIDevice *pciDev, Error **errp)
> +{
> +   PCIExampleDevState *d = PCI_EXAMPLE(pciDev);

Here you use "d" as a variable name, which arguably might be better
than any of the suggestions above :)

> +   uint8_t *pciCond = pciDev->config;
> +
> +   d->threwIrq = 0;
> +
> +   /* initiallise the memory region of the CPU to the device */

s/initiallise/initialize/

> +   memory_region_init_io(&d->mmio, OBJECT(d), &pci_example_mmio_ops, d,
> +           "pci-example-mmio", EXAMPLE_MMIO_SIZE);
> +
> +   memory_region_init_io(&d->portio, OBJECT(d), &pci_example_pio_ops, d,
> +           "pci-example-portio", EXAMPLE_PIO_SIZE);
> +
> +   memory_region_init_io(&d->irqio, OBJECT(d), &pci_example_irqio_ops, d,
> +           "pci-example-irqio", EXAMPLE_PIO_SIZE);
> +
> +   memory_region_init_io(&d->dmaio, OBJECT(d), &pci_example_dma_ops, d,
> +           "pci-example-dma-base", EXAMPLE_MMIO_SIZE);
> +
> +   /* alocate BARs */

s/alocate/allocate/

> +   pci_register_bar(pciDev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> +   pci_register_bar(pciDev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> +   pci_register_bar(pciDev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->irqio);
> +   pci_register_bar(pciDev, 3, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->dmaio);
> +
> +   /* give interrupt support.
> +    * a pci device has 4 pin for interrupt, here we use pin A */
> +   pci_config_set_interrupt_pin(pciCond, 1);
> +}
> +
> +
> +/* the destructor of pci_example_realize() */
> +static void pci_example_uninit(PCIDevice *dev)
> +{
> +    /* unregister BARs and other stuff */

Shouldn't the function actually do it, then? :)

> +}
> +
> +
> +/* class constructor */
> +static void pci_example_class_init(ObjectClass *klass, void *data)
> +{
> +   /* sort of dynamic cast */
> +   DeviceClass *dc = DEVICE_CLASS(klass);
> +   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +   k->realize = pci_example_realize;
> +   k->exit = pci_example_uninit;
> +
> +   /* some regular IDs in HEXA */
> +   k->vendor_id = PCI_VENDOR_ID_REDHAT;
> +   k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
> +   k->class_id = PCI_CLASS_OTHERS;
> +
> +   /* set the device bitmap category */
> +   set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> +   k->revision = 0x00;
> +   dc->desc = "PCI Example Device";
> +}
> +
> +/*---------------------------------------------------------------------------*/
> +/*                            QEMU overhead                                  */

Lol :)

> +/*---------------------------------------------------------------------------*/
> +
> +
> +/* Contains all the informations of the device we are creating.
> + * class_init will be called when we are defining our device. */
> +static const TypeInfo pci_example_info = {
> +    .name           = TYPE_PCI_EXAMPLE,
> +    .parent         = TYPE_PCI_DEVICE,
> +    .instance_size  = sizeof(PCIExampleDevState),
> +    .class_init     = pci_example_class_init,
> +    .interfaces     = (InterfaceInfo[]) {
> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +        { },
> +    },
> +};
> +
> +
> +/* function called before the qemu main it will define our device */

"Define our device type; this is done during startup of QEMU"

?

> +static void pci_example_register_types(void)
> +{
> +    type_register_static(&pci_example_info);
> +}
> +
> +/* make qemu register our device at qemu-booting */

I don't think you need this comment here as well (the type_init and the
function kind of form one logical unit.)

> +type_init(pci_example_register_types)
> +
> +
> +

I think the basic structure looks sound; I've focused on style
questions as this is supposed to be an example for others to copy from.
I'll leave the pci details to people more versed in pci :)

Re: [Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

[Eduardo Habkost]

Hi,

Thanks for the patch.

On Tue, Aug 28, 2018 at 10:24:26AM +0300, Yoni Bettan wrote:
>     - this is a simple example of how to write a pci device that supports
>       portio, mmio, irq and dma

Can we have some documentation on what are the goals of the
example device, and what exactly it does?


> 
> Signed-off-by: Yoni Bettan <ybettan@redhat.com>
> ---
>  hw/pci/Makefile.objs |   1 +
>  hw/pci/pci_example.c | 309 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 310 insertions(+)
>  create mode 100644 hw/pci/pci_example.c
> 
> diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
> index 9f905e6344..e684b72f90 100644
> --- a/hw/pci/Makefile.objs
> +++ b/hw/pci/Makefile.objs
> @@ -4,6 +4,7 @@ common-obj-$(CONFIG_PCI) += shpc.o
>  common-obj-$(CONFIG_PCI) += slotid_cap.o
>  common-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
>  common-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
> +common-obj-$(CONFIG_PCI) += pci_example.o

I am wondering how we can guarantee nobody will break the example
code while not including it by default on production builds.
Maybe disabling the device by default, adding a ./configure
--enable-examples option, and adding it to one of the test cases
on .travis.yml?

>  
>  common-obj-$(call lnot,$(CONFIG_PCI)) += pci-stub.o
>  common-obj-$(CONFIG_ALL) += pci-stub.o
> diff --git a/hw/pci/pci_example.c b/hw/pci/pci_example.c
> new file mode 100644
> index 0000000000..326c9b7fa1
> --- /dev/null
> +++ b/hw/pci/pci_example.c
> @@ -0,0 +1,309 @@
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +

Being an example device, it would be nice to explain what these
two macros are used for.


> +#define TYPE_PCI_EXAMPLE "pci-example"
> +
> +#define PCI_EXAMPLE(obj)  \
> +    OBJECT_CHECK(PCIExampleDevState, (obj), TYPE_PCI_EXAMPLE)
> +
> +

One line descriptions of the meaning of the macros below would be
nice to have.

> +#define ERROR -1

I'm not sure we need this macro.  Additional comments below where
the macro is actually used.


> +#define BLOCK_SIZE 64
> +#define EXAMPLE_MMIO_SIZE BLOCK_SIZE
> +#define EXAMPLE_PIO_SIZE BLOCK_SIZE

I don't see BLOCK_SIZE being used anywhere in the code.  Wouldn't
it be more readable if you just did:

  #define EXAMPLE_MMIO_SIZE 64
  #define EXAMPLE_PIO_SIZE  64

> +#define DMA_BUFF_SIZE 4096

I suggest DMA_BUF_SIZE, as "buf" is a more usual abbreviation for
"buffer".

> +
> +/*---------------------------------------------------------------------------*/
> +/*                                 PCI Struct                                */
> +/*---------------------------------------------------------------------------*/
> +
> +typedef struct PCIExampleDevState {
> +    /*< private >*/
> +    PCIDevice parent_obj;

One line description of what parent_obj is, or a pointer to QOM
documentation explaining it would be nice.

> +    /*< public >*/
> +
> +    /* memory region */

This comment seems redundant: the words "memory region" are
already spelled 4 times below.

> +    MemoryRegion portio;
> +    MemoryRegion mmio;
> +    MemoryRegion irqio;
> +    MemoryRegion dmaio;
> +
> +    /* data registers */
> +    uint64_t memData, ioData, dmaPhisicalBase;

Can we have a one-line description of each of the registers?

> +
> +    qemu_irq irq;
> +    /* for the driver to determine if this device caused the interrupt */
> +    uint64_t threwIrq;
> +
> +} PCIExampleDevState;
> +
> +
> +/*---------------------------------------------------------------------------*/
> +/*                         Read/Write functions                              */
> +/*---------------------------------------------------------------------------*/
> +
> +/* do nothing because the mmio read is done from DMA buffer */
> +static uint64_t pci_example_mmio_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return ERROR;

You can't return a negative value from a uint64_t function.  What
exactly you are trying to communicate to the caller by returning
0xffffffffffffffff here?

Same problem below[5].

> +}
> +
> +static void
> +pci_example_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;

You can write this as:
    PCIExampleDevState *pms = opaque;

Same below[1].


> +    PCIDevice *pciDev = (PCIDevice *)opaque;

I suggest using PCI_DEVICE(pms) here and below[2].


> +
> +    if (size != 1) {
> +        return;
> +    }

Why you want to support only 1-byte writes?

> +
> +    /* compute the result */
> +    pms->memData = val * 2;
> +
> +    /* write the result directly to phisical memory */
> +    cpu_physical_memory_write(pms->dmaPhisicalBase, &pms->memData,
> +            DMA_BUFF_SIZE);
> +
> +    /* raise an IRQ to notify DMA has finished  */
> +    pms->threwIrq = 1;
> +    pci_irq_assert(pciDev);
> +}
> +
> +static uint64_t pci_example_pio_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;

[1]

> +
> +    if (size != 1) {
> +        return ERROR;

Same problem as pci_example_mmio_read() above.

> +    }

Same as above: why you want to support only 1-byte reads?  Same
below[3].

> +
> +    return pms->ioData;
> +}
> +
> +static void
> +pci_example_pio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;

[1]

> +    PCIDevice *pciDev = (PCIDevice *)opaque;

[2]

> +
> +    if (size != 1) {

[3]

> +        return;
> +    }
> +
> +    pms->ioData = val * 2;
> +    pms->threwIrq = 1;
> +    pci_irq_assert(pciDev);
> +}
> +
> +static uint64_t pci_example_irqio_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +
> +    if (size != 1) {

[3]

> +        return ERROR;

[5]

> +    }
> +
> +    return pms->threwIrq;
> +}
> +
> +static void
> +pci_example_irqio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +    PCIDevice *pciDev = (PCIDevice *)opaque;
> +
> +    if (size != 1) {

[3]

> +        return;
> +    }
> +
> +    /* give the ability to assert IRQ , we will use it only to deassert IRQ */
> +    if (val) {
> +        pci_irq_assert(pciDev);
> +        pms->threwIrq = 1;
> +    } else {
> +        pms->threwIrq = 0;
> +        pci_irq_deassert(pciDev);
> +    }
> +}
> +
> +/* do nothing because physical DMA buffer addres is onlyt set and don't need to
> + * be red */
> +static uint64_t
> +pci_example_dma_base_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return ERROR;

[5]

> +}
> +
> +static void
> +pci_example_dma_base_write(void *opaque, hwaddr addr, uint64_t val,
> +        unsigned size)
> +{
> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> +
> +    if (size != 4) {
> +        return;
> +    }
> +
> +    /* notify the device about the physical address of the DMA buffer that the
> +     * driver has allocated */
> +    switch (addr) {
> +        /* lower bytes */
> +        case(0):
> +            pms->dmaPhisicalBase &= 0xffffffff00000000;
> +            break;
> +
> +        /* upper bytes */
> +        case(4):
> +            val <<= 32;
> +            pms->dmaPhisicalBase &= 0x00000000ffffffff;
> +            break;
> +    }
> +
> +    pms->dmaPhisicalBase |= val;
> +}
> +
> +/*---------------------------------------------------------------------------*/
> +/*                             PCI region ops                                */
> +/*---------------------------------------------------------------------------*/
> +
> +/* callback called when memory region representing the MMIO space is accessed */
> +static const MemoryRegionOps pci_example_mmio_ops = {
> +    .read = pci_example_mmio_read,
> +    .write = pci_example_mmio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,

Now I see you set min/max access size.  Maybe the size checks
above[3] can be asserts?

If I read the documentation correctly, a 64-bit write will become
a series of 1-byte writes, and the device behavior might be
confusing.  Supporting 64-bit writes would probably make it more
intuitive.


> +    },
> +};
> +
> +/* callback called when memory region representing the PIO space is accessed */
> +static const MemoryRegionOps pci_example_pio_ops = {
> +    .read = pci_example_pio_read,
> +    .write = pci_example_pio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,

[3]

> +    },
> +};
> +
> +/* callback called when memory region representing the IRQ space is accessed */
> +static const MemoryRegionOps pci_example_irqio_ops = {
> +    .read = pci_example_irqio_read,
> +    .write = pci_example_irqio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,

[3]

> +    },
> +};
> +
> +/* callback called when memory region representing the DMA space is accessed */
> +static const MemoryRegionOps pci_example_dma_ops = {
> +    .read = pci_example_dma_base_read,
> +    .write = pci_example_dma_base_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,

[3]

> +    },
> +};
> +
> +/*---------------------------------------------------------------------------*/
> +/*                             PCI functions                                 */
> +/*---------------------------------------------------------------------------*/
> +
> +/* this is called when lunching the vm with "-device <device name>" */
> +static void pci_example_realize(PCIDevice *pciDev, Error **errp)
> +{
> +   PCIExampleDevState *d = PCI_EXAMPLE(pciDev);
> +   uint8_t *pciCond = pciDev->config;
> +
> +   d->threwIrq = 0;

Is this really necessary?


> +
> +   /* initiallise the memory region of the CPU to the device */

What does "memory region of the CPU" means?


> +   memory_region_init_io(&d->mmio, OBJECT(d), &pci_example_mmio_ops, d,
> +           "pci-example-mmio", EXAMPLE_MMIO_SIZE);
> +
> +   memory_region_init_io(&d->portio, OBJECT(d), &pci_example_pio_ops, d,
> +           "pci-example-portio", EXAMPLE_PIO_SIZE);
> +
> +   memory_region_init_io(&d->irqio, OBJECT(d), &pci_example_irqio_ops, d,
> +           "pci-example-irqio", EXAMPLE_PIO_SIZE);
> +
> +   memory_region_init_io(&d->dmaio, OBJECT(d), &pci_example_dma_ops, d,
> +           "pci-example-dma-base", EXAMPLE_MMIO_SIZE);

Why you chose to register 64-byte regions instead of a smaller
1, 2, 4, or 8-byte region?

> +
> +   /* alocate BARs */
> +   pci_register_bar(pciDev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> +   pci_register_bar(pciDev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> +   pci_register_bar(pciDev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->irqio);
> +   pci_register_bar(pciDev, 3, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->dmaio);
> +
> +   /* give interrupt support.
> +    * a pci device has 4 pin for interrupt, here we use pin A */
> +   pci_config_set_interrupt_pin(pciCond, 1);
> +}
> +
> +
> +/* the destructor of pci_example_realize() */
> +static void pci_example_uninit(PCIDevice *dev)

I suggest calling it pci_example_exit, as the callback name is
PCIDeviceClass::exit.


> +{
> +    /* unregister BARs and other stuff */

If there's nothing the device needs to do on unrealize, you don't
need an unrealize function at all.

> +}
> +
> +
> +/* class constructor */
> +static void pci_example_class_init(ObjectClass *klass, void *data)
> +{
> +   /* sort of dynamic cast */
> +   DeviceClass *dc = DEVICE_CLASS(klass);
> +   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +   k->realize = pci_example_realize;
> +   k->exit = pci_example_uninit;
> +
> +   /* some regular IDs in HEXA */

What "HEXA" means here?

> +   k->vendor_id = PCI_VENDOR_ID_REDHAT;
> +   k->device_id = PCI_DEVICE_ID_REDHAT_TEST;

Hmm, this is the device ID of pci-testdev.  We can't just reuse
it.  Maybe it would be appropriate to use an ID on the
0x10f0-0x10ff range?  I assume it would be appropriate only if
the device is not compiled in by default.

I also wonder if we we could make pci-test our example device
instead of adding a new one.  What I really miss here is some
guide to point people to known-good examples of device emulation
code.  We could do that with a new example device, or choose some
existing good examples and make them better documented.


> +   k->class_id = PCI_CLASS_OTHERS;
> +
> +   /* set the device bitmap category */
> +   set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> +   k->revision = 0x00;
> +   dc->desc = "PCI Example Device";
> +}
> +
> +/*---------------------------------------------------------------------------*/
> +/*                            QEMU overhead                                  */
> +/*---------------------------------------------------------------------------*/
> +
> +
> +/* Contains all the informations of the device we are creating.
> + * class_init will be called when we are defining our device. */
> +static const TypeInfo pci_example_info = {
> +    .name           = TYPE_PCI_EXAMPLE,
> +    .parent         = TYPE_PCI_DEVICE,
> +    .instance_size  = sizeof(PCIExampleDevState),
> +    .class_init     = pci_example_class_init,
> +    .interfaces     = (InterfaceInfo[]) {
> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },

A comment explaining why we have
INTERFACE_CONVENTIONAL_PCI_DEVICE might be useful.

> +        { },
> +    },
> +};
> +
> +
> +/* function called before the qemu main it will define our device */
> +static void pci_example_register_types(void)
> +{
> +    type_register_static(&pci_example_info);
> +}
> +
> +/* make qemu register our device at qemu-booting */
> +type_init(pci_example_register_types)
> +
> +
> +
> -- 
> 2.17.1
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

[Yoni Bettan]

On 8/29/18 2:40 PM, Cornelia Huck wrote:
> On Tue, 28 Aug 2018 10:24:26 +0300
> Yoni Bettan <ybettan@redhat.com> wrote:

Thanks you for your review!
>
>>      - this is a simple example of how to write a pci device that supports
>>        portio, mmio, irq and dma
> Do you also plan to add example code for MSI(-X)?
>
> [Not just asking because s390 only supports MSI-X. Honestly :)]

For now my main focus is to finish the virtIO device ASAP, so adding 
MSI-X support is in low priority.
But if it is a deal breaker for this patch to be accepted I will add it.
In both cases I have no problem to add MSI-X support later because the 
main purpose is to have an example device describing "the right way",
so, after you bring it to my attention in my opinion, MSI-X support 
should be added.
I will mention that subject on V2 and see what others think about it.
>
>> Signed-off-by: Yoni Bettan <ybettan@redhat.com>
>> ---
>>   hw/pci/Makefile.objs |   1 +
>>   hw/pci/pci_example.c | 309 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 310 insertions(+)
>>   create mode 100644 hw/pci/pci_example.c
>>
> (...)
>
>> diff --git a/hw/pci/pci_example.c b/hw/pci/pci_example.c
>> new file mode 100644
>> index 0000000000..326c9b7fa1
>> --- /dev/null
>> +++ b/hw/pci/pci_example.c
>> @@ -0,0 +1,309 @@
>> +#include "qemu/osdep.h"
>> +#include "hw/pci/pci.h"
>> +
>> +#define TYPE_PCI_EXAMPLE "pci-example"
>> +
>> +#define PCI_EXAMPLE(obj)  \
>> +    OBJECT_CHECK(PCIExampleDevState, (obj), TYPE_PCI_EXAMPLE)
>> +
>> +
>> +#define ERROR -1
> It's probably not a good idea to obfuscate the return code. Either
> return -1, or a proper error code.

You are right, this will be repaired.
In addition another problem here is that a negative value is returned 
from functions that returns uint64_t so I will need to repair it anyway.
>
>> +#define BLOCK_SIZE 64
>> +#define EXAMPLE_MMIO_SIZE BLOCK_SIZE
>> +#define EXAMPLE_PIO_SIZE BLOCK_SIZE
>> +#define DMA_BUFF_SIZE 4096
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                                 PCI Struct                                */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +typedef struct PCIExampleDevState {
>> +    /*< private >*/
>> +    PCIDevice parent_obj;
>> +    /*< public >*/
>> +
>> +    /* memory region */
>> +    MemoryRegion portio;
>> +    MemoryRegion mmio;
>> +    MemoryRegion irqio;
>> +    MemoryRegion dmaio;
>> +
>> +    /* data registers */
>> +    uint64_t memData, ioData, dmaPhisicalBase;
> I think we want this to be mem_data, io_data, and dma_physical_base
> instead.

You are right, after reading 
https://github.com/portante/qemu/blob/master/CODING_STYLE I have notice 
that and this will be updated in V2.
>
>> +
>> +    qemu_irq irq;
>> +    /* for the driver to determine if this device caused the interrupt */
>> +    uint64_t threwIrq;
> Same here (threw_irq).

Same as above.
>
>> +
>> +} PCIExampleDevState;
>> +
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                         Read/Write functions                              */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +/* do nothing because the mmio read is done from DMA buffer */
>> +static uint64_t pci_example_mmio_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    return ERROR;
>> +}
>> +
>> +static void
>> +pci_example_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> I think our preferred style is to keep return value and function name
> on the same line and add line breaks in the parameter list, if needed.
> (Some more occurrences further down.)

No problem, I prefer this style to.
>
>> +{
>> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> peds? ped_state? (I'm not sure pms <-> PCIExampleDevState is obvious;
> I'm not sure either whether my suggestions are better :)

This device originally was called PCIMulDevState since it just multiply 
a number, before submitting the patch I changed it to PCIExampleDevState 
but forgot to update the variables names :)
I will repair that.
>
>> +    PCIDevice *pciDev = (PCIDevice *)opaque;
> pci_dev is better, I think. Also, I'd probably refer to pms->parent_obj
> (maybe call that one pci_device instead?) instead of casting.

I will update the name to fit to the coding style. About referencing to 
parent_obj I need to look again at the code to understand why I did it 
that way.
>
>> +
>> +    if (size != 1) {
>> +        return;
>> +    }
>> +
>> +    /* compute the result */
>> +    pms->memData = val * 2;
>> +
>> +    /* write the result directly to phisical memory */
>> +    cpu_physical_memory_write(pms->dmaPhisicalBase, &pms->memData,
>> +            DMA_BUFF_SIZE);
> Indentation seems off.

How would you have done it otherwise ? this is a simple "Enter" when 80 
characters are reached.
>
>> +
>> +    /* raise an IRQ to notify DMA has finished  */
>> +    pms->threwIrq = 1;
>> +    pci_irq_assert(pciDev);
>> +}
> (...)
>
>> +/* do nothing because physical DMA buffer addres is onlyt set and don't need to
>> + * be red */
> s/addres/address/
> s/onlyt/only/
> s/don't/doesn't/
> s/red/read/

Sorry for that, I didn't run spell check.
>
> Also, we prefer the
>
> /*
>   * comment style
>   */
>
> if it can't fit on one line.
>
> (...)

Got it.
>
>> +/*---------------------------------------------------------------------------*/
>> +/*                             PCI functions                                 */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +/* this is called when lunching the vm with "-device <device name>" */
> s/lunching/launching/
>
> Although that's simply the normal realization process, which could also
> be triggered by hotplug.

I will add this in the comment.
>
>> +static void pci_example_realize(PCIDevice *pciDev, Error **errp)
>> +{
>> +   PCIExampleDevState *d = PCI_EXAMPLE(pciDev);
> Here you use "d" as a variable name, which arguably might be better
> than any of the suggestions above :)

I will make it generic and give a purposeful name :)
>
>> +   uint8_t *pciCond = pciDev->config;
>> +
>> +   d->threwIrq = 0;
>> +
>> +   /* initiallise the memory region of the CPU to the device */
> s/initiallise/initialize/

I will run set spell on the whole file.
>
>> +   memory_region_init_io(&d->mmio, OBJECT(d), &pci_example_mmio_ops, d,
>> +           "pci-example-mmio", EXAMPLE_MMIO_SIZE);
>> +
>> +   memory_region_init_io(&d->portio, OBJECT(d), &pci_example_pio_ops, d,
>> +           "pci-example-portio", EXAMPLE_PIO_SIZE);
>> +
>> +   memory_region_init_io(&d->irqio, OBJECT(d), &pci_example_irqio_ops, d,
>> +           "pci-example-irqio", EXAMPLE_PIO_SIZE);
>> +
>> +   memory_region_init_io(&d->dmaio, OBJECT(d), &pci_example_dma_ops, d,
>> +           "pci-example-dma-base", EXAMPLE_MMIO_SIZE);
>> +
>> +   /* alocate BARs */
> s/alocate/allocate/

Same.
>
>> +   pci_register_bar(pciDev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>> +   pci_register_bar(pciDev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
>> +   pci_register_bar(pciDev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->irqio);
>> +   pci_register_bar(pciDev, 3, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->dmaio);
>> +
>> +   /* give interrupt support.
>> +    * a pci device has 4 pin for interrupt, here we use pin A */
>> +   pci_config_set_interrupt_pin(pciCond, 1);
>> +}
>> +
>> +
>> +/* the destructor of pci_example_realize() */
>> +static void pci_example_uninit(PCIDevice *dev)
>> +{
>> +    /* unregister BARs and other stuff */
> Shouldn't the function actually do it, then? :)

Yes it does, it was a prototype but I will make it full for submission.
Thanks for pointing on this one.
>
>> +}
>> +
>> +
>> +/* class constructor */
>> +static void pci_example_class_init(ObjectClass *klass, void *data)
>> +{
>> +   /* sort of dynamic cast */
>> +   DeviceClass *dc = DEVICE_CLASS(klass);
>> +   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +   k->realize = pci_example_realize;
>> +   k->exit = pci_example_uninit;
>> +
>> +   /* some regular IDs in HEXA */
>> +   k->vendor_id = PCI_VENDOR_ID_REDHAT;
>> +   k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
>> +   k->class_id = PCI_CLASS_OTHERS;
>> +
>> +   /* set the device bitmap category */
>> +   set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +
>> +   k->revision = 0x00;
>> +   dc->desc = "PCI Example Device";
>> +}
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                            QEMU overhead                                  */
> Lol :)
>
>> +/*---------------------------------------------------------------------------*/
>> +
>> +
>> +/* Contains all the informations of the device we are creating.
>> + * class_init will be called when we are defining our device. */
>> +static const TypeInfo pci_example_info = {
>> +    .name           = TYPE_PCI_EXAMPLE,
>> +    .parent         = TYPE_PCI_DEVICE,
>> +    .instance_size  = sizeof(PCIExampleDevState),
>> +    .class_init     = pci_example_class_init,
>> +    .interfaces     = (InterfaceInfo[]) {
>> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> +        { },
>> +    },
>> +};
>> +
>> +
>> +/* function called before the qemu main it will define our device */
> "Define our device type; this is done during startup of QEMU"
>
> ?

This is a pretty good suggestion!
>
>> +static void pci_example_register_types(void)
>> +{
>> +    type_register_static(&pci_example_info);
>> +}
>> +
>> +/* make qemu register our device at qemu-booting */
> I don't think you need this comment here as well (the type_init and the
> function kind of form one logical unit.)

No problem, I will remove it.
>
>> +type_init(pci_example_register_types)
>> +
>> +
>> +
> I think the basic structure looks sound; I've focused on style
> questions as this is supposed to be an example for others to copy from.
> I'll leave the pci details to people more versed in pci :)

Thanks again for this review!
Yoni

Re: [Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

[Yoni Bettan]

On 8/29/18 4:07 PM, Eduardo Habkost wrote:
> Hi,
>
> Thanks for the patch.
>
> On Tue, Aug 28, 2018 at 10:24:26AM +0300, Yoni Bettan wrote:
>>      - this is a simple example of how to write a pci device that supports
>>        portio, mmio, irq and dma
> Can we have some documentation on what are the goals of the
> example device, and what exactly it does?

I replied to the patch with explanation about the device and its 
purpose, I will add it to the commit message in V2.
>
>
>> Signed-off-by: Yoni Bettan <ybettan@redhat.com>
>> ---
>>   hw/pci/Makefile.objs |   1 +
>>   hw/pci/pci_example.c | 309 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 310 insertions(+)
>>   create mode 100644 hw/pci/pci_example.c
>>
>> diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
>> index 9f905e6344..e684b72f90 100644
>> --- a/hw/pci/Makefile.objs
>> +++ b/hw/pci/Makefile.objs
>> @@ -4,6 +4,7 @@ common-obj-$(CONFIG_PCI) += shpc.o
>>   common-obj-$(CONFIG_PCI) += slotid_cap.o
>>   common-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
>>   common-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
>> +common-obj-$(CONFIG_PCI) += pci_example.o
> I am wondering how we can guarantee nobody will break the example
> code while not including it by default on production builds.
> Maybe disabling the device by default, adding a ./configure
> --enable-examples option, and adding it to one of the test cases
> on .travis.yml?

I will add this as a question in V2 to here what is the opinion of the 
other about this.
>
>>   
>>   common-obj-$(call lnot,$(CONFIG_PCI)) += pci-stub.o
>>   common-obj-$(CONFIG_ALL) += pci-stub.o
>> diff --git a/hw/pci/pci_example.c b/hw/pci/pci_example.c
>> new file mode 100644
>> index 0000000000..326c9b7fa1
>> --- /dev/null
>> +++ b/hw/pci/pci_example.c
>> @@ -0,0 +1,309 @@
>> +#include "qemu/osdep.h"
>> +#include "hw/pci/pci.h"
>> +
> Being an example device, it would be nice to explain what these
> two macros are used for.

Good idea, I will do it.
>
>
>> +#define TYPE_PCI_EXAMPLE "pci-example"
>> +
>> +#define PCI_EXAMPLE(obj)  \
>> +    OBJECT_CHECK(PCIExampleDevState, (obj), TYPE_PCI_EXAMPLE)
>> +
>> +
> One line descriptions of the meaning of the macros below would be
> nice to have.
>
>> +#define ERROR -1
> I'm not sure we need this macro.  Additional comments below where
> the macro is actually used.

I will remove this macro.
>
>
>> +#define BLOCK_SIZE 64
>> +#define EXAMPLE_MMIO_SIZE BLOCK_SIZE
>> +#define EXAMPLE_PIO_SIZE BLOCK_SIZE
> I don't see BLOCK_SIZE being used anywhere in the code.  Wouldn't
> it be more readable if you just did:
>
>    #define EXAMPLE_MMIO_SIZE 64
>    #define EXAMPLE_PIO_SIZE  64

I will remove it.
>
>> +#define DMA_BUFF_SIZE 4096
> I suggest DMA_BUF_SIZE, as "buf" is a more usual abbreviation for
> "buffer".

No problem.
>
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                                 PCI Struct                                */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +typedef struct PCIExampleDevState {
>> +    /*< private >*/
>> +    PCIDevice parent_obj;
> One line description of what parent_obj is, or a pointer to QOM
> documentation explaining it would be nice.

Good idea.
>
>> +    /*< public >*/
>> +
>> +    /* memory region */
> This comment seems redundant: the words "memory region" are
> already spelled 4 times below.

I agree, I will remove it.
>
>> +    MemoryRegion portio;
>> +    MemoryRegion mmio;
>> +    MemoryRegion irqio;
>> +    MemoryRegion dmaio;
>> +
>> +    /* data registers */
>> +    uint64_t memData, ioData, dmaPhisicalBase;
> Can we have a one-line description of each of the registers?

Sure.
>
>> +
>> +    qemu_irq irq;
>> +    /* for the driver to determine if this device caused the interrupt */
>> +    uint64_t threwIrq;
>> +
>> +} PCIExampleDevState;
>> +
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                         Read/Write functions                              */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +/* do nothing because the mmio read is done from DMA buffer */
>> +static uint64_t pci_example_mmio_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    return ERROR;
> You can't return a negative value from a uint64_t function.  What
> exactly you are trying to communicate to the caller by returning
> 0xffffffffffffffff here?
>
> Same problem below[5].

You are right, this is a bug, I will solve this issue.
>
>> +}
>> +
>> +static void
>> +pci_example_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> +{
>> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> You can write this as:
>      PCIExampleDevState *pms = opaque;

Implicit casting, yes... I will remove the explicit cast and use 
PCI_EXAMPLE_DEVICE(opaque) instead.
>
> Same below[1].
>
>
>> +    PCIDevice *pciDev = (PCIDevice *)opaque;
> I suggest using PCI_DEVICE(pms) here and below[2].

same.
>
>
>> +
>> +    if (size != 1) {
>> +        return;
>> +    }
> Why you want to support only 1-byte writes?

I think this is enough for a simple example device.
>
>> +
>> +    /* compute the result */
>> +    pms->memData = val * 2;
>> +
>> +    /* write the result directly to phisical memory */
>> +    cpu_physical_memory_write(pms->dmaPhisicalBase, &pms->memData,
>> +            DMA_BUFF_SIZE);
>> +
>> +    /* raise an IRQ to notify DMA has finished  */
>> +    pms->threwIrq = 1;
>> +    pci_irq_assert(pciDev);
>> +}
>> +
>> +static uint64_t pci_example_pio_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> [1]
>
>> +
>> +    if (size != 1) {
>> +        return ERROR;
> Same problem as pci_example_mmio_read() above.

same.
>
>> +    }
> Same as above: why you want to support only 1-byte reads?  Same
> below[3].
>
>> +
>> +    return pms->ioData;
>> +}
>> +
>> +static void
>> +pci_example_pio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> +{
>> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
> [1]
>
>> +    PCIDevice *pciDev = (PCIDevice *)opaque;
> [2]
>
>> +
>> +    if (size != 1) {
> [3]
>
>> +        return;
>> +    }
>> +
>> +    pms->ioData = val * 2;
>> +    pms->threwIrq = 1;
>> +    pci_irq_assert(pciDev);
>> +}
>> +
>> +static uint64_t pci_example_irqio_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
>> +
>> +    if (size != 1) {
> [3]
>
>> +        return ERROR;
> [5]
>
>> +    }
>> +
>> +    return pms->threwIrq;
>> +}
>> +
>> +static void
>> +pci_example_irqio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> +{
>> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
>> +    PCIDevice *pciDev = (PCIDevice *)opaque;
>> +
>> +    if (size != 1) {
> [3]
>
>> +        return;
>> +    }
>> +
>> +    /* give the ability to assert IRQ , we will use it only to deassert IRQ */
>> +    if (val) {
>> +        pci_irq_assert(pciDev);
>> +        pms->threwIrq = 1;
>> +    } else {
>> +        pms->threwIrq = 0;
>> +        pci_irq_deassert(pciDev);
>> +    }
>> +}
>> +
>> +/* do nothing because physical DMA buffer addres is onlyt set and don't need to
>> + * be red */
>> +static uint64_t
>> +pci_example_dma_base_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    return ERROR;
> [5]
>
>> +}
>> +
>> +static void
>> +pci_example_dma_base_write(void *opaque, hwaddr addr, uint64_t val,
>> +        unsigned size)
>> +{
>> +    PCIExampleDevState *pms = (PCIExampleDevState *)opaque;
>> +
>> +    if (size != 4) {
>> +        return;
>> +    }
>> +
>> +    /* notify the device about the physical address of the DMA buffer that the
>> +     * driver has allocated */
>> +    switch (addr) {
>> +        /* lower bytes */
>> +        case(0):
>> +            pms->dmaPhisicalBase &= 0xffffffff00000000;
>> +            break;
>> +
>> +        /* upper bytes */
>> +        case(4):
>> +            val <<= 32;
>> +            pms->dmaPhisicalBase &= 0x00000000ffffffff;
>> +            break;
>> +    }
>> +
>> +    pms->dmaPhisicalBase |= val;
>> +}
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                             PCI region ops                                */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +/* callback called when memory region representing the MMIO space is accessed */
>> +static const MemoryRegionOps pci_example_mmio_ops = {
>> +    .read = pci_example_mmio_read,
>> +    .write = pci_example_mmio_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
> Now I see you set min/max access size.  Maybe the size checks
> above[3] can be asserts?
>
> If I read the documentation correctly, a 64-bit write will become
> a series of 1-byte writes, and the device behavior might be
> confusing.  Supporting 64-bit writes would probably make it more
> intuitive.

I will check this option.
>
>
>> +    },
>> +};
>> +
>> +/* callback called when memory region representing the PIO space is accessed */
>> +static const MemoryRegionOps pci_example_pio_ops = {
>> +    .read = pci_example_pio_read,
>> +    .write = pci_example_pio_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
> [3]
>
>> +    },
>> +};
>> +
>> +/* callback called when memory region representing the IRQ space is accessed */
>> +static const MemoryRegionOps pci_example_irqio_ops = {
>> +    .read = pci_example_irqio_read,
>> +    .write = pci_example_irqio_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
> [3]
>
>> +    },
>> +};
>> +
>> +/* callback called when memory region representing the DMA space is accessed */
>> +static const MemoryRegionOps pci_example_dma_ops = {
>> +    .read = pci_example_dma_base_read,
>> +    .write = pci_example_dma_base_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
> [3]
>
>> +    },
>> +};
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                             PCI functions                                 */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +/* this is called when lunching the vm with "-device <device name>" */
>> +static void pci_example_realize(PCIDevice *pciDev, Error **errp)
>> +{
>> +   PCIExampleDevState *d = PCI_EXAMPLE(pciDev);
>> +   uint8_t *pciCond = pciDev->config;
>> +
>> +   d->threwIrq = 0;
> Is this really necessary?

No, I will remove it since QEMU does this automatically.k
>
>
>> +
>> +   /* initiallise the memory region of the CPU to the device */
> What does "memory region of the CPU" means?

I will update this comment.

>
>
>> +   memory_region_init_io(&d->mmio, OBJECT(d), &pci_example_mmio_ops, d,
>> +           "pci-example-mmio", EXAMPLE_MMIO_SIZE);
>> +
>> +   memory_region_init_io(&d->portio, OBJECT(d), &pci_example_pio_ops, d,
>> +           "pci-example-portio", EXAMPLE_PIO_SIZE);
>> +
>> +   memory_region_init_io(&d->irqio, OBJECT(d), &pci_example_irqio_ops, d,
>> +           "pci-example-irqio", EXAMPLE_PIO_SIZE);
>> +
>> +   memory_region_init_io(&d->dmaio, OBJECT(d), &pci_example_dma_ops, d,
>> +           "pci-example-dma-base", EXAMPLE_MMIO_SIZE);
> Why you chose to register 64-byte regions instead of a smaller
> 1, 2, 4, or 8-byte region?

I will add it as a FIXME for now.
>
>> +
>> +   /* alocate BARs */
>> +   pci_register_bar(pciDev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>> +   pci_register_bar(pciDev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
>> +   pci_register_bar(pciDev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->irqio);
>> +   pci_register_bar(pciDev, 3, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->dmaio);
>> +
>> +   /* give interrupt support.
>> +    * a pci device has 4 pin for interrupt, here we use pin A */
>> +   pci_config_set_interrupt_pin(pciCond, 1);
>> +}
>> +
>> +
>> +/* the destructor of pci_example_realize() */
>> +static void pci_example_uninit(PCIDevice *dev)
> I suggest calling it pci_example_exit, as the callback name is
> PCIDeviceClass::exit.

OK.
>
>
>> +{
>> +    /* unregister BARs and other stuff */
> If there's nothing the device needs to do on unrealize, you don't
> need an unrealize function at all.

The device does need to unallocate BARs, I will add it as a FIXME for now.
>
>> +}
>> +
>> +
>> +/* class constructor */
>> +static void pci_example_class_init(ObjectClass *klass, void *data)
>> +{
>> +   /* sort of dynamic cast */
>> +   DeviceClass *dc = DEVICE_CLASS(klass);
>> +   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +   k->realize = pci_example_realize;
>> +   k->exit = pci_example_uninit;
>> +
>> +   /* some regular IDs in HEXA */
> What "HEXA" means here?

HEXA=Hexadecimal base - I will make it more clear.
>
>> +   k->vendor_id = PCI_VENDOR_ID_REDHAT;
>> +   k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
> Hmm, this is the device ID of pci-testdev.  We can't just reuse
> it.  Maybe it would be appropriate to use an ID on the
> 0x10f0-0x10ff range?  I assume it would be appropriate only if
> the device is not compiled in by default.

I will update the device ID once we decide how to compile this device 
because as I see it there are related.
>
> I also wonder if we we could make pci-test our example device
> instead of adding a new one.  What I really miss here is some
> guide to point people to known-good examples of device emulation
> code.  We could do that with a new example device, or choose some
> existing good examples and make them better documented.

I will check this option.
>
>
>> +   k->class_id = PCI_CLASS_OTHERS;
>> +
>> +   /* set the device bitmap category */
>> +   set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +
>> +   k->revision = 0x00;
>> +   dc->desc = "PCI Example Device";
>> +}
>> +
>> +/*---------------------------------------------------------------------------*/
>> +/*                            QEMU overhead                                  */
>> +/*---------------------------------------------------------------------------*/
>> +
>> +
>> +/* Contains all the informations of the device we are creating.
>> + * class_init will be called when we are defining our device. */
>> +static const TypeInfo pci_example_info = {
>> +    .name           = TYPE_PCI_EXAMPLE,
>> +    .parent         = TYPE_PCI_DEVICE,
>> +    .instance_size  = sizeof(PCIExampleDevState),
>> +    .class_init     = pci_example_class_init,
>> +    .interfaces     = (InterfaceInfo[]) {
>> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> A comment explaining why we have
> INTERFACE_CONVENTIONAL_PCI_DEVICE might be useful.

No problem.
>
>> +        { },
>> +    },
>> +};
>> +
>> +
>> +/* function called before the qemu main it will define our device */
>> +static void pci_example_register_types(void)
>> +{
>> +    type_register_static(&pci_example_info);
>> +}
>> +
>> +/* make qemu register our device at qemu-booting */
>> +type_init(pci_example_register_types)
>> +
>> +
>> +
>> -- 
>> 2.17.1
>>