[Qemu-devel] [PATCH 0/2] ppc-e500: Adding pci-pin to irq callback and some cleanup

[Qemu-devel] [PATCH 0/2] ppc-e500: Adding pci-pin to irq callback and some cleanup

[Bharat Bhushan]

First patch is doing some cleanup and second patch adds the PCI-PIN (INT[A-D])
to its interrupt number mapping callback.

Bharat Bhushan (2):
  ppc-e500: some pci related cleanup
  ppc-e500: implement PCI INTx routing

 hw/pci-host/ppce500.c |   34 +++++++++++++++++++++++++---------
 hw/ppc/e500.c         |   12 +++++++-----
 2 files changed, 32 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/2] ppc-e500: some pci related cleanup

[Bharat Bhushan]

 - Use PCI_NUM_PINS rather than hardcoding
 - use "pin" wherever possible

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 hw/pci-host/ppce500.c |   14 +++++++-------
 hw/ppc/e500.c         |   12 +++++++-----
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index f00793d..49bfcc6 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -87,7 +87,7 @@ struct PPCE500PCIState {
     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
     uint32_t gasket_time;
-    qemu_irq irq[4];
+    qemu_irq irq[PCI_NUM_PINS];
     uint32_t first_slot;
     /* mmio maps */
     MemoryRegion container;
@@ -252,26 +252,26 @@ static const MemoryRegionOps e500_pci_reg_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
+static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
 {
     int devno = pci_dev->devfn >> 3;
     int ret;
 
-    ret = ppce500_pci_map_irq_slot(devno, irq_num);
+    ret = ppce500_pci_map_irq_slot(devno, pin);
 
     pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
-           pci_dev->devfn, irq_num, ret, devno);
+           pci_dev->devfn, pin, ret, devno);
 
     return ret;
 }
 
-static void mpc85xx_pci_set_irq(void *opaque, int irq_num, int level)
+static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
 {
     qemu_irq *pic = opaque;
 
-    pci_debug("%s: PCI irq %d, level:%d\n", __func__, irq_num, level);
+    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
 
-    qemu_set_irq(pic[irq_num], level);
+    qemu_set_irq(pic[pin], level);
 }
 
 static const VMStateDescription vmstate_pci_outbound = {
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index cfdd84b..e437d06 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -610,7 +610,9 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
     target_long initrd_size = 0;
     target_ulong cur_base = 0;
     int i;
-    unsigned int pci_irq_nrs[4] = {1, 2, 3, 4};
+    /* irq num for pin INTA, INTB, INTC and INTD is 1, 2, 3 and
+     * 4 respectively */
+    unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
     qemu_irq **irqs, *mpic;
     DeviceState *dev;
     CPUPPCState *firstenv = NULL;
@@ -712,10 +714,10 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
     qdev_prop_set_uint32(dev, "first_slot", params->pci_first_slot);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
-    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
-    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
-    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
-    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
+    for (i = 0; i < PCI_NUM_PINS; i++) {
+        sysbus_connect_irq(s, i, mpic[pci_irq_nrs[i]]);
+    }
+
     memory_region_add_subregion(ccsr_addr_space, MPC8544_PCI_REGS_OFFSET,
                                 sysbus_mmio_get_region(s, 0));
 
-- 
1.7.0.4

[Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Bharat Bhushan]

This patch adds pci pin to irq_num routing callback
Without this patch we gets below warning

"
  PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
  qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
"

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index 49bfcc6..3c4cf9e 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -88,6 +88,7 @@ struct PPCE500PCIState {
     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
     uint32_t gasket_time;
     qemu_irq irq[PCI_NUM_PINS];
+    uint32_t irq_num[PCI_NUM_PINS];
     uint32_t first_slot;
     /* mmio maps */
     MemoryRegion container;
@@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
 
 static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
 {
-    qemu_irq *pic = opaque;
+    PPCE500PCIState *s = opaque;
+    qemu_irq *pic = s->irq;;
 
     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
 
     qemu_set_irq(pic[pin], level);
 }
 
+static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int pin)
+{
+    PCIINTxRoute route;
+    PPCE500PCIState *s = opaque;
+
+    route.mode = PCI_INTX_ENABLED;
+    route.irq = s->irq_num[pin];
+
+    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin, route.irq);
+    return route;
+}
+
 static const VMStateDescription vmstate_pci_outbound = {
     .name = "pci_outbound",
     .version_id = 0,
@@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
 
     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
         sysbus_init_irq(dev, &s->irq[i]);
+        s->irq_num[i] = i + 1;
     }
 
     memory_region_init(&s->pio, OBJECT(s), "pci-pio", PCIE500_PCI_IOLEN);
 
     b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
-                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
+                         mpc85xx_pci_map_irq, s, address_space_mem,
                          &s->pio, PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
     h->bus = b;
 
@@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
     sysbus_init_mmio(dev, &s->container);
     sysbus_init_mmio(dev, &s->pio);
+    pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
 
     return 0;
 }
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 1/2] ppc-e500: some pci related cleanup

[Alexander Graf]

On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:

> - Use PCI_NUM_PINS rather than hardcoding
> - use "pin" wherever possible

I assume you mean the PCI A/B/C/D pin with "pin".

> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
> hw/pci-host/ppce500.c |   14 +++++++-------
> hw/ppc/e500.c         |   12 +++++++-----
> 2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index f00793d..49bfcc6 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -87,7 +87,7 @@ struct PPCE500PCIState {
>     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
>     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>     uint32_t gasket_time;
> -    qemu_irq irq[4];
> +    qemu_irq irq[PCI_NUM_PINS];
>     uint32_t first_slot;
>     /* mmio maps */
>     MemoryRegion container;
> @@ -252,26 +252,26 @@ static const MemoryRegionOps e500_pci_reg_ops = {
>     .endianness = DEVICE_BIG_ENDIAN,
> };
> 
> -static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
> +static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)

