[Qemu-devel] [PATCH 0/7] hw/pci-bus: Fix PCI bus regression on several archs

[Qemu-devel] [PATCH 0/7] hw/pci-bus: Fix PCI bus regression on several archs

[Marcel Apfelbaum]

Commit b86eacb (hw/pci: delay bus_master_enable_region initialization) delayed
enabling the 'bus-master' region for PCI devices until 'machine-done' event.
The event registration happens in the PCIBus realize function, however
the PCI bus is not 'realized' *at all* for some machines.

It appears to be a hidden issue revealed by the above commit.
Thanks Mark Cave-Ayland and Leon Alrae for reporting it!

For all the series:
Reported-by: Leon Alrae <leon.alrae@imgtec.com>
Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Patch 1: Increase the timeout for the prom-env-test since I encountered random failures.
Patch 2-7: Separate fix for each arch so it will be easier for maintainers to ack.

Thanks,
Marcel

Marcel Apfelbaum (7):
  tests/prom-env-test: increase the test timeout
  hw/alpha: fix PCI bus initialization
  hw/mips: fix PCI bus initialization
  hw/apb: fix PCI bus initialization
  hw/grackle: fix PCI bus initialization
  hw/prep: realize the PCI root bus as part of the prep init
  hw/versatile: realize the PCI root bus as part of the versatile init

 hw/alpha/typhoon.c      |  2 +-
 hw/mips/gt64xxx_pci.c   |  2 +-
 hw/pci-host/apb.c       | 15 +++++++--------
 hw/pci-host/grackle.c   |  2 +-
 hw/pci-host/prep.c      |  1 +
 hw/pci-host/versatile.c |  1 +
 tests/prom-env-test.c   |  2 +-
 7 files changed, 13 insertions(+), 12 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/7] tests/prom-env-test: increase the test timeout

[Marcel Apfelbaum]

On a slower machine the test can take more than 30 seconds.
Increase the timeout to 100 seconds.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 tests/prom-env-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/prom-env-test.c b/tests/prom-env-test.c
index 6df57d2..7a62857 100644
--- a/tests/prom-env-test.c
+++ b/tests/prom-env-test.c
@@ -30,7 +30,7 @@ static void check_guest_memory(void)
     int i;
 
     /* Poll until code has run and modified memory. Wait at most 30 seconds */
