On 2023-07-18 at 16:44:51 -0700, Sean Christopherson wrote: > From: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx> > > In confidential computing usages, whether a page is private or shared is > necessary information for KVM to perform operations like page fault > handling, page zapping etc. There are other potential use cases for > per-page memory attributes, e.g. to make memory read-only (or no-exec, > or exec-only, etc.) without having to modify memslots. > > Introduce two ioctls (advertised by KVM_CAP_MEMORY_ATTRIBUTES) to allow > userspace to operate on the per-page memory attributes. > - KVM_SET_MEMORY_ATTRIBUTES to set the per-page memory attributes to > a guest memory range. > - KVM_GET_SUPPORTED_MEMORY_ATTRIBUTES to return the KVM supported > memory attributes. > > Use an xarray to store the per-page attributes internally, with a naive, > not fully optimized implementation, i.e. prioritize correctness over > performance for the initial implementation. > > Because setting memory attributes is roughly analogous to mprotect() on > memory that is mapped into the guest, zap existing mappings prior to > updating the memory attributes. Opportunistically provide an arch hook > for the post-set path (needed to complete invalidation anyways) in > anticipation of x86 needing the hook to update metadata related to > determining whether or not a given gfn can be backed with various sizes > of hugepages. > > It's possible that future usages may not require an invalidation, e.g. > if KVM ends up supporting RWX protections and userspace grants _more_ > protections, but again opt for simplicity and punt optimizations to > if/when they are needed. > > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx> > Link: https://lore.kernel.org/all/Y2WB48kD0J4VGynX@xxxxxxxxxx > Cc: Fuad Tabba <tabba@xxxxxxxxxx> > Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx> > Co-developed-by: Sean Christopherson <seanjc@xxxxxxxxxx> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > --- > Documentation/virt/kvm/api.rst | 60 ++++++++++++ > include/linux/kvm_host.h | 14 +++ > include/uapi/linux/kvm.h | 14 +++ > virt/kvm/Kconfig | 4 + > virt/kvm/kvm_main.c | 170 +++++++++++++++++++++++++++++++++ > 5 files changed, 262 insertions(+) > Only some trivial concerns below. [...] > @@ -1175,6 +1176,9 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > spin_lock_init(&kvm->mn_invalidate_lock); > rcuwait_init(&kvm->mn_memslots_update_rcuwait); > xa_init(&kvm->vcpu_array); > +#ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES > + xa_init(&kvm->mem_attr_array); > +#endif > > INIT_LIST_HEAD(&kvm->gpc_list); > spin_lock_init(&kvm->gpc_lock); > @@ -1346,6 +1350,9 @@ static void kvm_destroy_vm(struct kvm *kvm) > kvm_free_memslots(kvm, &kvm->__memslots[i][0]); > kvm_free_memslots(kvm, &kvm->__memslots[i][1]); > } > +#ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES > + xa_destroy(&kvm->mem_attr_array); > +#endif Is it better to make the destruction in reverse order from the creation? To put xa_destroy(&kvm->mem_attr_array) after cleanup_srcu_struct(&kvm->srcu), or put xa_init(&kvm->mem_attr_array) after init_srcu_struct(&kvm->irq_srcu). > cleanup_srcu_struct(&kvm->irq_srcu); > cleanup_srcu_struct(&kvm->srcu); > kvm_arch_free_vm(kvm); > @@ -2346,6 +2353,145 @@ static int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm, > } > #endif /* CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT */ [...] > +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)) > + 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; > + > + start = attrs->address >> PAGE_SHIFT; > + end = (attrs->address + attrs->size - 1 + PAGE_SIZE) >> PAGE_SHIFT; As the attrs->address/size are both garanteed to be non-zero, non-wrap and page aligned in prevous check. Is it OK to simplify the calculation, like: end = (attrs->address + attrs->size) >> PAGE_SHIFT; > + > + if (WARN_ON_ONCE(start == end)) > + return -EINVAL; Also, is this check possible to be hit? Maybe remove it? Thanks, Yilun > + > + /* > + * xarray tracks data using "unsigned long", and as a result so does > + * KVM. For simplicity, supports generic attributes only on 64-bit > + * architectures. > + */ > + BUILD_BUG_ON(sizeof(attrs->attributes) != sizeof(unsigned long)); > + > + return kvm_vm_set_mem_attributes(kvm, attrs->attributes, start, end); > +} > +#endif /* CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES */
