Re: [PATCH v19 011/130] KVM: Add new members to struct kvm_gfn_range to operate on

Edgecombe, Rick P
2024-04-26T15:58:01Z

On Fri, 2024-04-26 at 08:28 -0700, Sean Christopherson wrote:
> Hmm, yeah, I buy that argument.  We could even harded further by poisoning '0'
> to force KVM to explicitly.  Aha!  And maybe use a bitmap?
>
>         enum {
>                 BUGGY_KVM_INVALIDATION          = 0,
>                 PROCESS_SHARED                  = BIT(0),
>                 PROCESS_PRIVATE                 = BIT(1),
>                 PROCESS_PRIVATE_AND_SHARED      = PROCESS_SHARED |
> PROCESS_PRIVATE,
>         };

Seems like it would work for all who have been concerned. The previous objection
to the enum (can't find the mail) was for requiring logic like:

if (zap == PROCESS_PRIVATE_AND_SHARED || zap == PROCESS_PRIVATE)
do_private_zap_stuff();

We are trying to tie things up internally so we can jointly have something to
stare at again, as the patches are diverging. But will make this adjustment.

>
> > > https://lore.kernel.org/all/ZUO1Giju0GkUdF0o@google.com/
> >
> > Currently in our internal branch we switched to:
> > exclude_private
> > exclude_shared
> >
> > It came together bettter in the code that uses it.
>
> If the choice is between an enum and exclude_*, I would strongly prefer the
> enum.
> Using exclude_* results in inverted polarity for the code that triggers
> invalidations.

Right, the awkwardness lands in that code.

The processing code looks nice though:
https://lore.kernel.org/kvm/5210e6e6e2eb73b04cb7039084015612479ae2fe.camel@intel.com/

>
> > But I started to wonder if we actually really need exclude_shared. For TDX
> > zapping private memory has to be done with more care, because it cannot be
> > re-
> > populated without guest coordination. But for shared memory if we are
> > zapping a
> > range that includes both private and shared memory, I don't think it should
> > hurt
> > to zap the shared memory.
>
> Hell no, I am not risking taking on more baggage in KVM where userspace or
> some
> other subsystem comes to rely on KVM spuriously zapping SPTEs in response to
> an
> unrelated userspace action.

Hmm, I see the point. Thanks. This was just being left for later discussion
anyway.