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 <address@hidden> 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 <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/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 :)