diff mbox

[v4,3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init

Message ID 1501964994-5257-4-git-send-email-zuban32s@gmail.com
State New
Headers show

Commit Message

Aleksandr Bezzubikov Aug. 5, 2017, 8:29 p.m. UTC 
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 | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 src/hw/pci_ids.h |  3 +++
 2 files changed, 68 insertions(+), 4 deletions(-)

Comments

Marcel Apfelbaum Aug. 7, 2017, 4:28 p.m. UTC | #1 
On 05/08/2017 23:29, Aleksandr Bezzubikov wrote:
> In case of Red Hat Generic PCIE Root Port reserve additional buses,
> which number is provided in a vendor-specific capability.
> 

Hi Aleksandr,

It seems the subject/commit description does not cover
all that the patch does, not it also deals with other resources
as well.

> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>   src/fw/pciinit.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>   src/hw/pci_ids.h |  3 +++
>   2 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 864954f..d241d66 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,42 @@ 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) {

I think I already pointed out you should extract the code receiving
the limit into a different function. Also now you have a chance
to re-use the code for IO/MEM resources.

> +                u8 cap;
> +                do {
> +                    cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0);

Maybe I missed something, but how would the do-while will work
if you always use pci_find_capability with offset 0. It will
always start the search from 0 and find the same (first) capability,
right?
Maybe you need:
   cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, cap);

> +                } while (cap &&
> +                         pci_config_readb(bdf, cap + PCI_CAP_REDHAT_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 {
> +                        u32 tmp_res_bus = pci_config_readl(bdf,
> +                                                   cap + QEMU_PCI_CAP_BUS_RES);
> +                        if (tmp_res_bus != (u32)-1) {

I would extract the above check into a separate function
to make code more readable

    pci_qemu_res_cap_set(cap) { return cap != (u32)-1 }

then the code will look like:

                          if(pci_qemu_res_cap_set(res_bus)) {
> +                            res_bus = tmp_res_bus & 0xFF;
> +                            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;
> +                            }
> +                        }
> +                    }
> +                }
> +                res_bus = (*pci_bus > secbus + res_bus) ? *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);
>           }
> @@ -951,11 +985,38 @@ pci_region_map_one_entry(struct pci_region_entry *entry, u64 addr)
>   
>       u16 bdf = entry->dev->bdf;
>       u64 limit = addr + entry->size - 1;
> +
> +    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_REDHAT_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);

The above code should be re-used.

> +            } else {
> +                u32 offset = cap + QEMU_PCI_CAP_LIMITS_OFFSET + entry->type * 8;
> +                u64 tmp_limit = (pci_config_readl(bdf, offset) |
> +                            (u64)pci_config_readl(bdf, offset + 4) << 32);
> +                if (tmp_limit != (u64)-1) {
> +                    tmp_limit += addr - 1;
> +                    limit = (limit > tmp_limit) ? limit : tmp_limit;
> +                }


I think it is too late to read (and use) the cap value here,
because a lot of computations including IO/MEM ranges takes place 
earlier, see:
   pci_bios_check_devices

pci_bios_map_devices uses previously computed ranges, at this time
I think you shouldn't change ranges.

> +            }
> +        }
> +    }
> +
>       if (entry->type == PCI_REGION_TYPE_IO) {
>           pci_config_writeb(bdf, PCI_IO_BASE, addr >> PCI_IO_SHIFT);
> -        pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
> +        pci_config_writew(bdf, PCI_IO_BASE_UPPER16, limit >> 16);
>           pci_config_writeb(bdf, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT);
> -        pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, 0);
> +        pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, limit >> 16);

Judging the above code, it seems SeaBIOS does not support 32-bit IO,
I am not sure yuo can change that.


Thanks,
Marcel

>       }
>       if (entry->type == PCI_REGION_TYPE_MEM) {
>           pci_config_writew(bdf, PCI_MEMORY_BASE, addr >> PCI_MEMORY_SHIFT);
> 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
>   
>
Michael S. Tsirkin Aug. 8, 2017, 3:53 p.m. UTC | #2 
On Sat, Aug 05, 2017 at 11:29:54PM +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 | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  src/hw/pci_ids.h |  3 +++
>  2 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 864954f..d241d66 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,42 @@ 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_REDHAT_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);

