[v2] arm: Add first models of Xilinx Versal SoC

[Qemu-devel] [PATCH v2 02/12] net: cadence_gem: Announce availability of priority queues

Posted by Edgar E. Iglesias 7 years ago

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Announce the availability of the various priority queues.
This fixes an issue where guest kernels would miss to
configure secondary queues due to inproper feature bits.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index e560b7a142..901c173970 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1213,6 +1213,7 @@ static void gem_reset(DeviceState *d)
     int i;
     CadenceGEMState *s = CADENCE_GEM(d);
     const uint8_t *a;
+    uint32_t queues_mask;
 
     DB_PRINT("\n");
 
@@ -1229,7 +1230,10 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_DESCONF] = 0x02500111;
     s->regs[GEM_DESCONF2] = 0x2ab13fff;
     s->regs[GEM_DESCONF5] = 0x002f2045;
-    s->regs[GEM_DESCONF6] = 0x00000200;
+    s->regs[GEM_DESCONF6] = 0x0;
+
+    queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
+    s->regs[GEM_DESCONF6] |= queues_mask;
 
     /* Set MAC address */
     a = &s->conf.macaddr.a[0];
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 02/12] net: cadence_gem: Announce availability of priority queues

Posted by Peter Maydell 7 years ago

On 11 October 2018 at 03:19, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Announce the availability of the various priority queues.
> This fixes an issue where guest kernels would miss to
> configure secondary queues due to inproper feature bits.
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index e560b7a142..901c173970 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1213,6 +1213,7 @@ static void gem_reset(DeviceState *d)
>      int i;
>      CadenceGEMState *s = CADENCE_GEM(d);
>      const uint8_t *a;
> +    uint32_t queues_mask;
>
>      DB_PRINT("\n");
>
> @@ -1229,7 +1230,10 @@ static void gem_reset(DeviceState *d)
>      s->regs[GEM_DESCONF] = 0x02500111;
>      s->regs[GEM_DESCONF2] = 0x2ab13fff;
>      s->regs[GEM_DESCONF5] = 0x002f2045;
> -    s->regs[GEM_DESCONF6] = 0x00000200;
> +    s->regs[GEM_DESCONF6] = 0x0;
> +
> +    queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
> +    s->regs[GEM_DESCONF6] |= queues_mask;

This causes the clang runtime sanitizer to complain:
  /aarch64/qom/xilinx-zynq-a9:
/home/petmay01/linaro/qemu-for-merges/hw/net/cadence_gem.c:1306:19:
runtime error: shift exponent 64 is too large for 64-bit type
'unsigned long long'

because s->num_priority_queues is 1, and so you've
asked for a zero-width mask, which MAKE_64BIT_MASK
doesn't permit. What was the intention of the code here?
I don't think any value of num_priority_queues can
produce the old 0x200 value for this ID register.

I'm going to drop this patch and "net: cadence_gem:
Announce 64bit addressing support" (which depends
on this one) from my pullreq.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 02/12] net: cadence_gem: Announce availability of priority queues

Posted by Edgar E. Iglesias 7 years ago

On Tue, Oct 16, 2018 at 05:15:36PM +0100, Peter Maydell wrote:
> On 11 October 2018 at 03:19, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Announce the availability of the various priority queues.
> > This fixes an issue where guest kernels would miss to
> > configure secondary queues due to inproper feature bits.
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  hw/net/cadence_gem.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index e560b7a142..901c173970 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -1213,6 +1213,7 @@ static void gem_reset(DeviceState *d)
> >      int i;
> >      CadenceGEMState *s = CADENCE_GEM(d);
> >      const uint8_t *a;
> > +    uint32_t queues_mask;
> >
> >      DB_PRINT("\n");
> >
> > @@ -1229,7 +1230,10 @@ static void gem_reset(DeviceState *d)
> >      s->regs[GEM_DESCONF] = 0x02500111;
> >      s->regs[GEM_DESCONF2] = 0x2ab13fff;
> >      s->regs[GEM_DESCONF5] = 0x002f2045;
> > -    s->regs[GEM_DESCONF6] = 0x00000200;
> > +    s->regs[GEM_DESCONF6] = 0x0;
> > +
> > +    queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
> > +    s->regs[GEM_DESCONF6] |= queues_mask;
> 
> This causes the clang runtime sanitizer to complain:
>   /aarch64/qom/xilinx-zynq-a9:
> /home/petmay01/linaro/qemu-for-merges/hw/net/cadence_gem.c:1306:19:
> runtime error: shift exponent 64 is too large for 64-bit type
> 'unsigned long long'

Argh, my bad.


> 
> because s->num_priority_queues is 1, and so you've
> asked for a zero-width mask, which MAKE_64BIT_MASK
> doesn't permit. What was the intention of the code here?


The intention was a mask with zero bits set when only 1 queue is available.

DESCONF6 has a bit set per queue that is available,
except for queue 0, for which bit 0 is always 0.

For example configuration with 1 queue:
..000000

2 queues:
..000010

3 queues:
..000110

4 queues:
..001110

etc.

I'll post a v3 with this fixed.


> I don't think any value of num_priority_queues can
> produce the old 0x200 value for this ID register.

Right, the 0x200 value was bogus.

> 
> I'm going to drop this patch and "net: cadence_gem:
> Announce 64bit addressing support" (which depends
> on this one) from my pullreq.


Best regards,
Edgar