On Fri, Jan 10, 2025, Claudio Imbrenda wrote:
> On Fri, 10 Jan 2025 10:31:38 +0100
> Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
>
> > Am 08.01.25 um 19:14 schrieb Claudio Imbrenda:
> > > +static void kvm_s390_ucontrol_ensure_memslot(struct kvm *kvm, unsigned long addr)
> > > +{
> > > + struct kvm_userspace_memory_region2 region = {
> > > + .slot = addr / UCONTROL_SLOT_SIZE,
> > > + .memory_size = UCONTROL_SLOT_SIZE,
> > > + .guest_phys_addr = ALIGN_DOWN(addr, UCONTROL_SLOT_SIZE),
> > > + .userspace_addr = ALIGN_DOWN(addr, UCONTROL_SLOT_SIZE),
> > > + };
> > > + struct kvm_memory_slot *slot;
> > > +
> > > + mutex_lock(&kvm->slots_lock);
> > > + slot = gfn_to_memslot(kvm, addr);
> > > + if (!slot)
> > > + __kvm_set_memory_region(kvm, ®ion);
The return value definitely should be checked, especially if the memory regions
are not KVM-internal, i.e. if userspace is allowed to create memslots.
> > > + mutex_unlock(&kvm->slots_lock);
> > > +}
> > > +
> >
> > Would simply having one slot from 0 to TASK_SIZE also work? This could avoid the
> > construction of the fake slots during runtime.
>
> unfortunately memslots are limited to 4TiB.
> having bigger ones would require even more changes all across KVM (and
> maybe qemu too)
AFAIK, that limitation exists purely because of dirty bitmaps. IIUC, these "fake"
memslots are not intended to be visible to userspace, or at the very least don't
*need* to be visible to userspace.
Assuming that's true, they/it can/should be KVM-internal memslots, and those
should never be dirty-logged. x86 allocates metadata based on slot size, so in
practice creating a mega-slot will never succeed on x86, but the only size
limitation I see in s390 is on arch.mem_limit, but for ucontrol that's set to -1ull,
i.e. is a non-issue.
I have a series (that I need to refresh) to provide a dedicated API for creating
internal memslots, and to also enforce that flags == 0 for internal memslots,
i.e. to enforce that dirty logging is never enabled (see Link below). With that
I mind, I can't think of any reason to disallow a 0 => TASK_SIZE memslot so long
as it's KVM-defined.
Using a single memslot would hopefully allow s390 to unconditionally carve out a
KVM-internal memslot, i.e. not have to condition the logic on the type of VM. E.g.
#define KVM_INTERNAL_MEM_SLOTS 1
#define KVM_S390_UCONTROL_MEMSLOT (KVM_USER_MEM_SLOTS + 0)
And then I think just this?
---
From: Sean Christopherson <seanjc@xxxxxxxxxx>
Date: Fri, 10 Jan 2025 08:05:09 -0800
Subject: [PATCH] KVM: Do not restrict the size of KVM-internal memory regions
Exempt KVM-internal memslots from the KVM_MEM_MAX_NR_PAGES restriction, as
the limit on the number of pages exists purely to play nice with dirty
bitmap operations, which use 32-bit values to index the bitmaps, and dirty
logging isn't supported for KVM-internal memslots.
Link: https://lore.kernel.org/all/20240802205003.353672-6-seanjc@xxxxxxxxxx
Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
virt/kvm/kvm_main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8a0d0d37fb17..3cea406c34db 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1972,7 +1972,15 @@ int __kvm_set_memory_region(struct kvm *kvm,
return -EINVAL;
if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
return -EINVAL;
- if ((mem->memory_size >> PAGE_SHIFT) > KVM_MEM_MAX_NR_PAGES)
+
+ /*
+ * The size of userspace-defined memory regions is restricted in order
+ * to play nice with dirty bitmap operations, which are indexed with an
+ * "unsigned int". KVM's internal memory regions don't support dirty
+ * logging, and so are exempt.
+ */
+ if (id < KVM_USER_MEM_SLOTS &&
+ (mem->memory_size >> PAGE_SHIFT) > KVM_MEM_MAX_NR_PAGES)
return -EINVAL;
slots = __kvm_memslots(kvm, as_id);
base-commit: 1aadfba8419606d447d1961f25e2d312011ad45a
--
