> -----Original Message----- > From: Gavin Shan [mailto:gshan@xxxxxxxxxx] > Sent: 31 August 2023 02:43 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>; > qemu-devel@xxxxxxxxxx; qemu-arm@xxxxxxxxxx > Cc: peter.maydell@xxxxxxxxxx; ricarkol@xxxxxxxxxx; Jonathan Cameron > <jonathan.cameron@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; Linuxarm > <linuxarm@xxxxxxxxxx> > Subject: Re: [PATCH v3] arm/kvm: Enable support for > KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE > > Hi Shameer, > > On 8/30/23 21:48, Shameer Kolothum wrote: > > Now that we have Eager Page Split support added for ARM in the kernel, > > enable it in Qemu. This adds, > > -eager-split-size to -accel sub-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. > > > > Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@xxxxxxxxxx> > > --- > > v2: > https://lore.kernel.org/qemu-devel/20230815092709.1290-1-shameerali.kol > othum.thodi@xxxxxxxxxx/ > > -Addressed comments from Gavin(Thanks). > > RFC v1: > https://lore.kernel.org/qemu-devel/20230725150002.621-1-shameerali.kolo > thum.thodi@xxxxxxxxxx/ > > -Updated qemu-options.hx with description > > -Addressed review comments from Peter and Gavin(Thanks). > > --- > > accel/kvm/kvm-all.c | 1 + > > include/sysemu/kvm_int.h | 1 + > > qemu-options.hx | 15 +++++++++ > > target/arm/kvm.c | 68 > ++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 85 insertions(+) > > > > One more question below. Please check if it's worthy to be addressed in v4, > needed > to resolved other comments. Otherwise, it looks fine to me. > > Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx> Thanks. I will send out a v4 with the above tag and the below suggestion to get rid of the kvm_arm_eager_split_size_valid(). Shameer. > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index 2ba7521695..ff1578bb32 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -3763,6 +3763,7 @@ static void kvm_accel_instance_init(Object *obj) > > /* KVM dirty ring is by default off */ > > s->kvm_dirty_ring_size = 0; > > s->kvm_dirty_ring_with_bitmap = false; > > + s->kvm_eager_split_size = 0; > > s->notify_vmexit = NOTIFY_VMEXIT_OPTION_RUN; > > s->notify_window = 0; > > s->xen_version = 0; > > diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h > > index 511b42bde5..a5b9122cb8 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/qemu-options.hx b/qemu-options.hx > > index 29b98c3d4c..2e70704ee8 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -186,6 +186,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel, > > " split-wx=on|off (enable TCG split w^x > mapping)\n" > > " tb-size=n (TCG translation block cache size)\n" > > " dirty-ring-size=n (KVM dirty ring GFN count, > default 0)\n" > > + " eager-split-size=n (KVM Eager Page Split chunk > size, default 0, disabled. ARM only)\n" > > " > notify-vmexit=run|internal-error|disable,notify-window=n (enable notify VM > exit and set notify window, x86 only)\n" > > " thread=single|multi (enable multi-threaded > TCG)\n", QEMU_ARCH_ALL) > > SRST > > @@ -244,6 +245,20 @@ SRST > > is disabled (dirty-ring-size=0). When enabled, KVM will > instead > > record dirty pages in a bitmap. > > > > + ``eager-split-size=n`` > > + KVM implements dirty page logging at the PAGE_SIZE granularity > and > > + enabling dirty-logging on a huge-page requires breaking it into > > + PAGE_SIZE pages in the first place. KVM on ARM does this > splitting > > + lazily by default. There are performance benefits in doing > huge-page > > + split eagerly, especially in situations where TLBI costs associated > > + with break-before-make sequences are considerable and also if > guest > > + workloads are read intensive. The size here specifies how many > pages > > + to break at a time and needs to be a valid block size which is > > + 1GB/2MB/4KB, 32MB/16KB and 512MB/64KB for > 4KB/16KB/64KB PAGE_SIZE > > + respectively. Be wary of specifying a higher size as it will have an > > + impact on the memory. By default, this feature is disabled > > + (eager-split-size=0). > > + > > ``notify-vmexit=run|internal-error|disable,notify-window=n`` > > Enables or disables notify VM exit support on x86 host and > specify > > the corresponding notify window to trigger the VM exit if > enabled. > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > > index 23aeb09949..28d81ca790 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,12 @@ int > kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa) > > return ret > 0 ? ret : 40; > > } > > > > +static inline bool kvm_arm_eager_split_size_valid(uint64_t req_size, > > + uint32_t sizes) > > +{ > > + return req_size & sizes; > > +} > > + > > int kvm_arch_get_default_type(MachineState *ms) > > { > > bool fixed_ipa; > > @@ -287,6 +294,27 @@ 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) { > > + s->kvm_eager_split_size = 0; > > + warn_report("Eager Page Split support not available"); > > + } else if > (!kvm_arm_eager_split_size_valid(s->kvm_eager_split_size, > > + sizes)) { > > + error_report("Eager Page Split requested chunk size not > valid"); > > + ret = -EINVAL; > > + } else { > > + ret = kvm_vm_enable_cap(s, > KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE, 0, > > + s->kvm_eager_split_size); > > + if (ret < 0) { > > + error_report("Enabling of Eager Page Split failed: %s", > > + strerror(-ret)); > > + } > > + } > > + } > > + > > kvm_arm_init_debug(s); > > > > return ret; > > The function kvm_arm_eager_split_size_valid() was suggested by Peter if I'm > correct. > However, it seems we needn't it any more since it's called for once. Why not > simply to > have something like below? The detailed error message can help to explain > why we > need the condition of (s->kvm_eager_split_size & sizes) here. > > } else if (s->kvm_eager_split_size & sizes) { > error_report("Unsupported Eager Page Split chunk size 0x%lx by > 0x%x", > s->kvm_eager_split_size, sizes); > ret = -EINVAL; > } > > > @@ -1069,6 +1097,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, "Unable to set early-split-size after KVM has > been initialized"); > > + return; > > + } > > + > > + if (!visit_type_size(v, name, &value, errp)) { > > + return; > > + } > > + > > + if (value && !is_power_of_2(value)) { > > + 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", > > + "Eager Page Split chunk size for hugepages. (default: 0, > disabled)"); > > } > > Thanks, > Gavin
