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 <address@hidden>
> ---
>  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