Re: [PATCH v3 06/70] kvm: Introduce support for memory_attributes

[Xiaoyao Li]

On 12/21/2023 9:47 PM, Wang, Wei W wrote:
> On Thursday, December 21, 2023 7:54 PM, Li, Xiaoyao wrote:
> > On 12/21/2023 6:36 PM, Wang, Wei W wrote:
> > > No need to specifically check for KVM_MEMORY_ATTRIBUTE_PRIVATE there.
> > > I'm suggesting below:
> > >
> > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
> > > 2d9a2455de..63ba74b221 100644
> > > --- a/accel/kvm/kvm-all.c
> > > +++ b/accel/kvm/kvm-all.c
> > > @@ -1375,6 +1375,11 @@ static int kvm_set_memory_attributes(hwaddr
> > start, hwaddr size, uint64_t attr)
> > >        struct kvm_memory_attributes attrs;
> > >        int r;
> > >
> > > +    if ((attr & kvm_supported_memory_attributes) != attr) {
> > > +        error_report("KVM doesn't support memory attr %lx\n", attr);
> > > +        return -EINVAL;
> > > +    }
> >
> > In the case of setting a range of memory to shared while KVM doesn't support
> > private memory. Above check doesn't work. and following IOCTL fails.
>
> SHARED attribute uses the value 0, which indicates it's always supported, no?
> For the implementation, can you find in the KVM side where the ioctl
> would get failed in that case?

I'm worrying about the future case, that KVM supports other memory 
attribute than shared/private. For example, KVM supports RWX bits (bit 0 
- 2) but not shared/private bit.

This patch designs kvm_set_memory_attributes() to be common for all the 
bits (and for future bits), thus it leaves the support check to each 
caller function separately.

If you think it's unnecessary, I can change the name of 
kvm_set_memory_attributes() to kvm_set_memory_shared_private() to make 
it only for shared/private bit, then the check can be moved to it.

> static int kvm_vm_ioctl_set_mem_attributes(struct kvm *kvm,
>                                             struct kvm_memory_attributes *attrs)
> {
>          gfn_t start, end;
>
>          /* flags is currently not used. */
>          if (attrs->flags)
>                  return -EINVAL;
>          if (attrs->attributes & ~kvm_supported_mem_attributes(kvm)) ==> 0 here
>                  return -EINVAL;
>          if (attrs->size == 0 || attrs->address + attrs->size < attrs->address)
>                  return -EINVAL;
>          if (!PAGE_ALIGNED(attrs->address) || !PAGE_ALIGNED(attrs->size))
>                  return -EINVAL;