[Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Peter Maydell]

The reworking of the versatile PCI controller model so that it actually
behaved like hardware included an attempt to autodetect whether the
guest Linux kernel was assuming the old broken behaviour. Unfortunately
it turns out that there are several different variant broken kernels
which behave slightly differently (though none of them will work on
real hardware). The first two patches in this series improve the
autodetection so that we will work out of the box on more kernels.
The third patch adds a property for forcing the behaviour, so that
if there are further cases we didn't know about, at least users have
a command line workaround they can enable.

These patches should be applied for 1.5 because otherwise we're going
to break a bunch of users; apologies for the late submission, but
I only realised this problem today.

In particular they fix the problem reported in LP:1094564.

Peter Maydell (3):
  Revert "versatile_pci: Put the host bridge PCI device at slot 29"
  hw/pci-host/versatile.c: Update autodetect to detect newer kernels
  hw/pci-host/versatile.c: Provide property for forcing broken IRQ
    mapping

 hw/pci-host/versatile.c |   85 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH for-1.5 1/3] Revert "versatile_pci: Put the host bridge PCI device at slot 29"

[Peter Maydell]

This reverts commit 5f37ef92b7690423ac6311d3c597e182fc5f8fe6.
It turns out that some kernels incorrectly depend on the
old QEMU behaviour of not putting the host PCI bridge device
where the hardware puts it, because they use a swizzling IRQ
mapping which is incorrect but happens to match up with old
broken QEMU when the slot number mod 4 is zero. Since we
start PCI devices at 11, if we put the host bridge at 29
then the first real PCI device goes at 11 and doesn't work.
Not putting the host bridge at 29 means it defaults to 11,
so the first real PCI device is at 12 and works.

Since continuing with the old behaviour doesn't cause problems
for kernels which do work with hardware, the simplest fix for
this is to revert the change.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pci-host/versatile.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 540daf7..2bb09fa 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -331,8 +331,6 @@ static void pci_vpb_init(Object *obj)
 
     object_initialize(&s->pci_dev, TYPE_VERSATILE_PCI_HOST);
     qdev_set_parent_bus(DEVICE(&s->pci_dev), BUS(&s->pci_bus));
-    object_property_set_int(OBJECT(&s->pci_dev), PCI_DEVFN(29, 0), "addr",
-                            NULL);
 
     /* Window sizes for VersatilePB; realview_pci's init will override */
     s->mem_win_size[0] = 0x0c000000;
-- 
1.7.9.5

[Qemu-devel] [PATCH for-1.5 2/3] hw/pci-host/versatile.c: Update autodetect to detect newer kernels

[Peter Maydell]

Newer versatilepb kernels still don't get the IRQ mapping right
for the PCI controller, but they get it differently wrong (they add
a fixed +64 offset to everything they write to PCI_INTERRUPT_LINE).
Update the autodetection to handle these too, and include a more
detailed comment on the various different behaviours that might
be present.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pci-host/versatile.c |   69 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 7 deletions(-)

diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 2bb09fa..f19e2f5 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -28,6 +28,32 @@
  * this allows a newer kernel to use the INTERRUPT_LINE
  * registers arbitrarily once it has indicated that it isn't
  * broken in its init code somewhere.
+ *
+ * Unfortunately we have to cope with multiple different
+ * variants on the broken kernel behaviour:
+ *  phase I (before kernel commit 1bc39ac5d) kernels assume old
+ *   QEMU behaviour, so they use IRQ 27 for all slots
+ *  phase II (1bc39ac5d and later, but before e3e92a7be6) kernels
+ *   swizzle IRQs between slots, but do it wrongly, so they
+ *   work only for every fourth PCI card, and only if (like old
+ *   QEMU) the PCI host device is at slot 0 rather than where
+ *   the h/w actually puts it
+ *  phase III (e3e92a7be6 and later) kernels still swizzle IRQs between
+ *   slots wrongly, but add a fixed offset of 64 to everything
+ *   they write to PCI_INTERRUPT_LINE.
+ *
+ * We live in hope of a mythical phase IV kernel which might
+ * actually behave in ways that work on the hardware. Such a
+ * kernel should probably start off by writing some value neither
+ * 27 nor 91 to slot zero's PCI_INTERRUPT_LINE register to
+ * disable the autodetection. After that it can do what it likes.
+ *
+ * Slot % 4 | hw | I  | II | III
+ * -------------------------------
+ *   0      | 29 | 27 | 27 | 91
+ *   1      | 30 | 27 | 28 | 92
+ *   2      | 27 | 27 | 29 | 93
+ *   3      | 28 | 27 | 30 | 94
  */
 enum {
     PCI_VPB_IRQMAP_ASSUME_OK,
@@ -214,6 +240,41 @@ static const MemoryRegionOps pci_vpb_reg_ops = {
     },
 };
 