This function converts pin -> irq, so that's fine.

> {
>     int devno = pci_dev->devfn >> 3;
>     int ret;
> 
> -    ret = ppce500_pci_map_irq_slot(devno, irq_num);
> +    ret = ppce500_pci_map_irq_slot(devno, pin);
> 
>     pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
> -           pci_dev->devfn, irq_num, ret, devno);
> +           pci_dev->devfn, pin, ret, devno);
> 
>     return ret;
> }
> 
> -static void mpc85xx_pci_set_irq(void *opaque, int irq_num, int level)
> +static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)

While this one ...

> {
>     qemu_irq *pic = opaque;
> 
> -    pci_debug("%s: PCI irq %d, level:%d\n", __func__, irq_num, level);
> +    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> 
> -    qemu_set_irq(pic[irq_num], level);
> +    qemu_set_irq(pic[pin], level);

... sets an actual irq number on the PIC, so this is not a pin.


The rest looks good to me :).


Alex

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Alexander Graf]

On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:

> This patch adds pci pin to irq_num routing callback
> Without this patch we gets below warning
> 
> "
>  PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
>  qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> "
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
> hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> 1 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index 49bfcc6..3c4cf9e 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -88,6 +88,7 @@ struct PPCE500PCIState {
>     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>     uint32_t gasket_time;
>     qemu_irq irq[PCI_NUM_PINS];
> +    uint32_t irq_num[PCI_NUM_PINS];
>     uint32_t first_slot;
>     /* mmio maps */
>     MemoryRegion container;
> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
> 
> static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
> {
> -    qemu_irq *pic = opaque;
> +    PPCE500PCIState *s = opaque;
> +    qemu_irq *pic = s->irq;;

Double semicolon?

> 
>     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> 
>     qemu_set_irq(pic[pin], level);
> }
> 
> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int pin)
> +{
> +    PCIINTxRoute route;
> +    PPCE500PCIState *s = opaque;
> +
> +    route.mode = PCI_INTX_ENABLED;
> +    route.irq = s->irq_num[pin];
> +
> +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin, route.irq);
> +    return route;
> +}
> +
> static const VMStateDescription vmstate_pci_outbound = {
>     .name = "pci_outbound",
>     .version_id = 0,
> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> 
>     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
>         sysbus_init_irq(dev, &s->irq[i]);
> +        s->irq_num[i] = i + 1;

Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? I don't understand the purpose of this whole exercise to be honest.

Michael, could you please shed some light on this?


Alex

>     }
> 
>     memory_region_init(&s->pio, OBJECT(s), "pci-pio", PCIE500_PCI_IOLEN);
> 
>     b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> -                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
> +                         mpc85xx_pci_map_irq, s, address_space_mem,
>                          &s->pio, PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
>     h->bus = b;
> 
> @@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
>     sysbus_init_mmio(dev, &s->container);
>     sysbus_init_mmio(dev, &s->pio);
> +    pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
> 
>     return 0;
> }
> -- 
> 1.7.0.4
> 
> 

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Michael S. Tsirkin]

On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> 
> On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> 
> > This patch adds pci pin to irq_num routing callback
> > Without this patch we gets below warning
> > 
> > "
> >  PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> >  qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > "
> > 
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> > hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> > 1 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> > index 49bfcc6..3c4cf9e 100644
> > --- a/hw/pci-host/ppce500.c
> > +++ b/hw/pci-host/ppce500.c
> > @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> >     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> >     uint32_t gasket_time;
> >     qemu_irq irq[PCI_NUM_PINS];
> > +    uint32_t irq_num[PCI_NUM_PINS];
> >     uint32_t first_slot;
> >     /* mmio maps */
> >     MemoryRegion container;
> > @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
> > 
> > static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
> > {
> > -    qemu_irq *pic = opaque;
> > +    PPCE500PCIState *s = opaque;
> > +    qemu_irq *pic = s->irq;;
> 
> Double semicolon?
> 
> > 
> >     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> > 
> >     qemu_set_irq(pic[pin], level);
> > }
> > 
> > +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int pin)
> > +{
> > +    PCIINTxRoute route;
> > +    PPCE500PCIState *s = opaque;
> > +
> > +    route.mode = PCI_INTX_ENABLED;
> > +    route.irq = s->irq_num[pin];
> > +
> > +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin, route.irq);
> > +    return route;
> > +}
> > +
> > static const VMStateDescription vmstate_pci_outbound = {
> >     .name = "pci_outbound",
> >     .version_id = 0,
> > @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> > 
> >     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> >         sysbus_init_irq(dev, &s->irq[i]);
> > +        s->irq_num[i] = i + 1;
> 
> Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? I don't understand the purpose of this whole exercise to be honest.
> 
> Michael, could you please shed some light on this?
> 
> 
> Alex

This is printed by pci_device_route_intx_to_irq - it's used by device
assignment and vfio to figure out which irq does a
given pci device drive.

> >     }
> > 
> >     memory_region_init(&s->pio, OBJECT(s), "pci-pio", PCIE500_PCI_IOLEN);
> > 
> >     b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> > -                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
> > +                         mpc85xx_pci_map_irq, s, address_space_mem,
> >                          &s->pio, PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
> >     h->bus = b;
> > 
> > @@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> >     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
> >     sysbus_init_mmio(dev, &s->container);
> >     sysbus_init_mmio(dev, &s->pio);
> > +    pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
> > 
> >     return 0;
> > }
> > -- 
> > 1.7.0.4
> > 
> > 

Re: [Qemu-devel] [PATCH 1/2] ppc-e500: some pci related cleanup

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, December 19, 2013 3:18 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/2] ppc-e500: some pci related cleanup
> 
> 
> On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> 
> > - Use PCI_NUM_PINS rather than hardcoding
> > - use "pin" wherever possible
> 
> I assume you mean the PCI A/B/C/D pin with "pin".

