[Qemu-devel] [PATCH 0/2] two fixes for KVM GICv3 dist get/put functions

[Qemu-devel] [PATCH 0/2] two fixes for KVM GICv3 dist get/put functions

[Shannon Zhao]

Shannon Zhao (2):
  arm_gicv3_kvm: increase clroffset accordingly
  arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR

 hw/intc/arm_gicv3_kvm.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

-- 
2.0.4

[Qemu-devel] [PATCH 1/2] arm_gicv3_kvm: increase clroffset accordingly

[Shannon Zhao]

It forgot to increase clroffset during the loop. So it only clear the
first 4 bytes.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
---
 hw/intc/arm_gicv3_kvm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index ec37177..7000716 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -232,9 +232,10 @@ static void kvm_dist_getbmp(GICv3State *s, uint32_t offset, uint32_t *bmp)
 static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
                             uint32_t clroffset, uint32_t *bmp)
 {
-    uint32_t reg;
+    uint32_t reg, clroffset_index;
     int irq;
 
+    clroffset_index = clroffset;
     for_each_dist_irq_reg(irq, s->num_irq, 1) {
         /* If this bitmap is a set/clear register pair, first write to the
          * clear-reg to clear all bits before using the set-reg to write
@@ -242,7 +243,8 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
          */
         if (clroffset != 0) {
             reg = 0;
-            kvm_gicd_access(s, clroffset, &reg, true);
+            kvm_gicd_access(s, clroffset_index, &reg, true);
+            clroffset_index += 4;
         }
         reg = *gic_bmp_ptr32(bmp, irq);
         kvm_gicd_access(s, offset, &reg, true);
-- 
2.0.4

[Qemu-devel] [PATCH 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR

[Shannon Zhao]

It should skip to getting/putting the registers banked by GICR so that
it could get/put the correct ones.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
---
 hw/intc/arm_gicv3_kvm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 7000716..e967e4f 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -136,6 +136,8 @@ static void kvm_dist_get_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
     int irq;
 
     field = (uint32_t *)bmp;
+    /* The first 8 GICD_IPRIORITYR should skip. */
+    offset += (8 * sizeof(uint32_t));
     for_each_dist_irq_reg(irq, s->num_irq, 8) {
         kvm_gicd_access(s, offset, &reg, false);
         *field = reg;
@@ -150,6 +152,8 @@ static void kvm_dist_put_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
     int irq;
 
     field = (uint32_t *)bmp;
+    /* The first 8 GICD_IPRIORITYR should skip. */
+    offset += (8 * sizeof(uint32_t));
     for_each_dist_irq_reg(irq, s->num_irq, 8) {
         reg = *field;
         kvm_gicd_access(s, offset, &reg, true);
@@ -164,6 +168,8 @@ static void kvm_dist_get_edge_trigger(GICv3State *s, uint32_t offset,
     uint32_t reg;
     int irq;
 
+    /* The first 2 GICD_ICFGR should skip. */
+    offset += (2 * sizeof(uint32_t));
     for_each_dist_irq_reg(irq, s->num_irq, 2) {
         kvm_gicd_access(s, offset, &reg, false);
         reg = half_unshuffle32(reg >> 1);
@@ -181,6 +187,8 @@ static void kvm_dist_put_edge_trigger(GICv3State *s, uint32_t offset,
     uint32_t reg;
     int irq;
 
+    /* The first 2 GICD_ICFGR should skip. */
+    offset += (2 * sizeof(uint32_t));
     for_each_dist_irq_reg(irq, s->num_irq, 2) {
         reg = *gic_bmp_ptr32(bmp, irq);
         if (irq % 32 != 0) {
@@ -222,6 +230,8 @@ static void kvm_dist_getbmp(GICv3State *s, uint32_t offset, uint32_t *bmp)
     uint32_t reg;
     int irq;
 
+    /* The first 1 GICD_XXXX should skip. */
+    offset += (1 * sizeof(uint32_t));
     for_each_dist_irq_reg(irq, s->num_irq, 1) {
         kvm_gicd_access(s, offset, &reg, false);
         *gic_bmp_ptr32(bmp, irq) = reg;
@@ -236,6 +246,10 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
     int irq;
 
     clroffset_index = clroffset;
+    /* The first 1 GICD_XXXX should skip. */
+    offset += (1 * sizeof(uint32_t));
+    if (clroffset != 0)
+        clroffset_index += (1 * sizeof(uint32_t));
     for_each_dist_irq_reg(irq, s->num_irq, 1) {
         /* If this bitmap is a set/clear register pair, first write to the
          * clear-reg to clear all bits before using the set-reg to write
-- 
2.0.4

Re: [Qemu-devel] [PATCH 1/2] arm_gicv3_kvm: increase clroffset accordingly

[Peter Maydell]

On 19 March 2018 at 14:36, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> It forgot to increase clroffset during the loop. So it only clear the
> first 4 bytes.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> ---
>  hw/intc/arm_gicv3_kvm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index ec37177..7000716 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -232,9 +232,10 @@ static void kvm_dist_getbmp(GICv3State *s, uint32_t offset, uint32_t *bmp)
>  static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
>                              uint32_t clroffset, uint32_t *bmp)
>  {
> -    uint32_t reg;
> +    uint32_t reg, clroffset_index;
>      int irq;
>
> +    clroffset_index = clroffset;
>      for_each_dist_irq_reg(irq, s->num_irq, 1) {
>          /* If this bitmap is a set/clear register pair, first write to the
>           * clear-reg to clear all bits before using the set-reg to write
> @@ -242,7 +243,8 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
>           */
>          if (clroffset != 0) {
>              reg = 0;
> -            kvm_gicd_access(s, clroffset, &reg, true);
> +            kvm_gicd_access(s, clroffset_index, &reg, true);
> +            clroffset_index += 4;
>          }
>          reg = *gic_bmp_ptr32(bmp, irq);
>          kvm_gicd_access(s, offset, &reg, true);

You might as well just use 'clroffset' directly rather than
creating a new clroffset_index variable, I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR

[Peter Maydell]

On 19 March 2018 at 14:36, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> It should skip to getting/putting the registers banked by GICR so that
> it could get/put the correct ones.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>

Can you explain what the consequences of the current code are
(ie is there a bug here that we are fixing?), and why the changes
are correct, please?


> ---
>  hw/intc/arm_gicv3_kvm.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 7000716..e967e4f 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -136,6 +136,8 @@ static void kvm_dist_get_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
>      int irq;
>
>      field = (uint32_t *)bmp;
> +    /* The first 8 GICD_IPRIORITYR should skip. */

It would be helpful to explain why we can skip the first 8 registers;
similarly below.

(I suspect the answer here is "for the KVM GICv3, affinity routing is always
enabled, and the first 8 GICD_IPRIORITYR<n> registers are always RAZ/WI,
so we don't need to sync them", for instance.)

> +    offset += (8 * sizeof(uint32_t));
>      for_each_dist_irq_reg(irq, s->num_irq, 8) {
>          kvm_gicd_access(s, offset, &reg, false);
>          *field = reg;
> @@ -150,6 +152,8 @@ static void kvm_dist_put_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
>      int irq;
>
>      field = (uint32_t *)bmp;
> +    /* The first 8 GICD_IPRIORITYR should skip. */
> +    offset += (8 * sizeof(uint32_t));
>      for_each_dist_irq_reg(irq, s->num_irq, 8) {
>          reg = *field;
>          kvm_gicd_access(s, offset, &reg, true);
> @@ -164,6 +168,8 @@ static void kvm_dist_get_edge_trigger(GICv3State *s, uint32_t offset,
>      uint32_t reg;
>      int irq;
>
> +    /* The first 2 GICD_ICFGR should skip. */
> +    offset += (2 * sizeof(uint32_t));
>      for_each_dist_irq_reg(irq, s->num_irq, 2) {
>          kvm_gicd_access(s, offset, &reg, false);
>          reg = half_unshuffle32(reg >> 1);
> @@ -181,6 +187,8 @@ static void kvm_dist_put_edge_trigger(GICv3State *s, uint32_t offset,
>      uint32_t reg;
>      int irq;
>
> +    /* The first 2 GICD_ICFGR should skip. */
> +    offset += (2 * sizeof(uint32_t));
>      for_each_dist_irq_reg(irq, s->num_irq, 2) {
>          reg = *gic_bmp_ptr32(bmp, irq);
>          if (irq % 32 != 0) {
> @@ -222,6 +230,8 @@ static void kvm_dist_getbmp(GICv3State *s, uint32_t offset, uint32_t *bmp)
>      uint32_t reg;
>      int irq;
>
> +    /* The first 1 GICD_XXXX should skip. */
> +    offset += (1 * sizeof(uint32_t));
>      for_each_dist_irq_reg(irq, s->num_irq, 1) {
>          kvm_gicd_access(s, offset, &reg, false);
>          *gic_bmp_ptr32(bmp, irq) = reg;
> @@ -236,6 +246,10 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
>      int irq;
>
>      clroffset_index = clroffset;
> +    /* The first 1 GICD_XXXX should skip. */

This looks like placeholder text that hasn't been filled in ?

> +    offset += (1 * sizeof(uint32_t));
> +    if (clroffset != 0)
> +        clroffset_index += (1 * sizeof(uint32_t));
>      for_each_dist_irq_reg(irq, s->num_irq, 1) {
>          /* If this bitmap is a set/clear register pair, first write to the
>           * clear-reg to clear all bits before using the set-reg to write
> --
> 2.0.4

thanks
-- PMM