Hi Peter, On 28/03/18 20:55, Peter Maydell wrote: > On 27 March 2018 at 15:04, Eric Auger <eric.auger@xxxxxxxxxx> wrote: >> Now all the internals are ready to handle multiple redistributor >> regions, let's allow the userspace to register them. >> >> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx> >> --- >> virt/kvm/arm/vgic/vgic-kvm-device.c | 40 +++++++++++++++++++++++++++++++++++-- >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 ++-- >> virt/kvm/arm/vgic/vgic.h | 9 ++++++++- >> 3 files changed, 48 insertions(+), 5 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c >> index e7b5a86..a2b99e4 100644 >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c >> @@ -65,7 +65,8 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) >> { >> int r = 0; >> struct vgic_dist *vgic = &kvm->arch.vgic; >> - phys_addr_t *addr_ptr, alignment; >> + phys_addr_t *addr_ptr = NULL; >> + phys_addr_t alignment; >> uint64_t undef_value = VGIC_ADDR_UNDEF; >> >> mutex_lock(&kvm->lock); >> @@ -92,7 +93,7 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) >> if (r) >> break; >> if (write) { >> - r = vgic_v3_set_redist_base(kvm, *addr); >> + r = vgic_v3_set_redist_base(kvm, 0, *addr, 0); >> goto out; >> } >> rdreg = list_first_entry(&vgic->rd_regions, >> @@ -103,6 +104,40 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) >> addr_ptr = &rdreg->base; >> break; >> } >> + case KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION: >> + { >> + struct vgic_redist_region *rdreg; >> + uint8_t index; >> + >> + r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3); >> + if (r) >> + break; >> + >> + index = *addr & KVM_VGIC_V3_RDIST_INDEX_MASK; >> + >> + if (write) { >> + gpa_t base = *addr & KVM_VGIC_V3_RDIST_BASE_MASK; >> + uint32_t count = (*addr & KVM_VGIC_V3_RDIST_COUNT_MASK) >> + >> KVM_VGIC_V3_RDIST_COUNT_SHIFT; >> + uint8_t flags = (*addr & KVM_VGIC_V3_RDIST_FLAGS_MASK) >> + >> KVM_VGIC_V3_RDIST_FLAGS_SHIFT; >> + >> + if (!count || flags) >> + r = -EINVAL; >> + else >> + r = vgic_v3_set_redist_base(kvm, index, >> + base, count); >> + goto out; >> + } >> + >> + rdreg = vgic_v3_rdist_region_from_index(kvm, index); >> + if (!rdreg) >> + r = -ENODEV; > > Here you check whether rdreg is NULL... > >> + >> + *addr_ptr = rdreg->base & index & > > ...but here you dereference it anyway. Missing "goto out"/break/other > control flow? definitively, goto default. > >> + (uint64_t)rdreg->count << KVM_VGIC_V3_RDIST_COUNT_SHIFT; >> + break; >> + } > > I was looking for the code which checked "EINVAL if you > already used KVM_VGIC_V3_ADDR_TYPE_REDIST", but couldn't > see it. Is that handled by one of the helper functions? No you're right. At the moment I only check the legacy API is not called after a first redist region has been registered (with legacy or new API). I will add this check in next version in vgic_v3_insert_redist_region(). Also something I forgot to document is the userspace must register the REDIST_REGIONS (so using the new API) starting with index 0 and continuously incrementing the index by 1. Thanks Eric > >> default: >> r = -ENODEV; >> } >> @@ -674,6 +709,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev, >> switch (attr->attr) { >> case KVM_VGIC_V3_ADDR_TYPE_DIST: >> case KVM_VGIC_V3_ADDR_TYPE_REDIST: >> + case KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION: >> return 0; >> } >> break; >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> index 89faadc..45287a0 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> @@ -766,11 +766,11 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index, >> return ret; >> } >> >> -int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr) >> +int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count) >> { >> int ret; >> >> - ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0); >> + ret = vgic_v3_insert_redist_region(kvm, index, addr, count); >> if (ret) >> return ret; > > thanks > -- PMM >