| Message ID | 20231115071519.2864957-7-xiaoyao.li@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU Guest memfd + QEMU TDX support | expand |
On Wed, Nov 15, 2023 at 02:14:15AM -0500, 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)); This is an error condition rather than an warning condition. Also again I think __func__ is generally not required in an error message, if the error message text is suitably descriptive - applies to other patches in this series too. > + } > + 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; > + } > + > + return kvm_set_memory_attributes(start, size, 0); > +} > + > /* Called with KVMMemoryListener.slots_lock held */ > static void kvm_set_phys_mem(KVMMemoryListener *kml, > MemoryRegionSection *section, bool add) > @@ -2440,6 +2479,9 @@ static int kvm_init(MachineState *ms) > > kvm_guest_memfd_supported = kvm_check_extension(s, KVM_CAP_GUEST_MEMFD); > > + ret = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES); > + kvm_supported_memory_attributes = ret > 0 ? ret : 0; > + > if (object_property_find(OBJECT(current_machine), "kvm-type")) { > g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine), > "kvm-type", > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index fedc28c7d17f..0e88958190a4 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -540,4 +540,7 @@ bool kvm_dirty_ring_enabled(void); > uint32_t kvm_dirty_ring_size(void); > > int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp); > + > +int kvm_set_memory_attributes_private(hwaddr start, hwaddr size); > +int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size); > #endif > -- > 2.34.1 > With regards, Daniel
On 11/15/2023 6:38 PM, Daniel P. Berrangé wrote: > On Wed, Nov 15, 2023 at 02:14:15AM -0500, 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)); > > This is an error condition rather than an warning condition. > > Also again I think __func__ is generally not required in an error message, > if the error message text is suitably descriptive - applies to other > patches in this series too. Get it. >> + } >> + 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; >> + } >> + >> + return kvm_set_memory_attributes(start, size, 0); >> +} >> + >> /* Called with KVMMemoryListener.slots_lock held */ >> static void kvm_set_phys_mem(KVMMemoryListener *kml, >> MemoryRegionSection *section, bool add) >> @@ -2440,6 +2479,9 @@ static int kvm_init(MachineState *ms) >> >> kvm_guest_memfd_supported = kvm_check_extension(s, KVM_CAP_GUEST_MEMFD); >> >> + ret = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES); >> + kvm_supported_memory_attributes = ret > 0 ? ret : 0; >> + >> if (object_property_find(OBJECT(current_machine), "kvm-type")) { >> g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine), >> "kvm-type", >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h >> index fedc28c7d17f..0e88958190a4 100644 >> --- a/include/sysemu/kvm.h >> +++ b/include/sysemu/kvm.h >> @@ -540,4 +540,7 @@ bool kvm_dirty_ring_enabled(void); >> uint32_t kvm_dirty_ring_size(void); >> >> int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp); >> + >> +int kvm_set_memory_attributes_private(hwaddr start, hwaddr size); >> +int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size); >> #endif >> -- >> 2.34.1 >> > > With regards, > Daniel
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?
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.
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; + } + 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.
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.
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? 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;
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;
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?
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.
On Wednesday, January 10, 2024 12:32 AM, Li, Xiaoyao wrote: > 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. How is this different in your existing implementation? The official way of defining a feature is to take a bit (based on the first feature, *_PRIVATE, defined). Using 0 as an attr is a bit magic and it passes all the "&" based check. But using it for *_SHARED looks fine to me as semantically memory can always be shared and the ioctl will return with -ENOTTY anyway in your mentioned case.
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; + } + + return kvm_set_memory_attributes(start, size, 0); +} + /* Called with KVMMemoryListener.slots_lock held */ static void kvm_set_phys_mem(KVMMemoryListener *kml, MemoryRegionSection *section, bool add) @@ -2440,6 +2479,9 @@ static int kvm_init(MachineState *ms) kvm_guest_memfd_supported = kvm_check_extension(s, KVM_CAP_GUEST_MEMFD); + ret = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES); + kvm_supported_memory_attributes = ret > 0 ? ret : 0; + if (object_property_find(OBJECT(current_machine), "kvm-type")) { g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine), "kvm-type", diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index fedc28c7d17f..0e88958190a4 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -540,4 +540,7 @@ bool kvm_dirty_ring_enabled(void); uint32_t kvm_dirty_ring_size(void); int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp); + +int kvm_set_memory_attributes_private(hwaddr start, hwaddr size); +int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size); #endif
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(+)