Messages in this thread | | | Date | Thu, 18 Aug 2022 00:33:39 +0000 | From | Sean Christopherson <> | Subject | Re: [V3 08/11] KVM: selftests: add library for creating/interacting with SEV guests |
| |
On Wed, Aug 10, 2022, Peter Gonda wrote: > +enum { > + SEV_GSTATE_UNINIT = 0, > + SEV_GSTATE_LUPDATE, > + SEV_GSTATE_LSECRET, > + SEV_GSTATE_RUNNING, > +}; > +
Name the enum, e.g. enum sev_guest_state?
And s/GSTATE/GUEST? Ugh, AMD's documentation uses GSTATE.
But looking at the docs, I only see GSTATE_LAUNCH? Or does SEV have different status codes than -ES and/or -SNP?
> +struct kvm_vm *sev_get_vm(struct sev_vm *sev) > +{ > + return sev->vm;
Why bother with a wrapper?
> +} > + > +uint8_t sev_get_enc_bit(struct sev_vm *sev) > +{
Same here, IMO it just obfuscates code with no real benefit. ANd it's inconsistent, e.g. why have a wrapper for enc_bit but not sev->fd?
> + return sev->enc_bit; > +} > + > +void sev_ioctl(int sev_fd, int cmd, void *data) > +{ > + int ret; > + struct sev_issue_cmd arg; > + > + arg.cmd = cmd; > + arg.data = (unsigned long)data; > + ret = ioctl(sev_fd, SEV_ISSUE_CMD, &arg); > + TEST_ASSERT(ret == 0, > + "SEV ioctl %d failed, error: %d, fw_error: %d", > + cmd, ret, arg.error); > +} > + > +void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data) > +{ > + struct kvm_sev_cmd arg = {0}; > + int ret; > + > + arg.id = cmd; > + arg.sev_fd = sev->fd; > + arg.data = (__u64)data; > + > + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_OP, &arg); > + TEST_ASSERT(ret == 0, > + "SEV KVM ioctl %d failed, rc: %i errno: %i (%s), fw_error: %d", > + cmd, ret, errno, strerror(errno), arg.error); > +} > + > +/* Local helpers. */ > + > +static void
Don't split here, e.g. a grep/search for the function, should also show the return type and any attributes, e.g. "static" vs. something else is typically much more interesting than the parameters (and parameters is not a fully solvable problem).
> +sev_register_user_region(struct sev_vm *sev, void *hva, uint64_t size)
Align like so:
static void sev_register_user_region(struct sev_vm *sev, void *hva, uint64_t size)
or maybe even let it poke out.
> +{ > + struct kvm_enc_region range = {0}; > + int ret; > + > + pr_debug("%s: hva: %p, size: %lu\n", __func__, hva, size); > + > + range.addr = (__u64)hva; > + range.size = size; > + > + ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_REG_REGION, &range); > + TEST_ASSERT(ret == 0, "failed to register user range, errno: %i\n", errno); > +} > + > +static void > +sev_encrypt_phy_range(struct sev_vm *sev, vm_paddr_t gpa, uint64_t size)
Same thing here.
> +{ > + struct kvm_sev_launch_update_data ksev_update_data = {0}; > + > + pr_debug("%s: addr: 0x%lx, size: %lu\n", __func__, gpa, size); > + > + ksev_update_data.uaddr = (__u64)addr_gpa2hva(sev->vm, gpa); > + ksev_update_data.len = size; > + > + kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_UPDATE_DATA, &ksev_update_data); > +} > + > +static void sev_encrypt(struct sev_vm *sev) > +{ > + const struct sparsebit *enc_phy_pages; > + struct kvm_vm *vm = sev->vm; > + sparsebit_idx_t pg = 0; > + vm_paddr_t gpa_start; > + uint64_t memory_size; > + > + /* Only memslot 0 supported for now. */
Eww. Haven't looked at this in depth, but is there a way to avoid hardcoding the memslot in this code?
> +void sev_vm_launch(struct sev_vm *sev) > +{ > + struct kvm_sev_launch_start ksev_launch_start = {0}; > + struct kvm_sev_guest_status ksev_status = {0};
Doesn't " = {};" do the same thing? And for the status, and any other cases where userspace is reading, wouldn't it be better from a test coverage perspective to _not_ zero the data? Hmm, though I suppose false passes are possible in that case...
> + /* Need to use ucall_shared for synchronization. */ > + //ucall_init_ops(sev_get_vm(sev), NULL, &ucall_ops_halt);
Can this be deleted? If not, what's up?
| |