| Message ID | 20210726183816.1343022-2-erdemaktas@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX KVM selftests | expand |
On Mon, Jul 26, 2021 at 11:37:54AM -0700, Erdem Aktas wrote: > Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. > Changing the vm_create function to accept type parameter to create > new VM types. > > Signed-off-by: Erdem Aktas <erdemaktas@google.com> > Reviewed-by: Sean Christopherson <seanjc@google.com> > Reviewed-by: Peter Gonda <pgonda@google.com> > Reviewed-by: Marc Orr <marcorr@google.com> > Reviewed-by: Sagi Shahar <sagis@google.com> Reviewed-by: David Matlack <dmatlack@google.com> (aside from the nit below) > --- > .../testing/selftests/kvm/include/kvm_util.h | 1 + > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > index d53bfadd2..c63df42d6 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util.h > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, > void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); > > struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); nit: Consider using a more readable function name such as vm_create_with_type(). > void kvm_vm_free(struct kvm_vm *vmp); > void kvm_vm_restart(struct kvm_vm *vmp, int perm); > void kvm_vm_release(struct kvm_vm *vmp); > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index e5fbf16f7..70caa3882 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -180,13 +180,36 @@ _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) > * Return: > * Pointer to opaque structure that describes the created VM. > * > - * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K). > + * Wrapper VM Create function to create a VM with default type (0). > + */ > +struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) > +{ > + return __vm_create(mode, phy_pages, perm, 0); > +} > + > +/* > + * VM Create with a custom type > + * > + * Input Args: > + * mode - VM Mode (e.g. VM_MODE_P52V48_4K) > + * phy_pages - Physical memory pages > + * perm - permission > + * type - VM type > + * > + * Output Args: None > + * > + * Return: > + * Pointer to opaque structure that describes the created VM. > + * > + * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K) and the > + * type specified in type (e.g. KVM_X86_LEGACY_VM, KVM_X86_TDX_VM ...). > * When phy_pages is non-zero, a memory region of phy_pages physical pages > * is created and mapped starting at guest physical address 0. The file > * descriptor to control the created VM is created with the permissions > * given by perm (e.g. O_RDWR). > */ > -struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, > + int perm, int type) > { > struct kvm_vm *vm; > > @@ -200,7 +223,7 @@ struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) > INIT_LIST_HEAD(&vm->userspace_mem_regions); > > vm->mode = mode; > - vm->type = 0; > + vm->type = type; > > vm->pa_bits = vm_guest_mode_params[mode].pa_bits; > vm->va_bits = vm_guest_mode_params[mode].va_bits; > -- > 2.32.0.432.gabb21c7263-goog >
On Mon, Jul 26, 2021, David Matlack wrote: > On Mon, Jul 26, 2021 at 11:37:54AM -0700, Erdem Aktas wrote: > > Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. > > Changing the vm_create function to accept type parameter to create > > new VM types. > > > > Signed-off-by: Erdem Aktas <erdemaktas@google.com> > > Reviewed-by: Sean Christopherson <seanjc@google.com> *-by tags should not be added unless explicitly provided. IIRC, our internal gerrit will convert +1 to Reviewed-by, but I don't think that's the case here. This applies to all patches in this series. See "Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:" in Documentation/process/submitting-patches.rst for more info. > > Reviewed-by: Peter Gonda <pgonda@google.com> > > Reviewed-by: Marc Orr <marcorr@google.com> > > Reviewed-by: Sagi Shahar <sagis@google.com> > > Reviewed-by: David Matlack <dmatlack@google.com> > > (aside from the nit below) > > > --- > > .../testing/selftests/kvm/include/kvm_util.h | 1 + > > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- > > 2 files changed, 27 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > > index d53bfadd2..c63df42d6 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > > @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, > > void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); > > > > struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); > > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); > > nit: Consider using a more readable function name such as > vm_create_with_type(). Ha! This is why I don't like doing internal reviews :-D Erdem originally had vm_create_type(), I suggested __vm_create() as the double underscore scheme is more common in the kernel for cases where there's a default wrapper and an inner helper that implements the full API. Convention aside, the argument againsts ...with_type() are that it doesn't scale, e.g. if someone adds another parameter parameter for which vm_create() provides a default, and it doesn't self-document the relationship between vm_create() and the inner helper, e.g. by convention, based on names alone I know that vm_create() likely is a wrapper around __vm_create(). Compare that with the existing vm_create_default_with_vcpus() vm_create_default() vm_create_with_vcpus() vm_create() where the relationship between all the helpers is not immediately clear, and vm_create_with_vcpus() is a misnomer because it does much more than call vm_create() and instantiate vCPUs, e.g. it also instantiates the IRQ chip and loads the test into guest memory.
On Tue, Jul 27, 2021 at 08:47:44PM +0000, Sean Christopherson wrote: > On Mon, Jul 26, 2021, David Matlack wrote: > > On Mon, Jul 26, 2021 at 11:37:54AM -0700, Erdem Aktas wrote: > > > Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. > > > Changing the vm_create function to accept type parameter to create > > > new VM types. > > > > > > Signed-off-by: Erdem Aktas <erdemaktas@google.com> > > > Reviewed-by: Sean Christopherson <seanjc@google.com> > > *-by tags should not be added unless explicitly provided. IIRC, our internal > gerrit will convert +1 to Reviewed-by, but I don't think that's the case here. > This applies to all patches in this series. > > See "Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:" in > Documentation/process/submitting-patches.rst for more info. > > > > Reviewed-by: Peter Gonda <pgonda@google.com> > > > Reviewed-by: Marc Orr <marcorr@google.com> > > > Reviewed-by: Sagi Shahar <sagis@google.com> > > > > Reviewed-by: David Matlack <dmatlack@google.com> > > > > (aside from the nit below) > > > > > --- > > > .../testing/selftests/kvm/include/kvm_util.h | 1 + > > > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- > > > 2 files changed, 27 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > > > index d53bfadd2..c63df42d6 100644 > > > --- a/tools/testing/selftests/kvm/include/kvm_util.h > > > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > > > @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, > > > void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); > > > > > > struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); > > > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); > > > > nit: Consider using a more readable function name such as > > vm_create_with_type(). > > Ha! This is why I don't like doing internal reviews :-D +1 :) > > Erdem originally had vm_create_type(), I suggested __vm_create() as the double > underscore scheme is more common in the kernel for cases where there's a default > wrapper and an inner helper that implements the full API. > > Convention aside, the argument againsts ...with_type() are that it doesn't scale, > e.g. if someone adds another parameter parameter for which vm_create() provides a > default, and it doesn't self-document the relationship between vm_create() and > the inner helper, e.g. by convention, based on names alone I know that vm_create() > likely is a wrapper around __vm_create(). True, although with __vm_create() is not solving the scalability problem, it's just preventing scaling altogether (you can only have 1 wrapper function, vm_create). So if any caller wants to override one of the defaults they have to override all of them. I agree with you though in this case: __vm_create() is a better choice (especially given the existence of vm_create_with_vcpus). A better option than both (but would involve more work) would be to create an options struct with all optional arguments. Unfortunately C makes working with options structs a bit clumsy. But it's something to consider as the number of options passed to __vm_create increases. For example: struct vm_options { enum vm_guest_mode mode; uint64_t phy_pages; int perm; int type; }; struct kvm_vm *vm_create(const struct vm_options *options) { ... } static const struct vm_options default_vm_options = { .mode = VM_MODE_DEFAULT, .phy_pages = DEFAULT_GUEST_PHY_PAGES, .perm = O_RDWR, .type = DEFAULT_VM_TYPE, }; /* Create a VM with default options. */ vm = create_vm(&default_vm_options); /* Create a VM with TDX enabled. */ struct vm_options options = default_vm_options; options.type = VM_TYPE_TDX; vm = create_vm(&options); (I'm sure I ham-fisted the const stuff but you get the idea.) I'm toying with introducing an options struct to perf_test_util as well so this is very top of mind. > > Compare that with the existing > > vm_create_default_with_vcpus() > vm_create_default() > vm_create_with_vcpus() > vm_create() > > where the relationship between all the helpers is not immediately clear, and > vm_create_with_vcpus() is a misnomer because it does much more than call vm_create() > and instantiate vCPUs, e.g. it also instantiates the IRQ chip and loads the test > into guest memory.
On Wed, Jul 28, 2021 at 04:07:19PM +0000, David Matlack wrote: > On Tue, Jul 27, 2021 at 08:47:44PM +0000, Sean Christopherson wrote: > > On Mon, Jul 26, 2021, David Matlack wrote: > > > On Mon, Jul 26, 2021 at 11:37:54AM -0700, Erdem Aktas wrote: > > > > Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. > > > > Changing the vm_create function to accept type parameter to create > > > > new VM types. > > > > > > > > Signed-off-by: Erdem Aktas <erdemaktas@google.com> > > > > Reviewed-by: Sean Christopherson <seanjc@google.com> > > > > *-by tags should not be added unless explicitly provided. IIRC, our internal > > gerrit will convert +1 to Reviewed-by, but I don't think that's the case here. > > This applies to all patches in this series. > > > > See "Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:" in > > Documentation/process/submitting-patches.rst for more info. > > > > > > Reviewed-by: Peter Gonda <pgonda@google.com> > > > > Reviewed-by: Marc Orr <marcorr@google.com> > > > > Reviewed-by: Sagi Shahar <sagis@google.com> > > > > > > Reviewed-by: David Matlack <dmatlack@google.com> > > > > > > (aside from the nit below) > > > > > > > --- > > > > .../testing/selftests/kvm/include/kvm_util.h | 1 + > > > > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- > > > > 2 files changed, 27 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > > > > index d53bfadd2..c63df42d6 100644 > > > > --- a/tools/testing/selftests/kvm/include/kvm_util.h > > > > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > > > > @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, > > > > void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); > > > > > > > > struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); > > > > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); > > > > > > nit: Consider using a more readable function name such as > > > vm_create_with_type(). > > > > Ha! This is why I don't like doing internal reviews :-D > > +1 :) > > > > > Erdem originally had vm_create_type(), I suggested __vm_create() as the double > > underscore scheme is more common in the kernel for cases where there's a default > > wrapper and an inner helper that implements the full API. > > > > Convention aside, the argument againsts ...with_type() are that it doesn't scale, > > e.g. if someone adds another parameter parameter for which vm_create() provides a > > default, and it doesn't self-document the relationship between vm_create() and > > the inner helper, e.g. by convention, based on names alone I know that vm_create() > > likely is a wrapper around __vm_create(). > > True, although with __vm_create() is not solving the scalability > problem, it's just preventing scaling altogether (you can only have 1 > wrapper function, vm_create). So if any caller wants to override one of > the defaults they have to override all of them. > > I agree with you though in this case: __vm_create() is a better choice > (especially given the existence of vm_create_with_vcpus). > > A better option than both (but would involve more work) would be to > create an options struct with all optional arguments. Unfortunately C > makes working with options structs a bit clumsy. But it's something to > consider as the number of options passed to __vm_create increases. > > For example: > > struct vm_options { > enum vm_guest_mode mode; > uint64_t phy_pages; > int perm; > int type; > }; > > struct kvm_vm *vm_create(const struct vm_options *options) > { > ... > } > > static const struct vm_options default_vm_options = { > .mode = VM_MODE_DEFAULT, > .phy_pages = DEFAULT_GUEST_PHY_PAGES, > .perm = O_RDWR, > .type = DEFAULT_VM_TYPE, > }; > > /* Create a VM with default options. */ > vm = create_vm(&default_vm_options); > > /* Create a VM with TDX enabled. */ > struct vm_options options = default_vm_options; > options.type = VM_TYPE_TDX; > vm = create_vm(&options); I like this. Thanks, drew
On 7/27/2021 2:37 AM, Erdem Aktas wrote: > Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. > Changing the vm_create function to accept type parameter to create > new VM types. > > Signed-off-by: Erdem Aktas <erdemaktas@google.com> > Reviewed-by: Sean Christopherson <seanjc@google.com> > Reviewed-by: Peter Gonda <pgonda@google.com> > Reviewed-by: Marc Orr <marcorr@google.com> > Reviewed-by: Sagi Shahar <sagis@google.com> > --- > .../testing/selftests/kvm/include/kvm_util.h | 1 + > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > index d53bfadd2..c63df42d6 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util.h > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, > void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); > > struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); > void kvm_vm_free(struct kvm_vm *vmp); > void kvm_vm_restart(struct kvm_vm *vmp, int perm); > void kvm_vm_release(struct kvm_vm *vmp); > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index e5fbf16f7..70caa3882 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -180,13 +180,36 @@ _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) > * Return: > * Pointer to opaque structure that describes the created VM. > * > - * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K). > + * Wrapper VM Create function to create a VM with default type (0). Can we pass KVM_X86_LEGACY_VM (whatever name when it's upstreamed) instead of 0? > + */ > +struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) > +{ > + return __vm_create(mode, phy_pages, perm, 0); > +} > +
On Wed, 2021-08-04 at 14:09 +0800, Xiaoyao Li wrote: > On 7/27/2021 2:37 AM, Erdem Aktas wrote: > > Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. > > Changing the vm_create function to accept type parameter to create > > new VM types. > > > > Signed-off-by: Erdem Aktas <erdemaktas@google.com> > > Reviewed-by: Sean Christopherson <seanjc@google.com> > > Reviewed-by: Peter Gonda <pgonda@google.com> > > Reviewed-by: Marc Orr <marcorr@google.com> > > Reviewed-by: Sagi Shahar <sagis@google.com> > > --- > > .../testing/selftests/kvm/include/kvm_util.h | 1 + > > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- > > 2 files changed, 27 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > > index d53bfadd2..c63df42d6 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > > @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, > > void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); > > > > struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); > > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); > > void kvm_vm_free(struct kvm_vm *vmp); > > void kvm_vm_restart(struct kvm_vm *vmp, int perm); > > void kvm_vm_release(struct kvm_vm *vmp); > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > index e5fbf16f7..70caa3882 100644 > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > @@ -180,13 +180,36 @@ _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) > > * Return: > > * Pointer to opaque structure that describes the created VM. > > * > > - * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K). > > + * Wrapper VM Create function to create a VM with default type (0). > > Can we pass KVM_X86_LEGACY_VM (whatever name when it's upstreamed) > instead of 0? To be honest I would prefer this to be called something like KVM_X86_STANDARD_VM, or something. I don't think that normal unencrypted virtualization is already legacy, even if TDX docs claim that. Just my personal opinion. Best regards, Maxim Levitsky > > > + */ > > +struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) > > +{ > > + return __vm_create(mode, phy_pages, perm, 0); > > +} > > + > >
On 8/4/2021 10:24 PM, Maxim Levitsky wrote: > On Wed, 2021-08-04 at 14:09 +0800, Xiaoyao Li wrote: >> On 7/27/2021 2:37 AM, Erdem Aktas wrote: >>> Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. >>> Changing the vm_create function to accept type parameter to create >>> new VM types. >>> >>> Signed-off-by: Erdem Aktas <erdemaktas@google.com> >>> Reviewed-by: Sean Christopherson <seanjc@google.com> >>> Reviewed-by: Peter Gonda <pgonda@google.com> >>> Reviewed-by: Marc Orr <marcorr@google.com> >>> Reviewed-by: Sagi Shahar <sagis@google.com> >>> --- >>> .../testing/selftests/kvm/include/kvm_util.h | 1 + >>> tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- >>> 2 files changed, 27 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h >>> index d53bfadd2..c63df42d6 100644 >>> --- a/tools/testing/selftests/kvm/include/kvm_util.h >>> +++ b/tools/testing/selftests/kvm/include/kvm_util.h >>> @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, >>> void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); >>> >>> struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); >>> +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); >>> void kvm_vm_free(struct kvm_vm *vmp); >>> void kvm_vm_restart(struct kvm_vm *vmp, int perm); >>> void kvm_vm_release(struct kvm_vm *vmp); >>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c >>> index e5fbf16f7..70caa3882 100644 >>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c >>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c >>> @@ -180,13 +180,36 @@ _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) >>> * Return: >>> * Pointer to opaque structure that describes the created VM. >>> * >>> - * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K). >>> + * Wrapper VM Create function to create a VM with default type (0). >> >> Can we pass KVM_X86_LEGACY_VM (whatever name when it's upstreamed) >> instead of 0? > > To be honest I would prefer this to be called something like KVM_X86_STANDARD_VM, > or something. > > I don't think that normal unencrypted virtualization is already legacy, even if TDX > docs claim that. I'm not proposing to use this specific name introduced in TDX RFC series, but proposing to use the name defined in KVM in the future instead of hard-coded 0. Yes, KVM_X86_STANDARD_VM or KVM_X86_NORMAL_VM (proposed by Paolo) is better than KVM_X86_LEGACY_VM. > Just my personal opinion. > > Best regards, > Maxim Levitsky > >> >>> + */ >>> +struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) >>> +{ >>> + return __vm_create(mode, phy_pages, perm, 0); >>> +} >>> + >> >> > >
On Wed, 2021-08-04 at 22:42 +0800, Xiaoyao Li wrote: > On 8/4/2021 10:24 PM, Maxim Levitsky wrote: > > On Wed, 2021-08-04 at 14:09 +0800, Xiaoyao Li wrote: > > > On 7/27/2021 2:37 AM, Erdem Aktas wrote: > > > > Currently vm_create function only creates KVM_X86_LEGACY_VM type VMs. > > > > Changing the vm_create function to accept type parameter to create > > > > new VM types. > > > > > > > > Signed-off-by: Erdem Aktas <erdemaktas@google.com> > > > > Reviewed-by: Sean Christopherson <seanjc@google.com> > > > > Reviewed-by: Peter Gonda <pgonda@google.com> > > > > Reviewed-by: Marc Orr <marcorr@google.com> > > > > Reviewed-by: Sagi Shahar <sagis@google.com> > > > > --- > > > > .../testing/selftests/kvm/include/kvm_util.h | 1 + > > > > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++-- > > > > 2 files changed, 27 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > > > > index d53bfadd2..c63df42d6 100644 > > > > --- a/tools/testing/selftests/kvm/include/kvm_util.h > > > > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > > > > @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, > > > > void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); > > > > > > > > struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); > > > > +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); > > > > void kvm_vm_free(struct kvm_vm *vmp); > > > > void kvm_vm_restart(struct kvm_vm *vmp, int perm); > > > > void kvm_vm_release(struct kvm_vm *vmp); > > > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > > > index e5fbf16f7..70caa3882 100644 > > > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > > > @@ -180,13 +180,36 @@ _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) > > > > * Return: > > > > * Pointer to opaque structure that describes the created VM. > > > > * > > > > - * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K). > > > > + * Wrapper VM Create function to create a VM with default type (0). > > > > > > Can we pass KVM_X86_LEGACY_VM (whatever name when it's upstreamed) > > > instead of 0? > > > > To be honest I would prefer this to be called something like KVM_X86_STANDARD_VM, > > or something. > > > > I don't think that normal unencrypted virtualization is already legacy, even if TDX > > docs claim that. > > I'm not proposing to use this specific name introduced in TDX RFC > series, but proposing to use the name defined in KVM in the future > instead of hard-coded 0. > > Yes, KVM_X86_STANDARD_VM or KVM_X86_NORMAL_VM (proposed by Paolo) is > better than KVM_X86_LEGACY_VM. KVM_X86_NORMAL_VM is a very good name IMHO as well. Thanks! Best regards, Maxim Levitsky > > > Just my personal opinion. > > > > Best regards, > > Maxim Levitsky > > > > > > + */ > > > > +struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) > > > > +{ > > > > + return __vm_create(mode, phy_pages, perm, 0); > > > > +} > > > > +
Thank you all for all that great feedback! I will include them in my v2. On Wed, Aug 4, 2021 at 7:46 AM Maxim Levitsky <mlevitsk@redhat.com> wrote: > > > > > Can we pass KVM_X86_LEGACY_VM (whatever name when it's upstreamed) > > > > instead of 0? > > > I was originally thinking of doing this but Sean has suggested that we should use 0 to make it arch-agnostic for creating default VMs. +Sean Christopherson : What do you think? > > KVM_X86_NORMAL_VM is a very good name IMHO as well. > Thanks! Sounds good to me. > For example: > > struct vm_options { > enum vm_guest_mode mode; > uint64_t phy_pages; > int perm; > int type; > }; > > struct kvm_vm *vm_create(const struct vm_options *options) > { > ... > } > I liked this idea, I will see if I can include it in my v2. Thank you so much again. -Erdem
On Wed, Aug 04, 2021, Erdem Aktas wrote: > Thank you all for all that great feedback! I will include them in my v2. > > On Wed, Aug 4, 2021 at 7:46 AM Maxim Levitsky <mlevitsk@redhat.com> wrote: > > > > > > > Can we pass KVM_X86_LEGACY_VM (whatever name when it's upstreamed) > > > > > instead of 0? > > > > > I was originally thinking of doing this but Sean has suggested that we > should use 0 to make it arch-agnostic for creating default VMs. > +Sean Christopherson : What do you think? I hate passing '0', but KVM_X86_LEGACY_VM is worse because it's nonsensical for other architectures. > > > > KVM_X86_NORMAL_VM is a very good name IMHO as well. But that implies protected guests are abnormal! And KVM_X86_STANDARD_VM would imply protected guests are sub-standard! I'm only half-joking, e.g. if we get to the point where the majority of guests being run are protected guests, then !protected guests are no longer the "standard". Looking at other architectures, I think the least awful option is a generic KVM_VM_TYPE_AUTO, or maybe KVM_VM_TYPE_DEFAULT. That aligns with how '0' is used by PPC, MIPS, and arm64[*], and would work for x86 as well without implying what's normal/standard. [*] arm64 uses the type to specify the IPA width (I'm not even sure what that is), but thankfully interprets '0' as a default.
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index d53bfadd2..c63df42d6 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -88,6 +88,7 @@ int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id, void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size); struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm); +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm, int type); void kvm_vm_free(struct kvm_vm *vmp); void kvm_vm_restart(struct kvm_vm *vmp, int perm); void kvm_vm_release(struct kvm_vm *vmp); diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index e5fbf16f7..70caa3882 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -180,13 +180,36 @@ _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) * Return: * Pointer to opaque structure that describes the created VM. * - * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K). + * Wrapper VM Create function to create a VM with default type (0). + */ +struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) +{ + return __vm_create(mode, phy_pages, perm, 0); +} + +/* + * VM Create with a custom type + * + * Input Args: + * mode - VM Mode (e.g. VM_MODE_P52V48_4K) + * phy_pages - Physical memory pages + * perm - permission + * type - VM type + * + * Output Args: None + * + * Return: + * Pointer to opaque structure that describes the created VM. + * + * Creates a VM with the mode specified by mode (e.g. VM_MODE_P52V48_4K) and the + * type specified in type (e.g. KVM_X86_LEGACY_VM, KVM_X86_TDX_VM ...). * When phy_pages is non-zero, a memory region of phy_pages physical pages * is created and mapped starting at guest physical address 0. The file * descriptor to control the created VM is created with the permissions * given by perm (e.g. O_RDWR). */ -struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) +struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint64_t phy_pages, + int perm, int type) { struct kvm_vm *vm; @@ -200,7 +223,7 @@ struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm) INIT_LIST_HEAD(&vm->userspace_mem_regions); vm->mode = mode; - vm->type = 0; + vm->type = type; vm->pa_bits = vm_guest_mode_params[mode].pa_bits; vm->va_bits = vm_guest_mode_params[mode].va_bits;