Yes 

> 
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> > hw/pci-host/ppce500.c |   14 +++++++-------
> > hw/ppc/e500.c         |   12 +++++++-----
> > 2 files changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > f00793d..49bfcc6 100644
> > --- a/hw/pci-host/ppce500.c
> > +++ b/hw/pci-host/ppce500.c
> > @@ -87,7 +87,7 @@ struct PPCE500PCIState {
> >     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
> >     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> >     uint32_t gasket_time;
> > -    qemu_irq irq[4];
> > +    qemu_irq irq[PCI_NUM_PINS];
> >     uint32_t first_slot;
> >     /* mmio maps */
> >     MemoryRegion container;
> > @@ -252,26 +252,26 @@ static const MemoryRegionOps e500_pci_reg_ops = {
> >     .endianness = DEVICE_BIG_ENDIAN,
> > };
> >
> > -static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
> > +static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
> 
> This function converts pin -> irq, so that's fine.
> 
> > {
> >     int devno = pci_dev->devfn >> 3;
> >     int ret;
> >
> > -    ret = ppce500_pci_map_irq_slot(devno, irq_num);
> > +    ret = ppce500_pci_map_irq_slot(devno, pin);
> >
> >     pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
> > -           pci_dev->devfn, irq_num, ret, devno);
> > +           pci_dev->devfn, pin, ret, devno);
> >
> >     return ret;
> > }
> >
> > -static void mpc85xx_pci_set_irq(void *opaque, int irq_num, int level)
> > +static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
> 
> While this one ...
> 
> > {
> >     qemu_irq *pic = opaque;
> >
> > -    pci_debug("%s: PCI irq %d, level:%d\n", __func__, irq_num, level);
> > +    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> >
> > -    qemu_set_irq(pic[irq_num], level);
> > +    qemu_set_irq(pic[pin], level);
> 
> ... sets an actual irq number on the PIC, so this is not a pin.

pic[] is array of intA/B/C/B pin, no ?

Thanks
-Bharat

> 
> 
> The rest looks good to me :).
> 
> 
> Alex
> 
> 

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Bharat.Bhushan]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Thursday, December 19, 2013 3:55 AM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> 
> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> >
> > On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> >
> > > This patch adds pci pin to irq_num routing callback Without this
> > > patch we gets below warning
> > >
> > > "
> > >  PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > >  qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > > (e500-pcihost) "
> > >
> > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > > ---
> > > hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> > > 1 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > > 49bfcc6..3c4cf9e 100644
> > > --- a/hw/pci-host/ppce500.c
> > > +++ b/hw/pci-host/ppce500.c
> > > @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > >     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > >     uint32_t gasket_time;
> > >     qemu_irq irq[PCI_NUM_PINS];
> > > +    uint32_t irq_num[PCI_NUM_PINS];
> > >     uint32_t first_slot;
> > >     /* mmio maps */
> > >     MemoryRegion container;
> > > @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > > *pci_dev, int pin)
> > >
> > > static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > > -    qemu_irq *pic = opaque;
> > > +    PPCE500PCIState *s = opaque;
> > > +    qemu_irq *pic = s->irq;;
> >
> > Double semicolon?

Ok, will correct.

> >
> > >
> > >     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> > >
> > >     qemu_set_irq(pic[pin], level);
> > > }
> > >
> > > +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
> > > +pin) {
> > > +    PCIINTxRoute route;
> > > +    PPCE500PCIState *s = opaque;
> > > +
> > > +    route.mode = PCI_INTX_ENABLED;
> > > +    route.irq = s->irq_num[pin];
> > > +
> > > +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin,
> route.irq);
> > > +    return route;
> > > +}
> > > +
> > > static const VMStateDescription vmstate_pci_outbound = {
> > >     .name = "pci_outbound",
> > >     .version_id = 0,
> > > @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> > > *dev)
> > >
> > >     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > >         sysbus_init_irq(dev, &s->irq[i]);
> > > +        s->irq_num[i] = i + 1;
> >
> > Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? I don't
> understand the purpose of this whole exercise to be honest.
> >
> > Michael, could you please shed some light on this?
> >
> >
> > Alex
> 
> This is printed by pci_device_route_intx_to_irq - it's used by device assignment
> and vfio to figure out which irq does a given pci device drive.

Yes, exactly same reason.

Thanks
-Bharat

> 
> > >     }
> > >
> > >     memory_region_init(&s->pio, OBJECT(s), "pci-pio",
> > > PCIE500_PCI_IOLEN);
> > >
> > >     b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> > > -                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
> > > +                         mpc85xx_pci_map_irq, s, address_space_mem,
> > >                          &s->pio, PCI_DEVFN(s->first_slot, 0), 4,
> TYPE_PCI_BUS);
> > >     h->bus = b;
> > >
> > > @@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> > >     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
> > >     sysbus_init_mmio(dev, &s->container);
> > >     sysbus_init_mmio(dev, &s->pio);
> > > +    pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
> > >
> > >     return 0;
> > > }
> > > --
> > > 1.7.0.4
> > >
> > >
> 

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Michael S. Tsirkin]

