[4/9] hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions
diff mbox series

Message ID 1528879723-24675-5-git-send-email-eric.auger@redhat.com
State New
Headers show
Series
  • KVM/ARM: virt-3.0: Multiple redistributor regions and 256MB ECAM region
Related show

Commit Message

Auger Eric June 13, 2018, 8:48 a.m. UTC
Let's check if KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION is supported.
If not, we check the number of redist region is equal to 1 and use the
legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute. Otherwise we use
the new attribute and allow to register multiple regions to the
KVM device.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

---

v2 -> v3:
- In kvm_arm_gicv3_realize rename val into add_ormask local variable and
  add a comment
- start the redist region registration  from s->nb_redist_regions - 1
  downwards
---
 hw/intc/arm_gicv3_kvm.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

Comments

Andrew Jones June 14, 2018, 1:39 p.m. UTC | #1
On Wed, Jun 13, 2018 at 10:48:38AM +0200, Eric Auger wrote:
> Let's check if KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION is supported.
> If not, we check the number of redist region is equal to 1 and use the
> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute. Otherwise we use
> the new attribute and allow to register multiple regions to the
> KVM device.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> ---
> 
> v2 -> v3:
> - In kvm_arm_gicv3_realize rename val into add_ormask local variable and
>   add a comment
> - start the redist region registration  from s->nb_redist_regions - 1
>   downwards
> ---
>  hw/intc/arm_gicv3_kvm.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index ed7b719..52e6e70 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -753,6 +753,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>  {
>      GICv3State *s = KVM_ARM_GICV3(dev);
>      KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> +    bool multiple_redist_region_allowed;
>      Error *local_err = NULL;
>      int i;
>  
> @@ -789,6 +790,18 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    multiple_redist_region_allowed =
> +        kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                              KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
> +
> +    if (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
> +        error_setg(errp, "Multiple VGICv3 redistributor regions are not "
> +                   "supported by this host kernel");
> +        error_append_hint(errp, "A maximum of %d VCPUs can be used",
> +                          s->redist_region_count[0]);
> +        return;
> +    }
> +
>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
>                        0, &s->num_irq, true, &error_abort);
>  
> @@ -798,9 +811,23 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>  
>      kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>                              KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
> -    kvm_arm_register_device(&s->iomem_redist[0], -1,
> -                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> -                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
> +
> +    if (!multiple_redist_region_allowed) {
> +        kvm_arm_register_device(&s->iomem_redist[0], -1,
> +                                KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                                KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
> +    } else {
> +        for (i = s->nb_redist_regions - 1; i >= 0; i--) {

So we register in reverse-index order? Looking at your update to Linux doc
Documentation/virtual/kvm/devices/arm-vgic-v3.txt, shouldn't they be
registered in index-order? The doc says "Redistributor regions must be
registered in the incremental index order, starting from index 0."

Thanks,
drew

> +            /* Address mask made of the rdist region index and count */
> +            uint64_t addr_ormask =
> +                        i | ((uint64_t)s->redist_region_count[i] << 52);
> +
> +            kvm_arm_register_device(&s->iomem_redist[i], -1,
> +                                    KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                                    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
> +                                    s->dev_fd, addr_ormask);
> +        }
> +    }
>  
>      if (kvm_has_gsi_routing()) {
>          /* set up irq routing */
> -- 
> 2.5.5
> 
>
Auger Eric June 14, 2018, 1:48 p.m. UTC | #2
Hi Drew,

On 06/14/2018 03:39 PM, Andrew Jones wrote:
> On Wed, Jun 13, 2018 at 10:48:38AM +0200, Eric Auger wrote:
>> Let's check if KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION is supported.
>> If not, we check the number of redist region is equal to 1 and use the
>> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute. Otherwise we use
>> the new attribute and allow to register multiple regions to the
>> KVM device.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> ---
>>
>> v2 -> v3:
>> - In kvm_arm_gicv3_realize rename val into add_ormask local variable and
>>   add a comment
>> - start the redist region registration  from s->nb_redist_regions - 1
>>   downwards
>> ---
>>  hw/intc/arm_gicv3_kvm.c | 33 ++++++++++++++++++++++++++++++---
>>  1 file changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index ed7b719..52e6e70 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -753,6 +753,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>>  {
>>      GICv3State *s = KVM_ARM_GICV3(dev);
>>      KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
>> +    bool multiple_redist_region_allowed;
>>      Error *local_err = NULL;
>>      int i;
>>  
>> @@ -789,6 +790,18 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>>          return;
>>      }
>>  
>> +    multiple_redist_region_allowed =
>> +        kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +                              KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
>> +
>> +    if (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
>> +        error_setg(errp, "Multiple VGICv3 redistributor regions are not "
>> +                   "supported by this host kernel");
>> +        error_append_hint(errp, "A maximum of %d VCPUs can be used",
>> +                          s->redist_region_count[0]);
>> +        return;
>> +    }
>> +
>>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
>>                        0, &s->num_irq, true, &error_abort);
>>  
>> @@ -798,9 +811,23 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>>  
>>      kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>                              KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
>> -    kvm_arm_register_device(&s->iomem_redist[0], -1,
>> -                            KVM_DEV_ARM_VGIC_GRP_ADDR,
>> -                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
>> +
>> +    if (!multiple_redist_region_allowed) {
>> +        kvm_arm_register_device(&s->iomem_redist[0], -1,
>> +                                KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +                                KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
>> +    } else {
>> +        for (i = s->nb_redist_regions - 1; i >= 0; i--) {
> 
> So we register in reverse-index order? Looking at your update to Linux doc
> Documentation/virtual/kvm/devices/arm-vgic-v3.txt, shouldn't they be
> registered in index-order? The doc says "Redistributor regions must be
> registered in the incremental index order, starting from index 0."
kvm_arm_register_device adds the device in a QSLIST using
QSLIST_INSERT_HEAD. Then the list is read by kvm_arm_machine_init_done
from the HEAD. So we need to register in reversed order.

Thanks

Eric
> 
> Thanks,
> drew
> 
>> +            /* Address mask made of the rdist region index and count */
>> +            uint64_t addr_ormask =
>> +                        i | ((uint64_t)s->redist_region_count[i] << 52);
>> +
>> +            kvm_arm_register_device(&s->iomem_redist[i], -1,
>> +                                    KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +                                    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
>> +                                    s->dev_fd, addr_ormask);
>> +        }
>> +    }
>>  
>>      if (kvm_has_gsi_routing()) {
>>          /* set up irq routing */
>> -- 
>> 2.5.5
>>
>>
Andrew Jones June 14, 2018, 2:03 p.m. UTC | #3
On Thu, Jun 14, 2018 at 03:48:52PM +0200, Auger Eric wrote:
> Hi Drew,
> 
> On 06/14/2018 03:39 PM, Andrew Jones wrote:
> > On Wed, Jun 13, 2018 at 10:48:38AM +0200, Eric Auger wrote:
> >> Let's check if KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION is supported.
> >> If not, we check the number of redist region is equal to 1 and use the
> >> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute. Otherwise we use
> >> the new attribute and allow to register multiple regions to the
> >> KVM device.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >>
> >> ---
> >>
> >> v2 -> v3:
> >> - In kvm_arm_gicv3_realize rename val into add_ormask local variable and
> >>   add a comment
> >> - start the redist region registration  from s->nb_redist_regions - 1
> >>   downwards
> >> ---
> >>  hw/intc/arm_gicv3_kvm.c | 33 ++++++++++++++++++++++++++++++---
> >>  1 file changed, 30 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> >> index ed7b719..52e6e70 100644
> >> --- a/hw/intc/arm_gicv3_kvm.c
> >> +++ b/hw/intc/arm_gicv3_kvm.c
> >> @@ -753,6 +753,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
> >>  {
> >>      GICv3State *s = KVM_ARM_GICV3(dev);
> >>      KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> >> +    bool multiple_redist_region_allowed;
> >>      Error *local_err = NULL;
> >>      int i;
> >>  
> >> @@ -789,6 +790,18 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
> >>          return;
> >>      }
> >>  
> >> +    multiple_redist_region_allowed =
> >> +        kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >> +                              KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
> >> +
> >> +    if (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
> >> +        error_setg(errp, "Multiple VGICv3 redistributor regions are not "
> >> +                   "supported by this host kernel");
> >> +        error_append_hint(errp, "A maximum of %d VCPUs can be used",
> >> +                          s->redist_region_count[0]);
> >> +        return;
> >> +    }
> >> +
> >>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> >>                        0, &s->num_irq, true, &error_abort);
> >>  
> >> @@ -798,9 +811,23 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
> >>  
> >>      kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >>                              KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
> >> -    kvm_arm_register_device(&s->iomem_redist[0], -1,
> >> -                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> >> -                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
> >> +
> >> +    if (!multiple_redist_region_allowed) {
> >> +        kvm_arm_register_device(&s->iomem_redist[0], -1,
> >> +                                KVM_DEV_ARM_VGIC_GRP_ADDR,
> >> +                                KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
> >> +    } else {
> >> +        for (i = s->nb_redist_regions - 1; i >= 0; i--) {
> > 
> > So we register in reverse-index order? Looking at your update to Linux doc
> > Documentation/virtual/kvm/devices/arm-vgic-v3.txt, shouldn't they be
> > registered in index-order? The doc says "Redistributor regions must be
> > registered in the incremental index order, starting from index 0."
> kvm_arm_register_device adds the device in a QSLIST using
> QSLIST_INSERT_HEAD. Then the list is read by kvm_arm_machine_init_done
> from the HEAD. So we need to register in reversed order.

Oh, that's sort of horrible, as I can't even see where that's documented
anywhere. Can we at least document it here in a comment? Actually, we
should probably reverse the list first in kvm_arm_machine_init_done()
and document that devices should be registered in the order KVM needs
them to be - just in the name of sanity. But, I'm not sure what we'd
break if we reversed the order now though...

Thanks,
drew

> 
> Thanks
> 
> Eric
> > 
> > Thanks,
> > drew
> > 
> >> +            /* Address mask made of the rdist region index and count */
> >> +            uint64_t addr_ormask =
> >> +                        i | ((uint64_t)s->redist_region_count[i] << 52);
> >> +
> >> +            kvm_arm_register_device(&s->iomem_redist[i], -1,
> >> +                                    KVM_DEV_ARM_VGIC_GRP_ADDR,
> >> +                                    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
> >> +                                    s->dev_fd, addr_ormask);
> >> +        }
> >> +    }
> >>  
> >>      if (kvm_has_gsi_routing()) {
> >>          /* set up irq routing */
> >> -- 
> >> 2.5.5
> >>
> >>
>

Patch
diff mbox series

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index ed7b719..52e6e70 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -753,6 +753,7 @@  static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
 {
     GICv3State *s = KVM_ARM_GICV3(dev);
     KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
+    bool multiple_redist_region_allowed;
     Error *local_err = NULL;
     int i;
 
@@ -789,6 +790,18 @@  static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    multiple_redist_region_allowed =
+        kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+                              KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
+
+    if (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
+        error_setg(errp, "Multiple VGICv3 redistributor regions are not "
+                   "supported by this host kernel");
+        error_append_hint(errp, "A maximum of %d VCPUs can be used",
+                          s->redist_region_count[0]);
+        return;
+    }
+
     kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
                       0, &s->num_irq, true, &error_abort);
 
@@ -798,9 +811,23 @@  static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
 
     kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
-    kvm_arm_register_device(&s->iomem_redist[0], -1,
-                            KVM_DEV_ARM_VGIC_GRP_ADDR,
-                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
+
+    if (!multiple_redist_region_allowed) {
+        kvm_arm_register_device(&s->iomem_redist[0], -1,
+                                KVM_DEV_ARM_VGIC_GRP_ADDR,
+                                KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
+    } else {
+        for (i = s->nb_redist_regions - 1; i >= 0; i--) {
+            /* Address mask made of the rdist region index and count */
+            uint64_t addr_ormask =
+                        i | ((uint64_t)s->redist_region_count[i] << 52);
+
+            kvm_arm_register_device(&s->iomem_redist[i], -1,
+                                    KVM_DEV_ARM_VGIC_GRP_ADDR,
+                                    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
+                                    s->dev_fd, addr_ormask);
+        }
+    }
 
     if (kvm_has_gsi_routing()) {
         /* set up irq routing */