From: Auger Eric <eric.auger@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Eric Auger <eric.auger.pro@gmail.com>,
lkml - Kernel Mailing List <linux-kernel@vger.kernel.org>,
kvm-devel <kvm@vger.kernel.org>,
kvmarm@lists.cs.columbia.edu, Marc Zyngier <marc.zyngier@arm.com>,
Christoffer Dall <cdall@kernel.org>,
Andre Przywara <andre.przywara@arm.com>,
Andrew Jones <drjones@redhat.com>, Wei Huang <wei@redhat.com>
Subject: Re: [RFC v2 11/12] KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
Date: Wed, 28 Mar 2018 22:19:25 +0200 [thread overview]
Message-ID: <2121af1c-a752-99d8-d51e-d500430c910e@redhat.com> (raw)
In-Reply-To: <CAFEAcA_zXwS6EygjwPO9e-Ouy=seDdZt5i62itGP-W7VHugYTQ@mail.gmail.com>
Hi Peter,
On 28/03/18 20:55, Peter Maydell wrote:
> On 27 March 2018 at 15:04, Eric Auger <eric.auger@redhat.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@redhat.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
>
next prev parent reply other threads:[~2018-03-28 20:19 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-27 14:04 [RFC v2 00/12] KVM: arm/arm64: Allow multiple GICv3 redistributor regions Eric Auger
2018-03-27 14:04 ` [RFC v2 01/12] KVM: arm/arm64: Set dist->spis to NULL after kfree Eric Auger
2018-03-27 17:10 ` Marc Zyngier
2018-03-28 6:41 ` Auger Eric
2018-03-27 14:04 ` [RFC v2 02/12] KVM: arm/arm64: Document KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION Eric Auger
2018-04-09 21:38 ` Christoffer Dall
2018-04-12 15:51 ` Auger Eric
2018-03-27 14:04 ` [RFC v2 03/12] KVM: arm/arm64: Replace the single rdist region by a list Eric Auger
2018-03-27 14:04 ` [RFC v2 04/12] KVM: arm/arm64: Helper to locate free rdist index Eric Auger
2018-03-27 14:04 ` [RFC v2 05/12] KVM: arm/arm64: Revisit Redistributor TYPER last bit computation Eric Auger
2018-03-27 14:04 ` [RFC v2 06/12] KVM: arm/arm64: Helper to register a new redistributor region Eric Auger
2018-03-27 14:04 ` [RFC v2 07/12] KVM: arm/arm64: Adapt vgic_v3_check_base to multiple rdist regions Eric Auger
2018-03-27 14:04 ` [RFC v2 08/12] KVM: arm/arm64: Check vcpu redist base before registering an iodev Eric Auger
2018-03-27 14:04 ` [RFC v2 09/12] KVM: arm/arm64: Check all vcpu redistributors are set on map_resources Eric Auger
2018-03-27 14:04 ` [RFC v2 10/12] KVM: arm/arm64: Add KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION Eric Auger
2018-03-27 14:04 ` [RFC v2 11/12] KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION Eric Auger
2018-03-28 18:55 ` Peter Maydell
2018-03-28 20:19 ` Auger Eric [this message]
2018-03-27 14:04 ` [RFC v2 12/12] KVM: arm/arm64: Bump VGIC_V3_MAX_CPUS to 512 Eric Auger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2121af1c-a752-99d8-d51e-d500430c910e@redhat.com \
--to=eric.auger@redhat.com \
--cc=andre.przywara@arm.com \
--cc=cdall@kernel.org \
--cc=drjones@redhat.com \
--cc=eric.auger.pro@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-kernel@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=peter.maydell@linaro.org \
--cc=wei@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).