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

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