+static int pci_vpb_broken_irq(int slot, int irq)
+{
+    /* Determine whether this IRQ value for this slot represents a
+     * known broken Linux kernel behaviour for this slot.
+     * Return one of the PCI_VPB_IRQMAP_ constants:
+     *   BROKEN : if this definitely looks like a broken kernel
+     *   FORCE_OK : if this definitely looks good
+     *   ASSUME_OK : if we can't tell
+     */
+    slot %= PCI_NUM_PINS;
+
+    if (irq == 27) {
+        if (slot == 2) {
+            /* Might be a Phase I kernel, or might be a fixed kernel,
+             * since slot 2 is where we expect this IRQ.
+             */
+            return PCI_VPB_IRQMAP_ASSUME_OK;
+        }
+        /* Phase I kernel */
+        return PCI_VPB_IRQMAP_BROKEN;
+    }
+    if (irq == slot + 27) {
+        /* Phase II kernel */
+        return PCI_VPB_IRQMAP_BROKEN;
+    }
+    if (irq == slot + 27 + 64) {
+        /* Phase III kernel */
+        return PCI_VPB_IRQMAP_BROKEN;
+    }
+    /* Anything else must be a fixed kernel, possibly using an
+     * arbitrary irq map.
+     */
+    return PCI_VPB_IRQMAP_FORCE_OK;
+}
+
 static void pci_vpb_config_write(void *opaque, hwaddr addr,
                                  uint64_t val, unsigned size)
 {
@@ -221,13 +282,7 @@ static void pci_vpb_config_write(void *opaque, hwaddr addr,
     if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE
         && s->irq_mapping == PCI_VPB_IRQMAP_ASSUME_OK) {
         uint8_t devfn = addr >> 8;
-        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {
-            if (val == 27) {
-                s->irq_mapping = PCI_VPB_IRQMAP_BROKEN;
-            } else {
-                s->irq_mapping = PCI_VPB_IRQMAP_FORCE_OK;
-            }
-        }
+        s->irq_mapping = pci_vpb_broken_irq(PCI_SLOT(devfn), val);
     }
     pci_data_write(&s->pci_bus, addr, val, size);
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH for-1.5 3/3] hw/pci-host/versatile.c: Provide property for forcing broken IRQ mapping

[Peter Maydell]

Although we try our best to automatically detect broken versions
of Linux which assume the old broken IRQ mapping we used to implement
for our model of the Versatile PCI controller, it turns out that
some particularly new kernels manage to outwit the autodetection.

We therefore provide a property for enabling the old broken IRQ
mapping, so that if users happen to have such a kernel they can
work around its deficiencies with the command line option:
  -global versatile_pci.broken-irq-mapping=1

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pci-host/versatile.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index f19e2f5..2f996d9 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -54,6 +54,10 @@
  *   1      | 30 | 27 | 28 | 92
  *   2      | 27 | 27 | 29 | 93
  *   3      | 28 | 27 | 30 | 94
+ *
+ * Since our autodetection is not perfect we also provide a
+ * property so the user can make us start in BROKEN or FORCE_OK
+ * on reset if they know they have a bad or good kernel.
  */
 enum {
     PCI_VPB_IRQMAP_ASSUME_OK,
@@ -82,6 +86,7 @@ typedef struct {
     /* Constant for life of device: */
     int realview;
     uint32_t mem_win_size[3];
+    uint8_t irq_mapping_prop;
 
     /* Variable state: */
     uint32_t imap[3];
@@ -366,7 +371,7 @@ static void pci_vpb_reset(DeviceState *d)
     s->smap[2] = 0;
     s->selfid = 0;
     s->flags = 0;
-    s->irq_mapping = PCI_VPB_IRQMAP_ASSUME_OK;
+    s->irq_mapping = s->irq_mapping_prop;
 
     pci_vpb_update_all_windows(s);
 }
@@ -476,6 +481,12 @@ static const TypeInfo versatile_pci_host_info = {
     .class_init    = versatile_pci_host_class_init,
 };
 
+static Property pci_vpb_properties[] = {
+    DEFINE_PROP_UINT8("broken-irq-mapping", PCIVPBState, irq_mapping_prop,
+                      PCI_VPB_IRQMAP_ASSUME_OK),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void pci_vpb_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -483,6 +494,7 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
     dc->realize = pci_vpb_realize;
     dc->reset = pci_vpb_reset;
     dc->vmsd = &pci_vpb_vmstate;
+    dc->props = pci_vpb_properties;
 }
 
 static const TypeInfo pci_vpb_info = {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Linus Walleij]

On Tue, May 14, 2013 at 5:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:

> The reworking of the versatile PCI controller model so that it actually
> behaved like hardware included an attempt to autodetect whether the
> guest Linux kernel was assuming the old broken behaviour. Unfortunately
> it turns out that there are several different variant broken kernels
> which behave slightly differently (though none of them will work on
> real hardware). The first two patches in this series improve the
> autodetection so that we will work out of the box on more kernels.
> The third patch adds a property for forcing the behaviour, so that
> if there are further cases we didn't know about, at least users have
> a command line workaround they can enable.

This looks like a viable approach to me. So FWIW:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Arnd Bergmann]

