| Message ID | 1501964994-5257-3-git-send-email-zuban32s@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 <zuban32s@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, > --- /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. > +*/ > + > +/* 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. > + > +/* 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. > + > + > +/* 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
2017-08-07 18:52 GMT+03:00 Marcel Apfelbaum <marcel@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 <zuban32s@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
On 07/08/2017 19:02, Alexander Bezzubikov wrote: > 2017-08-07 18:52 GMT+03:00 Marcel Apfelbaum <marcel@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 <zuban32s@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. > We should not document signed values for unsigned fields, even if the reason is "best practices." >> >>> +*/ >>> + >>> +/* 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 > OK, so it should end with 'offset' to be clear. I was mislead. >>> + >>> +/* 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? Resource. I don't mind you change it how you seem fit, just make it clear what it does. Is about resource reserving, not a general capability. Thanks, Marcel > >>> + >>> + >>> +/* 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 > > >
diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h new file mode 100644 index 0000000..2c8ddb0 --- /dev/null +++ b/src/fw/dev-pci.h @@ -0,0 +1,50 @@ +#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 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 +Data: + +u32 bus_res; minimum bus number to reserve; + this is necessary for PCI Express Root Ports + to support PCIE-to-PCI bridge hotplug +u64 io; IO space to reserve +u64 mem; non-prefetchable memory space to reserve +u64 prefetchable_mem; prefetchable memory space to reserve + +If any field value in Data section is -1, +it means that such kind of reservation +is not needed and must be ignored. + +*/ + +/* Offset of vendor-specific capability type field */ +#define PCI_CAP_REDHAT_TYPE 3 + +/* List of valid Red Hat vendor-specific capability types */ +#define REDHAT_CAP_TYPE_QEMU 1 + + +/* 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 */
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 <zuban32s@gmail.com> --- src/fw/dev-pci.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 src/fw/dev-pci.h