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

Xiaoyao Li
2024-01-09T16:32:30Z

On 1/9/2024 10:53 PM, Wang, Wei W wrote:
> On Tuesday, January 9, 2024 1:47 PM, Li, Xiaoyao wrote:
>> 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.
>
> What's the exact issue?
> +#define KVM_MEMORY_ATTRIBUTE_READ (1ULL << 2)
> +#define KVM_MEMORY_ATTRIBUTE_WRITE (1ULL << 1)
> +#define KVM_MEMORY_ATTRIBUTE_EXE (1ULL << 0)
>
> They are checked via
> "if ((attr & kvm_supported_memory_attributes) != attr)" shared above in
> kvm_set_memory_attributes.
> In the case you described, kvm_supported_memory_attributes will be 0x7.
> Anything unexpected?

Sorry that I thought for wrong case.

It doesn't work on the case that KVM doesn't support memory_attribute,
e.g., an old KVM. In this case, 'kvm_supported_memory_attributes' is 0,
and 'attr' is 0 as well.