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". So you're still thinking of the
possibility to allow disabling the capability in future? 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'm making next revision (v10)
a final one :)
Thanks,
Gavin