-    for (i = 0; i < 3000; ++i) {
+    for (i = 0; i < 10000; ++i) {
         signature = readl(ADDRESS);
         if (signature == MAGIC) {
             break;
-- 
2.4.3

[Qemu-devel] [PATCH 2/7] hw/alpha: fix PCI bus initialization

[Marcel Apfelbaum]

Delay the host-bridge 'realization' until the
PCI root bus is attached.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/alpha/typhoon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 97721b5..883db13 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -824,7 +824,6 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
     int i;
 
     dev = qdev_create(NULL, TYPE_TYPHOON_PCI_HOST_BRIDGE);
-    qdev_init_nofail(dev);
 
     s = TYPHOON_PCI_HOST_BRIDGE(dev);
     phb = PCI_HOST_BRIDGE(dev);
@@ -889,6 +888,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
                          &s->pchip.reg_mem, &s->pchip.reg_io,
                          0, 64, TYPE_PCI_BUS);
     phb->bus = b;
+    qdev_init_nofail(dev);
 
     /* Host memory as seen from the PCI side, via the IOMMU.  */
     memory_region_init_iommu(&s->pchip.iommu, OBJECT(s), &typhoon_iommu_ops,
-- 
2.4.3

[Qemu-devel] [PATCH 3/7] hw/mips: fix PCI bus initialization

[Marcel Apfelbaum]

Delay the host-bridge 'realization' until the
PCI root bus is attached.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/mips/gt64xxx_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 3f4523d..4811843 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1167,7 +1167,6 @@ PCIBus *gt64120_register(qemu_irq *pic)
     DeviceState *dev;
 
     dev = qdev_create(NULL, TYPE_GT64120_PCI_HOST_BRIDGE);
-    qdev_init_nofail(dev);
     d = GT64120_PCI_HOST_BRIDGE(dev);
     phb = PCI_HOST_BRIDGE(dev);
     memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", UINT32_MAX);
@@ -1178,6 +1177,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
                                 &d->pci0_mem,
                                 get_system_io(),
                                 PCI_DEVFN(18, 0), 4, TYPE_PCI_BUS);
+    qdev_init_nofail(dev);
     memory_region_init_io(&d->ISD_mem, OBJECT(dev), &isd_mem_ops, d, "isd-mem", 0x1000);
 
     pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
-- 
2.4.3

[Qemu-devel] [PATCH 4/7] hw/apb: fix PCI bus initialization

[Marcel Apfelbaum]

Create and connect the PCI root bus to the
host bridge before the later is 'realized'.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-host/apb.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index babbbef..16587f8 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -670,6 +670,13 @@ PCIBus *pci_apb_init(hwaddr special_base,
 
     /* Ultrasparc PBM main bus */
     dev = qdev_create(NULL, TYPE_APB);
+    d = APB_DEVICE(dev);
+    phb = PCI_HOST_BRIDGE(dev);
+    phb->bus = pci_register_bus(DEVICE(phb), "pci",
+                                pci_apb_set_irq, pci_pbm_map_irq, d,
+                                &d->pci_mmio,
+                                get_system_io(),
+                                0, 32, TYPE_PCI_BUS);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
     /* apb_config */
@@ -678,18 +685,10 @@ PCIBus *pci_apb_init(hwaddr special_base,
     sysbus_mmio_map(s, 1, special_base + 0x1000000ULL);
     /* pci_ioport */
     sysbus_mmio_map(s, 2, special_base + 0x2000000ULL);
-    d = APB_DEVICE(dev);
 
     memory_region_init(&d->pci_mmio, OBJECT(s), "pci-mmio", 0x100000000ULL);
     memory_region_add_subregion(get_system_memory(), mem_base, &d->pci_mmio);
 
-    phb = PCI_HOST_BRIDGE(dev);
-    phb->bus = pci_register_bus(DEVICE(phb), "pci",
-                                pci_apb_set_irq, pci_pbm_map_irq, d,
-                                &d->pci_mmio,
-                                get_system_io(),
-                                0, 32, TYPE_PCI_BUS);
-
     *pbm_irqs = d->pbm_irqs;
     d->ivec_irqs = ivec_irqs;
 
-- 
2.4.3

[Qemu-devel] [PATCH 5/7] hw/grackle: fix PCI bus initialization

[Marcel Apfelbaum]

Delay the host-bridge 'realization' until the
PCI root bus is attached.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-host/grackle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index 8f91216..2c8acda 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -72,7 +72,6 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
     GrackleState *d;
 
     dev = qdev_create(NULL, TYPE_GRACKLE_PCI_HOST_BRIDGE);
-    qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
     phb = PCI_HOST_BRIDGE(dev);
     d = GRACKLE_PCI_HOST_BRIDGE(dev);
@@ -92,6 +91,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
                                 0, 4, TYPE_PCI_BUS);
 
     pci_create_simple(phb->bus, 0, "grackle");
+    qdev_init_nofail(dev);
 
     sysbus_mmio_map(s, 0, base);
     sysbus_mmio_map(s, 1, base + 0x00200000);
-- 
2.4.3

[Qemu-devel] [PATCH 6/7] hw/prep: realize the PCI root bus as part of the prep init

[Marcel Apfelbaum]

'Realize' the PCI root bus manually since the 'realize' mechanism
does not propagate to child devices yet.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-host/prep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 487e32e..5580293 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -247,6 +247,7 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
 
     /* TODO Remove once realize propagates to child devices. */
+    object_property_set_bool(OBJECT(&s->pci_bus), true, "realized", errp);
     object_property_set_bool(OBJECT(&s->pci_dev), true, "realized", errp);
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH 7/7] hw/versatile: realize the PCI root bus as part of the versatile init

[Marcel Apfelbaum]

'Realize' the PCI root bus manually since the 'realize' mechanism
does not propagate to child devices yet.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-host/versatile.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 0792c45..467cbb9 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -455,6 +455,7 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
     }
 
     /* TODO Remove once realize propagates to child devices. */
+    object_property_set_bool(OBJECT(&s->pci_bus), true, "realized", errp);
     object_property_set_bool(OBJECT(&s->pci_dev), true, "realized", errp);
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/7] tests/prom-env-test: increase the test timeout

[Thomas Huth]

