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

Shameer Kolothum via posted 1 patch 9 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230725150002.621-1-shameerali.kolothum.thodi@huawei.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Maydell <peter.maydell@linaro.org>
There is a newer version of this series
include/sysemu/kvm_int.h |  1 +
target/arm/kvm.c         | 73 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+)
[RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
Posted by Shameer Kolothum via 9 months, 2 weeks ago
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.10748106199843780930.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;
@@ -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)");
 }
-- 
2.34.1
Re: [RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
Posted by Gavin Shan 9 months ago
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.10748106199843780930.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.


> @@ -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
Re: [RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
Posted by Peter Maydell 9 months, 2 weeks ago
On Tue, 25 Jul 2023 at 16:01, Shameer Kolothum
<shameerali.kolothum.thodi@huawei.com> 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.

It looks from the code like you've added a new sub-option
to -accel, not a new global option. This is the right thing,
but your commit message should document the actual option syntax
to avoid confusion.

> 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.10748106199843780930.b4-ty@linux.dev/

Speaking of confusion, this message says "It's an optimization used
in Google Cloud since 2016 on x86, and for the last couple of months
on ARM." so I'm not sure why we've ended up with an Arm-specific
KVM_CAP and code in target/arm/kvm.c rather than something more
generic ?

If this is going to arrive for other architectures in the future
we should probably think about whether some of this code should
be generic, not arm-specific.

Also this seems to be an obscure tuning parameter -- it could
use good documentation so users have some idea when it can help.

As a more specific case of that: the kernel patchset says it
makes Arm do the same thing that x86 already does, and split
the huge pages automatically based on use of the dirty log.
If the kernel can do this automatically and we never felt
the need to provide a manual tuning knob for x86, do we even
need to expose the Arm manual control via QEMU?

Other than that, I have a few minor coding things below.

> +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;
> +        }
> +    }

We know req_size is a power of 2 here, so if you also explicitly
rule out 0 then you can do
     return sizes & (1 << ctz64(req_size));
rather than having to loop through. (Need to rule out 0
because otherwise ctz64() returns 64 and the shift is UB.)

> +
> +    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;
> @@ -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)) {

"if (!is_power_of_2(value))" is a clearer way to write this.

> +        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
-- PMM
RE: [RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
Posted by Shameerali Kolothum Thodi via 9 months, 1 week ago

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: 27 July 2023 16:43
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.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 Tue, 25 Jul 2023 at 16:01, Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com> 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.
> 
> It looks from the code like you've added a new sub-option
> to -accel, not a new global option. This is the right thing,
> but your commit message should document the actual option syntax
> to avoid confusion.

Ok. Will update the commit message.

> > 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/
> 
> Speaking of confusion, this message says "It's an optimization used
> in Google Cloud since 2016 on x86, and for the last couple of months
> on ARM." so I'm not sure why we've ended up with an Arm-specific
> KVM_CAP and code in target/arm/kvm.c rather than something more
> generic ?
> 
> If this is going to arrive for other architectures in the future
> we should probably think about whether some of this code should
> be generic, not arm-specific.
> 
> Also this seems to be an obscure tuning parameter -- it could
> use good documentation so users have some idea when it can help.
> 
> As a more specific case of that: the kernel patchset says it
> makes Arm do the same thing that x86 already does, and split
> the huge pages automatically based on use of the dirty log.
> If the kernel can do this automatically and we never felt
> the need to provide a manual tuning knob for x86, do we even
> need to expose the Arm manual control via QEMU?

From the history of the above series, it looks like, the main argument
for making this a user adjustable knob for ARM is because of the upfront
extra memory allocations required in kernel associated with splitting the
block page. 

https://lore.kernel.org/kvmarm/86v8ktkqfx.wl-maz@kernel.org/

https://lore.kernel.org/kvmarm/86h6w70zhc.wl-maz@kernel.org/

And the knob for x86 case is a kvm module_param(eager_page_split).
Not clear to me why x86 opted for a module_param per KVM but not
per VM user space one. The discussion can be found here,
https://lore.kernel.org/kvm/YaDrmNVsXSMXR72Z@xz-m1.local/#t


> Other than that, I have a few minor coding things below.
> 
> > +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;
> > +        }
> > +    }
> 
> We know req_size is a power of 2 here, so if you also explicitly
> rule out 0 then you can do
>      return sizes & (1 << ctz64(req_size));
> rather than having to loop through. (Need to rule out 0
> because otherwise ctz64() returns 64 and the shift is UB.)

Yes, missed that we already handled the != power of 2 case.
Will update as per your next comment on this patch. That
is much simpler. Thanks.

> 
> > +
> > +    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;
> > @@ -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)) {
> 
> "if (!is_power_of_2(value))" is a clearer way to write this.

Ok. Will update in next.

Thanks for taking a look and sorry for late reply, was away.

Shameer

> 
> > +        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
> -- PMM
Re: [RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
Posted by Peter Maydell 9 months, 2 weeks ago
On Thu, 27 Jul 2023 at 16:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 25 Jul 2023 at 16:01, Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com> wrote:
> > +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;
> > +        }
> > +    }
>
> We know req_size is a power of 2 here, so if you also explicitly
> rule out 0 then you can do
>      return sizes & (1 << ctz64(req_size));

Er, that's also over-complicated. Just
   return sizes & req_size;

should do (and catches the 0 case correctly again).

-- PMM