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 <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?

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