[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

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

[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 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 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 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(-)

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

[Bharat Bhushan]

This patch adds pci pin to irq_num routing callback.
This callback is called from pci_device_route_intx_to_irq to find which pci device
maps to which irq. This is used for pci-device passthrough using vfio.

Also 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)
"
and Legacy interrupt does not work with pci device passthrough.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
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 71e5ca9..ffea782 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 *irq = opaque;
+    PPCE500PCIState *s = opaque;
+    qemu_irq *pic = s->irq;
 
     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
 
     qemu_set_irq(irq[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 2/2] ppc-e500: implement PCI INTx routing

[Alexander Graf]

On 20.12.2013, at 10:42, Bharat Bhushan <r65777@freescale.com> wrote:

> This patch adds pci pin to irq_num routing callback.
> This callback is called from pci_device_route_intx_to_irq to find which pci device
> maps to which irq. This is used for pci-device passthrough using vfio.
> 
> Also 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)
> "
> and Legacy interrupt does not work with pci device passthrough.
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> 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 71e5ca9..ffea782 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 *irq = opaque;
> +    PPCE500PCIState *s = opaque;
> +    qemu_irq *pic = s->irq;
> 
>     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> 
>     qemu_set_irq(irq[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;

I still don't like how you duplicate the logic which MPIC irq line is associated with which pci host controller irq line. Please pass this information into the pcihost object somehow from ppce500_init() so that it's only at a single spot.

If you like, you can just use qom/qdev properties for that, but it needs to be configured from the outside.


Alex

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Friday, December 20, 2013 4:01 PM
> To: Bhushan Bharat-R65777
> Cc: Michael S. Tsirkin; Wood Scott-B07421; qemu-ppc; QEMU Developers; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> 
> 
> On 20.12.2013, at 10:42, Bharat Bhushan <r65777@freescale.com> wrote:
> 
> > This patch adds pci pin to irq_num routing callback.
> > This callback is called from pci_device_route_intx_to_irq to find
> > which pci device maps to which irq. This is used for pci-device passthrough
> using vfio.
> >
> > Also 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) "
> > and Legacy interrupt does not work with pci device passthrough.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > 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
> > 71e5ca9..ffea782 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 *irq = opaque;
> > +    PPCE500PCIState *s = opaque;
> > +    qemu_irq *pic = s->irq;
> >
> >     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> >
> >     qemu_set_irq(irq[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;
> 
> I still don't like how you duplicate the logic which MPIC irq line is associated
> with which pci host controller irq line. Please pass this information into the
> pcihost object somehow from ppce500_init() so that it's only at a single spot.
> 
> If you like, you can just use qom/qdev properties for that, but it needs to be
> configured from the outside.

Ok :)

I thought we guys agreed that let this patch come as is and Anthony's work will help globally.

Thanks
-Bharat

> 
> 
> Alex
> 
> 

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

[Michael S. Tsirkin]

On Fri, Dec 20, 2013 at 11:31:17AM +0100, Alexander Graf wrote:
> 
> On 20.12.2013, at 10:42, Bharat Bhushan <r65777@freescale.com> wrote:
> 
> > This patch adds pci pin to irq_num routing callback.
> > This callback is called from pci_device_route_intx_to_irq to find which pci device
> > maps to which irq. This is used for pci-device passthrough using vfio.
> > 
> > Also 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)
> > "
> > and Legacy interrupt does not work with pci device passthrough.
> > 
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > 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 71e5ca9..ffea782 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 *irq = opaque;
> > +    PPCE500PCIState *s = opaque;
> > +    qemu_irq *pic = s->irq;
> > 
> >     pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> > 
> >     qemu_set_irq(irq[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;
> 
> I still don't like how you duplicate the logic which MPIC irq line is associated with which pci host controller irq line. Please pass this information into the pcihost object somehow from ppce500_init() so that it's only at a single spot.
> 
> If you like, you can just use qom/qdev properties for that, but it needs to be configured from the outside.
> 
> 
> Alex


Maybe just call ppce500_pci_map_irq_slot instead of using irq_num?

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

[Alexander Graf]

> Am 22.12.2013 um 12:31 schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> 
>> On Fri, Dec 20, 2013 at 11:31:17AM +0100, Alexander Graf wrote:
>> 
>>> On 20.12.2013, at 10:42, Bharat Bhushan <r65777@freescale.com> wrote:
>>> 
>>> This patch adds pci pin to irq_num routing callback.
>>> This callback is called from pci_device_route_intx_to_irq to find which pci device
>>> maps to which irq. This is used for pci-device passthrough using vfio.
>>> 
>>> Also 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)
>>> "
>>> and Legacy interrupt does not work with pci device passthrough.
>>> 
>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> 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 71e5ca9..ffea782 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 *irq = opaque;
>>> +    PPCE500PCIState *s = opaque;
>>> +    qemu_irq *pic = s->irq;
>>> 
>>>    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
>>> 
>>>    qemu_set_irq(irq[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;
>> 
>> I still don't like how you duplicate the logic which MPIC irq line is associated with which pci host controller irq line. Please pass this information into the pcihost object somehow from ppce500_init() so that it's only at a single spot.
>> 
>> If you like, you can just use qom/qdev properties for that, but it needs to be configured from the outside.
>> 
>> 
>> Alex
> 
> 
> Maybe just call ppce500_pci_map_irq_slot instead of using irq_num?

That'd be a step into the wrong direction. Wiring is a configuration thing.

Alex

> 
>