When converting GPA ranges containing MMIO holes, the general intended logic is to skip MMIO ranges as long as it is a private->shared conversion, and to fail otherwise. To better enable some future refactorings to address bugs in the current code, go ahead and consolidate all of the MMIO handling to a self-contained helper. The only intended functional change is that the error message for shared->private conversion of MMIO ranges corresponding to a non-RAM/non-ROM region now report the range of the region that specifically caused the failure, as opposed to reporting the entire conversion range. There is also a stray "," in the error message which is fixed up as part of this refactoring. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 96 ++++++++++++++++++++++++++++++--------------- 1 file changed, 64 insertions(+), 32 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index db52bb9ffb..c824e4d963 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3334,6 +3334,67 @@ static void kvm_eat_signals(CPUState *cpu) } while (sigismember(&chkset, SIG_IPI)); } +/* + * This handles checking whether a conversion overlaps with any holes in + * the given section/range, and indicates whether or not the section/range + * can be safely skipped (which is generally allowed as long as the + * conversion is private->shared and not the other way around). + * + * Returns 0 if section is normal guest-memfd-backed memory, or is a hole or + * non-RAM/ROM region that should be skipped as MMIO. In the latter case, the + * 'skip' parameter will be set. Returns < 0 if the conversion request is not + * valid. + */ +static int handle_memory_hole(MemoryRegionSection *section, bool to_private, + bool *skip) +{ + MemoryRegion *mr = section->mr; + + if (!mr) { + /* + * TDX requires vMMIO region to be shared to inject #VE to guest. + * OVMF issues conservatively MapGPA(shared) on 32bit PCI MMIO + * region, and vIO-APIC 0xFEC00000 4K page. + * OVMF assigns 32bit PCI MMIO region to + * [top of low memory: typically 2GB=0xC000000, 0xFC00000) + * + * If the current lookup failed to find any regions, then skip the + * entire remaining range as a hole/MMIO. + */ + if (!to_private) { + *skip = true; + return 0; + } + return -EINVAL; + } + + /* + * Because vMMIO region must be shared, guest TD may convert vMMIO + * region to shared explicitly. Don't complain such case. See + * memory_region_type() for checking if the region is MMIO region. + */ + if (!memory_region_has_guest_memfd_private(mr)) { + if (!to_private && + !memory_region_is_ram(mr) && + !memory_region_is_ram_device(mr) && + !memory_region_is_rom(mr) && + !memory_region_is_romd(mr)) { + *skip = true; + return 0; + } else { + error_report("Convert non guest_memfd backed memory region " + "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s", + section->offset_within_address_space, + int128_get64(section->size), + to_private ? "private" : "shared"); + return -EINVAL; + } + } + + *skip = false; + return 0; +} + int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) { MemoryRegionSection section; @@ -3341,6 +3402,7 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) MemoryRegion *mr; RAMBlock *rb; void *addr; + bool skip; int ret = -EINVAL; trace_kvm_convert_memory(start, size, to_private ? "shared_to_private" : "private_to_shared"); @@ -3356,39 +3418,9 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) section = memory_region_find(get_system_memory(), start, size); mr = section.mr; - if (!mr) { - /* - * Ignore converting non-assigned region to shared. - * - * TDX requires vMMIO region to be shared to inject #VE to guest. - * OVMF issues conservatively MapGPA(shared) on 32bit PCI MMIO region, - * and vIO-APIC 0xFEC00000 4K page. - * OVMF assigns 32bit PCI MMIO region to - * [top of low memory: typically 2GB=0xC000000, 0xFC00000) - */ - if (!to_private) { - return 0; - } - return ret; - } - if (!memory_region_has_guest_memfd_private(mr)) { - /* - * Because vMMIO region must be shared, guest TD may convert vMMIO - * region to shared explicitly. Don't complain such case. See - * memory_region_type() for checking if the region is MMIO region. - */ - if (!to_private && - !memory_region_is_ram(mr) && - !memory_region_is_ram_device(mr) && - !memory_region_is_rom(mr) && - !memory_region_is_romd(mr)) { - ret = 0; - } else { - error_report("Convert non guest_memfd backed memory region " - "(0x%"HWADDR_PRIx" ,+ 0x%"HWADDR_PRIx") to %s", - start, size, to_private ? "private" : "shared"); - } + ret = handle_memory_hole(§ion, to_private, &skip); + if (ret || skip) { goto out_unref; } -- 2.43.0 From: Ashish Kalra Page conversion calls can span multiple memory regions, potentially resulting in a conversion failure if the memory range being converted extends beyond the boundaries of the referenced memory region. Handle the case of page conversion calls straddling across memory regions by looping through the subregions and handling conversions and related work section by section. Fixes: c15e5684071d ("kvm: handle KVM_EXIT_MEMORY_FAULT") Signed-off-by: Ashish Kalra Co-developed-by: Michael Roth Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 94 ++++++++++++++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 31 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index c824e4d963..9ba8462f5d 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3395,54 +3395,35 @@ static int handle_memory_hole(MemoryRegionSection *section, bool to_private, return 0; } -int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) +static int kvm_convert_section(MemoryRegionSection *section, bool to_private) { - MemoryRegionSection section; + hwaddr start = section->offset_within_address_space; + hwaddr size = int128_get64(section->size); + MemoryRegion *mr = section->mr; ram_addr_t offset; - MemoryRegion *mr; RAMBlock *rb; void *addr; - bool skip; int ret = -EINVAL; - trace_kvm_convert_memory(start, size, to_private ? "shared_to_private" : "private_to_shared"); - - if (!QEMU_PTR_IS_ALIGNED(start, qemu_real_host_page_size()) || - !QEMU_PTR_IS_ALIGNED(size, qemu_real_host_page_size())) { - return ret; - } - - if (!size) { - return ret; - } - - section = memory_region_find(get_system_memory(), start, size); - mr = section.mr; - - ret = handle_memory_hole(§ion, to_private, &skip); - if (ret || skip) { - goto out_unref; - } - if (to_private) { ret = kvm_set_memory_attributes_private(start, size); } else { ret = kvm_set_memory_attributes_shared(start, size); } if (ret) { - goto out_unref; + return ret; } - addr = memory_region_get_ram_ptr(mr) + section.offset_within_region; + addr = memory_region_get_ram_ptr(mr) + section->offset_within_region; rb = qemu_ram_block_from_host(addr, false, &offset); ret = ram_block_attributes_state_change(rb->attributes, offset, size, to_private); if (ret) { error_report("Failed to notify the listener the state change of " - "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s", - start, size, to_private ? "private" : "shared"); - goto out_unref; + "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s, ret %d", + start, size, to_private ? "private" : "shared", ret); + return ret; } if (to_private) { @@ -3451,15 +3432,66 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) * shared memory is backed by hugetlb, which is supposed to be * pre-allocated and doesn't need to be discarded */ - goto out_unref; + return 0; } ret = ram_block_discard_shared_range(rb, offset, size); } else { ret = ram_block_discard_guest_memfd_range(rb, offset, size); } -out_unref: - memory_region_unref(mr); + return ret; +} + +int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) +{ + int ret = -EINVAL; + + trace_kvm_convert_memory(start, size, to_private ? "shared_to_private" : "private_to_shared"); + + if (!QEMU_PTR_IS_ALIGNED(start, qemu_real_host_page_size()) || + !QEMU_PTR_IS_ALIGNED(size, qemu_real_host_page_size())) { + return ret; + } + + /* + * Page conversions can span multiple memory regions, for example, if two + * memory backends are added to support two different NUMA nodes/policies. + * Handle the covered sections accordingly. + */ + while (size) { + MemoryRegionSection section = memory_region_find(get_system_memory(), + start, size); + hwaddr section_end; + bool skip; + + /* + * If there's no region present, then the current hole "section" + * consumes the entire remaining range. In that case, update the + * relevant indices to terminate the loop after this iteration. + */ + section_end = section.mr + ? section.offset_within_address_space + int128_get64(section.size) + : start + size; + assert(section_end > start); + assert(section_end - start <= size); + + ret = handle_memory_hole(§ion, to_private, &skip); + if (ret || skip) { + memory_region_unref(section.mr); + break; + } + + ret = kvm_convert_section(§ion, to_private); + memory_region_unref(section.mr); + + if (ret) { + break; + } + + size -= section_end - start; + start = section_end; + } + return ret; } -- 2.43.0 Currently MMIO checks are done for ranges where memory_region_find() finds no regions within the entire range, or for cases where non-RAM/ROM regions at the beginning of the range. However, if the first region in the range is a normal RAM/ROM region, then the portion of the conversion range that overlaps the region is processed normally, but any MMIO holes that might be present at the beginning are ignored, so the checks are bypassed for those ranges. Plumb the 'start' GPA that was used to query memory_region_find(), and pass that into the MMIO-processing helper so that these gaps can be detected and MMIO checks can be applied appropriately. Fixes: c5d9425ef4da ("kvm/tdx: Don't complain when converting vMMIO region to shared") Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 9ba8462f5d..f0a79c9bda 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3344,9 +3344,14 @@ static void kvm_eat_signals(CPUState *cpu) * non-RAM/ROM region that should be skipped as MMIO. In the latter case, the * 'skip' parameter will be set. Returns < 0 if the conversion request is not * valid. + * + * 'start' corresponds to the starting range memory_region_find() was + * called for, and is used to determine if there are any MMIO holes preceding + * the region passed in so the appropriate checks can be made on those + * ranges. */ static int handle_memory_hole(MemoryRegionSection *section, bool to_private, - bool *skip) + hwaddr start, bool *skip) { MemoryRegion *mr = section->mr; @@ -3391,6 +3396,16 @@ static int handle_memory_hole(MemoryRegionSection *section, bool to_private, } } + /* + * At this point it has been determined that the region is itself + * can be processed as normal memory. However the caller still needs + * to know if there was a preceeding hole relative to 'start', so + * handle that case here. + */ + if (start < section->offset_within_address_space && to_private) { + return -EINVAL; + } + *skip = false; return 0; } @@ -3475,7 +3490,7 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) assert(section_end > start); assert(section_end - start <= size); - ret = handle_memory_hole(§ion, to_private, &skip); + ret = handle_memory_hole(§ion, to_private, start, &skip); if (ret || skip) { memory_region_unref(section.mr); break; -- 2.43.0 The current handling will properly handle MMIO checks for holes anchored at the beginning of the conversion range, but misses subsequent holes. The current handle_memory_hole() helper already processes all MMIO ranges up to and including the sections found at each iteration, so handle this by simply calling handle_memory_hole() for each additional section/sub-range. Fixes: c5d9425ef4da ("kvm/tdx: Don't complain when converting vMMIO region to shared") Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index f0a79c9bda..2dfd14c257 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3493,7 +3493,12 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) ret = handle_memory_hole(§ion, to_private, start, &skip); if (ret || skip) { memory_region_unref(section.mr); - break; + if (ret) { + break; + } + size -= section_end - start; + start = section_end; + continue; } ret = kvm_convert_section(§ion, to_private); -- 2.43.0 Currently QEMU supports using guest_memfd internally (separately from user-specified memory backends) to handle private memory for confidential VMs. While KVM can switch between guest_memfd-backed private memory and non-guest_memfd-backed shared memory via KVM_SET_MEMORY_ATTRIBUTES, the memory in the guest_memfd inode can only ever be private memory. This is distinct from upcoming in-place conversion support, where guest_memfd inodes can contain both private/shared memory and can convert between the 2 in-place. To help distinguish between these 2 uses of guest_memfd, add a dedicated helper to handle the private-only uses of guest_memfd, and add some additional sanity checks with that use-case in mind. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 15 +++++++++++++++ accel/stubs/kvm-stub.c | 6 ++++++ include/system/kvm.h | 1 + system/physmem.c | 6 +++--- 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 2dfd14c257..f1deb458ec 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -794,6 +794,11 @@ static int kvm_mem_flags(MemoryRegion *mr) } if (memory_region_has_guest_memfd_private(mr)) { assert(kvm_guest_memfd_supported); + /* + * memory_region_has_guest_memfd_private() is specifically pertaining to + * using guest_memfd to handle private memory use cases. + */ + assert(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE); flags |= KVM_MEM_GUEST_MEMFD; } return flags; @@ -4873,3 +4878,13 @@ int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp) return fd; } + +int kvm_create_guest_memfd_private(uint64_t size, Error **errp) +{ + if (!(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)) { + error_setg(errp, "KVM does not support using guest_memfd for private memory"); + return -1; + } + + return kvm_create_guest_memfd(size, 0, errp); +} diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c index acbd0785e0..9fe58efe91 100644 --- a/accel/stubs/kvm-stub.c +++ b/accel/stubs/kvm-stub.c @@ -145,6 +145,12 @@ int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp) return -ENOSYS; } +int kvm_create_guest_memfd_private(uint64_t size, Error **errp) +{ + error_setg(errp, "KVM is not enabled"); + return -ENOSYS; +} + bool kvm_private_memory_attribute_supported(void) { return false; diff --git a/include/system/kvm.h b/include/system/kvm.h index d29624034c..b1e43ddc93 100644 --- a/include/system/kvm.h +++ b/include/system/kvm.h @@ -548,6 +548,7 @@ void kvm_mark_guest_state_protected(void); bool kvm_hwpoisoned_mem(void); int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp); +int kvm_create_guest_memfd_private(uint64_t size, Error **errp); int kvm_set_memory_attributes_private(hwaddr start, uint64_t size); int kvm_set_memory_attributes_shared(hwaddr start, uint64_t size); diff --git a/system/physmem.c b/system/physmem.c index db3dda563d..86046d46df 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -2212,7 +2212,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp) } new_block->guest_memfd_private = - kvm_create_guest_memfd(new_block->max_length, 0, errp); + kvm_create_guest_memfd_private(new_block->max_length, errp); if (new_block->guest_memfd_private < 0) { qemu_mutex_unlock_ramlist(); goto out_free; @@ -2842,8 +2842,8 @@ int ram_block_rebind(Error **errp) if (block->guest_memfd_private >= 0) { close(block->guest_memfd_private); } - block->guest_memfd_private = kvm_create_guest_memfd( - block->max_length, 0, errp); + block->guest_memfd_private = + kvm_create_guest_memfd_private(block->max_length, errp); if (block->guest_memfd_private < 0) { qemu_mutex_unlock_ramlist(); return -1; -- 2.43.0 v13 of kernel support for in-place conversion is now in kvm-x86 staging, but not yet fully upstream, so pull in just the header definitions needed for in-place conversion for now. This patch should be replaced with a full kernel header sync once the kernel support fully lands. Signed-off-by: Michael Roth --- linux-headers/linux/kvm.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h index aea4ab5c69..7850bd4642 100644 --- a/linux-headers/linux/kvm.h +++ b/linux-headers/linux/kvm.h @@ -987,6 +987,7 @@ struct kvm_enable_cap { #define KVM_CAP_S390_VSIE_ESAMODE 248 #define KVM_CAP_S390_HPAGE_2G 249 #define KVM_CAP_PPC_COMPAT_CAPS 250 +#define KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES 252 struct kvm_irq_routing_irqchip { __u32 irqchip; @@ -1637,6 +1638,21 @@ struct kvm_memory_attributes { __u64 flags; }; +/* Available with KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES */ +#define KVM_SET_MEMORY_ATTRIBUTES2 _IOWR(KVMIO, 0xd2, struct kvm_memory_attributes2) + +struct kvm_memory_attributes2 { + union { + __u64 address; + __u64 offset; + }; + __u64 size; + __u64 attributes; + __u64 flags; + __u64 error_offset; + __u64 reserved[11]; +}; + #define KVM_MEMORY_ATTRIBUTE_PRIVATE (1ULL << 3) #define KVM_CREATE_GUEST_MEMFD _IOWR(KVMIO, 0xd4, struct kvm_create_guest_memfd) -- 2.43.0 For confidential guests, guest_memfd is currently used only for private guest memory, and normal guest memory comes from the configured memory backend just as it does for a non-confidential guest. It is now possible to use the same physical memory to back a particular GPA regardless of whether it is in a shared or private state. This avoids the need to rely on discarding memory between shared/private conversions (to avoid doubled memory usage), and is intended to be the primary mode of using guest_memfd for confidential guests moving forward, and future features like hugepage support will likely require it. Add an internal flag to enable this support. Since ConfidentialGuestSupport is already used to track some guest_memfd-related functionality (e.g. whether it is required for the configured machine), similarly introduce this flag as a member of the ConfidentialGuestSupport object. Also add the KVM-specific checks to enable this support, but leave the option disabled so that required changes can first be implemented for CGS variants that intend to make use of KVM's in-place conversion support. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 18 ++++++++++++++++++ hw/core/machine.c | 5 +++++ include/hw/core/boards.h | 1 + include/system/confidential-guest-support.h | 12 ++++++++++++ 4 files changed, 36 insertions(+) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index f1deb458ec..862539aed4 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3074,6 +3074,24 @@ static int kvm_init(AccelState *as, MachineState *ms) kvm_guest_memfd_flags_supported = kvm_vm_check_extension(s, KVM_CAP_GUEST_MEMFD_FLAGS); + if (machine_require_guest_memfd_convert_in_place(ms)) { + uint64_t guest_memfd_supported_memory_attributes; + + guest_memfd_supported_memory_attributes = + kvm_vm_check_extension(s, KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES); + + if (!(guest_memfd_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)) { + ret = -EINVAL; + error_report("In-place conversion is only supported if private " + "memory attributes can be set via guest_memfd. " + "Please ensure the 'gmem_in_place_conversion' KVM " + "module parameter is set to 1."); + goto err; + } + + kvm_supported_memory_attributes = guest_memfd_supported_memory_attributes; + } + if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) { s->kernel_irqchip_split = mc->default_kernel_irqchip_split ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; } diff --git a/hw/core/machine.c b/hw/core/machine.c index d9ac4b419a..e9cc6c2420 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1346,6 +1346,11 @@ bool machine_require_guest_memfd_private(MachineState *machine) return machine->cgs && machine->cgs->require_guest_memfd; } +bool machine_require_guest_memfd_convert_in_place(MachineState *machine) +{ + return machine->cgs && machine->cgs->convert_in_place; +} + static char *cpu_slot_to_string(const CPUArchId *cpu) { GString *s = g_string_new(NULL); diff --git a/include/hw/core/boards.h b/include/hw/core/boards.h index d0e4ad9820..9ea124b224 100644 --- a/include/hw/core/boards.h +++ b/include/hw/core/boards.h @@ -44,6 +44,7 @@ int machine_phandle_start(MachineState *machine); bool machine_dump_guest_core(MachineState *machine); bool machine_mem_merge(MachineState *machine); bool machine_require_guest_memfd_private(MachineState *machine); +bool machine_require_guest_memfd_convert_in_place(MachineState *machine); HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine); void machine_set_cpu_numa_node(MachineState *machine, const CpuInstanceProperties *props, diff --git a/include/system/confidential-guest-support.h b/include/system/confidential-guest-support.h index 5dca717308..2c9a9ff217 100644 --- a/include/system/confidential-guest-support.h +++ b/include/system/confidential-guest-support.h @@ -92,6 +92,18 @@ struct ConfidentialGuestSupport { * so 'ready' is not set, we'll abort. */ bool ready; + + /* + * CGS implementations will use this to indicate whether or not + * to enable in-place conversion for guest-memfd. + * + * If set, the machine re-uses physical pages when converting + * between shared/private (as opposed to using different + * physical pages depending on the access type, which is the + * default mode when 'require_guest_memfd' is set without + * additionally setting this flag). + */ + bool convert_in_place; }; typedef struct ConfidentialGuestSupportClass { -- 2.43.0 When convert-in-place=true, the guest_memfd instance created by memory-backend-memfd (when guest-memfd=on option is specified) should also be used internally for private memory. Do this by dup()'ing the guest_memfd FD provided by the backend so the separate cleanup paths for shared vs. private FDs can be managed in the same way they are currently for convert-in-place=false (where shared memory must come from something other than guest_memfd). Introduce a new RAM_GUEST_MEMFD_SHAREABLE flag that can be used to limit this dup()'ing to specific backend types like memory-backend-memfd. Signed-off-by: Michael Roth --- backends/hostmem-memfd.c | 1 + include/system/memory.h | 7 ++++++ system/physmem.c | 50 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c index 666593ff6c..c1ea6eefbd 100644 --- a/backends/hostmem-memfd.c +++ b/backends/hostmem-memfd.c @@ -90,6 +90,7 @@ have_fd: backend->aligned = true; ram_flags = backend->share ? RAM_SHARED : RAM_PRIVATE; ram_flags |= backend->reserve ? 0 : RAM_NORESERVE; + ram_flags |= RAM_GUEST_MEMFD_SHAREABLE; ram_flags |= backend->guest_memfd_private ? RAM_GUEST_MEMFD_PRIVATE : 0; return memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name, backend->size, ram_flags, fd, 0, errp); diff --git a/include/system/memory.h b/include/system/memory.h index 027ca81bd2..ef6c767021 100644 --- a/include/system/memory.h +++ b/include/system/memory.h @@ -274,6 +274,13 @@ typedef struct IOMMUTLBEvent { */ #define RAM_PRIVATE (1 << 13) +/* + * RAM is backed by a guest-memfd instance that supports being used as shared + * memory (as opposed to RAM_GUEST_MEMFD_PRIVATE which, by itself, entails + * the guest-memfd instance can only ever be used for private memory). + */ +#define RAM_GUEST_MEMFD_SHAREABLE (1 << 14) + static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, IOMMUNotifierFlag flags, hwaddr start, hwaddr end, diff --git a/system/physmem.c b/system/physmem.c index 86046d46df..b9b4864a94 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -59,6 +59,7 @@ #include "system/hostmem.h" #include "system/hw_accel.h" #include "system/xen-mapcache.h" +#include "system/confidential-guest-support.h" #include "trace.h" #ifdef CONFIG_FALLOCATE_PUNCH_HOLE @@ -2187,6 +2188,8 @@ static void ram_block_add(RAMBlock *new_block, Error **errp) if (new_block->flags & RAM_GUEST_MEMFD_PRIVATE) { int ret; + assert(current_machine->cgs); + if (!kvm_enabled()) { error_setg(errp, "cannot set up private guest memory for %s: KVM required", object_get_typename(OBJECT(current_machine->cgs))); @@ -2211,9 +2214,44 @@ static void ram_block_add(RAMBlock *new_block, Error **errp) goto out_free; } - new_block->guest_memfd_private = - kvm_create_guest_memfd_private(new_block->max_length, errp); + /* + * If both shared/private memory are handled by guest_memfd, make sure + * to re-use the guest_memfd inode that should have already been created + * for handling shared memory. + */ + if (machine_require_guest_memfd_convert_in_place(current_machine)) { + if (!(new_block->flags & RAM_GUEST_MEMFD_SHAREABLE)) { + error_setg(errp, "configured memory backend is not compatible" + " with in-place conversion"); + ram_block_coordinated_discard_require(false); + qemu_mutex_unlock_ramlist(); + goto out_free; + } + assert(new_block->fd >= 0); + + /* + * Current logic calculates guest_memfd_offset on the assumption + * that offset 0 corresponds to the first GPA that is backed by the + * RAM block/backend. For cases where the guest_memfd is only used + * for private memory and created internally as-needed this is + * always the case, but when re-using a guest_memfd that's also + * usable for shared memory (e.g. via memory-backend-guest-memfd) + * it's possible that guest_memfd might be mmap()'d starting at some + * non-zero offset. For now, this isn't a reachable condition, but + * assert this in case this ever changes and the logic needs to be + * updated to account for this. + */ + assert(new_block->fd_offset == 0); + + new_block->guest_memfd_private = qemu_dup(new_block->fd); + } else { + new_block->guest_memfd_private = + kvm_create_guest_memfd_private(new_block->max_length, &err); + } + if (new_block->guest_memfd_private < 0) { + error_propagate(errp, err); + ram_block_coordinated_discard_require(false); qemu_mutex_unlock_ramlist(); goto out_free; } @@ -2323,7 +2361,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, ram_addr_t max_size, assert((ram_flags & ~(RAM_SHARED | RAM_PMEM | RAM_NORESERVE | RAM_PROTECTED | RAM_NAMED_FILE | RAM_READONLY | RAM_READONLY_FD | RAM_GUEST_MEMFD_PRIVATE | - RAM_RESIZEABLE)) == 0); + RAM_RESIZEABLE | RAM_GUEST_MEMFD_SHAREABLE)) == 0); assert(max_size >= size); if (xen_enabled()) { @@ -2835,6 +2873,12 @@ int ram_block_rebind(Error **errp) { RAMBlock *block; + if (machine_require_guest_memfd_convert_in_place(current_machine)) { + error_setg(errp, + "rebind support is not yet enabled for in-place conversion"); + return -1; + } + qemu_mutex_lock_ramlist(); RAMBLOCK_FOREACH(block) { -- 2.43.0 Current hostmem code (which is callable even outside of KVM) specifies guest_memfd flags directly, which requires pulling in kernel headers. Additionally, it will eventually require additional knowledge that is more suitable for KVM-aware code (e.g. whether or not to default to private memory for confidential VMs). Instead, push the determination for what flags are needed down into the KVM/guest_memfd code so they can be handled internally based on VM type/configuration and avoid further potential leakage of KVM-specific code to general code. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 26 ++++++++++++++++++++++++-- accel/stubs/kvm-stub.c | 2 +- backends/hostmem-memfd.c | 5 +---- include/system/kvm.h | 2 +- 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 862539aed4..8cd1671ad5 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -4857,7 +4857,8 @@ void kvm_mark_guest_state_protected(void) kvm_state->guest_state_protected = true; } -int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp) +static int kvm_create_guest_memfd_flags(uint64_t size, uint64_t flags, + Error **errp) { int fd; struct kvm_create_guest_memfd guest_memfd = { @@ -4897,6 +4898,27 @@ int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp) return fd; } +int kvm_create_guest_memfd(uint64_t size, Error **errp) +{ + /* + * There isn't currently any use for non-mmap()'able gmem instances + * outside of kvm_create_guest_memfd_private(), so hardcode it here + * for general use. + * + * Additionally, *_INIT_SHARED is needed for non-confidential VMs, and + * confidential VMs will default to this as well to allow similar flows + * for initializing the VM's initial memory contents as with normal + * guests. In the future, the initial state may become a global policy + * decision based on the confidential VM configuration, in which case + * this would likely be the right place to decide whether or not to set + * the flag since it would likely be a globally-configured option. + */ + return kvm_create_guest_memfd_flags(size, + GUEST_MEMFD_FLAG_MMAP | + GUEST_MEMFD_FLAG_INIT_SHARED, + errp); +} + int kvm_create_guest_memfd_private(uint64_t size, Error **errp) { if (!(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)) { @@ -4904,5 +4926,5 @@ int kvm_create_guest_memfd_private(uint64_t size, Error **errp) return -1; } - return kvm_create_guest_memfd(size, 0, errp); + return kvm_create_guest_memfd_flags(size, 0, errp); } diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c index 9fe58efe91..26003b01a3 100644 --- a/accel/stubs/kvm-stub.c +++ b/accel/stubs/kvm-stub.c @@ -139,7 +139,7 @@ bool kvm_hwpoisoned_mem(void) return false; } -int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp) +int kvm_create_guest_memfd(uint64_t size, Error **errp) { error_setg(errp, "KVM is not enabled"); return -ENOSYS; diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c index c1ea6eefbd..d3b17232f0 100644 --- a/backends/hostmem-memfd.c +++ b/backends/hostmem-memfd.c @@ -70,10 +70,7 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) return false; } - fd = kvm_create_guest_memfd(backend->size, - GUEST_MEMFD_FLAG_MMAP | - GUEST_MEMFD_FLAG_INIT_SHARED, - errp); + fd = kvm_create_guest_memfd(backend->size, errp); } else { fd = qemu_memfd_create(TYPE_MEMORY_BACKEND_MEMFD, backend->size, m->hugetlb, m->hugetlbsize, m->seal ? diff --git a/include/system/kvm.h b/include/system/kvm.h index b1e43ddc93..2a1501bba6 100644 --- a/include/system/kvm.h +++ b/include/system/kvm.h @@ -547,7 +547,7 @@ void kvm_mark_guest_state_protected(void); */ bool kvm_hwpoisoned_mem(void); -int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp); +int kvm_create_guest_memfd(uint64_t size, Error **errp); int kvm_create_guest_memfd_private(uint64_t size, Error **errp); int kvm_set_memory_attributes_private(hwaddr start, uint64_t size); -- 2.43.0 memory_region_init_ram_guest_memfd_private() is called in some cases (legacy BIOS regions / IGVM regions) to allocate a new RAM region with a guest_memfd FD under the covers to handle private memory since the GPA range can be converted between shared/private guest RAM. When in-place conversion is enabled, the conversions happen with the guest_memfd inode itself, so the same inode must be used for both shared and private memory. Handle this accordingly when convert-in-place=true. Signed-off-by: Michael Roth --- system/memory.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/system/memory.c b/system/memory.c index f1910af8a0..4d302fb55b 100644 --- a/system/memory.c +++ b/system/memory.c @@ -3598,10 +3598,27 @@ bool memory_region_init_ram_guest_memfd_private(MemoryRegion *mr, uint64_t size, Error **errp) { - if (!memory_region_init_ram_flags_nomigrate(mr, owner, name, size, - RAM_GUEST_MEMFD_PRIVATE, errp)) { - return false; + if (machine_require_guest_memfd_convert_in_place(current_machine)) { + int fd = kvm_create_guest_memfd(size, errp); + if (fd < 0) { + return false; + } + + if (!memory_region_init_ram_from_fd(mr, owner, name, size, + RAM_SHARED | + RAM_GUEST_MEMFD_PRIVATE | + RAM_GUEST_MEMFD_SHAREABLE, + fd, 0, errp)) { + close(fd); + return false; + } + } else { + if (!memory_region_init_ram_flags_nomigrate(mr, owner, name, size, + RAM_GUEST_MEMFD_PRIVATE, errp)) { + return false; + } } + memory_region_register_ram(mr, owner); return true; } -- 2.43.0 Currently memory attribute conversions are followed up by other bookkeeping tasks like discarding unused memory or issuing iommufd notifications. Move these tasks to a separate post-conversions helper to better compartmentalize and track these tasks, and in doing so lay the groundwork for a pre-conversion helper which will be needed in the future. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 8cd1671ad5..b824b803cb 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3437,20 +3437,26 @@ static int kvm_convert_section(MemoryRegionSection *section, bool to_private) { hwaddr start = section->offset_within_address_space; hwaddr size = int128_get64(section->size); - MemoryRegion *mr = section->mr; - ram_addr_t offset; - RAMBlock *rb; - void *addr; - int ret = -EINVAL; + int ret; if (to_private) { ret = kvm_set_memory_attributes_private(start, size); } else { ret = kvm_set_memory_attributes_shared(start, size); } - if (ret) { - return ret; - } + + return ret; +} + +static int kvm_post_convert_section(MemoryRegionSection *section, bool to_private) +{ + hwaddr start = section->offset_within_address_space; + hwaddr size = int128_get64(section->size); + MemoryRegion *mr = section->mr; + ram_addr_t offset; + RAMBlock *rb; + void *addr; + int ret; addr = memory_region_get_ram_ptr(mr) + section->offset_within_region; rb = qemu_ram_block_from_host(addr, false, &offset); @@ -3525,6 +3531,12 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) } ret = kvm_convert_section(§ion, to_private); + if (ret) { + memory_region_unref(section.mr); + break; + } + + ret = kvm_post_convert_section(§ion, to_private); memory_region_unref(section.mr); if (ret) { -- 2.43.0 ram-block-attribute update notifications are currently sent after conversions from/to private pages to trigger DMA maps/unmaps of shared GPA ranges (respectively). However, with in-place conversion additional requirements on the kernel side come into play which require this behavior to be adjusted. For shared->private conversions: the attributes need to be set to private *after* the notification, since when using VFIO it may not be possible to update the attribute while it remains pinned due to the IOMMU mapping, so issue the notification first to ensure unmappings are done in advance. For private->shared conversions: the attributes need to be set to shared *before* the notification, since it will possibly result in the page being mapped into an IOMMU and trigger guest_memfd's fault handler, which will expect the page to have its attributes set to shared or otherwise SIGBUS. Implement this to enable passthrough support for CoCo guests with in-place conversion support enabled. For non-inplace conversion, pages mapped into the IOMMU are not the same physical pages as the one used for private accesses by the guest, so neither order risks DMA accesses to private memory and that path can be consolidated to use the same handling as well. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 60 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index b824b803cb..60ab5193c9 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3448,7 +3448,7 @@ static int kvm_convert_section(MemoryRegionSection *section, bool to_private) return ret; } -static int kvm_post_convert_section(MemoryRegionSection *section, bool to_private) +static int kvm_pre_convert_section(MemoryRegionSection *section, bool to_private) { hwaddr start = section->offset_within_address_space; hwaddr size = int128_get64(section->size); @@ -3458,16 +3458,66 @@ static int kvm_post_convert_section(MemoryRegionSection *section, bool to_privat void *addr; int ret; + if (!to_private) + return 0; + addr = memory_region_get_ram_ptr(mr) + section->offset_within_region; rb = qemu_ram_block_from_host(addr, false, &offset); + /* + * The attributes need to be set to private *after* the notification + * of a shared->private conversion, since when using VFIO it may not + * be possible to update the attribute while it remains pinned due + * to the IOMMU mapping, so issue the notification first to ensure + * unmappings are done in advance. + * + * There is an asymmetry here in that if the subsequent memory + * attribute update fails, this notification is out of sync with the + * state as tracked by guest_memfd, which isn't ideal, but memory + * attribute failures are not expected to be recoverable any way so + * there it would be a waste of time to roll back the notification and + * re-trigger things like mapping the page via iommufd. + */ ret = ram_block_attributes_state_change(rb->attributes, offset, size, to_private); if (ret) { error_report("Failed to notify the listener the state change of " "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s, ret %d", start, size, to_private ? "private" : "shared", ret); - return ret; + } + + return ret; +} + +static int kvm_post_convert_section(MemoryRegionSection *section, bool to_private) +{ + hwaddr start = section->offset_within_address_space; + hwaddr size = int128_get64(section->size); + MemoryRegion *mr = section->mr; + ram_addr_t offset; + RAMBlock *rb; + void *addr; + int ret; + + addr = memory_region_get_ram_ptr(mr) + section->offset_within_region; + rb = qemu_ram_block_from_host(addr, false, &offset); + + /* + * The attributes need to have been set to shared *before* the notification + * of a private->shared conversion, since it will possibly result in the + * page being mapped into an IOMMU when using VFIO and trigger + * guest_memfd's fault handler, which will expect the page to have its + * attributes set to shared. + */ + if (!to_private) { + ret = ram_block_attributes_state_change(rb->attributes, + offset, size, to_private); + if (ret) { + error_report("Failed to notify the listener the state change of " + "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s, ret %d", + start, size, to_private ? "private" : "shared", ret); + return ret; + } } if (to_private) { @@ -3530,6 +3580,12 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) continue; } + ret = kvm_pre_convert_section(§ion, to_private); + if (ret) { + memory_region_unref(section.mr); + break; + } + ret = kvm_convert_section(§ion, to_private); if (ret) { memory_region_unref(section.mr); -- 2.43.0 When using guest_memfd with support for shared memory / in-place conversion, it is necessary to use the guest_memfd ioctls to handle conversions instead of KVM ioctls. Implement support for this by looping through all the sections within a converison range. Implement everything in terms of the kvm_convert_memory() loop, which already deals with some special considerations regarding various holes / region types that might be encountered. Also update kvm_set_memory_attributes_*() to use the same common path when convert-in-place=false. This potentially results in a small change in behavior due to the additional MMIO checks/skips now being applied in that case (generally qemu-triggered during setup) rather than only for kvm_convert_memory() (generally guest-triggered), but this is arguably safer, and it provides similar behavior between convert-in-place=false vs. convert-in-place=true, the latter of which *must* skip MMIO holes because the regions (and associated guest_memfds) themselves track shared/private state internally and passing the whole conversion range through to KVM is not an option in that case. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 131 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 114 insertions(+), 17 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 60ab5193c9..2d4cba1a3b 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -1623,14 +1623,78 @@ static int kvm_set_memory_attributes(hwaddr start, uint64_t size, uint64_t attr) return r; } -int kvm_set_memory_attributes_private(hwaddr start, uint64_t size) +static int kvm_gmem_ioctl(int guest_memfd, unsigned long type, ...) { - return kvm_set_memory_attributes(start, size, KVM_MEMORY_ATTRIBUTE_PRIVATE); + int ret; + void *arg; + va_list ap; + + va_start(ap, type); + arg = va_arg(ap, void *); + va_end(ap); + + ret = ioctl(guest_memfd, type, arg); + if (ret == -1) { + ret = -errno; + } + return ret; } -int kvm_set_memory_attributes_shared(hwaddr start, uint64_t size) +static int guest_memfd_set_memory_attributes_fd(int guest_memfd, hwaddr offset, + uint64_t size, uint64_t attr) { - return kvm_set_memory_attributes(start, size, 0); + struct kvm_memory_attributes2 attrs = {0}; + int r; + + assert((attr & kvm_supported_memory_attributes) == attr); + attrs.attributes = attr; + attrs.offset = offset; + attrs.size = size; + attrs.flags = 0; + + /* + * guest_memfd may need to delay conversion requests due to + * the memory being in-use by the kernel. In most cases these + * will be transient uses. In some cases, userspace itself may + * be the cause of the memory being considered in-use, though + * QEMU currently takes steps to avoid this (e.g. via + * RamBlockAttributes). On that basis, this code loops + * indefinitely with the assumption that only transient cases + * will block, and that those will be for relatively short + * periods vs. the overall conversion path. + * If those assumptions at some point prove false, most likely + * this will manifest as guest-side lockups on their conversion + * path, which seems like the appropriate way to surface this + * situation to the guest owner rather than some hard timeout. + */ + do { + r = kvm_gmem_ioctl(guest_memfd, KVM_SET_MEMORY_ATTRIBUTES2, &attrs); + } while (r == -EAGAIN); + + if (r) { + error_report("failed to set memory (0x%" HWADDR_PRIx "+0x%" PRIx64 ") " + "with attr 0x%" PRIx64 " error '%s'", + offset, size, attr, strerror(-r)); + } + return r; +} + +static int guest_memfd_set_memory_section_attributes(MemoryRegionSection *section, uint64_t attr) +{ + hwaddr convert_offset, convert_size; + MemoryRegion *mr = section->mr; + RAMBlock *rb; + + assert(mr); + rb = mr->ram_block; + assert(rb->guest_memfd_private >= 0); + convert_offset = section->offset_within_region; + convert_size = int128_get64(section->size); + + return guest_memfd_set_memory_attributes_fd(rb->guest_memfd_private, + convert_offset, + convert_size, + attr); } bool kvm_private_memory_attribute_supported(void) @@ -3439,10 +3503,18 @@ static int kvm_convert_section(MemoryRegionSection *section, bool to_private) hwaddr size = int128_get64(section->size); int ret; - if (to_private) { - ret = kvm_set_memory_attributes_private(start, size); + if (machine_require_guest_memfd_convert_in_place(current_machine)) { + ret = guest_memfd_set_memory_section_attributes(section, + to_private ? KVM_MEMORY_ATTRIBUTE_PRIVATE + : 0); } else { - ret = kvm_set_memory_attributes_shared(start, size); + /* + * Without in-place conversion, attribute-tracking is handled by KVM + * across all guest memory rather than on a per-section/slot basis. + */ + ret = kvm_set_memory_attributes(start, size, + to_private ? KVM_MEMORY_ATTRIBUTE_PRIVATE + : 0); } return ret; @@ -3536,7 +3608,8 @@ static int kvm_post_convert_section(MemoryRegionSection *section, bool to_privat return ret; } -int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) +static int kvm_convert_memory_full(hwaddr start, hwaddr size, bool to_private, + bool pre_hooks, bool post_hooks) { int ret = -EINVAL; @@ -3580,10 +3653,12 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) continue; } - ret = kvm_pre_convert_section(§ion, to_private); - if (ret) { - memory_region_unref(section.mr); - break; + if (pre_hooks) { + ret = kvm_pre_convert_section(§ion, to_private); + if (ret) { + memory_region_unref(section.mr); + break; + } } ret = kvm_convert_section(§ion, to_private); @@ -3592,13 +3667,15 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) break; } - ret = kvm_post_convert_section(§ion, to_private); - memory_region_unref(section.mr); - - if (ret) { - break; + if (post_hooks) { + ret = kvm_post_convert_section(§ion, to_private); + if (ret) { + memory_region_unref(section.mr); + break; + } } + memory_region_unref(section.mr); size -= section_end - start; start = section_end; } @@ -3606,6 +3683,26 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) return ret; } +int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) +{ + return kvm_convert_memory_full(start, size, to_private, true, true); +} + +static int kvm_convert_memory_attributes(hwaddr start, hwaddr size, bool to_private) +{ + return kvm_convert_memory_full(start, size, to_private, false, false); +} + +int kvm_set_memory_attributes_private(hwaddr start, uint64_t size) +{ + return kvm_convert_memory_attributes(start, size, KVM_MEMORY_ATTRIBUTE_PRIVATE); +} + +int kvm_set_memory_attributes_shared(hwaddr start, uint64_t size) +{ + return kvm_convert_memory_attributes(start, size, 0); +} + int kvm_cpu_exec(CPUState *cpu) { struct kvm_run *run = cpu->kvm_run; -- 2.43.0 Without in-place conversion, QEMU can still access shared memory to load initial state into guest memory prior to launch even if the GPA's memory attributes default to private, since userspace is accessing a completely separate pool of memory. With in-place conversion, all these accesses would need to first be converted to shared, then back to private, since the memory all comes from guest_memfd and only shared memory can be accessed by userspace. Additionally, with in-place conversion, the most efficient way to set the default memory attribute state is via the presence/absence of the GUEST_MEMFD_FLAG_INIT_SHARED flag, which is handled in the guest_memfd creation routine, so if future in-place conversion implementations wish to set the default to something otherwise then doing so using that flag would be ideal rather than changing the default after guest_memfd creation by issuing a guest_memfd ioctl. To account for that and avoid sprinkling these differences in behavior throughout QEMU when in-place conversion is enabled, just default to shared. This does not compromise guest security, since Confidential VMs will necessarily enforce this via trusted entities, and simply generate implicit page state changes if their default expectations don't match KVM's. However, in most cases a guest will explicitly convert memory to a particular state before actually using it, so even these implicit conversion requests should be rare. If this ends up needing to be changed for other reasons in the future, the guest_memfd creation is the right place to handle it for in-place conversions. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 2d4cba1a3b..bfb8b4a190 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -1810,7 +1810,26 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml, abort(); } - if (memory_region_has_guest_memfd_private(mr)) { + /* + * Without in-place conversion, QEMU can still access shared memory + * to load initial state into guest memory prior to launch even if + * the GPA's memory attributes default to private, since userspace + * is accessing a completely separate pool of memory. With in-place + * conversion, all these accesses would need to first be converted + * to shared, then back to private, since the memory all comes from + * guest_memfd and only shared memory can be accessed by userspace. + * + * To avoid sprinkling these differences in behavior throughout QEMU + * when in-place conversion is enabled, just default to shared. This + * does not compromise guest security, since Confidential VMs will + * necessarily enforce this via trusted entities, and simply generate + * implicit page state changes if their default expectations don't + * match KVM's. However, in most cases a guest will explicitly + * convert memory to a particular state before actually using it, so + * even these implicit conversion requests should be rare. + */ + if (memory_region_has_guest_memfd_private(mr) && + !machine_require_guest_memfd_convert_in_place(current_machine)) { err = kvm_set_memory_attributes_private(start_addr, slot_size); if (err) { error_report("%s: failed to set memory attribute private: %s", -- 2.43.0 For in-place conversion, the source pointer is expected to be NULL since the data has already been written directly to guest memory and doesn't need to be copied in prior to encrypting it in-place for initial guest memory payload. Signed-off-by: Michael Roth --- target/i386/sev.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/target/i386/sev.c b/target/i386/sev.c index cc16c6b071..b28a31b7bf 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -1227,6 +1227,8 @@ sev_snp_launch_update(SevSnpGuestState *sev_snp_guest, int ret, fw_error; SnpCpuidInfo snp_cpuid_info; struct kvm_sev_snp_launch_update update = {0}; + bool convert_in_place = machine_require_guest_memfd_convert_in_place( + MACHINE(qdev_get_machine())); if (!data->hva || !data->len) { error_report("SNP_LAUNCH_UPDATE called with invalid address" @@ -1240,7 +1242,14 @@ sev_snp_launch_update(SevSnpGuestState *sev_snp_guest, memcpy(&snp_cpuid_info, data->hva, sizeof(snp_cpuid_info)); } - update.uaddr = (__u64)(unsigned long)data->hva; + /* + * For in-place conversion, the source pointer is expected to be NULL + * since the data has already been written directly to guest memory + * and only needs to be encrypted in-place for secure access. + */ + if (!convert_in_place) { + update.uaddr = (__u64)(unsigned long)data->hva; + } update.gfn_start = data->gpa >> TARGET_PAGE_BITS; update.len = data->len; update.type = data->type; -- 2.43.0 When SNP_LAUNCH_UPDATE fails to update the CPUID page, the CPUID page data passed in will be re-written with metadata about what CPUID fields didn't match with what trusted firmware expected. In the case of in-place conversion, this will have been the same page that was initially passed to SNP_LAUNCH_UPDATE, and so it will have been put in a private state prior to making the call. Make sure to switch it back to shared before accessing it for error-reporting, otherwise this path will generate a bus error. While here, sneak in a typo fixup for the error message that gets printed immediately afterward. Signed-off-by: Michael Roth --- target/i386/sev.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/target/i386/sev.c b/target/i386/sev.c index b28a31b7bf..e15e23c78d 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -1278,8 +1278,14 @@ sev_snp_launch_update(SevSnpGuestState *sev_snp_guest, ret, fw_error, fw_error_to_str(fw_error)); if (data->type == KVM_SEV_SNP_PAGE_TYPE_CPUID) { + if (convert_in_place && + kvm_set_memory_attributes_shared(data->gpa, data->len)) { + error_report("SEV-SNP: unable to access CPUID page to " + "check failure reasons"); + goto out; + } sev_snp_cpuid_report_mismatches(&snp_cpuid_info, data->hva); - error_report("SEV-SNP: failed update CPUID page"); + error_report("SEV-SNP: failed to update CPUID page"); } break; } -- 2.43.0 When using in-place conversion, there is no need to discard memory after conversion because the same memory will continue to be used after the conversion to back the same GPA. Discarding it would only cause unnecessary reallocation of memory after each conversion. Instead, only enable it for non-in-place conversions. Signed-off-by: Michael Roth --- accel/kvm/kvm-all.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index bfb8b4a190..dfcfb3b5cc 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3588,7 +3588,7 @@ static int kvm_post_convert_section(MemoryRegionSection *section, bool to_privat ram_addr_t offset; RAMBlock *rb; void *addr; - int ret; + int ret = 0; addr = memory_region_get_ram_ptr(mr) + section->offset_within_region; rb = qemu_ram_block_from_host(addr, false, &offset); @@ -3611,17 +3611,19 @@ static int kvm_post_convert_section(MemoryRegionSection *section, bool to_privat } } - if (to_private) { - if (rb->page_size != qemu_real_host_page_size()) { - /* - * shared memory is backed by hugetlb, which is supposed to be - * pre-allocated and doesn't need to be discarded - */ - return 0; + if (!machine_require_guest_memfd_convert_in_place(current_machine)) { + if (to_private) { + if (rb->page_size != qemu_real_host_page_size()) { + /* + * shared memory is backed by hugetlb, which is supposed to be + * pre-allocated and doesn't need to be discarded + */ + return 0; + } + ret = ram_block_discard_shared_range(rb, offset, size); + } else { + ret = ram_block_discard_guest_memfd_range(rb, offset, size); } - ret = ram_block_discard_shared_range(rb, offset, size); - } else { - ret = ram_block_discard_guest_memfd_range(rb, offset, size); } return ret; -- 2.43.0 The sev-snp-guest object is currently used to provide confidential guest by using guest_memfd for private GPA ranges, and for shared GPA ranges the normal memory backend is used, i.e. "out-of-place" conversion between shared/private. Now that "in-place" conversion support is available in QEMU, add a parameter to select between these 2 modes of utilizing guest_memfd (it cannot be enabled transparently due to noticeable differences in behavior (e.g. memory usage accounting and lack of hugetlb support for shared GPA ranges) that userspace needs to be aware of when selecting the mode). Note that the actual capability checks for in-place conversion support in the host kernel will happen later in kvm_arch_init(). Signed-off-by: Michael Roth --- qapi/qom.json | 8 +++++++- target/i386/sev.c | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/qapi/qom.json b/qapi/qom.json index 49f222796a..84166c2457 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -1099,6 +1099,11 @@ # @tsc-frequency: set secure TSC frequency. Only valid if Secure TSC # is enabled (default: zero) (since 11.1) # +# @convert-in-place: If true, the same physical pages are reused +# when memory is converted between shared and private states. +# If false (default), separate allocations are used depending +# on whether the page is private or shared. (since 11.2) +# # Since: 9.1 ## { 'struct': 'SevSnpGuestProperties', @@ -1112,7 +1117,8 @@ '*host-data': 'str', '*vcek-disabled': 'bool', '*secure-tsc': 'bool', - '*tsc-frequency': 'uint32' } } + '*tsc-frequency': 'uint32', + '*convert-in-place': 'bool'} } ## # @TdxGuestProperties: diff --git a/target/i386/sev.c b/target/i386/sev.c index e15e23c78d..e77ca12fc7 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -3270,6 +3270,20 @@ sev_snp_guest_set_tsc_frequency(Object *obj, Visitor *v, const char *name, SEV_SNP_GUEST(obj)->tsc_khz = value / 1000; } +static bool +sev_snp_guest_get_convert_in_place(Object *obj, Error **errp) +{ + return CONFIDENTIAL_GUEST_SUPPORT(obj)->convert_in_place; +} + +static void +sev_snp_guest_set_convert_in_place(Object *obj, bool value, Error **errp) +{ + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj); + + cgs->convert_in_place = value; +} + static void sev_snp_guest_class_init(ObjectClass *oc, const void *data) { @@ -3311,6 +3325,9 @@ sev_snp_guest_class_init(ObjectClass *oc, const void *data) object_class_property_add(oc, "tsc-frequency", "uint32", sev_snp_guest_get_tsc_frequency, sev_snp_guest_set_tsc_frequency, NULL, NULL); + object_class_property_add_bool(oc, "convert-in-place", + sev_snp_guest_get_convert_in_place, + sev_snp_guest_set_convert_in_place); } static void -- 2.43.0 When in-place conversion is enabled, both the shared memory and private memory must come from the same guest_memfd instance. Thus, the memory-backend-memfd options must in turn correspond to a guest_memfd instance, e.g. guest-memfd=on must be specified. Since there is no use-case for enabling in-place conversion without setting guest-memfd=on, just set it automatically if in-place conversion is enabled (unless guest-memfd has been explicitly set to 'off', in which case generate an informative error message). Also update the 'guest-memfd' documentation to note this behavior. Signed-off-by: Michael Roth --- backends/hostmem-memfd.c | 13 ++++++++++++- docs/system/guest-memfd.rst | 21 +++++++++++++++++++++ qapi/qom.json | 5 +++-- 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c index d3b17232f0..11873b4e0e 100644 --- a/backends/hostmem-memfd.c +++ b/backends/hostmem-memfd.c @@ -21,6 +21,8 @@ #include "system/kvm.h" #include #include "qapi/qapi-visit-common.h" +#include "hw/core/boards.h" +#include "hw/core/qdev.h" OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendMemfd, MEMORY_BACKEND_MEMFD) @@ -45,6 +47,7 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) { HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(backend); g_autofree char *name = host_memory_backend_get_name(backend); + MachineState *machine = MACHINE(qdev_get_machine()); int fd = cpr_find_fd(name, 0); uint32_t ram_flags; @@ -57,7 +60,9 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) goto have_fd; } - if (m->guest_memfd == ON_OFF_AUTO_ON) { + if (m->guest_memfd == ON_OFF_AUTO_ON || + (m->guest_memfd == ON_OFF_AUTO_AUTO && + machine_require_guest_memfd_convert_in_place(machine))) { /* * NOTE: guest-memfd ignores seal=on/off because it always * implicitly seals the FD by definition. @@ -72,6 +77,12 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) fd = kvm_create_guest_memfd(backend->size, errp); } else { + if (machine_require_guest_memfd_convert_in_place(machine)) { + error_setg(errp, "the current machine requires in-place conversion," + " which is not compatible with setting" + " guest-memfd=off"); + return false; + } fd = qemu_memfd_create(TYPE_MEMORY_BACKEND_MEMFD, backend->size, m->hugetlb, m->hugetlbsize, m->seal ? F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL : 0, diff --git a/docs/system/guest-memfd.rst b/docs/system/guest-memfd.rst index d62a214a91..4477756424 100644 --- a/docs/system/guest-memfd.rst +++ b/docs/system/guest-memfd.rst @@ -53,6 +53,27 @@ exposed as an experimental for the time being: * guest_memfd does not currently support the hugetlb=on option * guest_memfd does not currently support Transparent Huge Pages +In-place conversion +------------------- + +Newer kernels now support using a guest-memfd instance for both +private and shared memory at the same time by re-using the same +physical backing pages if a guest converts a GPA range between +shared and private. This is known as "in-place conversion". + +For some confidential computing architectures, like SEV-SNP and +TDX, this mode offers better performance due to not needing to +deallocate/reallocate between 2 separate pools of shared vs. private +memory every time the guest converts memory between shared/private. +For other architectures, this is the only supported mode of +operation. + +When in-place conversion is enabled, it is necessary to use the +memory-backend-memfd backend with guest-memfd=on as shown in the +"Usage" section. Because of this, guest-memfd=auto (which is the +default) will result in the parameter being automatically enabled +if the confidential guest type enables in-place conversion. + References ---------- diff --git a/qapi/qom.json b/qapi/qom.json index 84166c2457..5d66e67087 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -756,8 +756,9 @@ # @guest-memfd: if 'on', use guest-memfd to back the memory region. # See the :doc:`/system/guest-memfd` documentation for more # details. If 'auto', the option will default to 'off' -# currently, but in the future it may result in the option being -# set to 'on' for QEMU configurations that explicitly require it. +# unless the QEMU configuration explicitly requires it to be set, +# e.g. if in-place conversion is enabled for confidential guest +# types like sev-snp-guest. # (default: auto, since: 11.2) # # Features: -- 2.43.0