Re: [Qemu-devel] [PATCH v4 3/4] target-arm: Extend the gic node properties

Re: [Qemu-devel] [PATCH v4 3/4] target-arm: Extend the gic node properties

[Claudio Fontana]

Hello Christoffer,

just one question:

> In preparation for adding the GICv2m which requires address specifiers
> and is a subnode of the gic, we extend the gic DT definition to specify
> the #address-cells and #size-cells properties and add an empty ranges
> property properties of the DT node, since this is required to add the
> v2m node as a child of the gic node.
>
> Note that we must also expand the irq-map to reference the gic with the
> right address-cells as a consequence of this change.
>
> Reviewed-by: Eric Auger <address@hidden>
> Suggested-by: Shanker Donthineni <address@hidden>
> Signed-off-by: Christoffer Dall <address@hidden>
> ---
> Changes since v3:
>  - Rewrote patch and changed authorship and tags accordingly
>  - Fixed spelling in commit message
> Changes since v2:
>  - New separate patch factoring out changes to existing code for eased
>    bisectability in case we broke something
>  - The above fixes the issue with non-MSI compatible guests.
>
>  hw/arm/virt.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e5235ef..387dac8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -317,6 +317,9 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
>                                       2, vbi->memmap[VIRT_GIC_DIST].size,
>                                       2, vbi->memmap[VIRT_GIC_CPU].base,
>                                       2, vbi->memmap[VIRT_GIC_CPU].size);
> +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
> +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
> +    qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
>  }
>  
> @@ -585,7 +588,7 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, 
> uint32_t gic_phandle,
>                                  int first_irq, const char *nodename)
>  {
>      int devfn, pin;
> -    uint32_t full_irq_map[4 * 4 * 8] = { 0 };
> +    uint32_t full_irq_map[4 * 4 * 10] = { 0 };
>      uint32_t *irq_map = full_irq_map;
>  
>      for (devfn = 0; devfn <= 0x18; devfn += 0x8) {
> @@ -598,13 +601,13 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, 
> uint32_t gic_phandle,
>              uint32_t map[] = {
>                  devfn << 8, 0, 0,                           /* devfn */
>                  pin + 1,                                    /* PCI pin */
> -                gic_phandle, irq_type, irq_nr, irq_level }; /* GIC irq */
> +                gic_phandle, 0, 0, irq_type, irq_nr, irq_level }; /* GIC irq */

can you help me understand how to decode this?
We are adding two 32bit "0" values in the map.
So now gic_phandle field occupies a total of 3 x 32bit values.
Is this really what was intended?

If the #address-cell and #size-cells of the intc are 2 and 2 respectively,
shouldn't this be

gic_phandle, 0, irq_type, irq_nr, irq_level ?

Too much time since I read this code I guess..

Thanks!

CLaudio

>  
>              /* Convert map to big endian */
> -            for (i = 0; i < 8; i++) {
> +            for (i = 0; i < 10; i++) {
>                  irq_map[i] = cpu_to_be32(map[i]);
>              }
> -            irq_map += 8;
> +            irq_map += 10;
>          }
>      }
>  
> -- 
> 2.1.2.330.g565301e.dirty
>

Re: [Qemu-devel] [PATCH v4 3/4] target-arm: Extend the gic node properties

[Christoffer Dall]

[whoops, re-adding qemu-devel]

