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 | expand |
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 > >
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 >> >>
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 > >> > >> >
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 */