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?