On 14.07.2016 15:43, Marcel Apfelbaum wrote:
> On a slower machine the test can take more than 30 seconds.
> Increase the timeout to 100 seconds.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  tests/prom-env-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/prom-env-test.c b/tests/prom-env-test.c
> index 6df57d2..7a62857 100644
> --- a/tests/prom-env-test.c
> +++ b/tests/prom-env-test.c
> @@ -30,7 +30,7 @@ static void check_guest_memory(void)
>      int i;
>  
>      /* Poll until code has run and modified memory. Wait at most 30 seconds */
> -    for (i = 0; i < 3000; ++i) {
> +    for (i = 0; i < 10000; ++i) {
>          signature = readl(ADDRESS);
>          if (signature == MAGIC) {
>              break;

Ack, but could you please also update the comment before the for-loop?

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH 1/7] tests/prom-env-test: increase the test timeout

[Marcel Apfelbaum]

On 07/14/2016 04:51 PM, Thomas Huth wrote:
> On 14.07.2016 15:43, Marcel Apfelbaum wrote:
>> On a slower machine the test can take more than 30 seconds.
>> Increase the timeout to 100 seconds.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   tests/prom-env-test.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/prom-env-test.c b/tests/prom-env-test.c
>> index 6df57d2..7a62857 100644
>> --- a/tests/prom-env-test.c
>> +++ b/tests/prom-env-test.c
>> @@ -30,7 +30,7 @@ static void check_guest_memory(void)
>>       int i;
>>
>>       /* Poll until code has run and modified memory. Wait at most 30 seconds */
>> -    for (i = 0; i < 3000; ++i) {
>> +    for (i = 0; i < 10000; ++i) {
>>           signature = readl(ADDRESS);
>>           if (signature == MAGIC) {
>>               break;
>
> Ack, but could you please also update the comment before the for-loop?
>

Sure, I'll take care of it once I'll get comments for the other patches.
Thanks,
Marcel

>   Thanks,
>    Thomas
>

Re: [Qemu-devel] [PATCH 3/7] hw/mips: fix PCI bus initialization

[Leon Alrae]

On Thu, Jul 14, 2016 at 04:43:42PM +0300, Marcel Apfelbaum wrote:
> Delay the host-bridge 'realization' until the
> PCI root bus is attached.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/mips/gt64xxx_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Leon Alrae <leon.alrae@imgtec.com>
Tested-by: Leon Alrae <leon.alrae@imgtec.com>

Thanks,
Leon

> 
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 3f4523d..4811843 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1167,7 +1167,6 @@ PCIBus *gt64120_register(qemu_irq *pic)
>      DeviceState *dev;
>  
>      dev = qdev_create(NULL, TYPE_GT64120_PCI_HOST_BRIDGE);
> -    qdev_init_nofail(dev);
>      d = GT64120_PCI_HOST_BRIDGE(dev);
>      phb = PCI_HOST_BRIDGE(dev);
>      memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", UINT32_MAX);
> @@ -1178,6 +1177,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
>                                  &d->pci0_mem,
>                                  get_system_io(),
>                                  PCI_DEVFN(18, 0), 4, TYPE_PCI_BUS);
> +    qdev_init_nofail(dev);
>      memory_region_init_io(&d->ISD_mem, OBJECT(dev), &isd_mem_ops, d, "isd-mem", 0x1000);
>  
>      pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH 0/7] hw/pci-bus: Fix PCI bus regression on several archs

[Mark Cave-Ayland]

On 14/07/16 14:43, Marcel Apfelbaum wrote:

> Commit b86eacb (hw/pci: delay bus_master_enable_region initialization) delayed
> enabling the 'bus-master' region for PCI devices until 'machine-done' event.
> The event registration happens in the PCIBus realize function, however
> the PCI bus is not 'realized' *at all* for some machines.
> 
> It appears to be a hidden issue revealed by the above commit.
> Thanks Mark Cave-Ayland and Leon Alrae for reporting it!
> 
> For all the series:
> Reported-by: Leon Alrae <leon.alrae@imgtec.com>
> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Patch 1: Increase the timeout for the prom-env-test since I encountered random failures.
> Patch 2-7: Separate fix for each arch so it will be easier for maintainers to ack.
> 
> Thanks,
> Marcel

Hi Marcel,

I can confirm that this patchset fixes the boot regression for me under
qemu-system-sparc64 - thanks a lot!

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/7] tests/prom-env-test: increase the test timeout

[Marcel Apfelbaum]

On 07/14/2016 04:51 PM, Thomas Huth wrote:
> On 14.07.2016 15:43, Marcel Apfelbaum wrote:
>> On a slower machine the test can take more than 30 seconds.
>> Increase the timeout to 100 seconds.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   tests/prom-env-test.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/prom-env-test.c b/tests/prom-env-test.c
>> index 6df57d2..7a62857 100644
>> --- a/tests/prom-env-test.c
>> +++ b/tests/prom-env-test.c
>> @@ -30,7 +30,7 @@ static void check_guest_memory(void)
>>       int i;
>>
>>       /* Poll until code has run and modified memory. Wait at most 30 seconds */
>> -    for (i = 0; i < 3000; ++i) {
>> +    for (i = 0; i < 10000; ++i) {
>>           signature = readl(ADDRESS);
>>           if (signature == MAGIC) {
>>               break;
>
> Ack, but could you please also update the comment before the for-loop?
>

Hi Michael,
It seems a V2 is not needed, can you tweak the comment or do you prefer me to send the series/patch again?

Thanks,
Marcel

>   Thanks,
>    Thomas
>