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@xxxxxxxxx>
> >> ---
> >> 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.