On Thu, Dec 19, 2013 at 03:39:58PM +0000, Bharat.Bhushan@freescale.com wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Thursday, December 19, 2013 3:55 AM
> > To: Alexander Graf
> > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan
> > Bharat-R65777
> > Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> > 
> > On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> > >
> > > On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> > >
> > > > This patch adds pci pin to irq_num routing callback Without this
> > > > patch we gets below warning
> > > >
> > > > "
> > > >  PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > > >  qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > > > (e500-pcihost) "
> > > >
> > > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > > > ---
> > > > hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> > > > 1 files changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > > > 49bfcc6..3c4cf9e 100644
> > > > --- a/hw/pci-host/ppce500.c
> > > > +++ b/hw/pci-host/ppce500.c
> > > > @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > > >     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > > >     uint32_t gasket_time;
> > > >     qemu_irq irq[PCI_NUM_PINS];
> > > > +    uint32_t irq_num[PCI_NUM_PINS];
> > > >     uint32_t first_slot;
> > > >     /* mmio maps */
> > > >     MemoryRegion container;
> > > > @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > > > *pci_dev, int pin)
> > > >
> > > > static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > > > -    qemu_irq *pic = opaque;
> > > > +    PPCE500PCIState *s = opaque;
> > > > +    qemu_irq *pic = s->irq;;
> > >
> > > Double semicolon?
> 
> Ok, will correct.
> 
> > >
> > > >
> > > >     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> > > >
> > > >     qemu_set_irq(pic[pin], level);
> > > > }
> > > >
> > > > +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
> > > > +pin) {
> > > > +    PCIINTxRoute route;
> > > > +    PPCE500PCIState *s = opaque;
> > > > +
> > > > +    route.mode = PCI_INTX_ENABLED;
> > > > +    route.irq = s->irq_num[pin];
> > > > +
> > > > +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin,
> > route.irq);
> > > > +    return route;
> > > > +}
> > > > +
> > > > static const VMStateDescription vmstate_pci_outbound = {
> > > >     .name = "pci_outbound",
> > > >     .version_id = 0,
> > > > @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> > > > *dev)
> > > >
> > > >     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > > >         sysbus_init_irq(dev, &s->irq[i]);
> > > > +        s->irq_num[i] = i + 1;
> > >
> > > Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? I don't
> > understand the purpose of this whole exercise to be honest.
> > >
> > > Michael, could you please shed some light on this?
> > >
> > >
> > > Alex
> > 
> > This is printed by pci_device_route_intx_to_irq - it's used by device assignment
> > and vfio to figure out which irq does a given pci device drive.
> 
> Yes, exactly same reason.
> 
> Thanks
> -Bharat


you probably should say in commit log 
"this makes vfio work on ppc" - assumng it works for you of course.
as it is it makes it look like you are just trying to shut off
a warning.

> > 
> > > >     }
> > > >
> > > >     memory_region_init(&s->pio, OBJECT(s), "pci-pio",
> > > > PCIE500_PCI_IOLEN);
> > > >
> > > >     b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> > > > -                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
> > > > +                         mpc85xx_pci_map_irq, s, address_space_mem,
> > > >                          &s->pio, PCI_DEVFN(s->first_slot, 0), 4,
> > TYPE_PCI_BUS);
> > > >     h->bus = b;
> > > >
> > > > @@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> > > >     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
> > > >     sysbus_init_mmio(dev, &s->container);
> > > >     sysbus_init_mmio(dev, &s->pio);
> > > > +    pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
> > > >
> > > >     return 0;
> > > > }
> > > > --
> > > > 1.7.0.4
> > > >
> > > >
> > 

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Bharat.Bhushan]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Thursday, December 19, 2013 9:20 PM
> To: Bhushan Bharat-R65777
> Cc: Alexander Graf; Wood Scott-B07421; QEMU Developers; qemu-ppc
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> 
> On Thu, Dec 19, 2013 at 03:39:58PM +0000, Bharat.Bhushan@freescale.com wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > Sent: Thursday, December 19, 2013 3:55 AM
> > > To: Alexander Graf
> > > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers;
> > > qemu-ppc; Bhushan
> > > Bharat-R65777
> > > Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> > >
> > > On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> > > >
> > > > On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> > > >
> > > > > This patch adds pci pin to irq_num routing callback Without this
> > > > > patch we gets below warning
> > > > >
> > > > > "
> > > > >  PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > > > >  qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > > > > (e500-pcihost) "
> > > > >
> > > > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > > > > ---
> > > > > hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> > > > > 1 files changed, 18 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > > > > 49bfcc6..3c4cf9e 100644
> > > > > --- a/hw/pci-host/ppce500.c
> > > > > +++ b/hw/pci-host/ppce500.c
> > > > > @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > > > >     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > > > >     uint32_t gasket_time;
> > > > >     qemu_irq irq[PCI_NUM_PINS];
> > > > > +    uint32_t irq_num[PCI_NUM_PINS];
> > > > >     uint32_t first_slot;
> > > > >     /* mmio maps */
> > > > >     MemoryRegion container;
> > > > > @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > > > > *pci_dev, int pin)
> > > > >
> > > > > static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > > > > -    qemu_irq *pic = opaque;
> > > > > +    PPCE500PCIState *s = opaque;
> > > > > +    qemu_irq *pic = s->irq;;
> > > >
> > > > Double semicolon?
> >
> > Ok, will correct.
> >
> > > >
> > > > >
> > > > >     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin ,
> > > > > level);
> > > > >
> > > > >     qemu_set_irq(pic[pin], level); }
> > > > >
> > > > > +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque,
> > > > > +int
> > > > > +pin) {
> > > > > +    PCIINTxRoute route;
> > > > > +    PPCE500PCIState *s = opaque;
> > > > > +
> > > > > +    route.mode = PCI_INTX_ENABLED;
> > > > > +    route.irq = s->irq_num[pin];
> > > > > +
> > > > > +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__,
> > > > > + pin,
> > > route.irq);
> > > > > +    return route;
> > > > > +}
> > > > > +
> > > > > static const VMStateDescription vmstate_pci_outbound = {
> > > > >     .name = "pci_outbound",
> > > > >     .version_id = 0,
> > > > > @@ -350,12 +364,13 @@ static int
> > > > > e500_pcihost_initfn(SysBusDevice
> > > > > *dev)
> > > > >
> > > > >     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > > > >         sysbus_init_irq(dev, &s->irq[i]);
> > > > > +        s->irq_num[i] = i + 1;
> > > >
> > > > Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()?
> > > > I don't
> > > understand the purpose of this whole exercise to be honest.
> > > >
> > > > Michael, could you please shed some light on this?
> > > >
> > > >
> > > > Alex
> > >
> > > This is printed by pci_device_route_intx_to_irq - it's used by
> > > device assignment and vfio to figure out which irq does a given pci device
> drive.
> >
> > Yes, exactly same reason.
> >
> > Thanks
> > -Bharat
> 
> 
> you probably should say in commit log
> "this makes vfio work on ppc" - assumng it works for you of course.

Yes :)