On Wednesday 15 May 2013, Linus Walleij wrote:
> On Tue, May 14, 2013 at 5:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > The reworking of the versatile PCI controller model so that it actually
> > behaved like hardware included an attempt to autodetect whether the
> > guest Linux kernel was assuming the old broken behaviour. Unfortunately
> > it turns out that there are several different variant broken kernels
> > which behave slightly differently (though none of them will work on
> > real hardware). The first two patches in this series improve the
> > autodetection so that we will work out of the box on more kernels.
> > The third patch adds a property for forcing the behaviour, so that
> > if there are further cases we didn't know about, at least users have
> > a command line workaround they can enable.
> 
> This looks like a viable approach to me. So FWIW:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

FWIW, I plan to really get this done in the kernel for 3.11 properly
and rework the entire versatile and realview code base to work without
any platform specific code in arch/arm. The plan is to use the new
infrastructure for PCI and put that code into drivers/pci/host,
and have it scan the hardware using DT only. We can have a backwards
compatibility setup for versatile-pb without DT, but in the long
run, I would prefer to kill off that boot option.

If this works out, any Linux kernel built for any platform should
be able to boot the versatilepb, versatileab, integratorcp, realview-eb,
realview-eb-mpcore, realview-pb-a8, realview-pbx-a9, vexpress-a9
and vexpress-a15 models, as long as you pass a -cpu option matching
a CPU enabled in the kernel, and all the drivers are enabled.

I remember there was a way to autogenerate the dtb blob in qemu at
some point, based on the devices enabled in the model. Did that ever
make it in?

	Arnd

Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Peter Maydell]

On 16 May 2013 17:58, Arnd Bergmann <arnd@arndb.de> wrote:
> FWIW, I plan to really get this done in the kernel for 3.11 properly
> and rework the entire versatile and realview code base to work without
> any platform specific code in arch/arm. The plan is to use the new
> infrastructure for PCI and put that code into drivers/pci/host,
> and have it scan the hardware using DT only. We can have a backwards
> compatibility setup for versatile-pb without DT, but in the long
> run, I would prefer to kill off that boot option.

That sounds cool.

> I remember there was a way to autogenerate the dtb blob in qemu at
> some point, based on the devices enabled in the model. Did that ever
> make it in?

Nope, and it never will, except for the extremely limited case
of mach-virt plus some basic stuff like virtio devices. For
ARM you absolutely must pass in the device tree blob that
came with the kernel version you're booting or it's all liable
to crash horribly, fail to find devices or otherwise misbehave,
so the only reliable thing for QEMU to do is make minimal
modifications to the dtb the user gives us.
[Extended version of this rant here:
https://lists.gnu.org/archive/html/qemu-devel/2013-05/msg01877.html ]

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Linus Walleij]

On Thu, May 16, 2013 at 6:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> FWIW, I plan to really get this done in the kernel for 3.11 properly
> and rework the entire versatile and realview code base to work without
> any platform specific code in arch/arm.

Sweet!


> The plan is to use the new
> infrastructure for PCI and put that code into drivers/pci/host,
> and have it scan the hardware using DT only. We can have a backwards
> compatibility setup for versatile-pb without DT, but in the long
> run, I would prefer to kill off that boot option.

Do we have this on a topic branch in ARM SoC now? I
need a baseline to send a pull request for my cleanup of the
Integrator PCI, and I'd probably like to move the PCIv3
controller as well if I can use the same baseline as you for
this.

Yours,
Linus Walleij

Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels

[Peter Maydell]

On 17 May 2013 12:50, Linus Walleij <linus.walleij@linaro.org> wrote:
> Do we have this on a topic branch in ARM SoC now? I
> need a baseline to send a pull request for my cleanup of the
> Integrator PCI

The good news here is QEMU only models the Integrator/CP,
which has no PCI, so all you need to test is real hardware :-)

-- PMM