Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size
Hi Alexandru and Eric, On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote: > Hi Alexandru, > > On 9/10/21 10:28 AM, Alexandru Elisei wrote: > > Hi Ricardo, > > > > On 9/9/21 5:47 PM, Ricardo Koller wrote: > >> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote: > >>> Hi Ricardo, > >>> > >>> On 9/8/21 10:03 PM, Ricardo Koller wrote: > Extend vgic_v3_check_base() to verify that the redistributor regions > don't go above the VM-specified IPA size (phys_size). This can happen > when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with: > > base + size > phys_size AND base < phys_size > > vgic_v3_check_base() is used to check the redist regions bases when > setting them (with the vcpus added so far) and when attempting the first > vcpu-run. > > Signed-off-by: Ricardo Koller > --- > arch/arm64/kvm/vgic/vgic-v3.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c > b/arch/arm64/kvm/vgic/vgic-v3.c > index 66004f61cd83..5afd9f6f68f6 100644 > --- a/arch/arm64/kvm/vgic/vgic-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm) > if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) < > rdreg->base) > return false; > + > +if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) > > +kvm_phys_size(kvm)) > +return false; > >>> Looks to me like this same check (and the overflow one before it) is done > >>> when > >>> adding a new Redistributor region in kvm_vgic_addr() -> > >>> vgic_v3_set_redist_base() > >>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can > >>> tell, > >>> kvm_vgic_addr() handles both ways of setting the Redistributor address. > >>> > >>> Without this patch, did you manage to set a base address such that base + > >>> size > > >>> kvm_phys_size()? > >>> > >> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way > >> to get to this situation is with the selftest in patch 2. I then tried > >> an extra experiment: map the first redistributor, run the first vcpu, > >> and access the redist from inside the guest. KVM didn't complain in any > >> of these steps. > > Yes, Eric pointed out that I was mistaken and there is no check being done > > for > > base + size > kvm_phys_size(). > > > > What I was trying to say is that this check is better done when the user > > creates a > > Redistributor region, not when a VCPU is first run. We have everything we > > need to > > make the check when a region is created, why wait until the VCPU is run? > > > > For example, vgic_v3_insert_redist_region() is called each time the adds a > > new > > Redistributor region (via either of the two APIs), and already has a check > > for the > > upper limit overflowing (identical to the check in vgic_v3_check_base()). I > > would > > add the check against the maximum IPA size there. > you seem to refer to an old kernel as vgic_v3_insert_redist_region was > renamed into vgic_v3_alloc_redist_region in > e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region() > > I think in case you use the old rdist API you do not know yet the size > of the redist region at this point (count=0), hence Ricardo's choice to > do the check latter. Just wanted to add one more detail. vgic_v3_check_base() is also called when creating the redistributor region (via vgic_v3_set_redist_base -> vgic_register_redist_iodev). This patch reuses that check for the old redist API to also check for "base + size > kvm_phys_size()" with a size calculated using the vcpus added so far. > > > > Also, because vgic_v3_insert_redist_region() already checks for overflow, I > > believe the overflow check in vgic_v3_check_base() is redundant. > > It's redundant for the new redist API, but still needed for the old redist API. > > As far as I can tell, vgic_v3_check_base() is there to make sure that the > > Distributor doesn't overlap with any of the Redistributors, and because the > > Redistributors and the Distributor can be created in any order, we defer > > the check > > until the first VCPU is run. I might be wrong about this, someone please > > correct > > me if I'm wrong. > > > > Also, did you verify that KVM is also doing this check for GICv2? KVM does > > something similar and calls vgic_v2_check_base() when mapping the GIC > > resources, > > and I don't see a check for the maximum IPA size in that function either. > > I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it > checks the base @) > It seems that GICv2 suffers from the same problem. The cpu interface base is checked but the end can extend above IPA size. Note that the cpu interface is 8KBs and vgic_check_ioaddr() is only
Re: [PATCH v4 09/18] KVM: arm64: selftests: Add guest support to get the vcpuid
On Fri, Sep 10, 2021 at 1:10 AM Andrew Jones wrote:
>
> On Thu, Sep 09, 2021 at 10:10:56AM -0700, Raghavendra Rao Ananta wrote:
> > On Thu, Sep 9, 2021 at 12:56 AM Andrew Jones wrote:
> > >
> > > On Thu, Sep 09, 2021 at 01:38:09AM +, Raghavendra Rao Ananta wrote:
> ...
> > > > + for (i = 0; i < KVM_MAX_VCPUS; i++) {
> > > > + vcpuid = vcpuid_map[i].vcpuid;
> > > > + GUEST_ASSERT_1(vcpuid != VM_VCPUID_MAP_INVAL, mpidr);
> > >
> > > We don't want this assert if it's possible to have sparse maps, which
> > > it probably isn't ever going to be, but...
> > >
> > If you look at the way the array is arranged, the element with
> > VM_VCPUID_MAP_INVAL acts as a sentinel for us and all the proper
> > elements would lie before this. So, I don't think we'd have a sparse
> > array here.
>
> If we switch to my suggestion of adding map entries at vcpu-add time and
> removing them at vcpu-rm time, then the array may become sparse depending
> on the order of removals.
>
Oh, I get it now. But like you mentioned, we add entries to the map
while the vCPUs are getting added and then sync_global_to_guest()
later. This seems like a lot of maintainance, unless I'm interpreting
it wrong or not seeing an advantage.
I like your idea of coming up an arch-independent interface, however.
So I modified it similar to the familiar ucall interface that we have
and does everything in one shot to avoid any confusion:
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h
b/tools/testing/selftests/kvm/include/kvm_util.h
index 010b59b13917..0e87cb0c980b 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -400,4 +400,24 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t
vcpu_id, struct ucall *uc);
int vm_get_stats_fd(struct kvm_vm *vm);
int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
+#define VM_CPUID_MAP_INVAL -1
+
+struct vm_cpuid_map {
+ uint64_t hw_cpuid;
+ int vcpuid;
+};
+
+/*
+ * Create a vcpuid:hw_cpuid map and export it to the guest
+ *
+ * Input Args:
+ * vm - KVM VM.
+ *
+ * Output Args: None
+ *
+ * Must be called after all the vCPUs are added to the VM
+ */
+void vm_cpuid_map_init(struct kvm_vm *vm);
+int guest_get_vcpuid(void);
+
#endif /* SELFTEST_KVM_UTIL_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c
b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index db64ee206064..e796bb3984a6 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -16,6 +16,8 @@
static vm_vaddr_t exception_handlers;
+static struct vm_cpuid_map cpuid_map[KVM_MAX_VCPUS];
+
static uint64_t page_align(struct kvm_vm *vm, uint64_t v)
{
return (v + vm->page_size) & ~(vm->page_size - 1);
@@ -426,3 +428,42 @@ void vm_install_exception_handler(struct kvm_vm
*vm, int vector,
assert(vector < VECTOR_NUM);
handlers->exception_handlers[vector][0] = handler;
}
+
+void vm_cpuid_map_init(struct kvm_vm *vm)
+{
+ int i = 0;
+ struct vcpu *vcpu;
+ struct vm_cpuid_map *map;
+
+ TEST_ASSERT(!list_empty(>vcpus), "vCPUs must have been created\n");
+
+ list_for_each_entry(vcpu, >vcpus, list) {
+ map = _map[i++];
+ map->vcpuid = vcpu->id;
+ get_reg(vm, vcpu->id,
KVM_ARM64_SYS_REG(SYS_MPIDR_EL1), >hw_cpuid);
+ map->hw_cpuid &= MPIDR_HWID_BITMASK;
+ }
+
+ if (i < KVM_MAX_VCPUS)
+ cpuid_map[i].vcpuid = VM_CPUID_MAP_INVAL;
+
+ sync_global_to_guest(vm, cpuid_map);
+}
+
+int guest_get_vcpuid(void)
+{
+ int i, vcpuid;
+ uint64_t mpidr = read_sysreg(mpidr_el1) & MPIDR_HWID_BITMASK;
+
+ for (i = 0; i < KVM_MAX_VCPUS; i++) {
+ vcpuid = cpuid_map[i].vcpuid;
+
+ /* Was this vCPU added to the VM after the map was
initialized? */
+ GUEST_ASSERT_1(vcpuid != VM_CPUID_MAP_INVAL, mpidr);
+
+ if (mpidr == cpuid_map[i].hw_cpuid)
+ return vcpuid;
+ }
+
+ /* We should not be reaching here */
+ GUEST_ASSERT_1(0, mpidr);
+ return -1;
+}
This would ensure that we don't have a sparse array and can use the
last non-vCPU element as a sentinal node.
If you still feel preparing the map as and when the vCPUs are created
makes more sense, I can go for it.
Regards,
Raghavendra
> Thanks,
> drew
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 0/4] Fix racing TLBI with ASID/VMID reallocation
On Fri, Sep 10, 2021 at 09:06:31AM +, Shameerali Kolothum Thodi wrote: > > [2] > > https://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/kernel-tla.git/commit/ > > I am going through the ASID TLA+ model and in the above commit, it appears > that the > different ASID check(=> ActiveAsid(c1) # ActiveAsid(c2)) for the Invariant > UniqueASIDActiveTask is now removed. > > Just wondering why that is not relevant anymore? It's still relevant. I probably deleted it by mistake, I'll add it back now. Thanks for carefully looking at this commit. -- Catalin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
RE: [PATCH 0/4] Fix racing TLBI with ASID/VMID reallocation
> -Original Message-
> From: Will Deacon [mailto:w...@kernel.org]
> Sent: 06 August 2021 12:31
> To: linux-arm-ker...@lists.infradead.org
> Cc: kernel-t...@android.com; Will Deacon ; Catalin
> Marinas ; Marc Zyngier ; Jade
> Alglave ; Shameerali Kolothum Thodi
> ; kvmarm@lists.cs.columbia.edu;
> linux-a...@vger.kernel.org
> Subject: [PATCH 0/4] Fix racing TLBI with ASID/VMID reallocation
>
> Hi all,
>
> While reviewing Shameer's reworked VMID allocator [1] and discussing
> with Marc, we spotted a race between TLB invalidation (which typically
> takes an ASID or VMID argument) and reallocation of ASID/VMID for the
> context being targetted.
>
> The first patch spells out an example with try_to_unmap_one() in a
> comment, which Catalin has kindly modelled in TLA+ at [2].
>
> Although I'm posting all this together for ease of review, the intention
> is that the first patch will go via arm64 with the latter going via kvm.
>
> Cheers,
>
> Will
>
> [1]
> https://lore.kernel.org/r/20210729104009.382-1-shameerali.kolothum.thodi
> @huawei.com
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/kernel-tla.git/commi
> t/
Hi Catalin,
I am going through the ASID TLA+ model and in the above commit, it appears that
the
different ASID check(=> ActiveAsid(c1) # ActiveAsid(c2)) for the Invariant
UniqueASIDActiveTask is now removed.
Just wondering why that is not relevant anymore?
Thanks,
Shameer
> Cc: Catalin Marinas
> Cc: Marc Zyngier
> Cc: Jade Alglave
> Cc: Shameer Kolothum
> Cc:
> Cc:
>
> --->8
>
> Marc Zyngier (3):
> KVM: arm64: Move kern_hyp_va() usage in __load_guest_stage2() into the
> callers
> KVM: arm64: Convert the host S2 over to __load_guest_stage2()
> KVM: arm64: Upgrade VMID accesses to {READ,WRITE}_ONCE
>
> Will Deacon (1):
> arm64: mm: Fix TLBI vs ASID rollover
>
> arch/arm64/include/asm/kvm_mmu.h | 17 ++-
> arch/arm64/include/asm/mmu.h | 29
> ---
> arch/arm64/include/asm/tlbflush.h | 11 +++
> arch/arm64/kvm/arm.c | 2 +-
> arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 +-
> arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 ++--
> arch/arm64/kvm/hyp/nvhe/switch.c | 4 ++-
> arch/arm64/kvm/hyp/nvhe/tlb.c | 2 +-
> arch/arm64/kvm/hyp/vhe/switch.c | 2 +-
> arch/arm64/kvm/hyp/vhe/tlb.c | 2 +-
> arch/arm64/kvm/mmu.c | 2 +-
> 11 files changed, 52 insertions(+), 27 deletions(-)
>
> --
> 2.32.0.605.g8dce9f2422-goog
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size
Hi Alexandru, On 9/10/21 10:28 AM, Alexandru Elisei wrote: > Hi Ricardo, > > On 9/9/21 5:47 PM, Ricardo Koller wrote: >> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote: >>> Hi Ricardo, >>> >>> On 9/8/21 10:03 PM, Ricardo Koller wrote: Extend vgic_v3_check_base() to verify that the redistributor regions don't go above the VM-specified IPA size (phys_size). This can happen when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with: base + size > phys_size AND base < phys_size vgic_v3_check_base() is used to check the redist regions bases when setting them (with the vcpus added so far) and when attempting the first vcpu-run. Signed-off-by: Ricardo Koller --- arch/arm64/kvm/vgic/vgic-v3.c | 4 1 file changed, 4 insertions(+) diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c index 66004f61cd83..5afd9f6f68f6 100644 --- a/arch/arm64/kvm/vgic/vgic-v3.c +++ b/arch/arm64/kvm/vgic/vgic-v3.c @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm) if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) < rdreg->base) return false; + + if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) > + kvm_phys_size(kvm)) + return false; >>> Looks to me like this same check (and the overflow one before it) is done >>> when >>> adding a new Redistributor region in kvm_vgic_addr() -> >>> vgic_v3_set_redist_base() >>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can >>> tell, >>> kvm_vgic_addr() handles both ways of setting the Redistributor address. >>> >>> Without this patch, did you manage to set a base address such that base + >>> size > >>> kvm_phys_size()? >>> >> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way >> to get to this situation is with the selftest in patch 2. I then tried >> an extra experiment: map the first redistributor, run the first vcpu, >> and access the redist from inside the guest. KVM didn't complain in any >> of these steps. > Yes, Eric pointed out that I was mistaken and there is no check being done for > base + size > kvm_phys_size(). > > What I was trying to say is that this check is better done when the user > creates a > Redistributor region, not when a VCPU is first run. We have everything we > need to > make the check when a region is created, why wait until the VCPU is run? > > For example, vgic_v3_insert_redist_region() is called each time the adds a new > Redistributor region (via either of the two APIs), and already has a check > for the > upper limit overflowing (identical to the check in vgic_v3_check_base()). I > would > add the check against the maximum IPA size there. you seem to refer to an old kernel as vgic_v3_insert_redist_region was renamed into vgic_v3_alloc_redist_region in e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region() I think in case you use the old rdist API you do not know yet the size of the redist region at this point (count=0), hence Ricardo's choice to do the check latter. > > Also, because vgic_v3_insert_redist_region() already checks for overflow, I > believe the overflow check in vgic_v3_check_base() is redundant. > > As far as I can tell, vgic_v3_check_base() is there to make sure that the > Distributor doesn't overlap with any of the Redistributors, and because the > Redistributors and the Distributor can be created in any order, we defer the > check > until the first VCPU is run. I might be wrong about this, someone please > correct > me if I'm wrong. > > Also, did you verify that KVM is also doing this check for GICv2? KVM does > something similar and calls vgic_v2_check_base() when mapping the GIC > resources, > and I don't see a check for the maximum IPA size in that function either. I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it checks the base @) Thanks Eric > > Thanks, > > Alex > >> Thanks, >> Ricardo >> >>> Thanks, >>> >>> Alex >>> } if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base)) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 02/18] KVM: arm64: selftests: Add sysreg.h
On Thu, Sep 09, 2021 at 01:06:31PM -0700, Raghavendra Rao Ananta wrote: > On Thu, Sep 9, 2021 at 10:18 AM Mark Brown wrote: > > > create mode 100644 tools/testing/selftests/kvm/include/aarch64/sysreg.h > > Can we arrange to copy this at build time rather than having a duplicate > > copy we need to keep in sync? We have some stuff to do this for uapi > > headers already. > That's a great idea actually (I wasn't aware of it). But, probably > should've mentioned it earlier, I had a hard time compiling the header > as is so I modified it a little bit and made the definitions of > [write|read]_sysreg_s() similar to the ones in kvm-unit-tests. > I'll try my best to get the original format working and try to > implement your idea if it works. One option would be to do something like split out the bits that can be shared into a separate header which can be included from both places and then have the header with the unsharable bits include that. Something like sysreg.h and sysreg_defs.h for example. signature.asc Description: PGP signature ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size
Hi Ricardo, On 9/9/21 5:47 PM, Ricardo Koller wrote: > On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote: >> Hi Ricardo, >> >> On 9/8/21 10:03 PM, Ricardo Koller wrote: >>> Extend vgic_v3_check_base() to verify that the redistributor regions >>> don't go above the VM-specified IPA size (phys_size). This can happen >>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with: >>> >>> base + size > phys_size AND base < phys_size >>> >>> vgic_v3_check_base() is used to check the redist regions bases when >>> setting them (with the vcpus added so far) and when attempting the first >>> vcpu-run. >>> >>> Signed-off-by: Ricardo Koller >>> --- >>> arch/arm64/kvm/vgic/vgic-v3.c | 4 >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c >>> index 66004f61cd83..5afd9f6f68f6 100644 >>> --- a/arch/arm64/kvm/vgic/vgic-v3.c >>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c >>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm) >>> if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) < >>> rdreg->base) >>> return false; >>> + >>> + if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) > >>> + kvm_phys_size(kvm)) >>> + return false; >> Looks to me like this same check (and the overflow one before it) is done >> when >> adding a new Redistributor region in kvm_vgic_addr() -> >> vgic_v3_set_redist_base() >> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can >> tell, >> kvm_vgic_addr() handles both ways of setting the Redistributor address. >> >> Without this patch, did you manage to set a base address such that base + >> size > >> kvm_phys_size()? >> > Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way > to get to this situation is with the selftest in patch 2. I then tried > an extra experiment: map the first redistributor, run the first vcpu, > and access the redist from inside the guest. KVM didn't complain in any > of these steps. Yes, Eric pointed out that I was mistaken and there is no check being done for base + size > kvm_phys_size(). What I was trying to say is that this check is better done when the user creates a Redistributor region, not when a VCPU is first run. We have everything we need to make the check when a region is created, why wait until the VCPU is run? For example, vgic_v3_insert_redist_region() is called each time the adds a new Redistributor region (via either of the two APIs), and already has a check for the upper limit overflowing (identical to the check in vgic_v3_check_base()). I would add the check against the maximum IPA size there. Also, because vgic_v3_insert_redist_region() already checks for overflow, I believe the overflow check in vgic_v3_check_base() is redundant. As far as I can tell, vgic_v3_check_base() is there to make sure that the Distributor doesn't overlap with any of the Redistributors, and because the Redistributors and the Distributor can be created in any order, we defer the check until the first VCPU is run. I might be wrong about this, someone please correct me if I'm wrong. Also, did you verify that KVM is also doing this check for GICv2? KVM does something similar and calls vgic_v2_check_base() when mapping the GIC resources, and I don't see a check for the maximum IPA size in that function either. Thanks, Alex > > Thanks, > Ricardo > >> Thanks, >> >> Alex >> >>> } >>> >>> if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base)) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 09/18] KVM: arm64: selftests: Add guest support to get the vcpuid
On Thu, Sep 09, 2021 at 10:10:56AM -0700, Raghavendra Rao Ananta wrote:
> On Thu, Sep 9, 2021 at 12:56 AM Andrew Jones wrote:
> >
> > On Thu, Sep 09, 2021 at 01:38:09AM +, Raghavendra Rao Ananta wrote:
...
> > > + for (i = 0; i < KVM_MAX_VCPUS; i++) {
> > > + vcpuid = vcpuid_map[i].vcpuid;
> > > + GUEST_ASSERT_1(vcpuid != VM_VCPUID_MAP_INVAL, mpidr);
> >
> > We don't want this assert if it's possible to have sparse maps, which
> > it probably isn't ever going to be, but...
> >
> If you look at the way the array is arranged, the element with
> VM_VCPUID_MAP_INVAL acts as a sentinel for us and all the proper
> elements would lie before this. So, I don't think we'd have a sparse
> array here.
If we switch to my suggestion of adding map entries at vcpu-add time and
removing them at vcpu-rm time, then the array may become sparse depending
on the order of removals.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/2] KVM: arm64: selftests: test for vgic redist above the VM IPA size
Hi Ricardo,
On 9/9/21 8:22 PM, Ricardo Koller wrote:
> On Thu, Sep 09, 2021 at 03:54:31PM +0200, Eric Auger wrote:
>> Hi Ricardo,
>>
>> On 9/8/21 11:03 PM, Ricardo Koller wrote:
>>> This test attempts (and fails) to set a redistributor region using the
>>> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST that's partially above the
>>> VM-specified IPA size.
>>>
>>> Signed-off-by: Ricardo Koller
>>> ---
>>> .../testing/selftests/kvm/aarch64/vgic_init.c | 44 +++
>>> 1 file changed, 44 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> index 623f31a14326..6dd7b5e91421 100644
>>> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> @@ -285,6 +285,49 @@ static void test_vcpus_then_vgic(void)
>>> vm_gic_destroy();
>>> }
>>>
>>> +static void test_redist_above_vm_pa_bits(enum vm_guest_mode mode)
>>> +{
>>> + struct vm_gic v;
>>> + int ret, i;
>>> + uint32_t vcpuids[] = { 1, 2, 3, 4, };
>>> + int pa_bits = vm_guest_mode_params[mode].pa_bits;
>>> + uint64_t addr, psize = 1ULL << pa_bits;
>>> +
>>> + /* Add vcpu 1 */
>>> + v.vm = vm_create_with_vcpus(mode, 1, DEFAULT_GUEST_PHY_PAGES,
>>> + 0, 0, guest_code, vcpuids);
>>> + v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>>> +
>>> + /* Set space for half a redist, we have 1 vcpu, so this fails. */
>>> + addr = psize - 0x1;
>>> + ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +KVM_VGIC_V3_ADDR_TYPE_REDIST, , true);
>>> + TEST_ASSERT(ret && errno == EINVAL, "not enough space for one redist");
>>> +
>>> + /* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
>>> + addr = psize - (3 * 2 * 0x1);
>>> + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +KVM_VGIC_V3_ADDR_TYPE_REDIST, , true);
>> I think you need to test both the old API (KVM_VGIC_V3_ADDR_TYPE_REDIST)
>> and the new one (KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION).
>>
>> Can't you integrate those new checks in existing tests,
>> subtest_redist_regions() and subtest_dist_rdist() which already tests
>> base addr beyond IPA limit (but not range end unfortunately). look for
>> E2BIG.
>>
> Had some issues adapting subtest_dist_rdist() as the IPA range check for
> ADDR_TYPE_REDIST is done at 1st vcpu run. subtest_dist_rdist() is
> already used to set overlapping dist/redist regions, which is then
> checked to generate EINVAL on 1st vcpu run. If subtest_dist_rdist() is
> also used to set the redist region above phys_size, then there won't be
> a way of checking that the vcpu run fails because of both the overlap
> and IPA issue. It was simpler and cleaner to just add a new function
> for the ADDR_TYPE_REDIST IPA range test. Will adapt
OK I see, then effectively adding a new test was more straightforward.
> subtest_redist_regions() as the check for ADDR_TYPE_REDIST_REGION can be
> done when setting the regions.
OK
>
> Related Question:
>
> Both the KVM_VGIC_V3_ADDR_TYPE_REDIST and KVM_RUN currently return
> EINVAL with my proposed change (not E2BIG). I will change
> KVM_VGIC_V3_ADDR_TYPE_REDIST to fail with E2BIG, but will leave KVM_RUN
> failing with EINVAL. Would you say that's the correct behavior?
This looks OK to me, as long as the KVM uapi doc documents is aligned.
Thanks
Eric
>
> Thanks,
> Ricardo
>
>> Thanks
>>
>> Eric
>>> +
>>> + addr = 0x0;
>>> + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> + KVM_VGIC_V3_ADDR_TYPE_DIST, , true);
>>> +
>>> + /* Add three vcpus (2, 3, 4). */
>>> + for (i = 1; i < 4; ++i)
>>> + vm_vcpu_add_default(v.vm, vcpuids[i], guest_code);
>>> +
>>> + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>>> + KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>>> +
>>> + /* Attempt to run a vcpu without enough redist space. */
>>> + ret = run_vcpu(v.vm, vcpuids[3]);
>>> + TEST_ASSERT(ret && errno == EINVAL,
>>> + "redist base+size above IPA detected on 1st vcpu run");
>>> +
>>> + vm_gic_destroy();
>>> +}
>>> +
>>> static void test_new_redist_regions(void)
>>> {
>>> void *dummy = NULL;
>>> @@ -542,6 +585,7 @@ int main(int ac, char **av)
>>> test_kvm_device();
>>> test_vcpus_then_vgic();
>>> test_vgic_then_vcpus();
>>> + test_redist_above_vm_pa_bits(VM_MODE_DEFAULT);
>>> test_new_redist_regions();
>>> test_typer_accesses();
>>> test_last_bit_redist_regions();
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