> as it is it makes it look like you are just trying to shut off a warning.

Ok I will update the log.
vfio on powerpc is not yet upstream so I restrained myself from writing that.

Thanks
-Bharat

> 
> > >
> > > > >     }
> > > > >
> > > > >     memory_region_init(&s->pio, OBJECT(s), "pci-pio",
> > > > > PCIE500_PCI_IOLEN);
> > > > >
> > > > >     b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> > > > > -                         mpc85xx_pci_map_irq, s->irq,
> address_space_mem,
> > > > > +                         mpc85xx_pci_map_irq, s,
> > > > > + address_space_mem,
> > > > >                          &s->pio, PCI_DEVFN(s->first_slot, 0),
> > > > > 4,
> > > TYPE_PCI_BUS);
> > > > >     h->bus = b;
> > > > >
> > > > > @@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> > > > >     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s-
> >iomem);
> > > > >     sysbus_init_mmio(dev, &s->container);
> > > > >     sysbus_init_mmio(dev, &s->pio);
> > > > > +    pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
> > > > >
> > > > >     return 0;
> > > > > }
> > > > > --
> > > > > 1.7.0.4
> > > > >
> > > > >
> > >
> 

Re: [Qemu-devel] [PATCH 1/2] ppc-e500: some pci related cleanup

[Alexander Graf]

On 19.12.2013, at 16:38, Bharat.Bhushan@freescale.com wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, December 19, 2013 3:18 AM
>> To: Bhushan Bharat-R65777
>> Cc: Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan Bharat-R65777
>> Subject: Re: [PATCH 1/2] ppc-e500: some pci related cleanup
>> 
>> 
>> On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
>> 
>>> - Use PCI_NUM_PINS rather than hardcoding
>>> - use "pin" wherever possible
>> 
>> I assume you mean the PCI A/B/C/D pin with "pin".
> 
> Yes 
> 
>> 
>>> 
>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> ---
>>> hw/pci-host/ppce500.c |   14 +++++++-------
>>> hw/ppc/e500.c         |   12 +++++++-----
>>> 2 files changed, 14 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
>>> f00793d..49bfcc6 100644
>>> --- a/hw/pci-host/ppce500.c
>>> +++ b/hw/pci-host/ppce500.c
>>> @@ -87,7 +87,7 @@ struct PPCE500PCIState {
>>>    struct pci_outbound pob[PPCE500_PCI_NR_POBS];
>>>    struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>>>    uint32_t gasket_time;
>>> -    qemu_irq irq[4];
>>> +    qemu_irq irq[PCI_NUM_PINS];
>>>    uint32_t first_slot;
>>>    /* mmio maps */
>>>    MemoryRegion container;
>>> @@ -252,26 +252,26 @@ static const MemoryRegionOps e500_pci_reg_ops = {
>>>    .endianness = DEVICE_BIG_ENDIAN,
>>> };
>>> 
>>> -static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
>>> +static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
>> 
>> This function converts pin -> irq, so that's fine.
>> 
>>> {
>>>    int devno = pci_dev->devfn >> 3;
>>>    int ret;
>>> 
>>> -    ret = ppce500_pci_map_irq_slot(devno, irq_num);
>>> +    ret = ppce500_pci_map_irq_slot(devno, pin);
>>> 
>>>    pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
>>> -           pci_dev->devfn, irq_num, ret, devno);
>>> +           pci_dev->devfn, pin, ret, devno);
>>> 
>>>    return ret;
>>> }
>>> 
>>> -static void mpc85xx_pci_set_irq(void *opaque, int irq_num, int level)
>>> +static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
>> 
>> While this one ...
>> 
>>> {
>>>    qemu_irq *pic = opaque;
>>> 
>>> -    pci_debug("%s: PCI irq %d, level:%d\n", __func__, irq_num, level);
>>> +    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
>>> 
>>> -    qemu_set_irq(pic[irq_num], level);
>>> +    qemu_set_irq(pic[pin], level);
>> 
>> ... sets an actual irq number on the PIC, so this is not a pin.
> 
> pic[] is array of intA/B/C/B pin, no ?

Then we should also rename the variable :).


Alex

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Alexander Graf]

On 19.12.2013, at 16:39, Bharat.Bhushan@freescale.com wrote:

> 
> 
>> -----Original Message-----
>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
>> Sent: Thursday, December 19, 2013 3:55 AM
>> To: Alexander Graf
>> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
>> 
>> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
>>> 
>>> On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
>>> 
>>>> This patch adds pci pin to irq_num routing callback Without this
>>>> patch we gets below warning
>>>> 
>>>> "
>>>> PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
>>>> qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
>>>> (e500-pcihost) "
>>>> 
>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>> ---
>>>> hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
>>>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
>>>> 49bfcc6..3c4cf9e 100644
>>>> --- a/hw/pci-host/ppce500.c
>>>> +++ b/hw/pci-host/ppce500.c
>>>> @@ -88,6 +88,7 @@ struct PPCE500PCIState {
>>>>    struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>>>>    uint32_t gasket_time;
>>>>    qemu_irq irq[PCI_NUM_PINS];
>>>> +    uint32_t irq_num[PCI_NUM_PINS];
>>>>    uint32_t first_slot;
>>>>    /* mmio maps */
>>>>    MemoryRegion container;
>>>> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
>>>> *pci_dev, int pin)
>>>> 
>>>> static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
>>>> -    qemu_irq *pic = opaque;
>>>> +    PPCE500PCIState *s = opaque;
>>>> +    qemu_irq *pic = s->irq;;
>>> 
>>> Double semicolon?
> 
> Ok, will correct.
> 
>>> 
>>>> 
>>>>    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
>>>> 
>>>>    qemu_set_irq(pic[pin], level);
>>>> }
>>>> 
>>>> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
>>>> +pin) {
>>>> +    PCIINTxRoute route;
>>>> +    PPCE500PCIState *s = opaque;
>>>> +
>>>> +    route.mode = PCI_INTX_ENABLED;
>>>> +    route.irq = s->irq_num[pin];
>>>> +
>>>> +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin,
>> route.irq);
>>>> +    return route;
>>>> +}
>>>> +
>>>> static const VMStateDescription vmstate_pci_outbound = {
>>>>    .name = "pci_outbound",
>>>>    .version_id = 0,
>>>> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
>>>> *dev)
>>>> 
>>>>    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
>>>>        sysbus_init_irq(dev, &s->irq[i]);
>>>> +        s->irq_num[i] = i + 1;
>>> 
>>> Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? I don't
>> understand the purpose of this whole exercise to be honest.
>>> 
>>> Michael, could you please shed some light on this?
>>> 
>>> 
>>> Alex
>> 
>> This is printed by pci_device_route_intx_to_irq - it's used by device assignment
>> and vfio to figure out which irq does a given pci device drive.
> 
> Yes, exactly same reason.

Is there any way we could get rid of the information duplication? The fact that INTA/B/C/D are mapped to 1,2,3,4 is really a configuration parameter that should only live at a single spot.


Alex

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Michael S. Tsirkin]

On Thu, Dec 19, 2013 at 05:28:17PM +0100, Alexander Graf wrote:
> 
> On 19.12.2013, at 16:39, Bharat.Bhushan@freescale.com wrote:
> 
> > 
> > 
> >> -----Original Message-----
> >> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> >> Sent: Thursday, December 19, 2013 3:55 AM
> >> To: Alexander Graf
> >> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan
> >> Bharat-R65777
> >> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> >> 
> >> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> >>> 
> >>> On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> >>> 
> >>>> This patch adds pci pin to irq_num routing callback Without this
> >>>> patch we gets below warning
> >>>> 
> >>>> "
> >>>> PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> >>>> qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> >>>> (e500-pcihost) "
> >>>> 
> >>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>> ---
> >>>> hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> >>>> 1 files changed, 18 insertions(+), 2 deletions(-)
> >>>> 
> >>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> >>>> 49bfcc6..3c4cf9e 100644
> >>>> --- a/hw/pci-host/ppce500.c
> >>>> +++ b/hw/pci-host/ppce500.c
> >>>> @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> >>>>    struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> >>>>    uint32_t gasket_time;
> >>>>    qemu_irq irq[PCI_NUM_PINS];
> >>>> +    uint32_t irq_num[PCI_NUM_PINS];
> >>>>    uint32_t first_slot;
> >>>>    /* mmio maps */
> >>>>    MemoryRegion container;
> >>>> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> >>>> *pci_dev, int pin)
> >>>> 
> >>>> static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> >>>> -    qemu_irq *pic = opaque;
> >>>> +    PPCE500PCIState *s = opaque;
> >>>> +    qemu_irq *pic = s->irq;;
> >>> 
> >>> Double semicolon?
> > 
> > Ok, will correct.
> > 
> >>> 
> >>>> 
> >>>>    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> >>>> 
> >>>>    qemu_set_irq(pic[pin], level);
> >>>> }
> >>>> 
> >>>> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
> >>>> +pin) {
> >>>> +    PCIINTxRoute route;
> >>>> +    PPCE500PCIState *s = opaque;
> >>>> +
> >>>> +    route.mode = PCI_INTX_ENABLED;
> >>>> +    route.irq = s->irq_num[pin];
> >>>> +
> >>>> +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin,
> >> route.irq);
> >>>> +    return route;
> >>>> +}
> >>>> +
> >>>> static const VMStateDescription vmstate_pci_outbound = {
> >>>>    .name = "pci_outbound",
> >>>>    .version_id = 0,
> >>>> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> >>>> *dev)
> >>>> 
> >>>>    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> >>>>        sysbus_init_irq(dev, &s->irq[i]);
> >>>> +        s->irq_num[i] = i + 1;
> >>> 
> >>> Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? I don't
> >> understand the purpose of this whole exercise to be honest.
> >>> 
> >>> Michael, could you please shed some light on this?
> >>> 
> >>> 
> >>> Alex
> >> 
> >> This is printed by pci_device_route_intx_to_irq - it's used by device assignment
> >> and vfio to figure out which irq does a given pci device drive.
> > 
> > Yes, exactly same reason.
> 
> Is there any way we could get rid of the information duplication? The fact that INTA/B/C/D are mapped to 1,2,3,4 is really a configuration parameter that should only live at a single spot.
> 
> 
> Alex

