Re: [PATCH RFC 8/9] KVM: Add dirty-ring-size property

[Peter Xu]

On Wed, Mar 25, 2020 at 08:00:31PM +0000, Dr. David Alan Gilbert wrote:
> > @@ -2077,6 +2079,33 @@ static int kvm_init(MachineState *ms)
> >      s->memory_listener.listener.coalesced_io_add = 
> > kvm_coalesce_mmio_region;
> >      s->memory_listener.listener.coalesced_io_del = 
> > kvm_uncoalesce_mmio_region;
> >  
> > +    /*
> > +     * Enable KVM dirty ring if supported, otherwise fall back to
> > +     * dirty logging mode
> > +     */
> > +    if (s->kvm_dirty_ring_size > 0) {
> > +        /* Read the max supported pages */
> > +        ret = kvm_vm_check_extension(kvm_state, KVM_CAP_DIRTY_LOG_RING);
> > +        if (ret > 0) {
> > +            if (s->kvm_dirty_ring_size > ret) {
> > +                error_report("KVM dirty ring size %d too big (maximum is 
> > %d). "
> > +                             "Please use a smaller value.",
> > +                             s->kvm_dirty_ring_size, ret);
> > +                goto err;
> > +            }
> > +
> > +            ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0,
> > +                                    s->kvm_dirty_ring_size);
> > +            if (ret) {
> > +                error_report("Enabling of KVM dirty ring failed: %d", ret);
> > +                goto err;
> > +            }
> > +
> > +            s->kvm_dirty_gfn_count =
> > +                s->kvm_dirty_ring_size / sizeof(struct kvm_dirty_gfn);
> 
> What happens if I was to pass dirty-ring-size=1 ?
> Then the count would be 0 and things would get upset somewhere?
> Do you need to check for a minimum postive value?

The above kvm_vm_enable_cap() should fail directly and QEMU will stop.
Yes we should check it, but kernel will do that in all cases, so I
just didn't do that explicitly again in the userspace.

I was planning this to be an advanced feature so the user of this
parameter should know the rules to pass values in.

Of course we can introduce another global knob to enable/disable this
feature as a whole, then I can offer a default value for the size
parameter.  I just didn't bother that in this RFC version, but I can
switch to that if that's preferred.

[...]

> > @@ -3065,6 +3123,12 @@ static void kvm_accel_class_init(ObjectClass *oc, 
> > void *data)
> >          NULL, NULL, &error_abort);
> >      object_class_property_set_description(oc, "kvm-shadow-mem",
> >          "KVM shadow MMU size", &error_abort);
> > +
> > +    object_class_property_add(oc, "dirty-ring-size", "int",
> > +        kvm_get_dirty_ring_size, kvm_set_dirty_ring_size,
> > +        NULL, NULL, &error_abort);
> 
> I don't think someone passing in a non-number should cause an abort;
> it should exit, but I don't think it should abort/core.

It won't - &error_abort is for the operation to add the property.  It
should never fail.

User illegal input will look like this (a graceful exit):

$ ./x86_64-softmmu/qemu-system-x86_64 -accel kvm,dirty-ring-size=a
qemu-system-x86_64: -accel kvm,dirty-ring-size=a: Parameter 'dirty-ring-size' 
expects int64

> 
> > +    object_class_property_set_description(oc, "dirty-ring-size",
> > +        "KVM dirty ring size (<=0 to disable)", &error_abort);
> >  }
> >  
> >  static const TypeInfo kvm_accel_type = {
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 224a8e8712..140bd38726 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -119,6 +119,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> >      "                kernel-irqchip=on|off|split controls accelerated 
> > irqchip support (default=on)\n"
> >      "                kvm-shadow-mem=size of KVM shadow MMU in bytes\n"
> >      "                tb-size=n (TCG translation block cache size)\n"
> > +    "                dirty-ring-size=n (KVM dirty ring size in Bytes)\n"
> >      "                thread=single|multi (enable multi-threaded TCG)\n", 
> > QEMU_ARCH_ALL)
> >  STEXI
> >  @item -accel @var{name}[,prop=@var{value}[,...]]
> > @@ -140,6 +141,8 @@ irqchip completely is not recommended except for 
> > debugging purposes.
> >  Defines the size of the KVM shadow MMU.
> >  @item tb-size=@var{n}
> >  Controls the size (in MiB) of the TCG translation block cache.
> > +@item dirty-ring-size=@val{n}
> > +Controls the size (in Bytes) of KVM dirty ring (<=0 to disable).
> 
> I don't see the point in allowing < 0 ; I'd ban it.

Sure; I can switch to an uint64.

Thanks,

-- 
Peter Xu