Re: [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init

2017-07-31 Thread Michael S. Tsirkin 
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 

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

Re: [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init

2017-07-31 Thread Kevin O'Connor 
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 
> ---
>  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

Re: [Qemu-devel] [PATCH v3 3/3] pci: enable RedHat PCI bridges to reserve additional buses on PCI init

2017-07-31 Thread Marcel Apfelbaum 

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 
---
  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_DC2900xdc29
  
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))