Yes. In fact I had the idea to only have something like
pci_device_route_intx_to_irq and call it once for all interrupts
and cache that, then use this to send interrupts directly to apic.
Redo this each time routing changes.
I had a patch like this (and I think Jan had one too), but Anthony said
he'll rewrite all interrupt routing using QOM so I dropped it. I'll try
to resurrect it.

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Bharat.Bhushan]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Friday, December 20, 2013 12:02 AM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> 
> On Thu, Dec 19, 2013 at 05:28:17PM +0100, Alexander Graf wrote:
> >
> > On 19.12.2013, at 16:39, Bharat.Bhushan@freescale.com wrote:
> >
> > >
> > >
> > >> -----Original Message-----
> > >> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > >> Sent: Thursday, December 19, 2013 3:55 AM
> > >> To: Alexander Graf
> > >> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers;
> > >> qemu-ppc; Bhushan
> > >> Bharat-R65777
> > >> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> > >>
> > >> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> > >>>
> > >>> On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> > >>>
> > >>>> This patch adds pci pin to irq_num routing callback Without this
> > >>>> patch we gets below warning
> > >>>>
> > >>>> "
> > >>>> PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > >>>> qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > >>>> (e500-pcihost) "
> > >>>>
> > >>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > >>>> ---
> > >>>> hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> > >>>> 1 files changed, 18 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > >>>> 49bfcc6..3c4cf9e 100644
> > >>>> --- a/hw/pci-host/ppce500.c
> > >>>> +++ b/hw/pci-host/ppce500.c
> > >>>> @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > >>>>    struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > >>>>    uint32_t gasket_time;
> > >>>>    qemu_irq irq[PCI_NUM_PINS];
> > >>>> +    uint32_t irq_num[PCI_NUM_PINS];
> > >>>>    uint32_t first_slot;
> > >>>>    /* mmio maps */
> > >>>>    MemoryRegion container;
> > >>>> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > >>>> *pci_dev, int pin)
> > >>>>
> > >>>> static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > >>>> -    qemu_irq *pic = opaque;
> > >>>> +    PPCE500PCIState *s = opaque;
> > >>>> +    qemu_irq *pic = s->irq;;
> > >>>
> > >>> Double semicolon?
> > >
> > > Ok, will correct.
> > >
> > >>>
> > >>>>
> > >>>>    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin ,
> > >>>> level);
> > >>>>
> > >>>>    qemu_set_irq(pic[pin], level); }
> > >>>>
> > >>>> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
> > >>>> +pin) {
> > >>>> +    PCIINTxRoute route;
> > >>>> +    PPCE500PCIState *s = opaque;
> > >>>> +
> > >>>> +    route.mode = PCI_INTX_ENABLED;
> > >>>> +    route.irq = s->irq_num[pin];
> > >>>> +
> > >>>> +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__,
> > >>>> + pin,
> > >> route.irq);
> > >>>> +    return route;
> > >>>> +}
> > >>>> +
> > >>>> static const VMStateDescription vmstate_pci_outbound = {
> > >>>>    .name = "pci_outbound",
> > >>>>    .version_id = 0,
> > >>>> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> > >>>> *dev)
> > >>>>
> > >>>>    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > >>>>        sysbus_init_irq(dev, &s->irq[i]);
> > >>>> +        s->irq_num[i] = i + 1;
> > >>>
> > >>> Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()?
> > >>> I don't
> > >> understand the purpose of this whole exercise to be honest.
> > >>>
> > >>> Michael, could you please shed some light on this?
> > >>>
> > >>>
> > >>> Alex
> > >>
> > >> This is printed by pci_device_route_intx_to_irq - it's used by
> > >> device assignment and vfio to figure out which irq does a given pci device
> drive.
> > >
> > > Yes, exactly same reason.
> >
> > Is there any way we could get rid of the information duplication? The fact
> that INTA/B/C/D are mapped to 1,2,3,4 is really a configuration parameter that
> should only live at a single spot.
> >
> >
> > Alex
> 
> Yes. In fact I had the idea to only have something like
> pci_device_route_intx_to_irq and call it once for all interrupts and cache that,
> then use this to send interrupts directly to apic.
> Redo this each time routing changes.
> I had a patch like this (and I think Jan had one too), but Anthony said he'll
> rewrite all interrupt routing using QOM so I dropped it. I'll try to resurrect
> it.

So do we want to have this patch almost in this shape and hope Anthony's changes will handle this well or wait for Anthony patches first ?

Thanks
-Bharat

> 

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Michael S. Tsirkin]