On Thu, Jul 9, 2015 at 5:07 PM, Alexander Graf <agraf@suse.de> wrote:
> On 07/09/15 16:44, Christoffer Dall wrote:
>>
>> On Thu, Jul 09, 2015 at 04:02:14PM +0200, Claudio Fontana wrote:
>>>
>>> Hello Christoffer,
>>>
>>> just one question:
>>>
>>>> In preparation for adding the GICv2m which requires address specifiers
>>>> and is a subnode of the gic, we extend the gic DT definition to specify
>>>> the #address-cells and #size-cells properties and add an empty ranges
>>>> property properties of the DT node, since this is required to add the
>>>> v2m node as a child of the gic node.
>>>>
>>>> Note that we must also expand the irq-map to reference the gic with the
>>>> right address-cells as a consequence of this change.
>>>>
>>>> Reviewed-by: Eric Auger <address@hidden>
>>>> Suggested-by: Shanker Donthineni <address@hidden>
>>>> Signed-off-by: Christoffer Dall <address@hidden>
>>>> ---
>>>> Changes since v3:
>>>>   - Rewrote patch and changed authorship and tags accordingly
>>>>   - Fixed spelling in commit message
>>>> Changes since v2:
>>>>   - New separate patch factoring out changes to existing code for eased
>>>>     bisectability in case we broke something
>>>>   - The above fixes the issue with non-MSI compatible guests.
>>>>
>>>>   hw/arm/virt.c | 11 +++++++----
>>>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index e5235ef..387dac8 100644
>>>> --- a/hw/arm/virt.c
>>>> +++ b/hw/arm/virt.c
>>>> @@ -317,6 +317,9 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
>>>>                                        2,
>>>> vbi->memmap[VIRT_GIC_DIST].size,
>>>>                                        2,
>>>> vbi->memmap[VIRT_GIC_CPU].base,
>>>>                                        2,
>>>> vbi->memmap[VIRT_GIC_CPU].size);
>>>> +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
>>>> +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
>>>> +    qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
>>>>       qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle",
>>>> vbi->gic_phandle);
>>>>   }
>>>>   @@ -585,7 +588,7 @@ static void create_pcie_irq_map(const
>>>> VirtBoardInfo *vbi,
>>>> uint32_t gic_phandle,
>>>>                                   int first_irq, const char *nodename)
>>>>   {
>>>>       int devfn, pin;
>>>> -    uint32_t full_irq_map[4 * 4 * 8] = { 0 };
>>>> +    uint32_t full_irq_map[4 * 4 * 10] = { 0 };
>>>>       uint32_t *irq_map = full_irq_map;
>>>>         for (devfn = 0; devfn <= 0x18; devfn += 0x8) {
>>>> @@ -598,13 +601,13 @@ static void create_pcie_irq_map(const
>>>> VirtBoardInfo *vbi,
>>>> uint32_t gic_phandle,
>>>>               uint32_t map[] = {
>>>>                   devfn << 8, 0, 0,                           /* devfn
>>>> */
>>>>                   pin + 1,                                    /* PCI pin
>>>> */
>>>> -                gic_phandle, irq_type, irq_nr, irq_level }; /* GIC irq
>>>> */
>>>> +                gic_phandle, 0, 0, irq_type, irq_nr, irq_level }; /*
>>>> GIC irq */
>>>
>>> can you help me understand how to decode this?
>>> We are adding two 32bit "0" values in the map.
>>> So now gic_phandle field occupies a total of 3 x 32bit values.
>>> Is this really what was intended?
>>>
>>> If the #address-cell and #size-cells of the intc are 2 and 2
>>> respectively,
>>> shouldn't this be
>>>
>>> gic_phandle, 0, irq_type, irq_nr, irq_level ?
>>>
>>> Too much time since I read this code I guess..
>>>
>> I'll be honest and say that I don't fully understand the details of the
>> interrupt-map specification, and I cannot seem to find it online either
>> (the working link I had before gives me a 404 these days).
>>
>> So I followed the suggestion from Shanker and it worked with adding two
>> zeroes, not with only adding one, so I figured it was the correct thing
>> to do.
>>
>> Perhaps Alex Graf who wrote the original code to generate the interrupt
>> map can help?
>
>
> The page works quite well for me:
>
>   http://devicetree.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

It explains it conceptually, yes, but it's hardly a spec.  At least I
can't understand from that page why the entries in the map have to be
changed based on size-cells and address-cells in the interrupt
controller...

I didn't think the bits we needed to add were related to the phandle;
I always thought a phandle was just an internal to the DT 32-bit
number to refer to a different node.

>
> When it comes to dt bits where I'm uncertain, I usually end up asking Rob
> though :).
>

Rob, can you help us out here?  Beers are on me at SFO15 ;)

-Christoffer

Re: [Qemu-devel] [PATCH v4 3/4] target-arm: Extend the gic node properties

[Peter Maydell]

On 9 July 2015 at 17:03, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> [whoops, re-adding qemu-devel]
>
> On Thu, Jul 9, 2015 at 5:07 PM, Alexander Graf <agraf@suse.de> wrote:
>> On 07/09/15 16:44, Christoffer Dall wrote:
>>> I'll be honest and say that I don't fully understand the details of the
>>> interrupt-map specification, and I cannot seem to find it online either
>>> (the working link I had before gives me a 404 these days).

Try http://www.firmware.org/1275/practice/imap/imap0_9d.pdf

> It explains it conceptually, yes, but it's hardly a spec.  At least I
> can't understand from that page why the entries in the map have to be
> changed based on size-cells and address-cells in the interrupt
> controller...

