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

Xiaoyao Li
2024-01-09T05:47:33Z

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;