Re: [PATCH v9 5/8] KVM: Register/unregister the guest private memory regions

[Sean Christopherson]

Paolo, any thoughts before I lead things further astray?

On Fri, Nov 04, 2022, Chao Peng wrote:
> On Thu, Nov 03, 2022 at 11:04:53PM +0000, Sean Christopherson wrote:
> > On Tue, Oct 25, 2022, Chao Peng wrote:
> > > @@ -4708,6 +4802,24 @@ static long kvm_vm_ioctl(struct file *filp,
> > >           r = kvm_vm_ioctl_set_memory_region(kvm, &mem);
> > >           break;
> > >   }
> > > +#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM
> > > + case KVM_MEMORY_ENCRYPT_REG_REGION:
> > > + case KVM_MEMORY_ENCRYPT_UNREG_REGION: {
> > 
> > I'm having second thoughts about usurping 
> > KVM_MEMORY_ENCRYPT_(UN)REG_REGION.  Aside
> > from the fact that restricted/protected memory may not be encrypted, there 
> > are
> > other potential use cases for per-page memory attributes[*], e.g. to make 
> > memory
> > read-only (or no-exec, or exec-only, etc...) without having to modify 
> > memslots.
> > 
> > Any paravirt use case where the attributes of a page are effectively 
> > dictated by
> > the guest is going to run into the exact same performance problems with 
> > memslots,
> > which isn't suprising in hindsight since shared vs. private is really just 
> > an
> > attribute, albeit with extra special semantics.
> > 
> > And if we go with a brand new ioctl(), maybe someday in the very distant 
> > future
> > we can deprecate and delete KVM_MEMORY_ENCRYPT_(UN)REG_REGION.
> > 
> > Switching to a new ioctl() should be a minor change, i.e. shouldn't throw 
> > too big
> > of a wrench into things.
> > 
> > Something like:
> > 
> >   KVM_SET_MEMORY_ATTRIBUTES
> > 
> >   struct kvm_memory_attributes {
> >     __u64 address;
> >     __u64 size;
> >     __u64 flags;

Oh, this is half-baked.  I lost track of which flags were which.  What I 
intended
was a separate, initially-unused flags, e.g.

 struct kvm_memory_attributes {
        __u64 address;
        __u64 size;
        __u64 attributes;
        __u64 flags;
  }

so that KVM can tweak behavior and/or extend the effective size of the struct.

> I like the idea of adding a new ioctl(). But putting all attributes into
> a flags in uAPI sounds not good to me, e.g. forcing userspace to set all
> attributes in one call can cause pain for userspace, probably for KVM
> implementation as well. For private<->shared memory conversion, we
> actually only care the KVM_MEM_ATTR_SHARED or KVM_MEM_ATTR_PRIVATE bit,

Not necessarily, e.g. I can see pKVM wanting to convert from RW+PRIVATE => 
RO+SHARED
or even RW+PRIVATE => NONE+SHARED so that the guest can't write/access the 
memory
while it's accessible from the host.

And if this does extend beyond shared/private, dropping from RWX=>R, i.e. 
dropping
WX permissions, would also be a common operation.

Hmm, typing that out makes me think that if we do end up supporting other 
"attributes",
i.e. protections, we should go straight to full RWX protections instead of doing
things piecemeal, i.e. add individual protections instead of combinations like
NO_EXEC and READ_ONLY.  The protections would have to be inverted for backwards
compatibility, but that's easy enough to handle.  The semantics could be like
protection keys, which also have inverted persmissions, where the final 
protections
are the combination of memslot+attributes, i.e. a read-only memslot couldn't be 
made
writable via attributes.

E.g. userspace could do "NO_READ | NO_WRITE | NO_EXEC" to temporarily block 
access
to memory without needing to delete the memslot.  KVM would need to disallow
unsupported combinations, e.g. disallowed effective protections would be:

  - W or WX [unless there's an arch that supports write-only memory]
  - R or RW [until KVM plumbs through support for no-exec, or it's unsupported 
in hardware]
  - X       [until KVM plumbs through support for exec-only, or it's 
unsupported in hardware]

Anyways, that's all future work...

> but we force userspace to set other irrelevant bits as well if use this
> API.

They aren't irrelevant though, as the memory attributes are all describing the
allowed protections for a given page.  If there's a use case where userspace 
"can't"
keep track of the attributes for whatever reason, then userspace could do a RMW
to set/clear attributes.  Alternatively, the ioctl() could take an "operation" 
and
support WRITE/OR/AND to allow setting/clearing individual flags, e.g. tweak the
above to be: 
 
 struct kvm_memory_attributes {
        __u64 address;
        __u64 size;
        __u64 attributes;
        __u32 operation;
        __u32 flags;
  }

> I looked at kvm_device_attr, sounds we can do similar:

The device attributes deal with isolated, arbitrary values, whereas memory 
attributes
are flags, i.e. devices are 1:1 whereas memory is 1:MANY.  There is no "unset" 
for
device attributes, because they aren't flags.  Device attributes vs. memory 
attributes
really are two very different things that just happen to use a common name.

If it helped clarify things without creating naming problems, we could even use
PROTECTIONS instead of ATTRIBUTES.

>   KVM_SET_MEMORY_ATTR
> 
>   struct kvm_memory_attr {
>       __u64 address;
>       __u64 size;
> #define KVM_MEM_ATTR_SHARED   BIT(0)
> #define KVM_MEM_ATTR_READONLY BIT(1)
> #define KVM_MEM_ATTR_NOEXEC   BIT(2)
>       __u32 attr;

As above, letting userspace set only a single attribute would prevent setting
(or clearing) multiple attributes in a single ioctl().

>       __u32 pad;
>   }
> 
> I'm not sure if we need KVM_GET_MEMORY_ATTR/KVM_HAS_MEMORY_ATTR as well,

Definitely would need to communicate to userspace that various attributes are
supported.  That doesn't necessarily require a common ioctl(), but I don't see
any reason not to add a common helper, and adding a common helper would mean
KVM_CAP_PRIVATE_MEM can go away.  But it should return a bitmask so that 
userspace
can do a single query to get all supported attributes, e.g. 
KVM_SUPPORTED_MEMORY_ATTRIBUTES.  

As for KVM_GET_MEMORY_ATTRIBUTES, we wouldn't necessarily have to provide such 
an
API, e.g. we could hold off until someone came along with a RMW use case (as 
above).
That said, debug would likely be a nightmare without KVM_GET_MEMORY_ATTRIBUTES,
so it's probably best to add it straightway.

> but sounds like we need a KVM_UNSET_MEMORY_ATTR.

No need if the setter operates on all attributes.

> Since we are exposing the attribute directly to userspace I also think
> we'd better treat shared memory as the default, so even when the private
> memory is not used, the bit can still be meaningful. So define BIT(0) as
> KVM_MEM_ATTR_PRIVATE instead of KVM_MEM_ATTR_SHARED.

Ah, right.