The interrupt map entries are:
 * child unit interrupt specifier [4 cells for PCI, determined
   by #address-cells + #interrupt-cells for the PCI controller node]
 * interrupt parent phandle
 * parent unit interrupt specifier [number of cells determined
   by #address-cells + #interrupt-cells for the interrupt controller]

So the extra two zeroes aren't part of the phandle, they're
the result of the parent unit-interrupt-specifier now being
5 cells because of #address-cells being defined in the GIC node.

(https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/gic.txt
is unfortunately not clear about what the extra two cells
in the unit-interrupt-specifier are actually for.)

> I didn't think the bits we needed to add were related to the phandle;
> I always thought a phandle was just an internal to the DT 32-bit
> number to refer to a different node.

Yep.

-- PMM

Re: [Qemu-devel] [PATCH v4 3/4] target-arm: Extend the gic node properties

[Christoffer Dall]

On Thu, Jul 9, 2015 at 6:28 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 July 2015 at 17:03, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> [whoops, re-adding qemu-devel]
>>
>> On Thu, Jul 9, 2015 at 5:07 PM, Alexander Graf <agraf@suse.de> wrote:
>>> On 07/09/15 16:44, Christoffer Dall wrote:
>>>> I'll be honest and say that I don't fully understand the details of the
>>>> interrupt-map specification, and I cannot seem to find it online either
>>>> (the working link I had before gives me a 404 these days).
>
> Try http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
>

that's the one, thanks for the working url.

>> It explains it conceptually, yes, but it's hardly a spec.  At least I
>> can't understand from that page why the entries in the map have to be
>> changed based on size-cells and address-cells in the interrupt
>> controller...
>
> The interrupt map entries are:
>  * child unit interrupt specifier [4 cells for PCI, determined
>    by #address-cells + #interrupt-cells for the PCI controller node]
>  * interrupt parent phandle
>  * parent unit interrupt specifier [number of cells determined
>    by #address-cells + #interrupt-cells for the interrupt controller]
>
> So the extra two zeroes aren't part of the phandle, they're
> the result of the parent unit-interrupt-specifier now being
> 5 cells because of #address-cells being defined in the GIC node.

Thanks for clearing this up!

I owe you a cup of your favorite poison then:)

-Christoffer

[Qemu-devel] [PATCH v4 0/4] Add support for for GICv2m and MSIs to arm-virt

[Christoffer Dall]

Now when we have a host generic PCIe controller in the virt board, it
would be nice to be able to use MSIs so that we can eventually enable
VHOST with KVM.

With these patches you can use MSIs with TCG and with KVM, but you still
need some fixes for the mapping of the IRQ index to the GSI number for
IRQFD to work.  A separate series that enables IRQFD and vhost
is available: "ARM adaptations for vhost irqfd setup"
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01054.html)

Tested with KVM on XGene and with TCG by configuring a virtio-pci
network adapter for the guest and verifying MSIs going through as
expected.

Rebased on target-arm.next, see the individual patches for detailed
changelogs.

Christoffer Dall (4):
  target-arm: Add GIC phandle to VirtBoardInfo
  arm_gicv2m: Add GICv2m widget to support MSIs
  target-arm: Extend the gic node properties
  target-arm: Add the GICv2m to the virt board

 hw/arm/virt.c         |  73 ++++++++++++++-----
 hw/intc/Makefile.objs |   1 +
 hw/intc/arm_gicv2m.c  | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |   2 +
 4 files changed, 248 insertions(+), 18 deletions(-)
 create mode 100644 hw/intc/arm_gicv2m.c

-- 
2.1.2.330.g565301e.dirty

[Qemu-devel] [PATCH v4 3/4] target-arm: Extend the gic node properties

[Christoffer Dall]

In preparation for adding the GICv2m which requires address specifiers
and is a subnode of the gic, we extend the gic DT definition to specify
the #address-cells and #size-cells properties and add an empty ranges
property properties of the DT node, since this is required to add the
v2m node as a child of the gic node.

Note that we must also expand the irq-map to reference the gic with the
right address-cells as a consequence of this change.

Reviewed-by: Eric Auger <eric.auger@linaro.org>
Suggested-by: Shanker Donthineni <shankerd@codeaurora.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes since v3:
 - Rewrote patch and changed authorship and tags accordingly
 - Fixed spelling in commit message
Changes since v2:
 - New separate patch factoring out changes to existing code for eased
   bisectability in case we broke something
 - The above fixes the issue with non-MSI compatible guests.

 hw/arm/virt.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e5235ef..387dac8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -317,6 +317,9 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
                                      2, vbi->memmap[VIRT_GIC_DIST].size,
                                      2, vbi->memmap[VIRT_GIC_CPU].base,
                                      2, vbi->memmap[VIRT_GIC_CPU].size);
+    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
+    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
+    qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
 }
 
@@ -585,7 +588,7 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
                                 int first_irq, const char *nodename)
 {
     int devfn, pin;
-    uint32_t full_irq_map[4 * 4 * 8] = { 0 };
+    uint32_t full_irq_map[4 * 4 * 10] = { 0 };
     uint32_t *irq_map = full_irq_map;
 
     for (devfn = 0; devfn <= 0x18; devfn += 0x8) {
@@ -598,13 +601,13 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
             uint32_t map[] = {
                 devfn << 8, 0, 0,                           /* devfn */
                 pin + 1,                                    /* PCI pin */
-                gic_phandle, irq_type, irq_nr, irq_level }; /* GIC irq */
+                gic_phandle, 0, 0, irq_type, irq_nr, irq_level }; /* GIC irq */
 
             /* Convert map to big endian */
-            for (i = 0; i < 8; i++) {
+            for (i = 0; i < 10; i++) {
                 irq_map[i] = cpu_to_be32(map[i]);
             }
-            irq_map += 8;
+            irq_map += 10;
         }
     }
 
-- 
2.1.2.330.g565301e.dirty