RE: [RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Gavin Shan [mailto:gshan@redhat.com]
> Sent: 07 August 2023 06:53
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: peter.maydell@linaro.org; ricarkol@google.com; kvm@vger.kernel.org;
> Jonathan Cameron <jonathan.cameron@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [RFC PATCH] arm/kvm: Enable support for
> KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
> 
> 
> On 7/26/23 01:00, Shameer Kolothum wrote:
> > Now that we have Eager Page Split support added for ARM in the kernel[0],
> > enable it in Qemu. This adds,
> >   -eager-split-size to Qemu options to set the eager page split chunk size.
> >   -enable KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE.
> >
> > The chunk size specifies how many pages to break at a time, using a
> > single allocation. Bigger the chunk size, more pages need to be
> > allocated ahead of time.
> >
> > Notes:
> >   - I am not sure whether we need to call kvm_vm_check_extension() for
> >     KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE or not as kernel seems to
> disable
> >     eager page size by default and it will return zero always.
> >
> >    -ToDo: Update qemu-options.hx
> >
> > [0]:
> https://lore.kernel.org/all/168426111477.3193133.1074810619984378093
> 0.b4-ty@linux.dev/
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   include/sysemu/kvm_int.h |  1 +
> >   target/arm/kvm.c         | 73
> ++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 74 insertions(+)
> >
> > diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
> > index 511b42bde5..03a1660d40 100644
> > --- a/include/sysemu/kvm_int.h
> > +++ b/include/sysemu/kvm_int.h
> > @@ -116,6 +116,7 @@ struct KVMState
> >       uint64_t kvm_dirty_ring_bytes;  /* Size of the per-vcpu dirty ring
> */
> >       uint32_t kvm_dirty_ring_size;   /* Number of dirty GFNs per ring
> */
> >       bool kvm_dirty_ring_with_bitmap;
> > +    uint64_t kvm_eager_split_size; /* Eager Page Splitting chunk size */
> >       struct KVMDirtyRingReaper reaper;
> >       NotifyVmexitOption notify_vmexit;
> >       uint32_t notify_window;
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index b4c7654f49..985d901062 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -30,6 +30,7 @@
> >   #include "exec/address-spaces.h"
> >   #include "hw/boards.h"
> >   #include "hw/irq.h"
> > +#include "qapi/visitor.h"
> >   #include "qemu/log.h"
> >
> >   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> > @@ -247,6 +248,23 @@ int
> kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
> >       return ret > 0 ? ret : 40;
> >   }
> >
> > +static bool kvm_arm_eager_split_size_valid(uint64_t req_size, uint32_t
> sizes)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < sizeof(uint32_t) * BITS_PER_BYTE; i++) {
> > +        if (!(sizes & (1 << i))) {
> > +            continue;
> > +        }
> > +
> > +        if (req_size == (1 << i)) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >   int kvm_arch_init(MachineState *ms, KVMState *s)
> >   {
> >       int ret = 0;
> > @@ -280,6 +298,21 @@ int kvm_arch_init(MachineState *ms, KVMState
> *s)
> >           }
> >       }
> >
> > +    if (s->kvm_eager_split_size) {
> > +        uint32_t sizes;
> > +
> > +        sizes = kvm_vm_check_extension(s,
> KVM_CAP_ARM_SUPPORTED_BLOCK_SIZES);
> > +        if (!sizes) {
> > +            error_report("Eager Page Split not supported on host");
> > +        } else if
> (!kvm_arm_eager_split_size_valid(s->kvm_eager_split_size,
> > +                                                   sizes)) {
> > +            error_report("Eager Page Split requested chunk size not
> valid");
> > +        } else if (kvm_vm_enable_cap(s,
> KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE, 0,
> > +                                     s->kvm_eager_split_size)) {
> > +            error_report("Failed to set Eager Page Split chunk size");
> > +        }
> > +    }
> > +
> >       kvm_arm_init_debug(s);
> >
> >       return ret;
> 
> Do we really want to fail when KVM_CAP_ARM_SUPPORTED_BLOCK_SIZES
> isn't supported?
> I think the appropriate behavior is to warn and clear s->kvm_eager_split_size
> for this specific case, similar to what we are doing for 
> s->kvm_dirty_ring_size
> in kvm_dirty_ring_init(). With this, the behavior is backwards compatible to
> the
> old host kernels.

Ok. That makes sense. Will update.

Thanks,
Shameer

> 
> > @@ -1062,6 +1095,46 @@ bool
> kvm_arch_cpu_check_are_resettable(void)
> >       return true;
> >   }
> >
> > +static void kvm_arch_get_eager_split_size(Object *obj, Visitor *v,
> > +                                          const char *name, void
> *opaque,
> > +                                          Error **errp)
> > +{
> > +    KVMState *s = KVM_STATE(obj);
> > +    uint64_t value = s->kvm_eager_split_size;
> > +
> > +    visit_type_size(v, name, &value, errp);
> > +}
> > +
> > +static void kvm_arch_set_eager_split_size(Object *obj, Visitor *v,
> > +                                          const char *name, void
> *opaque,
> > +                                          Error **errp)
> > +{
> > +    KVMState *s = KVM_STATE(obj);
> > +    uint64_t value;
> > +
> > +    if (s->fd != -1) {
> > +        error_setg(errp, "Cannot set properties after the accelerator has
> been initialized");
> > +        return;
> > +    }
> > +
> > +    if (!visit_type_size(v, name, &value, errp)) {
> > +        return;
> > +    }
> > +
> > +    if (value & (value - 1)) {
> > +        error_setg(errp, "early-split-size must be a power of two.");
> > +        return;
> > +    }
> > +
> > +    s->kvm_eager_split_size = value;
> > +}
> > +
> >   void kvm_arch_accel_class_init(ObjectClass *oc)
> >   {
> > +    object_class_property_add(oc, "eager-split-size", "size",
> > +                              kvm_arch_get_eager_split_size,
> > +                              kvm_arch_set_eager_split_size, NULL,
> NULL);
> > +
> > +    object_class_property_set_description(oc, "eager-split-size",
> > +        "Configure Eager Page Split chunk size for hugepages. (default: 0,
> disabled)");
> >   }
> 
> Thanks,
> Gavin