diff mbox series

[RFCv1,6/8] kvm: Add helper kvm_dirty_ring_init()

Message ID 20230206112010.99871-7-gshan@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/arm/virt: Support dirty ring | expand

Commit Message

Gavin Shan Feb. 6, 2023, 11:20 a.m. UTC
Due to multiple capabilities associated with the dirty ring for different
architectures: KVM_CAP_DIRTY_{LOG_RING, LOG_RING_ACQ_REL} for x86 and
arm64 separately. There will be more to be done in order to support the
dirty ring for arm64.

Lets add helper kvm_dirty_ring_init() to enable the dirty ring. With this,
the code looks a bit clean.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 accel/kvm/kvm-all.c | 73 ++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 27 deletions(-)

Comments

Juan Quintela Feb. 8, 2023, 10:11 p.m. UTC | #1
Gavin Shan <gshan@redhat.com> wrote:
> Due to multiple capabilities associated with the dirty ring for different
> architectures: KVM_CAP_DIRTY_{LOG_RING, LOG_RING_ACQ_REL} for x86 and
> arm64 separately. There will be more to be done in order to support the
> dirty ring for arm64.
>
> Lets add helper kvm_dirty_ring_init() to enable the dirty ring. With this,
> the code looks a bit clean.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 73 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 9ec117c441..399ef0f7e6 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1476,6 +1476,49 @@ static int kvm_dirty_ring_reaper_init(KVMState *s)
>      return 0;
>  }
>  
> +static int kvm_dirty_ring_init(KVMState *s)
> +{
> +    uint64_t ring_bytes;
> +    int ret;
> +
> +    /*
> +     * Read the max supported pages. Fall back to dirty logging mode
> +     * if the dirty ring isn't supported.
> +     */
> +    ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
> +    if (ret <= 0) {
> +        warn_report("KVM dirty ring not available, using bitmap method");
> +        s->kvm_dirty_ring_size = 0;
> +        return 0;
> +    }
> +
> +    ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
> +    if (ring_bytes > ret) {
> +        error_report("KVM dirty ring size %" PRIu32 " too big "
> +                     "(maximum is %ld).  Please use a smaller value.",
> +                     s->kvm_dirty_ring_size,
> +                     (long)ret / sizeof(struct kvm_dirty_gfn));
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
> +    if (ret) {
> +        error_report("Enabling of KVM dirty ring failed: %s. "
> +                     "Suggested minimum value is 1024.", strerror(-ret));
> +        ret = -EIO;
> +    }
> +
> +out:
> +    if (ret) {
> +        s->kvm_dirty_ring_size = 0;
> +    } else {
> +        s->kvm_dirty_ring_bytes = ring_bytes;
> +    }
> +
> +    return ret;
> +}

If you split it, you don't need the goto.

static int kvm_dirty_ring_init(KVMState *s)
{
    s->kvm_dirty_ring_size = 0;
    /*
     * Read the max supported pages. Fall back to dirty logging mode
     * if the dirty ring isn't supported.
     */
    int ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
    if (ret <= 0) {
        warn_report("KVM dirty ring not available, using bitmap method");
        return 0;
    }

    uint64_t ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
    if (ring_bytes > ret) {
        error_report("KVM dirty ring size %" PRIu32 " too big "
                     "(maximum is %ld).  Please use a smaller value.",
                     s->kvm_dirty_ring_size,
                     (long)ret / sizeof(struct kvm_dirty_gfn));
        return -EINVAL;
    }

    ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
    if (ret) {
        error_report("Enabling of KVM dirty ring failed: %s. "
                     "Suggested minimum value is 1024.", strerror(-ret));
        return -EIO;
    }

    s->kvm_dirty_ring_bytes = ring_bytes;
    return ret;
}



> +
>  static void kvm_region_add(MemoryListener *listener,
>                             MemoryRegionSection *section)
>  {
> @@ -2545,33 +2588,9 @@ static int kvm_init(MachineState *ms)
>       * dirty logging mode
>       */
>      if (s->kvm_dirty_ring_size > 0) {
> -        uint64_t ring_bytes;
> -
> -        ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
> -
> -        /* Read the max supported pages */
> -        ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
> -        if (ret > 0) {
> -            if (ring_bytes > ret) {
> -                error_report("KVM dirty ring size %" PRIu32 " too big "
> -                             "(maximum is %ld).  Please use a smaller value.",
> -                             s->kvm_dirty_ring_size,
> -                             (long)ret / sizeof(struct kvm_dirty_gfn));
> -                ret = -EINVAL;
> -                goto err;
> -            }
> -
> -            ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
> -            if (ret) {
> -                error_report("Enabling of KVM dirty ring failed: %s. "
> -                             "Suggested minimum value is 1024.", strerror(-ret));
> -                goto err;
> -            }
> -
> -            s->kvm_dirty_ring_bytes = ring_bytes;
> -         } else {
> -             warn_report("KVM dirty ring not available, using bitmap method");
> -             s->kvm_dirty_ring_size = 0;
> +        ret = kvm_dirty_ring_init(s);
> +        if (ret < 0) {
> +            goto err;
>          }
>      }
Gavin Shan Feb. 9, 2023, 9:43 a.m. UTC | #2
On 2/9/23 9:11 AM, Juan Quintela wrote:
> Gavin Shan <gshan@redhat.com> wrote:
>> Due to multiple capabilities associated with the dirty ring for different
>> architectures: KVM_CAP_DIRTY_{LOG_RING, LOG_RING_ACQ_REL} for x86 and
>> arm64 separately. There will be more to be done in order to support the
>> dirty ring for arm64.
>>
>> Lets add helper kvm_dirty_ring_init() to enable the dirty ring. With this,
>> the code looks a bit clean.
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   accel/kvm/kvm-all.c | 73 ++++++++++++++++++++++++++++-----------------
>>   1 file changed, 46 insertions(+), 27 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 9ec117c441..399ef0f7e6 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1476,6 +1476,49 @@ static int kvm_dirty_ring_reaper_init(KVMState *s)
>>       return 0;
>>   }
>>   
>> +static int kvm_dirty_ring_init(KVMState *s)
>> +{
>> +    uint64_t ring_bytes;
>> +    int ret;
>> +
>> +    /*
>> +     * Read the max supported pages. Fall back to dirty logging mode
>> +     * if the dirty ring isn't supported.
>> +     */
>> +    ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
>> +    if (ret <= 0) {
>> +        warn_report("KVM dirty ring not available, using bitmap method");
>> +        s->kvm_dirty_ring_size = 0;
>> +        return 0;
>> +    }
>> +
>> +    ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
>> +    if (ring_bytes > ret) {
>> +        error_report("KVM dirty ring size %" PRIu32 " too big "
>> +                     "(maximum is %ld).  Please use a smaller value.",
>> +                     s->kvm_dirty_ring_size,
>> +                     (long)ret / sizeof(struct kvm_dirty_gfn));
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
>> +    if (ret) {
>> +        error_report("Enabling of KVM dirty ring failed: %s. "
>> +                     "Suggested minimum value is 1024.", strerror(-ret));
>> +        ret = -EIO;
>> +    }
>> +
>> +out:
>> +    if (ret) {
>> +        s->kvm_dirty_ring_size = 0;
>> +    } else {
>> +        s->kvm_dirty_ring_bytes = ring_bytes;
>> +    }
>> +
>> +    return ret;
>> +}
> 
> If you split it, you don't need the goto.
> 
> static int kvm_dirty_ring_init(KVMState *s)
> {
>      s->kvm_dirty_ring_size = 0;
>      /*
>       * Read the max supported pages. Fall back to dirty logging mode
>       * if the dirty ring isn't supported.
>       */
>      int ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
>      if (ret <= 0) {
>          warn_report("KVM dirty ring not available, using bitmap method");
>          return 0;
>      }
> 
>      uint64_t ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
>      if (ring_bytes > ret) {
>          error_report("KVM dirty ring size %" PRIu32 " too big "
>                       "(maximum is %ld).  Please use a smaller value.",
>                       s->kvm_dirty_ring_size,
>                       (long)ret / sizeof(struct kvm_dirty_gfn));
>          return -EINVAL;
>      }
> 
>      ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
>      if (ret) {
>          error_report("Enabling of KVM dirty ring failed: %s. "
>                       "Suggested minimum value is 1024.", strerror(-ret));
>          return -EIO;
>      }
> 
>      s->kvm_dirty_ring_bytes = ring_bytes;
>      return ret;
> }
> 

Ok, thanks for your review. The goto will be removed in next revision.

> 
> 
>> +
>>   static void kvm_region_add(MemoryListener *listener,
>>                              MemoryRegionSection *section)
>>   {
>> @@ -2545,33 +2588,9 @@ static int kvm_init(MachineState *ms)
>>        * dirty logging mode
>>        */
>>       if (s->kvm_dirty_ring_size > 0) {
>> -        uint64_t ring_bytes;
>> -
>> -        ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
>> -
>> -        /* Read the max supported pages */
>> -        ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
>> -        if (ret > 0) {
>> -            if (ring_bytes > ret) {
>> -                error_report("KVM dirty ring size %" PRIu32 " too big "
>> -                             "(maximum is %ld).  Please use a smaller value.",
>> -                             s->kvm_dirty_ring_size,
>> -                             (long)ret / sizeof(struct kvm_dirty_gfn));
>> -                ret = -EINVAL;
>> -                goto err;
>> -            }
>> -
>> -            ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
>> -            if (ret) {
>> -                error_report("Enabling of KVM dirty ring failed: %s. "
>> -                             "Suggested minimum value is 1024.", strerror(-ret));
>> -                goto err;
>> -            }
>> -
>> -            s->kvm_dirty_ring_bytes = ring_bytes;
>> -         } else {
>> -             warn_report("KVM dirty ring not available, using bitmap method");
>> -             s->kvm_dirty_ring_size = 0;
>> +        ret = kvm_dirty_ring_init(s);
>> +        if (ret < 0) {
>> +            goto err;
>>           }
>>       }
> 
Thanks,
Gavin
diff mbox series

Patch

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 9ec117c441..399ef0f7e6 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1476,6 +1476,49 @@  static int kvm_dirty_ring_reaper_init(KVMState *s)
     return 0;
 }
 
+static int kvm_dirty_ring_init(KVMState *s)
+{
+    uint64_t ring_bytes;
+    int ret;
+
+    /*
+     * Read the max supported pages. Fall back to dirty logging mode
+     * if the dirty ring isn't supported.
+     */
+    ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
+    if (ret <= 0) {
+        warn_report("KVM dirty ring not available, using bitmap method");
+        s->kvm_dirty_ring_size = 0;
+        return 0;
+    }
+
+    ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
+    if (ring_bytes > ret) {
+        error_report("KVM dirty ring size %" PRIu32 " too big "
+                     "(maximum is %ld).  Please use a smaller value.",
+                     s->kvm_dirty_ring_size,
+                     (long)ret / sizeof(struct kvm_dirty_gfn));
+        ret = -EINVAL;
+        goto out;
+    }
+
+    ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
+    if (ret) {
+        error_report("Enabling of KVM dirty ring failed: %s. "
+                     "Suggested minimum value is 1024.", strerror(-ret));
+        ret = -EIO;
+    }
+
+out:
+    if (ret) {
+        s->kvm_dirty_ring_size = 0;
+    } else {
+        s->kvm_dirty_ring_bytes = ring_bytes;
+    }
+
+    return ret;
+}
+
 static void kvm_region_add(MemoryListener *listener,
                            MemoryRegionSection *section)
 {
@@ -2545,33 +2588,9 @@  static int kvm_init(MachineState *ms)
      * dirty logging mode
      */
     if (s->kvm_dirty_ring_size > 0) {
-        uint64_t ring_bytes;
-
-        ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn);
-
-        /* Read the max supported pages */
-        ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING);
-        if (ret > 0) {
-            if (ring_bytes > ret) {
-                error_report("KVM dirty ring size %" PRIu32 " too big "
-                             "(maximum is %ld).  Please use a smaller value.",
-                             s->kvm_dirty_ring_size,
-                             (long)ret / sizeof(struct kvm_dirty_gfn));
-                ret = -EINVAL;
-                goto err;
-            }
-
-            ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
-            if (ret) {
-                error_report("Enabling of KVM dirty ring failed: %s. "
-                             "Suggested minimum value is 1024.", strerror(-ret));
-                goto err;
-            }
-
-            s->kvm_dirty_ring_bytes = ring_bytes;
-         } else {
-             warn_report("KVM dirty ring not available, using bitmap method");
-             s->kvm_dirty_ring_size = 0;
+        ret = kvm_dirty_ring_init(s);
+        if (ret < 0) {
+            goto err;
         }
     }