Re: [Qemu-devel] [PATCH v5 2/4] pcie: Add support for Single Root I/O Virtualization (SR/IOV)

[Knut Omang <knut.omang@oracle.com>]

On Sun, 2015-10-18 at 14:02 +0300, Marcel Apfelbaum wrote:

> > +#define SRIOV_ID(dev) \
> > +    (dev)->name, PCI_SLOT((dev)->devfn), PCI_FUNC((dev)->devfn)
> 
> This is a little "hacky" but it is used only for tracing and inside a
> C file.
> I am OK with it.

For the sake of readability of the trace calls and to avoid the trace
lines from disturbing the code overview too much - I would have had to
split the line of each trace call.

> > +++ b/include/hw/pci/pcie_sriov.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * pcie_sriov.h:
> > + *
> > + * Implementation of SR/IOV emulation support.
> > + *
> > + * Copyright (c) 2015 Knut Omang <knut.omang@oracle.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2
> > or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef QEMU_PCIE_SRIOV_H
> > +#define QEMU_PCIE_SRIOV_H
> > +
> > +struct PCIESriovPF {
> > +    uint16_t num_vfs;           /* Number of virtual functions
> > created */
> > +    uint8_t vf_bar_type[PCI_NUM_REGIONS];  /* Store type for each
> > VF bar */
> > +    const char *vfname;         /* Reference to the device type
> > used for the VFs */
> > +    PCIDevice **vf;             /* Pointer to an array of num_vfs
> > VF devices */
> > +};
> > +
> > +struct PCIESriovVF {
> > +    PCIDevice *pf;              /* Pointer back to owner physical
> > function */
> > +};
> 
> 
> Regarding naming conventions, maybe you it should be PCIESRIOVPF o Ir
> PCIESRIOVPf,
> but this would not justify another version (in my opinion)

Yes, I did think about that, but didn't like the readability of either
of these, still wanted to try to follow conventions - this was the
tradeoff I ended at. SR/IOV is kind of complicated compared to e.g. AER
in that the '/' has to go anyway :-D

> > +
> > +/* Optionally add supported page sizes to the mask of supported
> > page sizes
> > + * Page size values are interpreted as opt_sup_pgsize << 12.
> > + */
> > +void pcie_sriov_pf_add_sup_pgsize(PCIDevice *dev, uint16_t
> > opt_sup_pgsize);
> > +
> > +/* SR/IOV capability config write handler */
> > +void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
> > +                             uint32_t val, int len);
> > +
> > +/* Reset SR/IOV VF Enable bit to unregister all VFs */
> > +void pcie_sriov_pf_disable_vfs(PCIDevice *dev);
> > +
> > +#endif /* QEMU_PCIE_SRIOV_H */
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index ee1ce1d..0b89316 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -54,6 +54,8 @@ typedef struct PCIDevice PCIDevice;
> >   typedef struct PCIEAERErr PCIEAERErr;
> >   typedef struct PCIEAERLog PCIEAERLog;
> >   typedef struct PCIEAERMsg PCIEAERMsg;
> > +typedef struct PCIESriovPF PCIESriovPF;
> > +typedef struct PCIESriovVF PCIESriovVF;
> >   typedef struct PCIEPort PCIEPort;
> >   typedef struct PCIESlot PCIESlot;
> >   typedef struct PCIExpressDevice PCIExpressDevice;
> > diff --git a/trace-events b/trace-events
> > index a0ddc6b..fd51c5f 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -1531,6 +1531,11 @@ xen_pv_mmio_write(uint64_t addr) "WARNING:
> > write to Xen PV Device MMIO space (ad
> >   pci_cfg_read(const char *dev, unsigned devid, unsigned fnid,
> > unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x"
> >   pci_cfg_write(const char *dev, unsigned devid, unsigned fnid,
> > unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x"
> > 
> > +# hw/pci/pcie_sriov.c
> > +sriov_register_vfs(const char *name, int slot, int function, int
> > num_vfs) "%s %02x:%x: creating %d vf devs"
> > +sriov_unregister_vfs(const char *name, int slot, int function, int
> > num_vfs) "%s %02x:%x: Unregistering %d vf devs"
> > +sriov_config_write(const char *name, int slot, int fun, uint32_t
> > offset, uint32_t val, uint32_t len) "%s %02x:%x: sriov offset 0x%x
> > val 0x%x len %d"
> > +
> >   # hw/vfio/pci.c
> >   vfio_intx_interrupt(const char *name, char line) " (%s) Pin %c"
> >   vfio_intx_eoi(const char *name) " (%s) EOI"
> > 
> 
> Thanks for all the work, besides the questions above, from my point
> of view:
> 
> 
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Knut

> Thanks,
> Marcel