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

Xiaoyao Li
2023-12-21T11:53:58Z

On 12/21/2023 6:36 PM, Wang, Wei W wrote:
> On Thursday, December 21, 2023 2:11 PM, Li, Xiaoyao wrote:
>> On 12/12/2023 9:56 PM, Wang, Wei W wrote:
>>> On Wednesday, November 15, 2023 3:14 PM, Xiaoyao Li wrote:
>>>> Introduce the helper functions to set the attributes of a range of
>>>> memory to private or shared.
>>>>
>>>> This is necessary to notify KVM the private/shared attribute of each gpa
>> range.
>>>> KVM needs the information to decide the GPA needs to be mapped at
>>>> hva- based shared memory or guest_memfd based private memory.
>>>>
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> ---
>>>> accel/kvm/kvm-all.c | 42
>> ++++++++++++++++++++++++++++++++++++++++++
>>>> include/sysemu/kvm.h | 3 +++
>>>> 2 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>>>> 69afeb47c9c0..76e2404d54d2 100644
>>>> --- a/accel/kvm/kvm-all.c
>>>> +++ b/accel/kvm/kvm-all.c
>>>> @@ -102,6 +102,7 @@ bool kvm_has_guest_debug; static int
>>>> kvm_sstep_flags; static bool kvm_immediate_exit; static bool
>>>> kvm_guest_memfd_supported;
>>>> +static uint64_t kvm_supported_memory_attributes;
>>>> static hwaddr kvm_max_slot_size = ~0;
>>>>
>>>> static const KVMCapabilityInfo kvm_required_capabilites[] = { @@
>>>> -1305,6
>>>> +1306,44 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size)
>>>> kvm_max_slot_size = max_slot_size;
>>>> }
>>>>
>>>> +static int kvm_set_memory_attributes(hwaddr start, hwaddr size,
>>>> +uint64_t attr) {
>>>> + struct kvm_memory_attributes attrs;
>>>> + int r;
>>>> +
>>>> + attrs.attributes = attr;
>>>> + attrs.address = start;
>>>> + attrs.size = size;
>>>> + attrs.flags = 0;
>>>> +
>>>> + r = kvm_vm_ioctl(kvm_state, KVM_SET_MEMORY_ATTRIBUTES, &attrs);
>>>> + if (r) {
>>>> + warn_report("%s: failed to set memory (0x%lx+%#zx) with attr
>>>> + 0x%lx
>>>> error '%s'",
>>>> + __func__, start, size, attr, strerror(errno));
>>>> + }
>>>> + return r;
>>>> +}
>>>> +
>>>> +int kvm_set_memory_attributes_private(hwaddr start, hwaddr size) {
>>>> + if (!(kvm_supported_memory_attributes &
>>>> KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
>>>> + error_report("KVM doesn't support PRIVATE memory attribute\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + return kvm_set_memory_attributes(start, size,
>>>> +KVM_MEMORY_ATTRIBUTE_PRIVATE); }
>>>> +
>>>> +int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size) {
>>>> + if (!(kvm_supported_memory_attributes &
>>>> KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
>>>> + error_report("KVM doesn't support PRIVATE memory attribute\n");
>>>> + return -EINVAL;
>>>> + }
>>>
>>> Duplicate code in kvm_set_memory_attributes_shared/private.
>>> Why not move the check into kvm_set_memory_attributes?
>>
>> Because it's not easy to put the check into there.
>>
>> Both setting and clearing one bit require the capability check. If moving the
>> check into kvm_set_memory_attributes(), the check of
>> KVM_MEMORY_ATTRIBUTE_PRIVATE will have to become unconditionally,
>> which is not aligned to the function name because the name is not restricted to
>> shared/private attribute only.
>
> 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.

> attrs.attributes = attr;
> attrs.address = start;
> attrs.size = size;
> @@ -1390,21 +1395,11 @@ static int kvm_set_memory_attributes(hwaddr start, hwaddr size, uint64_t attr)
>
> int kvm_set_memory_attributes_private(hwaddr start, hwaddr size)
> {
> - if (!(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> - error_report("KVM doesn't support PRIVATE memory attribute\n");
> - return -EINVAL;
> - }
> -
> return kvm_set_memory_attributes(start, size, KVM_MEMORY_ATTRIBUTE_PRIVATE);
> }
>
> int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size)
> {
> - if (!(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> - error_report("KVM doesn't support PRIVATE memory attribute\n");
> - return -EINVAL;
> - }
> -
> return kvm_set_memory_attributes(start, size, 0);
> }
>
> Maybe you don't even need the kvm_set_memory_attributes_shared/private wrappers.