On Fri, Dec 20, 2013 at 04:15:09AM +0000, Bharat.Bhushan@freescale.com wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Friday, December 20, 2013 12:02 AM
> > To: Alexander Graf
> > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc
> > Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> > 
> > On Thu, Dec 19, 2013 at 05:28:17PM +0100, Alexander Graf wrote:
> > >
> > > On 19.12.2013, at 16:39, Bharat.Bhushan@freescale.com wrote:
> > >
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > >> Sent: Thursday, December 19, 2013 3:55 AM
> > > >> To: Alexander Graf
> > > >> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers;
> > > >> qemu-ppc; Bhushan
> > > >> Bharat-R65777
> > > >> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> > > >>
> > > >> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> > > >>>
> > > >>> On 28.11.2013, at 07:35, Bharat Bhushan <r65777@freescale.com> wrote:
> > > >>>
> > > >>>> This patch adds pci pin to irq_num routing callback Without this
> > > >>>> patch we gets below warning
> > > >>>>
> > > >>>> "
> > > >>>> PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > > >>>> qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > > >>>> (e500-pcihost) "
> > > >>>>
> > > >>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > > >>>> ---
> > > >>>> hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
> > > >>>> 1 files changed, 18 insertions(+), 2 deletions(-)
> > > >>>>
> > > >>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > > >>>> 49bfcc6..3c4cf9e 100644
> > > >>>> --- a/hw/pci-host/ppce500.c
> > > >>>> +++ b/hw/pci-host/ppce500.c
> > > >>>> @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > > >>>>    struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > > >>>>    uint32_t gasket_time;
> > > >>>>    qemu_irq irq[PCI_NUM_PINS];
> > > >>>> +    uint32_t irq_num[PCI_NUM_PINS];
> > > >>>>    uint32_t first_slot;
> > > >>>>    /* mmio maps */
> > > >>>>    MemoryRegion container;
> > > >>>> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > > >>>> *pci_dev, int pin)
> > > >>>>
> > > >>>> static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > > >>>> -    qemu_irq *pic = opaque;
> > > >>>> +    PPCE500PCIState *s = opaque;
> > > >>>> +    qemu_irq *pic = s->irq;;
> > > >>>
> > > >>> Double semicolon?
> > > >
> > > > Ok, will correct.
> > > >
> > > >>>
> > > >>>>
> > > >>>>    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin ,
> > > >>>> level);
> > > >>>>
> > > >>>>    qemu_set_irq(pic[pin], level); }
> > > >>>>
> > > >>>> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
> > > >>>> +pin) {
> > > >>>> +    PCIINTxRoute route;
> > > >>>> +    PPCE500PCIState *s = opaque;
> > > >>>> +
> > > >>>> +    route.mode = PCI_INTX_ENABLED;
> > > >>>> +    route.irq = s->irq_num[pin];
> > > >>>> +
> > > >>>> +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__,
> > > >>>> + pin,
> > > >> route.irq);
> > > >>>> +    return route;
> > > >>>> +}
> > > >>>> +
> > > >>>> static const VMStateDescription vmstate_pci_outbound = {
> > > >>>>    .name = "pci_outbound",
> > > >>>>    .version_id = 0,
> > > >>>> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> > > >>>> *dev)
> > > >>>>
> > > >>>>    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > > >>>>        sysbus_init_irq(dev, &s->irq[i]);
> > > >>>> +        s->irq_num[i] = i + 1;
> > > >>>
> > > >>> Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()?
> > > >>> I don't
> > > >> understand the purpose of this whole exercise to be honest.
> > > >>>
> > > >>> Michael, could you please shed some light on this?
> > > >>>
> > > >>>
> > > >>> Alex
> > > >>
> > > >> This is printed by pci_device_route_intx_to_irq - it's used by
> > > >> device assignment and vfio to figure out which irq does a given pci device
> > drive.
> > > >
> > > > Yes, exactly same reason.
> > >
> > > Is there any way we could get rid of the information duplication? The fact
> > that INTA/B/C/D are mapped to 1,2,3,4 is really a configuration parameter that
> > should only live at a single spot.
> > >
> > >
> > > Alex
> > 
> > Yes. In fact I had the idea to only have something like
> > pci_device_route_intx_to_irq and call it once for all interrupts and cache that,
> > then use this to send interrupts directly to apic.
> > Redo this each time routing changes.
> > I had a patch like this (and I think Jan had one too), but Anthony said he'll
> > rewrite all interrupt routing using QOM so I dropped it. I'll try to resurrect
> > it.
> 
> So do we want to have this patch almost in this shape and hope Anthony's changes will handle this well or wait for Anthony patches first ?
> 
> Thanks
> -Bharat

I think your patch is the right thing to do ATM.


> > 

Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing

[Michael S. Tsirkin]

On Thu, Nov 28, 2013 at 12:05:33PM +0530, Bharat Bhushan wrote:
> This patch adds pci pin to irq_num routing callback
> Without this patch we gets below warning
> 
> "
>   PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
>   qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> "
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>


Please tweak commit log to explain thereal motivation for the change
which is to use vfio on ppc.
Besides that

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  hw/pci-host/ppce500.c |   20 ++++++++++++++++++--
>  1 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index 49bfcc6..3c4cf9e 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -88,6 +88,7 @@ struct PPCE500PCIState {
>      struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>      uint32_t gasket_time;
>      qemu_irq irq[PCI_NUM_PINS];
> +    uint32_t irq_num[PCI_NUM_PINS];
>      uint32_t first_slot;
>      /* mmio maps */
>      MemoryRegion container;
> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
>  
>  static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
>  {
> -    qemu_irq *pic = opaque;
> +    PPCE500PCIState *s = opaque;
> +    qemu_irq *pic = s->irq;;
>  
>      pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
>  
>      qemu_set_irq(pic[pin], level);
>  }
>  
> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int pin)
> +{
> +    PCIINTxRoute route;
> +    PPCE500PCIState *s = opaque;
> +
> +    route.mode = PCI_INTX_ENABLED;
> +    route.irq = s->irq_num[pin];
> +
> +    pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin, route.irq);
> +    return route;
> +}
> +
>  static const VMStateDescription vmstate_pci_outbound = {
>      .name = "pci_outbound",
>      .version_id = 0,
> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>  
>      for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
>          sysbus_init_irq(dev, &s->irq[i]);
> +        s->irq_num[i] = i + 1;
>      }
>  
>      memory_region_init(&s->pio, OBJECT(s), "pci-pio", PCIE500_PCI_IOLEN);
>  
>      b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> -                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
> +                         mpc85xx_pci_map_irq, s, address_space_mem,
>                           &s->pio, PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
>      h->bus = b;
>  
> @@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>      memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
>      sysbus_init_mmio(dev, &s->container);
>      sysbus_init_mmio(dev, &s->pio);
> +    pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
>  
>      return 0;
>  }
> -- 
> 1.7.0.4
> 
> 

[Qemu-devel] [PATCH 0/2] ppc-e500: Adding pci-pin to irq callback and some cleanup

[Bharat Bhushan]

First patch is doing some cleanup and second patch adds the PCI-PIN (INT[A-D])
to its interrupt number mapping callback. This is required to support pci device
passthrough using VFIO.

Bharat Bhushan (2):
  ppc-e500: some pci related cleanup
  ppc-e500: implement PCI INTx routing

 hw/pci-host/ppce500.c |   34 +++++++++++++++++++++++++---------
 hw/ppc/e500.c         |   12 +++++++-----
 2 files changed, 32 insertions(+), 14 deletions(-)