Re: [SeaBIOS] [PATCH v4 2/3] pci: add QEMU-specific PCI capability structure

[Alexander Bezzubikov]

2017-08-07 18:52 GMT+03:00 Marcel Apfelbaum :
> On 05/08/2017 23:29, 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
> >
> > src/fw/dev-pci.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 50 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..2c8ddb0
>
> Hi Aleksandr,

Hi Marcel,
> > --- /dev/null
> > +++ b/src/fw/dev-pci.h
> > @@ -0,0 +1,50 @@
> > +#ifndef _PCI_CAP_H
> > +#define _PCI_CAP_H
> >
> > +#include "types.h"
> >
> > +/*
>
> Please use the standard comment:
>
> /*
>
> */
> > +QEMU-specific vendor(Red Hat)-specific capability.
> > +It's intended to provide some hints for firmware to init PCI devices.
> >
> > +Its structure 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_CAP_TYP_QEMU=1 exists
>
> Typo o the line before, but I think you don't need it there.
> > +Data:
> >
> > +u32 bus_res;            minimum bus number to reserve;
> >                    this is necessary for PCI Express Root Ports
> >                    to support PCIE-to-PCI bridge hotplug
>
> I would add a broader class of usage:
>      necessary for nesting PCI bridges hotplug.
> > +u64 io;                 IO space to reserve
> > +u64 mem;                non-prefetchable memory space to reserve
> > +u64 prefetchable_mem;   prefetchable memory space to reserve
>
> Layout looks good to me.
> > +If any field value in Data section is -1,
> > +it means that such kind of reservation
> > +is not needed and must be ignored.
>
> -1 is not a valid value for unsigned fields, you may
> want to say 0xff..f or some other way.

I meant cast to unsigned here (because we still use unsigned types),
but if it can mislead someone I will change this.
> > +*/
> >
> > +/* Offset of vendor-specific capability type field */
> > +#define PCI_CAP_REDHAT_TYPE  3
>
> May I ask why why '3'? I am not against it, I just
> want to understand the number.

This is actually an offset to this field
> > +/* List of valid Red Hat vendor-specific capability types */
> > +#define REDHAT_CAP_TYPE_QEMU    1
>
> I think I pointed this in another thread, the name is
> too vague, please change it to something like:
>    REDHAT_CAP_RES_RESERVE_QEMU
> that narrows down the intend.

What does the first 'RES' mean?
> > +/* Offsets of QEMU capability fields */
> > +#define QEMU_PCI_CAP_BUS_RES        4
> > +#define QEMU_PCI_CAP_LIMITS_OFFSET  8
> > +#define QEMU_PCI_CAP_IO             8
> > +#define QEMU_PCI_CAP_MEM            16
> > +#define QEMU_PCI_CAP_PREF_MEM       24
> > +#define QEMU_PCI_CAP_SIZE           32
> >
> > +#endif /* _PCI_CAP_H */
>
> The layout looks good to me.
>
> Thanks,
> Marcel