* [Qemu-devel] [PATCH v3 0/3] Allow RedHat PCI bridges reserve more buses than necessary during init @ 2017-07-28 23:34 Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 1/3] pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device Aleksandr Bezzubikov ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Aleksandr Bezzubikov @ 2017-07-28 23:34 UTC (permalink / raw) To: seabios Cc: marcel, mst, kevin, lersek, qemu-devel, kraxel, Aleksandr Bezzubikov Now PCI bridges get a bus range number on a system init, basing on currently plugged devices. That's why when one wants to hotplug another bridge, it needs his child bus, which the parent is unable to provide (speaking about virtual device). The suggested workaround is to have vendor-specific capability in Red Hat PCI bridges that contains number of additional bus to reserve (as well as various space limit hints, unused for now) on BIOS PCI init. So this capability is intented only for pure QEMU->SeaBIOS usage. Considering all aforesaid, this series is directly connected with QEMU series (v3) "Generic PCIE-PCI Bridge". Although the new PCI capability is supposed to contain various limits along with bus number to reserve, now only its full layout is proposed. And only bus_reserve field is used in QEMU and BIOS. Limits usage is still a subject for implementation as now the main goal of this series to provide necessary support from the firmware side to PCIE-PCI bridge hotplug. Changes v2->v3: 1. Merge commit 2 (Red Hat vendor ID) into commit 4 - addresses Marcel's comment, and add Generic PCIE Root Port device ID - addresses Michael's comment. 2. Changes of the capability layout (QEMU side has the same changes): - add 'type' field to distinguish multiple RedHat-specific capabilities - addresses Michael's comment - do not mimiс PCI Config space register layout, but use mutually exclusive differently sized fields for IO and prefetchable memory limits - addresses Laszlo's comment - use defines instead of structure and offsetof - addresses Michael's comment 3. Interpret 'bus_reserve' field as a minimum necessary range to reserve - addresses Gerd's comment 4. pci_find_capability moved to pci.c - addresses Kevin's comment 5. Move capability layout header to src/fw/dev-pci.h - addresses Kevin's comment 6. Add the capability documentation - addresses Michael's comment 7. Add capability length and bus_reserve field sanity checks - addresses Michael's comment Changes v1->v2: 1. New #define for Red Hat vendor added (addresses Konrad's comment). 2. Refactored pci_find_capability function (addresses Marcel's comment). 3. Capability reworked: - data type added; - reserve space in a structure for IO, memory and prefetchable memory limits. Aleksandr Bezzubikov (3): pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device pci: add QEMU-specific PCI capability structure pci: enable RedHat PCI bridges to reserve additional buses on PCI init src/fw/dev-pci.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/fw/pciinit.c | 41 +++++++++++++++++++++++++++++++---- src/hw/pci.c | 25 +++++++++++++++++++++ src/hw/pci.h | 1 + src/hw/pci_ids.h | 3 +++ src/hw/pcidevice.c | 24 --------------------- src/hw/pcidevice.h | 1 - src/hw/virtio-pci.c | 6 +++--- src/types.h | 2 ++ 9 files changed, 133 insertions(+), 32 deletions(-) create mode 100644 src/fw/dev-pci.h -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 1/3] pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device 2017-07-28 23:34 [Qemu-devel] [PATCH v3 0/3] Allow RedHat PCI bridges reserve more buses than necessary during init Aleksandr Bezzubikov @ 2017-07-28 23:34 ` Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init Aleksandr Bezzubikov 2 siblings, 0 replies; 19+ messages in thread From: Aleksandr Bezzubikov @ 2017-07-28 23:34 UTC (permalink / raw) To: seabios Cc: marcel, mst, kevin, lersek, qemu-devel, kraxel, Aleksandr Bezzubikov Refactor pci_find_capability function to get bdf instead of a whole pci_device* as the only necessary field for this function is still bdf. Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> --- src/fw/pciinit.c | 4 ++-- src/hw/pci.c | 25 +++++++++++++++++++++++++ src/hw/pci.h | 1 + src/hw/pcidevice.c | 24 ------------------------ src/hw/pcidevice.h | 1 - src/hw/virtio-pci.c | 6 +++--- 6 files changed, 31 insertions(+), 30 deletions(-) diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 08221e6..864954f 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -762,7 +762,7 @@ static int pci_bus_hotplug_support(struct pci_bus *bus, u8 pcie_cap) return downstream_port && slot_implemented; } - shpc_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_SHPC, 0); + shpc_cap = pci_find_capability(bus->bus_dev->bdf, PCI_CAP_ID_SHPC, 0); return !!shpc_cap; } @@ -844,7 +844,7 @@ static int pci_bios_check_devices(struct pci_bus *busses) */ parent = &busses[0]; int type; - u8 pcie_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_EXP, 0); + u8 pcie_cap = pci_find_capability(s->bus_dev->bdf, PCI_CAP_ID_EXP, 0); int hotplug_support = pci_bus_hotplug_support(s, pcie_cap); for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { u64 align = (type == PCI_REGION_TYPE_IO) ? diff --git a/src/hw/pci.c b/src/hw/pci.c index 8e3d617..50d9d2d 100644 --- a/src/hw/pci.c +++ b/src/hw/pci.c @@ -58,6 +58,30 @@ pci_config_maskw(u16 bdf, u32 addr, u16 off, u16 on) pci_config_writew(bdf, addr, val); } +u8 pci_find_capability(u16 bdf, u8 cap_id, u8 cap) +{ + int i; + u16 status = pci_config_readw(bdf, PCI_STATUS); + + if (!(status & PCI_STATUS_CAP_LIST)) + return 0; + + if (cap == 0) { + /* find first */ + cap = pci_config_readb(bdf, PCI_CAPABILITY_LIST); + } else { + /* find next */ + cap = pci_config_readb(bdf, cap + PCI_CAP_LIST_NEXT); + } + for (i = 0; cap && i <= 0xff; i++) { + if (pci_config_readb(bdf, cap + PCI_CAP_LIST_ID) == cap_id) + return cap; + cap = pci_config_readb(bdf, cap + PCI_CAP_LIST_NEXT); + } + + return 0; +} + // Helper function for foreachbdf() macro - return next device int pci_next(int bdf, int bus) @@ -107,3 +131,4 @@ pci_reboot(void) outb(v|6, PORT_PCI_REBOOT); /* Actually do the reset */ udelay(50); } + diff --git a/src/hw/pci.h b/src/hw/pci.h index ee6e196..2e30e28 100644 --- a/src/hw/pci.h +++ b/src/hw/pci.h @@ -39,6 +39,7 @@ u32 pci_config_readl(u16 bdf, u32 addr); u16 pci_config_readw(u16 bdf, u32 addr); u8 pci_config_readb(u16 bdf, u32 addr); void pci_config_maskw(u16 bdf, u32 addr, u16 off, u16 on); +u8 pci_find_capability(u16 bdf, u8 cap_id, u8 cap); int pci_next(int bdf, int bus); int pci_probe_host(void); void pci_reboot(void); diff --git a/src/hw/pcidevice.c b/src/hw/pcidevice.c index cfebf66..8853cf7 100644 --- a/src/hw/pcidevice.c +++ b/src/hw/pcidevice.c @@ -134,30 +134,6 @@ pci_find_init_device(const struct pci_device_id *ids, void *arg) return NULL; } -u8 pci_find_capability(struct pci_device *pci, u8 cap_id, u8 cap) -{ - int i; - u16 status = pci_config_readw(pci->bdf, PCI_STATUS); - - if (!(status & PCI_STATUS_CAP_LIST)) - return 0; - - if (cap == 0) { - /* find first */ - cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); - } else { - /* find next */ - cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT); - } - for (i = 0; cap && i <= 0xff; i++) { - if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) - return cap; - cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT); - } - - return 0; -} - // Enable PCI bus-mastering (ie, DMA) support on a pci device void pci_enable_busmaster(struct pci_device *pci) diff --git a/src/hw/pcidevice.h b/src/hw/pcidevice.h index 354b549..225d545 100644 --- a/src/hw/pcidevice.h +++ b/src/hw/pcidevice.h @@ -69,7 +69,6 @@ int pci_init_device(const struct pci_device_id *ids , struct pci_device *pci, void *arg); struct pci_device *pci_find_init_device(const struct pci_device_id *ids , void *arg); -u8 pci_find_capability(struct pci_device *pci, u8 cap_id, u8 cap); void pci_enable_busmaster(struct pci_device *pci); u16 pci_enable_iobar(struct pci_device *pci, u32 addr); void *pci_enable_membar(struct pci_device *pci, u32 addr); diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index e5c2c33..96f9c6b 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -19,7 +19,7 @@ #include "malloc.h" // free #include "output.h" // dprintf #include "pci.h" // pci_config_readl -#include "pcidevice.h" // pci_find_capability +#include "pcidevice.h" // struct pci_device #include "pci_regs.h" // PCI_BASE_ADDRESS_0 #include "string.h" // memset #include "virtio-pci.h" @@ -381,7 +381,7 @@ fail: void vp_init_simple(struct vp_device *vp, struct pci_device *pci) { - u8 cap = pci_find_capability(pci, PCI_CAP_ID_VNDR, 0); + u8 cap = pci_find_capability(pci->bdf, PCI_CAP_ID_VNDR, 0); struct vp_cap *vp_cap; const char *mode; u32 offset, base, mul; @@ -479,7 +479,7 @@ void vp_init_simple(struct vp_device *vp, struct pci_device *pci) vp_cap->cap, type, vp_cap->bar, addr, offset, mode); } - cap = pci_find_capability(pci, PCI_CAP_ID_VNDR, cap); + cap = pci_find_capability(pci->bdf, PCI_CAP_ID_VNDR, cap); } if (vp->common.cap && vp->notify.cap && vp->isr.cap && vp->device.cap) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-28 23:34 [Qemu-devel] [PATCH v3 0/3] Allow RedHat PCI bridges reserve more buses than necessary during init Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 1/3] pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device Aleksandr Bezzubikov @ 2017-07-28 23:34 ` Aleksandr Bezzubikov 2017-07-31 10:48 ` Marcel Apfelbaum 2017-07-31 14:00 ` Michael S. Tsirkin 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init Aleksandr Bezzubikov 2 siblings, 2 replies; 19+ messages in thread From: Aleksandr Bezzubikov @ 2017-07-28 23:34 UTC (permalink / raw) To: seabios Cc: marcel, mst, kevin, lersek, qemu-devel, kraxel, Aleksandr Bezzubikov 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 | 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. + +*/ + +/* Offset of vendor-specific capability type field */ +#define PCI_CAP_VNDR_SPEC_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_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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure Aleksandr Bezzubikov @ 2017-07-31 10:48 ` Marcel Apfelbaum 2017-07-31 14:00 ` Michael S. Tsirkin 1 sibling, 0 replies; 19+ messages in thread From: Marcel Apfelbaum @ 2017-07-31 10:48 UTC (permalink / raw) To: Aleksandr Bezzubikov, seabios; +Cc: mst, kevin, lersek, qemu-devel, kraxel On 29/07/2017 2:34, 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 | 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" > + > +/* > + Hi Aleksander, > +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 > + Maybe we should name it "mem". And if I remember right Gerd suggested keeping them all 32 bits: u32 mem_res > +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 I must have missed it, but why do we need io_8 field? > +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 I don't see any io_16 field. Maybe only one field: u32 io_res > + > +u32 prefetchable_32; non-prefetchable memory limit > + in case of 32-bit limit value Name and comment mismatch > +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 Name and comment mismatch It should look like: - u32 bus_res - u32 io_res - u32 mem_res, - u32 mem_prefetchable_32, - u64 mem_prefetchable_64, (mutually exclusive with the above) Does it look right to all? > +If any field in Data section is 0, > +it means that such kind of reservation > +is not needed. > + > +*/ > + > +/* Offset of vendor-specific capability type field */ > +#define PCI_CAP_VNDR_SPEC_TYPE 3 > + > +/* List of valid Red Hat vendor-specific capability types */ > +#define REDHAT_CAP_TYPE_QEMU 1 Maybe we should be more concrete: REDHAT_CAP_TYPE_RES_RESERVE > + > + > +/* 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 */ > I know the exact layout is less important for your current project, but is important to get it right the first time. Thanks, Marcel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure Aleksandr Bezzubikov 2017-07-31 10:48 ` Marcel Apfelbaum @ 2017-07-31 14:00 ` Michael S. Tsirkin 2017-07-31 14:09 ` Marcel Apfelbaum 1 sibling, 1 reply; 19+ messages in thread From: Michael S. Tsirkin @ 2017-07-31 14:00 UTC (permalink / raw) To: Aleksandr Bezzubikov; +Cc: seabios, marcel, kevin, lersek, qemu-devel, kraxel 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 <zuban32s@gmail.com> > --- > 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. 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". > + > +*/ > + > +/* 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-31 14:00 ` Michael S. Tsirkin @ 2017-07-31 14:09 ` Marcel Apfelbaum 2017-07-31 18:54 ` Alexander Bezzubikov 0 siblings, 1 reply; 19+ messages in thread From: Marcel Apfelbaum @ 2017-07-31 14:09 UTC (permalink / raw) To: Michael S. Tsirkin, Aleksandr Bezzubikov Cc: seabios, kevin, lersek, qemu-devel, kraxel 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 <zuban32s@gmail.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-31 14:09 ` Marcel Apfelbaum @ 2017-07-31 18:54 ` Alexander Bezzubikov 2017-07-31 18:57 ` Michael S. Tsirkin 0 siblings, 1 reply; 19+ messages in thread From: Alexander Bezzubikov @ 2017-07-31 18:54 UTC (permalink / raw) To: Marcel Apfelbaum Cc: Michael S. Tsirkin, seabios, Kevin OConnor, lersek, qemu-devel, Gerd Hoffmann 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: > 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 <zuban32s@gmail.com> >>> --- >>> 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. I really don't like this 'mutually exclusive' fields approach because IMHO it increases confusion level when undertanding this capability structure. But - if we came to consensus on that, then IO fields should be used in the same way, because as I understand, this 'mutual exclusivity' serves to distinguish cases when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT registers. And this is how both IO and PREFETCHABLE works, isn't it? >> >> > > 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. > Personally I agree with Marcel since all this fields express reservations of some objects. > We need a consensus here :) > Absolutely :) > 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 > > -- Alexander Bezzubikov ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-31 18:54 ` Alexander Bezzubikov @ 2017-07-31 18:57 ` Michael S. Tsirkin 2017-07-31 19:01 ` Alexander Bezzubikov 0 siblings, 1 reply; 19+ messages in thread From: Michael S. Tsirkin @ 2017-07-31 18:57 UTC (permalink / raw) To: Alexander Bezzubikov Cc: Marcel Apfelbaum, seabios, Kevin OConnor, lersek, qemu-devel, Gerd Hoffmann On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: > 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: > > 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 <zuban32s@gmail.com> > >>> --- > >>> 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. > > I really don't like this 'mutually exclusive' fields approach because > IMHO it increases confusion level when undertanding this capability structure. > But - if we came to consensus on that, then IO fields should be used > in the same way, > because as I understand, this 'mutual exclusivity' serves to distinguish cases > when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT > registers. > And this is how both IO and PREFETCHABLE works, isn't it? I would just use simeple 64 bit registers. PCI spec has an ugly format with fields spread all over the place but that is because of compatibility concerns. It makes not sense to spend cycles just to be similarly messy. > >> > >> > > > > 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. > > > > Personally I agree with Marcel since all this fields express > reservations of some objects. > > > We need a consensus here :) > > > > Absolutely :) > > > 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 > > > > > > > > -- > Alexander Bezzubikov ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-31 18:57 ` Michael S. Tsirkin @ 2017-07-31 19:01 ` Alexander Bezzubikov 2017-08-01 13:38 ` Marcel Apfelbaum 0 siblings, 1 reply; 19+ messages in thread From: Alexander Bezzubikov @ 2017-07-31 19:01 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Marcel Apfelbaum, seabios, Kevin OConnor, lersek, qemu-devel, Gerd Hoffmann 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: > On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >> > 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 <zuban32s@gmail.com> >> >>> --- >> >>> 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. >> >> I really don't like this 'mutually exclusive' fields approach because >> IMHO it increases confusion level when undertanding this capability structure. >> But - if we came to consensus on that, then IO fields should be used >> in the same way, >> because as I understand, this 'mutual exclusivity' serves to distinguish cases >> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >> registers. >> And this is how both IO and PREFETCHABLE works, isn't it? > > I would just use simeple 64 bit registers. PCI spec has an ugly format > with fields spread all over the place but that is because of > compatibility concerns. It makes not sense to spend cycles just > to be similarly messy. Then I suggest to use exactly one field of a maximum possible size for each reserving object, and get rid of mutually exclusive fields. Then it can be something like that (order and names can be changed): u8 bus; u16 non_pref; u32 io; u64 pref; > > >> >> >> >> >> > >> > 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. >> > >> >> Personally I agree with Marcel since all this fields express >> reservations of some objects. >> >> > We need a consensus here :) >> > >> >> Absolutely :) >> >> > 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 >> > >> > >> >> >> >> -- >> Alexander Bezzubikov -- Alexander Bezzubikov ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-07-31 19:01 ` Alexander Bezzubikov @ 2017-08-01 13:38 ` Marcel Apfelbaum 2017-08-01 17:28 ` Alexander Bezzubikov 0 siblings, 1 reply; 19+ messages in thread From: Marcel Apfelbaum @ 2017-08-01 13:38 UTC (permalink / raw) To: Alexander Bezzubikov, Michael S. Tsirkin Cc: seabios, Kevin OConnor, lersek, qemu-devel, Gerd Hoffmann On 31/07/2017 22:01, Alexander Bezzubikov wrote: > 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: >> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>> 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 <zuban32s@gmail.com> >>>>>> --- >>>>>> 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. >>> >>> I really don't like this 'mutually exclusive' fields approach because >>> IMHO it increases confusion level when undertanding this capability structure. >>> But - if we came to consensus on that, then IO fields should be used >>> in the same way, >>> because as I understand, this 'mutual exclusivity' serves to distinguish cases >>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>> registers. >>> And this is how both IO and PREFETCHABLE works, isn't it? >> >> I would just use simeple 64 bit registers. PCI spec has an ugly format >> with fields spread all over the place but that is because of >> compatibility concerns. It makes not sense to spend cycles just >> to be similarly messy. > > Then I suggest to use exactly one field of a maximum possible size > for each reserving object, and get rid of mutually exclusive fields. > Then it can be something like that (order and names can be changed): > u8 bus; > u16 non_pref; > u32 io; > u64 pref; > I think Michael suggested: u64 bus_res; u64 mem_res; u64 io_res; u64 mem_pref_res; OR: u32 bus_res; u32 mem_res; u32 io_res; u64 mem_pref_res; We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's requests. Thanks, Marcel > >> >> >>>>> >>>>> >>>> >>>> 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. >>>> >>> >>> Personally I agree with Marcel since all this fields express >>> reservations of some objects. >>> >>>> We need a consensus here :) >>>> >>> >>> Absolutely :) >>> >>>> 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 >>>> >>>> >>> >>> >>> >>> -- >>> Alexander Bezzubikov > > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-08-01 13:38 ` Marcel Apfelbaum @ 2017-08-01 17:28 ` Alexander Bezzubikov 2017-08-04 18:59 ` Alexander Bezzubikov 0 siblings, 1 reply; 19+ messages in thread From: Alexander Bezzubikov @ 2017-08-01 17:28 UTC (permalink / raw) To: Marcel Apfelbaum Cc: Michael S. Tsirkin, seabios, Kevin OConnor, lersek, qemu-devel, Gerd Hoffmann 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: > On 31/07/2017 22:01, Alexander Bezzubikov wrote: >> >> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: >>> >>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>> >>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>>> >>>>> 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 <zuban32s@gmail.com> >>>>>>> --- >>>>>>> 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. >>>> >>>> >>>> I really don't like this 'mutually exclusive' fields approach because >>>> IMHO it increases confusion level when undertanding this capability >>>> structure. >>>> But - if we came to consensus on that, then IO fields should be used >>>> in the same way, >>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>> cases >>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>> registers. >>>> And this is how both IO and PREFETCHABLE works, isn't it? >>> >>> >>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>> with fields spread all over the place but that is because of >>> compatibility concerns. It makes not sense to spend cycles just >>> to be similarly messy. >> >> >> Then I suggest to use exactly one field of a maximum possible size >> for each reserving object, and get rid of mutually exclusive fields. >> Then it can be something like that (order and names can be changed): >> u8 bus; >> u16 non_pref; >> u32 io; >> u64 pref; >> > > I think Michael suggested: > > u64 bus_res; > u64 mem_res; > u64 io_res; > u64 mem_pref_res; > > OR: > u32 bus_res; > u32 mem_res; > u32 io_res; > u64 mem_pref_res; > > > We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's > requests. Let's dwell on the second option (with -1 as 'ignore' sign), if no new objections. > > Thanks, > Marcel > > >> >>> >>> >>>>>> >>>>>> >>>>> >>>>> 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. >>>>> >>>> >>>> Personally I agree with Marcel since all this fields express >>>> reservations of some objects. >>>> >>>>> We need a consensus here :) >>>>> >>>> >>>> Absolutely :) >>>> >>>>> 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 >>>>> >>>>> >>>>> >>>> >>>> >>>> >>>> -- >>>> Alexander Bezzubikov >> >> >> >> > -- Aleksandr Bezzubikov ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-08-01 17:28 ` Alexander Bezzubikov @ 2017-08-04 18:59 ` Alexander Bezzubikov 2017-08-04 20:28 ` Laszlo Ersek 0 siblings, 1 reply; 19+ messages in thread From: Alexander Bezzubikov @ 2017-08-04 18:59 UTC (permalink / raw) To: Marcel Apfelbaum Cc: Michael S. Tsirkin, seabios, Kevin OConnor, Laszlo Ersek, qemu-devel, Gerd Hoffmann 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov <zuban32s@gmail.com>: > 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>> >>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: >>>> >>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>> >>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>>>> >>>>>> 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 <zuban32s@gmail.com> >>>>>>>> --- >>>>>>>> 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. >>>>> >>>>> >>>>> I really don't like this 'mutually exclusive' fields approach because >>>>> IMHO it increases confusion level when undertanding this capability >>>>> structure. >>>>> But - if we came to consensus on that, then IO fields should be used >>>>> in the same way, >>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>> cases >>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>> registers. >>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>> >>>> >>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>> with fields spread all over the place but that is because of >>>> compatibility concerns. It makes not sense to spend cycles just >>>> to be similarly messy. >>> >>> >>> Then I suggest to use exactly one field of a maximum possible size >>> for each reserving object, and get rid of mutually exclusive fields. >>> Then it can be something like that (order and names can be changed): >>> u8 bus; >>> u16 non_pref; >>> u32 io; >>> u64 pref; >>> >> >> I think Michael suggested: >> >> u64 bus_res; >> u64 mem_res; >> u64 io_res; >> u64 mem_pref_res; >> >> OR: >> u32 bus_res; >> u32 mem_res; >> u32 io_res; >> u64 mem_pref_res; >> >> >> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >> requests. > > Let's dwell on the second option (with -1 as 'ignore' sign), if no new > objections. > BTW, talking about limit values provided in the capability - do we want to completely override existing PCI resources allocation mechanism being used in SeaBIOS, I mean, to assign capability values hardly, not taking into consideration any existing checks, or somehow make this process soft (not an obvious way, can lead to another big discussion)? In other words, how do we plan to use IO/MEM/PREF limits provided in this capability in application to the PCIE Root Port, what result is this supposed to achieve? >> >> Thanks, >> Marcel >> >> >>> >>>> >>>> >>>>>>> >>>>>>> >>>>>> >>>>>> 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. >>>>>> >>>>> >>>>> Personally I agree with Marcel since all this fields express >>>>> reservations of some objects. >>>>> >>>>>> We need a consensus here :) >>>>>> >>>>> >>>>> Absolutely :) >>>>> >>>>>> 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 >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Alexander Bezzubikov >>> >>> >>> >>> >> > > > > -- > Aleksandr Bezzubikov -- Aleksandr Bezzubikov ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-08-04 18:59 ` Alexander Bezzubikov @ 2017-08-04 20:28 ` Laszlo Ersek 2017-08-04 20:47 ` Alexander Bezzubikov 0 siblings, 1 reply; 19+ messages in thread From: Laszlo Ersek @ 2017-08-04 20:28 UTC (permalink / raw) To: Alexander Bezzubikov, Marcel Apfelbaum Cc: Michael S. Tsirkin, seabios, Kevin OConnor, qemu-devel, Gerd Hoffmann On 08/04/17 20:59, Alexander Bezzubikov wrote: > 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov <zuban32s@gmail.com>: >> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>>> >>>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: >>>>> >>>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>>> >>>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>>>>> >>>>>>> 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 <zuban32s@gmail.com> >>>>>>>>> --- >>>>>>>>> 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. >>>>>> >>>>>> >>>>>> I really don't like this 'mutually exclusive' fields approach because >>>>>> IMHO it increases confusion level when undertanding this capability >>>>>> structure. >>>>>> But - if we came to consensus on that, then IO fields should be used >>>>>> in the same way, >>>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>>> cases >>>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>>> registers. >>>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>>> >>>>> >>>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>>> with fields spread all over the place but that is because of >>>>> compatibility concerns. It makes not sense to spend cycles just >>>>> to be similarly messy. >>>> >>>> >>>> Then I suggest to use exactly one field of a maximum possible size >>>> for each reserving object, and get rid of mutually exclusive fields. >>>> Then it can be something like that (order and names can be changed): >>>> u8 bus; >>>> u16 non_pref; >>>> u32 io; >>>> u64 pref; >>>> >>> >>> I think Michael suggested: >>> >>> u64 bus_res; >>> u64 mem_res; >>> u64 io_res; >>> u64 mem_pref_res; >>> >>> OR: >>> u32 bus_res; >>> u32 mem_res; >>> u32 io_res; >>> u64 mem_pref_res; >>> >>> >>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >>> requests. >> >> Let's dwell on the second option (with -1 as 'ignore' sign), if no new >> objections. >> > > BTW, talking about limit values provided in the capability - do we > want to completely override > existing PCI resources allocation mechanism being used in SeaBIOS, I > mean, to assign capability > values hardly, not taking into consideration any existing checks, > or somehow make this process soft (not an obvious way, can lead to > another big discussion)? > > In other words, how do we plan to use IO/MEM/PREF limits provided in > this capability > in application to the PCIE Root Port, what result is this supposed to achieve? I think Gerd spoke about this earlier: when determining a given kind of aperture for a given bridge, pick the maximum of: - the actual cumulative need of the devices behind the bridge, and - the hint for the given kind of aperture. So basically, do the same thing as before, but if the hint is larger, grow the aperture to that. This is how I recall the discussion anyway. Thanks Laszlo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-08-04 20:28 ` Laszlo Ersek @ 2017-08-04 20:47 ` Alexander Bezzubikov 2017-08-06 19:58 ` Marcel Apfelbaum 0 siblings, 1 reply; 19+ messages in thread From: Alexander Bezzubikov @ 2017-08-04 20:47 UTC (permalink / raw) To: Laszlo Ersek Cc: Marcel Apfelbaum, Michael S. Tsirkin, seabios, Kevin OConnor, qemu-devel, Gerd Hoffmann 2017-08-04 23:28 GMT+03:00 Laszlo Ersek <lersek@redhat.com>: > On 08/04/17 20:59, Alexander Bezzubikov wrote: >> 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov <zuban32s@gmail.com>: >>> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>>>> >>>>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: >>>>>> >>>>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>>>> >>>>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>>>>>> >>>>>>>> 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 <zuban32s@gmail.com> >>>>>>>>>> --- >>>>>>>>>> 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. >>>>>>> >>>>>>> >>>>>>> I really don't like this 'mutually exclusive' fields approach because >>>>>>> IMHO it increases confusion level when undertanding this capability >>>>>>> structure. >>>>>>> But - if we came to consensus on that, then IO fields should be used >>>>>>> in the same way, >>>>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>>>> cases >>>>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>>>> registers. >>>>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>>>> >>>>>> >>>>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>>>> with fields spread all over the place but that is because of >>>>>> compatibility concerns. It makes not sense to spend cycles just >>>>>> to be similarly messy. >>>>> >>>>> >>>>> Then I suggest to use exactly one field of a maximum possible size >>>>> for each reserving object, and get rid of mutually exclusive fields. >>>>> Then it can be something like that (order and names can be changed): >>>>> u8 bus; >>>>> u16 non_pref; >>>>> u32 io; >>>>> u64 pref; >>>>> >>>> >>>> I think Michael suggested: >>>> >>>> u64 bus_res; >>>> u64 mem_res; >>>> u64 io_res; >>>> u64 mem_pref_res; >>>> >>>> OR: >>>> u32 bus_res; >>>> u32 mem_res; >>>> u32 io_res; >>>> u64 mem_pref_res; >>>> >>>> >>>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >>>> requests. >>> >>> Let's dwell on the second option (with -1 as 'ignore' sign), if no new >>> objections. >>> >> >> BTW, talking about limit values provided in the capability - do we >> want to completely override >> existing PCI resources allocation mechanism being used in SeaBIOS, I >> mean, to assign capability >> values hardly, not taking into consideration any existing checks, >> or somehow make this process soft (not an obvious way, can lead to >> another big discussion)? >> >> In other words, how do we plan to use IO/MEM/PREF limits provided in >> this capability >> in application to the PCIE Root Port, what result is this supposed to achieve? > > I think Gerd spoke about this earlier: when determining a given kind of > aperture for a given bridge, pick the maximum of: > - the actual cumulative need of the devices behind the bridge, and > - the hint for the given kind of aperture. > > So basically, do the same thing as before, but if the hint is larger, > grow the aperture to that. Looks like current SeaBIOS resource allocation implementation is incorrect. E.g. let's look at IO for pcie-root-port (and ioh3420 too) - we have 2 different pci_region_entry objects, where one of them have bar = -1 (that respresent a bridge region as it as in the comment there) and size = 0. This leads to the next situation after the whole BIOS init: root port's IO has base=0x5000,size=0x4FFF. And then Linux reconfigures this values itself. Should the size of the pci_region be checked for emptiness before creation or this isn't an error for some reason? > > This is how I recall the discussion anyway. > > Thanks > Laszlo -- Aleksandr Bezzubikov ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure 2017-08-04 20:47 ` Alexander Bezzubikov @ 2017-08-06 19:58 ` Marcel Apfelbaum 0 siblings, 0 replies; 19+ messages in thread From: Marcel Apfelbaum @ 2017-08-06 19:58 UTC (permalink / raw) To: Alexander Bezzubikov, Laszlo Ersek Cc: Michael S. Tsirkin, seabios, Kevin OConnor, qemu-devel, Gerd Hoffmann On 04/08/2017 23:47, Alexander Bezzubikov wrote: > 2017-08-04 23:28 GMT+03:00 Laszlo Ersek <lersek@redhat.com>: >> On 08/04/17 20:59, Alexander Bezzubikov wrote: >>> 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov <zuban32s@gmail.com>: >>>> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>>> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>>>>> >>>>>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: >>>>>>> >>>>>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>>>>> >>>>>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >>>>>>>>> >>>>>>>>> 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 <zuban32s@gmail.com> >>>>>>>>>>> --- >>>>>>>>>>> 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. >>>>>>>> >>>>>>>> >>>>>>>> I really don't like this 'mutually exclusive' fields approach because >>>>>>>> IMHO it increases confusion level when undertanding this capability >>>>>>>> structure. >>>>>>>> But - if we came to consensus on that, then IO fields should be used >>>>>>>> in the same way, >>>>>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>>>>> cases >>>>>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>>>>> registers. >>>>>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>>>>> >>>>>>> >>>>>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>>>>> with fields spread all over the place but that is because of >>>>>>> compatibility concerns. It makes not sense to spend cycles just >>>>>>> to be similarly messy. >>>>>> >>>>>> >>>>>> Then I suggest to use exactly one field of a maximum possible size >>>>>> for each reserving object, and get rid of mutually exclusive fields. >>>>>> Then it can be something like that (order and names can be changed): >>>>>> u8 bus; >>>>>> u16 non_pref; >>>>>> u32 io; >>>>>> u64 pref; >>>>>> >>>>> >>>>> I think Michael suggested: >>>>> >>>>> u64 bus_res; >>>>> u64 mem_res; >>>>> u64 io_res; >>>>> u64 mem_pref_res; >>>>> >>>>> OR: >>>>> u32 bus_res; >>>>> u32 mem_res; >>>>> u32 io_res; >>>>> u64 mem_pref_res; >>>>> >>>>> >>>>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >>>>> requests. >>>> >>>> Let's dwell on the second option (with -1 as 'ignore' sign), if no new >>>> objections. >>>> >>> >>> BTW, talking about limit values provided in the capability - do we >>> want to completely override >>> existing PCI resources allocation mechanism being used in SeaBIOS, I >>> mean, to assign capability >>> values hardly, not taking into consideration any existing checks, >>> or somehow make this process soft (not an obvious way, can lead to >>> another big discussion)? >>> >>> In other words, how do we plan to use IO/MEM/PREF limits provided in >>> this capability >>> in application to the PCIE Root Port, what result is this supposed to achieve? >> >> I think Gerd spoke about this earlier: when determining a given kind of >> aperture for a given bridge, pick the maximum of: >> - the actual cumulative need of the devices behind the bridge, and >> - the hint for the given kind of aperture. >> >> So basically, do the same thing as before, but if the hint is larger, >> grow the aperture to that. > > Looks like current SeaBIOS resource allocation implementation is incorrect. > E.g. let's look at IO for pcie-root-port (and ioh3420 too) - we have 2 different > pci_region_entry objects, where one of them have bar = -1 (that > respresent a bridge > region as it as in the comment there) and size = 0. This leads to the > next situation after > the whole BIOS init: root port's IO has base=0x5000,size=0x4FFF. > And then Linux reconfigures this values itself. > Should the size of the pci_region be checked for emptiness before creation or > this isn't an error for some reason? I am not aware of such issue, however there is a patch that prevents SeaBIOS to allocate IO range for PCIe Root Ports if the device attached to it does not require IO ports. (or if a Root Port is empty) Can you provide more info? (a debug log) Or a patch that could fix the issue you see? So we can better understand the problem. Thanks, Marcel > >> >> This is how I recall the discussion anyway. >> >> Thanks >> Laszlo > > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init 2017-07-28 23:34 [Qemu-devel] [PATCH v3 0/3] Allow RedHat PCI bridges reserve more buses than necessary during init Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 1/3] pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure Aleksandr Bezzubikov @ 2017-07-28 23:34 ` Aleksandr Bezzubikov 2017-07-31 11:00 ` Marcel Apfelbaum ` (2 more replies) 2 siblings, 3 replies; 19+ messages in thread From: Aleksandr Bezzubikov @ 2017-07-28 23:34 UTC (permalink / raw) To: seabios Cc: marcel, mst, kevin, lersek, qemu-devel, kraxel, Aleksandr Bezzubikov In case of Red Hat Generic PCIE Root Port reserve additional buses, which number is provided in a vendor-specific capability. Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> --- src/fw/pciinit.c | 37 +++++++++++++++++++++++++++++++++++-- src/hw/pci_ids.h | 3 +++ src/types.h | 2 ++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 864954f..a302a85 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -15,6 +15,7 @@ #include "hw/pcidevice.h" // pci_probe_devices #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL #include "hw/pci_regs.h" // PCI_COMMAND +#include "fw/dev-pci.h" // qemu_pci_cap #include "list.h" // struct hlist_node #include "malloc.h" // free #include "output.h" // dprintf @@ -578,9 +579,41 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) pci_bios_init_bus_rec(secbus, pci_bus); if (subbus != *pci_bus) { + u8 res_bus = 0; + if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT && + pci_config_readw(bdf, PCI_DEVICE_ID) == + PCI_DEVICE_ID_REDHAT_ROOT_PORT) { + u8 cap; + do { + cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); + } while (cap && + pci_config_readb(bdf, cap + PCI_CAP_VNDR_SPEC_TYPE) != + REDHAT_CAP_TYPE_QEMU); + if (cap) { + u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS); + if (cap_len != QEMU_PCI_CAP_SIZE) { + dprintf(1, "PCI: QEMU cap length %d is invalid\n", + cap_len); + } else { + res_bus = pci_config_readb(bdf, + cap + QEMU_PCI_CAP_BUS_RES); + if ((u8)(res_bus + secbus) < secbus || + (u8)(res_bus + secbus) < res_bus) { + dprintf(1, "PCI: bus_reserve value %d is invalid\n", + res_bus); + res_bus = 0; + } else { + dprintf(1, "PCI: QEMU cap is found, value = %u\n", + res_bus); + } + } + } + res_bus = MAX(*pci_bus, secbus + res_bus); + } dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", - subbus, *pci_bus); - subbus = *pci_bus; + subbus, res_bus); + subbus = res_bus; + *pci_bus = res_bus; } else { dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); } diff --git a/src/hw/pci_ids.h b/src/hw/pci_ids.h index 4ac73b4..38fa2ca 100644 --- a/src/hw/pci_ids.h +++ b/src/hw/pci_ids.h @@ -2263,6 +2263,9 @@ #define PCI_DEVICE_ID_KORENIX_JETCARDF0 0x1600 #define PCI_DEVICE_ID_KORENIX_JETCARDF1 0x16ff +#define PCI_VENDOR_ID_REDHAT 0x1b36 +#define PCI_DEVICE_ID_REDHAT_ROOT_PORT 0x000C + #define PCI_VENDOR_ID_TEKRAM 0x1de1 #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 diff --git a/src/types.h b/src/types.h index 19d9f6c..75d9108 100644 --- a/src/types.h +++ b/src/types.h @@ -122,6 +122,8 @@ extern void __force_link_error__only_in_16bit(void) __noreturn; typeof(divisor) __divisor = divisor; \ (((x) + ((__divisor) / 2)) / (__divisor)); \ }) +#define MIN(a, b) (((a) < (b)) ? (a) : (b)) +#define MAX(a, b) (((a) > (b)) ? (a) : (b)) #define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1) #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) #define ALIGN_DOWN(x,a) ((x) & ~((typeof(x))(a)-1)) -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init Aleksandr Bezzubikov @ 2017-07-31 11:00 ` Marcel Apfelbaum 2017-07-31 13:50 ` Kevin O'Connor 2017-07-31 13:56 ` Michael S. Tsirkin 2 siblings, 0 replies; 19+ messages in thread From: Marcel Apfelbaum @ 2017-07-31 11:00 UTC (permalink / raw) To: Aleksandr Bezzubikov, seabios; +Cc: mst, kevin, lersek, qemu-devel, kraxel On 29/07/2017 2:34, Aleksandr Bezzubikov wrote: > In case of Red Hat Generic PCIE Root Port reserve additional buses, > which number is provided in a vendor-specific capability. > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > --- > src/fw/pciinit.c | 37 +++++++++++++++++++++++++++++++++++-- > src/hw/pci_ids.h | 3 +++ > src/types.h | 2 ++ > 3 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index 864954f..a302a85 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -15,6 +15,7 @@ > #include "hw/pcidevice.h" // pci_probe_devices > #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL > #include "hw/pci_regs.h" // PCI_COMMAND > +#include "fw/dev-pci.h" // qemu_pci_cap > #include "list.h" // struct hlist_node > #include "malloc.h" // free > #include "output.h" // dprintf > @@ -578,9 +579,41 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) > pci_bios_init_bus_rec(secbus, pci_bus); > > if (subbus != *pci_bus) { > + u8 res_bus = 0; > + if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT && > + pci_config_readw(bdf, PCI_DEVICE_ID) == > + PCI_DEVICE_ID_REDHAT_ROOT_PORT) { > + u8 cap; > + do { > + cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); > + } while (cap && > + pci_config_readb(bdf, cap + PCI_CAP_VNDR_SPEC_TYPE) != > + REDHAT_CAP_TYPE_QEMU); I suggest to extract the bus_reserve computation in a different function. > + if (cap) { > + u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS); > + if (cap_len != QEMU_PCI_CAP_SIZE) { > + dprintf(1, "PCI: QEMU cap length %d is invalid\n", > + cap_len); > + } else { > + res_bus = pci_config_readb(bdf, > + cap + QEMU_PCI_CAP_BUS_RES); > + if ((u8)(res_bus + secbus) < secbus || > + (u8)(res_bus + secbus) < res_bus) { What do you check here, "garbage" values? Even so, all values are unsigned, are you checking for overflow? > + dprintf(1, "PCI: bus_reserve value %d is invalid\n", > + res_bus); > + res_bus = 0; > + } else { > + dprintf(1, "PCI: QEMU cap is found, value = %u\n", > + res_bus); > + } > + } > + } > + res_bus = MAX(*pci_bus, secbus + res_bus); Did you re-check the reboot "issue"? Thanks, Marcel > + } > dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", > - subbus, *pci_bus); > - subbus = *pci_bus; > + subbus, res_bus); > + subbus = res_bus; > + *pci_bus = res_bus; > } else { > dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); > } > diff --git a/src/hw/pci_ids.h b/src/hw/pci_ids.h > index 4ac73b4..38fa2ca 100644 > --- a/src/hw/pci_ids.h > +++ b/src/hw/pci_ids.h > @@ -2263,6 +2263,9 @@ > #define PCI_DEVICE_ID_KORENIX_JETCARDF0 0x1600 > #define PCI_DEVICE_ID_KORENIX_JETCARDF1 0x16ff > > +#define PCI_VENDOR_ID_REDHAT 0x1b36 > +#define PCI_DEVICE_ID_REDHAT_ROOT_PORT 0x000C > + > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > diff --git a/src/types.h b/src/types.h > index 19d9f6c..75d9108 100644 > --- a/src/types.h > +++ b/src/types.h > @@ -122,6 +122,8 @@ extern void __force_link_error__only_in_16bit(void) __noreturn; > typeof(divisor) __divisor = divisor; \ > (((x) + ((__divisor) / 2)) / (__divisor)); \ > }) > +#define MIN(a, b) (((a) < (b)) ? (a) : (b)) > +#define MAX(a, b) (((a) > (b)) ? (a) : (b)) > #define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1) > #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) > #define ALIGN_DOWN(x,a) ((x) & ~((typeof(x))(a)-1)) > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init Aleksandr Bezzubikov 2017-07-31 11:00 ` Marcel Apfelbaum @ 2017-07-31 13:50 ` Kevin O'Connor 2017-07-31 13:56 ` Michael S. Tsirkin 2 siblings, 0 replies; 19+ messages in thread From: Kevin O'Connor @ 2017-07-31 13:50 UTC (permalink / raw) To: Aleksandr Bezzubikov; +Cc: seabios, marcel, mst, lersek, qemu-devel, kraxel On Sat, Jul 29, 2017 at 02:34:32AM +0300, Aleksandr Bezzubikov wrote: > In case of Red Hat Generic PCIE Root Port reserve additional buses, > which number is provided in a vendor-specific capability. > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > --- > src/fw/pciinit.c | 37 +++++++++++++++++++++++++++++++++++-- > src/hw/pci_ids.h | 3 +++ > src/types.h | 2 ++ > 3 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index 864954f..a302a85 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -15,6 +15,7 @@ > #include "hw/pcidevice.h" // pci_probe_devices > #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL > #include "hw/pci_regs.h" // PCI_COMMAND > +#include "fw/dev-pci.h" // qemu_pci_cap > #include "list.h" // struct hlist_node > #include "malloc.h" // free > #include "output.h" // dprintf > @@ -578,9 +579,41 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) > pci_bios_init_bus_rec(secbus, pci_bus); > > if (subbus != *pci_bus) { > + u8 res_bus = 0; > + if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT && > + pci_config_readw(bdf, PCI_DEVICE_ID) == > + PCI_DEVICE_ID_REDHAT_ROOT_PORT) { > + u8 cap; > + do { > + cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); > + } while (cap && > + pci_config_readb(bdf, cap + PCI_CAP_VNDR_SPEC_TYPE) != > + REDHAT_CAP_TYPE_QEMU); > + if (cap) { > + u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS); > + if (cap_len != QEMU_PCI_CAP_SIZE) { > + dprintf(1, "PCI: QEMU cap length %d is invalid\n", > + cap_len); > + } else { > + res_bus = pci_config_readb(bdf, > + cap + QEMU_PCI_CAP_BUS_RES); > + if ((u8)(res_bus + secbus) < secbus || > + (u8)(res_bus + secbus) < res_bus) { > + dprintf(1, "PCI: bus_reserve value %d is invalid\n", > + res_bus); > + res_bus = 0; > + } else { > + dprintf(1, "PCI: QEMU cap is found, value = %u\n", > + res_bus); > + } > + } > + } > + res_bus = MAX(*pci_bus, secbus + res_bus); > + } > dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", > - subbus, *pci_bus); > - subbus = *pci_bus; > + subbus, res_bus); > + subbus = res_bus; > + *pci_bus = res_bus; > } else { > dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); > } > diff --git a/src/hw/pci_ids.h b/src/hw/pci_ids.h > index 4ac73b4..38fa2ca 100644 > --- a/src/hw/pci_ids.h > +++ b/src/hw/pci_ids.h > @@ -2263,6 +2263,9 @@ > #define PCI_DEVICE_ID_KORENIX_JETCARDF0 0x1600 > #define PCI_DEVICE_ID_KORENIX_JETCARDF1 0x16ff > > +#define PCI_VENDOR_ID_REDHAT 0x1b36 > +#define PCI_DEVICE_ID_REDHAT_ROOT_PORT 0x000C > + > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > diff --git a/src/types.h b/src/types.h > index 19d9f6c..75d9108 100644 > --- a/src/types.h > +++ b/src/types.h > @@ -122,6 +122,8 @@ extern void __force_link_error__only_in_16bit(void) __noreturn; > typeof(divisor) __divisor = divisor; \ > (((x) + ((__divisor) / 2)) / (__divisor)); \ > }) > +#define MIN(a, b) (((a) < (b)) ? (a) : (b)) > +#define MAX(a, b) (((a) > (b)) ? (a) : (b)) It seems there are many different implementations of min/max macros and how they handle the issue of multiple evaluation. It may be a good idea to add these to SeaBIOS, but not as part of a PCI patch. For now, so as not to complicate this patch, I suggest you just open-code the single use of MAX in your patch. Otherwise, I'm okay with the SeaBIOS patch series. The QEMU series will need to be committed prior to committing the SeaBIOS patches. However, please continue to address Marcel's (and other's) comments. -Kevin ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init Aleksandr Bezzubikov 2017-07-31 11:00 ` Marcel Apfelbaum 2017-07-31 13:50 ` Kevin O'Connor @ 2017-07-31 13:56 ` Michael S. Tsirkin 2 siblings, 0 replies; 19+ messages in thread From: Michael S. Tsirkin @ 2017-07-31 13:56 UTC (permalink / raw) To: Aleksandr Bezzubikov; +Cc: seabios, marcel, kevin, lersek, qemu-devel, kraxel On Sat, Jul 29, 2017 at 02:34:32AM +0300, Aleksandr Bezzubikov wrote: > In case of Red Hat Generic PCIE Root Port reserve additional buses, > which number is provided in a vendor-specific capability. > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> Why does this ignore io/memory reserve parts of the capability? > --- > src/fw/pciinit.c | 37 +++++++++++++++++++++++++++++++++++-- > src/hw/pci_ids.h | 3 +++ > src/types.h | 2 ++ > 3 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index 864954f..a302a85 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -15,6 +15,7 @@ > #include "hw/pcidevice.h" // pci_probe_devices > #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL > #include "hw/pci_regs.h" // PCI_COMMAND > +#include "fw/dev-pci.h" // qemu_pci_cap > #include "list.h" // struct hlist_node > #include "malloc.h" // free > #include "output.h" // dprintf > @@ -578,9 +579,41 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) > pci_bios_init_bus_rec(secbus, pci_bus); > > if (subbus != *pci_bus) { > + u8 res_bus = 0; > + if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT && > + pci_config_readw(bdf, PCI_DEVICE_ID) == > + PCI_DEVICE_ID_REDHAT_ROOT_PORT) { > + u8 cap; > + do { > + cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); > + } while (cap && > + pci_config_readb(bdf, cap + PCI_CAP_VNDR_SPEC_TYPE) != > + REDHAT_CAP_TYPE_QEMU); > + if (cap) { > + u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS); > + if (cap_len != QEMU_PCI_CAP_SIZE) { > + dprintf(1, "PCI: QEMU cap length %d is invalid\n", > + cap_len); > + } else { > + res_bus = pci_config_readb(bdf, > + cap + QEMU_PCI_CAP_BUS_RES); > + if ((u8)(res_bus + secbus) < secbus || > + (u8)(res_bus + secbus) < res_bus) { > + dprintf(1, "PCI: bus_reserve value %d is invalid\n", > + res_bus); > + res_bus = 0; > + } else { > + dprintf(1, "PCI: QEMU cap is found, value = %u\n", > + res_bus); > + } > + } > + } > + res_bus = MAX(*pci_bus, secbus + res_bus); > + } > dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", > - subbus, *pci_bus); > - subbus = *pci_bus; > + subbus, res_bus); > + subbus = res_bus; > + *pci_bus = res_bus; > } else { > dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); > } > diff --git a/src/hw/pci_ids.h b/src/hw/pci_ids.h > index 4ac73b4..38fa2ca 100644 > --- a/src/hw/pci_ids.h > +++ b/src/hw/pci_ids.h > @@ -2263,6 +2263,9 @@ > #define PCI_DEVICE_ID_KORENIX_JETCARDF0 0x1600 > #define PCI_DEVICE_ID_KORENIX_JETCARDF1 0x16ff > > +#define PCI_VENDOR_ID_REDHAT 0x1b36 > +#define PCI_DEVICE_ID_REDHAT_ROOT_PORT 0x000C > + > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > diff --git a/src/types.h b/src/types.h > index 19d9f6c..75d9108 100644 > --- a/src/types.h > +++ b/src/types.h > @@ -122,6 +122,8 @@ extern void __force_link_error__only_in_16bit(void) __noreturn; > typeof(divisor) __divisor = divisor; \ > (((x) + ((__divisor) / 2)) / (__divisor)); \ > }) > +#define MIN(a, b) (((a) < (b)) ? (a) : (b)) > +#define MAX(a, b) (((a) > (b)) ? (a) : (b)) > #define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1) > #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) > #define ALIGN_DOWN(x,a) ((x) & ~((typeof(x))(a)-1)) > -- > 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-08-06 19:58 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-28 23:34 [Qemu-devel] [PATCH v3 0/3] Allow RedHat PCI bridges reserve more buses than necessary during init Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 1/3] pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device Aleksandr Bezzubikov 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure Aleksandr Bezzubikov 2017-07-31 10:48 ` Marcel Apfelbaum 2017-07-31 14:00 ` Michael S. Tsirkin 2017-07-31 14:09 ` Marcel Apfelbaum 2017-07-31 18:54 ` Alexander Bezzubikov 2017-07-31 18:57 ` Michael S. Tsirkin 2017-07-31 19:01 ` Alexander Bezzubikov 2017-08-01 13:38 ` Marcel Apfelbaum 2017-08-01 17:28 ` Alexander Bezzubikov 2017-08-04 18:59 ` Alexander Bezzubikov 2017-08-04 20:28 ` Laszlo Ersek 2017-08-04 20:47 ` Alexander Bezzubikov 2017-08-06 19:58 ` Marcel Apfelbaum 2017-07-28 23:34 ` [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init Aleksandr Bezzubikov 2017-07-31 11:00 ` Marcel Apfelbaum 2017-07-31 13:50 ` Kevin O'Connor 2017-07-31 13:56 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.