Re: [SeaBIOS] [PATCH v4 2/3] pci: add QEMU-specific PCI capability structure
On 07/08/2017 19:02, Alexander Bezzubikov wrote: 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 000..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_QEMU1 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_RES4 +#define QEMU_PCI_CAP_LIMITS_OFFSET 8 +#define QEMU_PCI_CAP_IO 8 +#define QEMU_PCI_CAP_MEM16 +#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 ___ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v4 2/3] pci: add QEMU-specific PCI capability structure
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 000..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_QEMU1 > > > 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_RES4 >> +#define QEMU_PCI_CAP_LIMITS_OFFSET 8 >> +#define QEMU_PCI_CAP_IO 8 >> +#define QEMU_PCI_CAP_MEM16 >> +#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 ___ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v4 2/3] pci: add QEMU-specific PCI capability structure
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 000..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_QEMU1 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_RES4 +#define QEMU_PCI_CAP_LIMITS_OFFSET 8 +#define QEMU_PCI_CAP_IO 8 +#define QEMU_PCI_CAP_MEM16 +#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 ___ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
[SeaBIOS] [PATCH v4 2/3] pci: add QEMU-specific PCI capability structure
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 000..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_QEMU1 + + +/* Offsets of QEMU capability fields */ +#define QEMU_PCI_CAP_BUS_RES4 +#define QEMU_PCI_CAP_LIMITS_OFFSET 8 +#define QEMU_PCI_CAP_IO 8 +#define QEMU_PCI_CAP_MEM16 +#define QEMU_PCI_CAP_PREF_MEM 24 +#define QEMU_PCI_CAP_SIZE 32 + +#endif /* _PCI_CAP_H */ -- 2.7.4 ___ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios