Date:   Wed, 28 Mar 2018 22:19:25 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Peter Maydell <peter.maydell@...aro.org>
Cc:     Eric Auger <eric.auger.pro@...il.com>,
        lkml - Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kvm-devel <kvm@...r.kernel.org>, kvmarm@...ts.cs.columbia.edu,
        Marc Zyngier <marc.zyngier@....com>,
        Christoffer Dall <cdall@...nel.org>,
        Andre Przywara <andre.przywara@....com>,
        Andrew Jones <drjones@...hat.com>, Wei Huang <wei@...hat.com>
Subject: Re: [RFC v2 11/12] KVM: arm/arm64: Implement
 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

Hi Peter,

On 28/03/18 20:55, Peter Maydell wrote:
> On 27 March 2018 at 15:04, Eric Auger <eric.auger@...hat.com> 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@...hat.com>
>> ---
>>  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
> 

Powered by blists - more mailing lists