Re: [PATCH v9 3/7] KVM: Support dirty ring in conjunction with bitmap

[Sean Christopherson <seanjc@google.com>]

On Wed, Nov 09, 2022, Gavin Shan wrote:
> Hi Sean,
> 
> On 11/9/22 8:05 AM, Sean Christopherson wrote:
> > On Wed, Nov 09, 2022, Gavin Shan wrote:
> > > On 11/9/22 12:25 AM, Sean Christopherson wrote:
> > > > I have no objection to disallowing userspace from disabling the combo, but I
> > > > think it's worth requiring cap->args[0] to be '0' just in case we change our minds
> > > > in the future.
> > > > 
> > > 
> > > I assume you're suggesting to have non-zero value in cap->args[0] to enable the
> > > capability.
> > > 
> > >      if (!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP) ||
> > >          !kvm->dirty_ring_size || !cap->args[0])
> > >          return r;
> > 
> > I was actually thinking of taking the lazy route and requiring userspace to zero
> > the arg, i.e. treat it as a flags extensions.  Oh, wait, that's silly.  I always
> > forget that `cap->flags` exists.
> > 
> > Just this?
> > 
> > 	if (!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP) ||
> > 	    !kvm->dirty_ring_size || cap->flags)
> > 		return r;
> > 
> > It'll be kinda awkward if KVM ever does add a flag to disable the bitmap, but
> > that's seems quite unlikely and not the end of the world if it does happen.  And
> > on the other hand, requiring '0' is less weird and less annoying for userspace
> > _now_.
> > 
> 
> I don't quiet understand the term "lazy route".

"lazy" in that requiring a non-zero value would mean adding another #define,
otherwise the extensibility is limited to two values.  Again, unlikely to matter,
but it wouldn't make sense to go through the effort to provide some extensibility
and then only allow for one possible extension.  If KVM is "lazy" and just requires
flags to be '0', then there's no need for more #defines, and userspace doesn't
have to pass more values in its enabling.

> So you're still thinking of the possibility to allow disabling the capability
> in future?

Yes, or more likely, tweaking the behavior of ring+bitmap.  As is, the behavior
is purely a fallback for a single case where KVM can't push to the dirty ring due
to not having a running vCPU.  It's possible someone might come up with a use case
where they want KVM to do something different, e.g. fallback to the bitmap if the
ring is full.

In other words, it's mostly to hedge against futures we haven't thought of.  Reserving
cap->flags is cheap and easy for both KVM and userspace, so there's no real reason
not to do so.

> If so, cap->flags or cap->args[0] can be used. For now, we just
> need a binding between cap->flags/args[0] with the operation of enabling the
> capability. For example, "cap->flags == 0x0" means to enable the capability
> for now, and "cap->flags != 0x0" to disable the capability in future.
> 
> The suggested changes look good to me in either way. Sean, can I grab your
> reviewed-by with your comments addressed?

I'll look at v10, I don't like providing reviews that are conditional on changes
that are more than nits.

That said, there're no remaining issues that can't be sorted out on top, so don't
hold up v10 if I don't look at it in a timely manner for whatever reason.  I agree
with Marc that it'd be good to get this in -next sooner than later.