I would do cap_len < QEMU_PCI_CAP_SIZE here - this way you can extend
the capability without breaking things.

> +                    } else {
> +                        u32 tmp_res_bus = pci_config_readl(bdf,
> +                                                   cap + QEMU_PCI_CAP_BUS_RES);
> +                        if (tmp_res_bus != (u32)-1) {
> +                            res_bus = tmp_res_bus & 0xFF;
> +                            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;
> +                            }
> +                        }
> +                    }
> +                }
> +                res_bus = (*pci_bus > secbus + res_bus) ? *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);
>          }
> @@ -951,11 +985,38 @@ pci_region_map_one_entry(struct pci_region_entry *entry, u64 addr)
>  
>      u16 bdf = entry->dev->bdf;
>      u64 limit = addr + entry->size - 1;
> +
> +    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_REDHAT_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);

Same here.

> +            } else {
> +                u32 offset = cap + QEMU_PCI_CAP_LIMITS_OFFSET + entry->type * 8;

what is this doing exactly? type is an enum so it's a trick. I'd rather
have an inline with a switch. Use it for bus as well.

> +                u64 tmp_limit = (pci_config_readl(bdf, offset) |
> +                            (u64)pci_config_readl(bdf, offset + 4) << 32);
> +                if (tmp_limit != (u64)-1) {
> +                    tmp_limit += addr - 1;
> +                    limit = (limit > tmp_limit) ? limit : tmp_limit;
> +                }
> +            }
> +        }
> +    }
> +
>      if (entry->type == PCI_REGION_TYPE_IO) {
>          pci_config_writeb(bdf, PCI_IO_BASE, addr >> PCI_IO_SHIFT);
> -        pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
> +        pci_config_writew(bdf, PCI_IO_BASE_UPPER16, limit >> 16);
>          pci_config_writeb(bdf, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT);
> -        pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, 0);
> +        pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, limit >> 16);
>      }
>      if (entry->type == PCI_REGION_TYPE_MEM) {
>          pci_config_writew(bdf, PCI_MEMORY_BASE, addr >> PCI_MEMORY_SHIFT);
> 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
>  
> -- 
> 2.7.4
diff mbox

Patch

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index 864954f..d241d66 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,42 @@  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_REDHAT_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 {
+                        u32 tmp_res_bus = pci_config_readl(bdf,
+                                                   cap + QEMU_PCI_CAP_BUS_RES);
+                        if (tmp_res_bus != (u32)-1) {
+                            res_bus = tmp_res_bus & 0xFF;
+                            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;
+                            }
+                        }
+                    }
+                }
+                res_bus = (*pci_bus > secbus + res_bus) ? *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);
         }
@@ -951,11 +985,38 @@  pci_region_map_one_entry(struct pci_region_entry *entry, u64 addr)
 
     u16 bdf = entry->dev->bdf;
     u64 limit = addr + entry->size - 1;
+
+    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_REDHAT_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 {
+                u32 offset = cap + QEMU_PCI_CAP_LIMITS_OFFSET + entry->type * 8;
+                u64 tmp_limit = (pci_config_readl(bdf, offset) |
+                            (u64)pci_config_readl(bdf, offset + 4) << 32);
+                if (tmp_limit != (u64)-1) {
+                    tmp_limit += addr - 1;
+                    limit = (limit > tmp_limit) ? limit : tmp_limit;
+                }
+            }
+        }
+    }
+
     if (entry->type == PCI_REGION_TYPE_IO) {
         pci_config_writeb(bdf, PCI_IO_BASE, addr >> PCI_IO_SHIFT);
-        pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
+        pci_config_writew(bdf, PCI_IO_BASE_UPPER16, limit >> 16);
         pci_config_writeb(bdf, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT);
-        pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, 0);
+        pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, limit >> 16);
     }
     if (entry->type == PCI_REGION_TYPE_MEM) {
         pci_config_writew(bdf, PCI_MEMORY_BASE, addr >> PCI_MEMORY_SHIFT);
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