Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

[Marcel Apfelbaum]

On 31/07/2017 17:00, Michael S. Tsirkin wrote:
> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
> > On PCI init PCI bridge devices may need some
> > extra info about bus number to reserve, IO, memory and
> > prefetchable memory limits. QEMU can provide this
> > with special vendor-specific PCI capability.
> >
> > This capability is intended to be used only
> > for Red Hat PCI bridges, i.e. QEMU cooperation.
> >
> > Signed-off-by: Aleksandr Bezzubikov <address@hidden>
> > ---
> >   src/fw/dev-pci.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 62 insertions(+)
> >   create mode 100644 src/fw/dev-pci.h
> >
> > diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
> > new file mode 100644
> > index 0000000..fbd49ed
> > --- /dev/null
> > +++ b/src/fw/dev-pci.h
> > @@ -0,0 +1,62 @@
> > +#ifndef _PCI_CAP_H
> > +#define _PCI_CAP_H
> > +
> > +#include "types.h"
> > +
> > +/*
> > +
> > +QEMU-specific vendor(Red Hat)-specific capability.
> > +It's intended to provide some hints for firmware to init PCI devices.
> > +
> > +Its is shown below:
> > +
> > +Header:
> > +
> > +u8 id;       Standard PCI Capability Header field
> > +u8 next;     Standard PCI Capability Header field
> > +u8 len;      Standard PCI Capability Header field
> > +u8 type;     Red Hat vendor-specific capability type:
> > +               now only REDHAT_QEMU_CAP 1 exists
> > +Data:
> > +
> > +u16 non_prefetchable_16;     non-prefetchable memory limit
> > +
> > +u8 bus_res;  minimum bus number to reserve;
> > +             this is necessary for PCI Express Root Ports
> > +             to support PCIE-to-PCI bridge hotplug
> > +
> > +u8 io_8;     IO limit in case of 8-bit limit value
> > +u32 io_32;   IO limit in case of 16-bit limit value
> > +             io_8 and io_16 are mutually exclusive, in other words,
> > +             they can't be non-zero simultaneously
> > +
> > +u32 prefetchable_32;         non-prefetchable memory limit
> > +                             in case of 32-bit limit value
> > +u64 prefetchable_64;         non-prefetchable memory limit
> > +                             in case of 64-bit limit value
> > +                             prefetachable_32 and prefetchable_64 are
> > +                             mutually exclusive, in other words,
> > +                             they can't be non-zero simultaneously
> > +If any field in Data section is 0,
> > +it means that such kind of reservation
> > +is not needed.

Hi Michael,

> We also want a way to say "no hint for this type".
>
> One way to achive this would be to have instead multiple
> vendor specific capabilities, one for each of
> bus#/io/mem/prefetch. 0 would mean do not reserve anything,
> absence of capability would mean "no info, up to firmware".

First version of the series was implemented exactly like you propose,
however Gerd preferred only one capability with multiple fields.

I personally like the simplicity of vendor cap per io/mem/bus,
even if it is on the expense of the limited PCI Config space.

We need a consensus here :)

Thanks,
Marcel

> > +
> > +*/
> > +
> > +/* Offset of vendor-specific capability type field */
> > +#define PCI_CAP_VNDR_SPEC_TYPE  3
>
> This is a QEMU specific thing. Please name it as such.
>
> > +
> > +/* List of valid Red Hat vendor-specific capability types */
> > +#define REDHAT_CAP_TYPE_QEMU    1
> > +
> > +
> > +/* Offsets of QEMU capability fields */
> > +#define QEMU_PCI_CAP_NON_PREF   4
> > +#define QEMU_PCI_CAP_BUS_RES    6
> > +#define QEMU_PCI_CAP_IO_8       7
> > +#define QEMU_PCI_CAP_IO_32      8
> > +#define QEMU_PCI_CAP_PREF_32    12
> > +#define QEMU_PCI_CAP_PREF_64    16
> > +#define QEMU_PCI_CAP_SIZE       24
> > +
> > +#endif /* _PCI_CAP_H */
> > --
> > 2.7.4