2017-08-07 18:52 GMT+03:00 Marcel Apfelbaum <mar...@redhat.com>: > 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 <zuban...@gmail.com> >> --- >> 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 -- Aleksandr Bezzubikov
