Re: [Qemu-devel] [PATCH 4/9] hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions

[Andrew Jones]

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 <address@hidden>
> >> Reviewed-by: Peter Maydell <address@hidden>
> >>
> >> ---
> >>
> >> 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
> >>
> >>
>