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

[Marcel Apfelbaum]

Hi Yoni, Cornelia,

On 09/20/2018 04:59 PM, Cornelia Huck wrote:
> On Wed,  5 Sep 2018 12:38:00 +0300
> Yoni Bettan <address@hidden> wrote:
>
> > The main goal is to add 2 example devices to be used as templates or guideline
> > for contributors when they wish to create a new device, the first is a PCI
> > device and the second will be a VirtIO device.

BTW, it seems we have another device in QEMU for educational
purposes, see hw/misc/edu.c.
Maybe we can "connect" them, or not.

> > 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.
> >
> > Those 2 devices have the same functionality and are multiplying a
> > number by 2, for now only numbers in range [0:127] (result is in range [0:254])
> > are supported (1-byte size input\output).
> >
> > * PCI device:
> >    - its driver is located at https://github.com/ybettan/QemuDeviceDrivers.git

I think we may add the driver to QEMU'S contrib directory, after all
is a documentation project, maybe is better to have it all in the same 
place.

> > * virtIO device:
> >    - still not done.
> >    - its driver isn't written yet but will be added to the same Github 
> > repository
> >      written above.

Again, maybe we can have it in QEMU's contrib directory.

> >    - when this device is done the next step is to insert it into the virtio
> >      specification.
> In which way? You might want to reserve an id for 'testing purposes' or
> somesuch, which should be easy to do right now.

I think we can "link" a virtio requirement to parts of the implementation.
This will make the virtio spec more approachable.

For example: "Interrupts are working like this ..., a possible 
implementation
would be <link to QEMU/contrib/file/line nr> "


I personally find very handy a demo project "explaining" a spec doc.

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

Nice!

> > Signed-off-by: Yoni Bettan <address@hidden>
> > ---
> >
> > Some questions I would like hear your opinion on:
> >
> > 1. How in your opinion can we make this device compile with qemu to ensure it
> >     won't break but without enabling it by default on production builds ?
> Add CONFIG_EXAMPLE_DEVICES; distributions etc. can then disable it.

+1
> > 2. Do you thinks this device should also support MSI-X ?
> If you want to provide a template that people can copy from, it
> probably makes sense. But it certainly can be done in a later patch.
>
> > V2:
> > - Added an explanation on why should we have such devices in the commit message.
> > - Added some questions at the end of the patch according to Eduardo
> >    Habkost's review and Cornelia Huck's review in V1.
> > - Added comments on unclear parts.
> > - Removed unnecessary macros.
> > - Changed names to fit CODING_SYLE.
> >
> >
> >   hw/pci/Makefile.objs |   1 +
> >   hw/pci/pci_example.c | 327 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 328 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..ca510a2b3b
> > --- /dev/null
> > +++ b/hw/pci/pci_example.c
> > @@ -0,0 +1,327 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/pci/pci.h"
> > +
> > +/* this will be the name of the device from qemu point of view */
> s/qemu/QEMU/
>
> > +#define TYPE_PCI_EXAMPLE "pci-example"
> > +
> > +/* each device should have a macro to be cast to using OBJECT_CHECK macro */
> > +#define PCI_EXAMPLE_DEVICE(obj)  \
> > +    OBJECT_CHECK(PCIExampleDevice, (obj), TYPE_PCI_EXAMPLE)
> > +
> > +#define EXAMPLE_MMIO_SIZE 8
> > +#define EXAMPLE_PIO_SIZE 8
> > +#define DMA_BUF_SIZE 4096
> > +
> > +/*---------------------------------------------------------------------------*/
> > +/*                                 PCI Struct                                */
> > +/*---------------------------------------------------------------------------*/
> > +
> > +typedef struct PCIExampleDevice {
> > +    /*< private >*/
> > +    /* this device inherits from PCIDevice according to QEMU Object Model */
> > +    PCIDevice parent_obj;
> > +    /*< public >*/
> > +
> > +    MemoryRegion portio;
> > +    MemoryRegion mmio;
> > +    MemoryRegion irqio;
> > +    MemoryRegion dmaio;

You may want to add a concise explanation of the above
fields for people new to QEMU/PCI world.

The same comment for all the code. Since is a documentation
project I think you should add as much info as possible.

Thanks,
Marcel

[...]