Re: [PATCH 6/9] kvm: s390: Add configuration dump functionality

Janosch Frank
2022-05-10T14:45:37Z

On 5/9/22 19:51, Claudio Imbrenda wrote:
> On Thu, 28 Apr 2022 13:00:59 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> Sometimes dumping inside of a VM fails, is unavailable or doesn't
>> yield the required data. For these occasions we dump the VM from the
>> outside, writing memory and cpu data to a file.
>>
>> Up to now PV guests only supported dumping from the inside of the
>> guest through dumpers like KDUMP. A PV guest can be dumped from the
>> hypervisor but the data will be stale and / or encrypted.
>>
>> To get the actual state of the PV VM we need the help of the
>> Ultravisor who safeguards the VM state. New UV calls have been added
>> to initialize the dump, dump storage state data, dump cpu data and
>> complete the dump process. We expose these calls in this patch via a
>> new UV ioctl command.
>>
>> The sensitive parts of the dump data are encrypted, the dump key is
>> derived from the Customer Communication Key (CCK). This ensures that
>> only the owner of the VM who has the CCK can decrypt the dump data.
>>
>> The memory is dumped / read via a normal export call and a re-import
>> after the dump initialization is not needed (no re-encryption with a
>> dump key).
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
[...]
>> +/*
>> + * kvm_s390_pv_dump_stor_state
>> + *
>> + * @kvm: pointer to the guest's KVM struct
>> + * @buff_user: Userspace pointer where we will write the results to
>> + * @gaddr: Starting absolute guest address for which the storage
>> state
>> + * is requested. This value will be updated with the last
>> + * address for which data was written when returning to
>> + * userspace.
>> + * @buff_user_len: Length of the buff_user buffer
>> + * @rc: Pointer to where the uvcb return code is stored
>> + * @rrc: Pointer to where the uvcb return reason code is stored
>> + *
>> + * Return:
>> + * 0 on success
>> + * -ENOMEM if allocating the cache fails
>> + * -EINVAL if gaddr is not aligned to 1MB
>> + * -EINVAL if buff_user_len is not aligned to
>> uv_info.conf_dump_storage_state_len
>> + * -EINVAL if the UV call fails, rc and rrc will be set in this case
>> + * -EFAULT if copying the result to buff_user failed
>> + */
>> +int kvm_s390_pv_dump_stor_state(struct kvm *kvm, void __user
>> *buff_user,
>> + u64 *gaddr, u64 buff_user_len, u16
>> *rc, u16 *rrc) +{
>> + struct uv_cb_dump_stor_state uvcb = {
>> + .header.cmd = UVC_CMD_DUMP_CONF_STOR_STATE,
>> + .header.len = sizeof(uvcb),
>> + .config_handle = kvm->arch.pv.handle,
>> + .gaddr = *gaddr,
>> + .dump_area_origin = 0,
>> + };
>> + size_t buff_kvm_size;
>> + size_t size_done = 0;
>> + u8 *buff_kvm = NULL;
>> + int cc, ret;
>> +
>> + ret = -EINVAL;
>> + /* UV call processes 1MB guest storage chunks at a time */
>> + if (*gaddr & ~HPAGE_MASK)
>> + goto out;
>> +
>> + /*
>> + * We provide the storage state for 1MB chunks of guest
>> + * storage. The buffer will need to be aligned to
>> + * conf_dump_storage_state_len so we don't end on a partial
>> + * chunk.
>> + */
>> + if (!buff_user_len ||
>> + buff_user_len & (uv_info.conf_dump_storage_state_len -
>> 1))
>
> why not use the IS_ALIGNED macro?

Habits.
I'll fix this one and the one above.

>
>> + goto out;
>> +
>> + /*
>> + * Allocate a buffer from which we will later copy to the user process.
>> + *
>> + * We don't want userspace to dictate our buffer size so we limit it to DUMP_BUFF_LEN.
>> + */
>> + ret = -ENOMEM;
>> + buff_kvm_size = buff_user_len <= DUMP_BUFF_LEN ? buff_user_len : DUMP_BUFF_LEN;

This will be converted to min() to make it more readable.

>> + buff_kvm = vzalloc(buff_kvm_size);
>> + if (!buff_kvm)
>> + goto out;
>> +
>> + ret = 0;
>> + uvcb.dump_area_origin = (u64)buff_kvm;
>> + /* We will loop until the user buffer is filled or an error occurs */
>> + do {
>> + /* Get a page of data */
>
> are you getting a page or a block of size conf_dump_storage_state_len ?

Will be changed to:
/* Get 1MB worth of guest storage state data */

I think that comment has historical reasons, we once discussed to always
write one page instead of the "one 1MB worth of guest storage state".

>
>> + cc = uv_call_sched(0, (u64)&uvcb);
>> +
>> + /* All or nothing */
>> + if (cc) {
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + size_done += uv_info.conf_dump_storage_state_len;
>> + uvcb.dump_area_origin +=
>> uv_info.conf_dump_storage_state_len;
>> + uvcb.gaddr += HPAGE_SIZE;
>> + buff_user_len -= PAGE_SIZE;
>
> same here ^ (should it be -= uv_info.conf_dump_storage_state_len ?)

Yes

>
>> +
>> + /* KVM Buffer full, time to copy to the process */
>> + if (!buff_user_len ||
>> + uvcb.dump_area_origin == (uintptr_t)buff_kvm +
>> buff_kvm_size) { +
>
> why not ... || size_done == DUMP_BUFF_LEN ?
>
>> + if (copy_to_user(buff_user, buff_kvm,
>> + uvcb.dump_area_origin -
>> (uintptr_t)buff_kvm)) {
>
> aren't you trying to copy size_done bytes?
>
>> + ret = -EFAULT;
>> + break;
>> + }
>> +
>> + buff_user += size_done;
>> + size_done = 0;
>> + uvcb.dump_area_origin = (u64)buff_kvm;
>> + }
>> + } while (buff_user_len);
>> +
>> + /* Report back where we ended dumping */
>> + *gaddr = uvcb.gaddr;
>> +
>> + /* Lets only log errors, we don't want to spam */
>> +out:
>> + if (ret)
>> + KVM_UV_EVENT(kvm, 3,
>> + "PROTVIRT DUMP STORAGE STATE: addr %llx
>> ret %d, uvcb rc %x rrc %x",
>> + uvcb.gaddr, ret, uvcb.header.rc,
>> uvcb.header.rrc);
>> + *rc = uvcb.header.rc;
>> + *rrc = uvcb.header.rrc;
>> + vfree(buff_kvm);
>> +
>> + return ret;
>> +}
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 2eba89d7ec29..b34850907291 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1645,6 +1645,20 @@ struct kvm_s390_pv_unp {
>> __u64 tweak;
>> };
>>
>> +enum pv_cmd_dmp_id {
>> + KVM_PV_DUMP_INIT,
>> + KVM_PV_DUMP_CONFIG_STOR_STATE,
>> + KVM_PV_DUMP_COMPLETE,
>> +};
>> +
>> +struct kvm_s390_pv_dmp {
>> + __u64 subcmd;
>> + __u64 buff_addr;
>> + __u64 buff_len;
>> + __u64 gaddr; /* For dump storage state */
>> + __u64 reserved[4];
>> +};
>> +
>> enum pv_cmd_info_id {
>> KVM_PV_INFO_VM,
>> KVM_PV_INFO_DUMP,
>> @@ -1688,6 +1702,7 @@ enum pv_cmd_id {
>> KVM_PV_PREP_RESET,
>> KVM_PV_UNSHARE_ALL,
>> KVM_PV_INFO,
>> + KVM_PV_DUMP,
>> };
>>
>> struct kvm_pv_cmd {
>