On 18.02.20 10:44, David Hildenbrand wrote:
> On 14.02.20 23:26, Christian Borntraeger wrote:
>> From: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>
>> diag 308 subcode 0 and 1 require several KVM and Ultravisor interactions.
>> Specific to these "soft" reboots are
>>
>> * The "unshare all" UVC
>> * The "prepare for reset" UVC
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> [borntraeger@xxxxxxxxxx: patch merging, splitting, fixing]
>> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>> ---
>> arch/s390/include/asm/uv.h | 4 ++++
>> arch/s390/kvm/kvm-s390.c | 22 ++++++++++++++++++++++
>> include/uapi/linux/kvm.h | 2 ++
>> 3 files changed, 28 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
>> index 839cb3a89986..254d5769d136 100644
>> --- a/arch/s390/include/asm/uv.h
>> +++ b/arch/s390/include/asm/uv.h
>> @@ -36,6 +36,8 @@
>> #define UVC_CMD_SET_SEC_CONF_PARAMS 0x0300
>> #define UVC_CMD_UNPACK_IMG 0x0301
>> #define UVC_CMD_VERIFY_IMG 0x0302
>> +#define UVC_CMD_PREPARE_RESET 0x0320
>> +#define UVC_CMD_SET_UNSHARE_ALL 0x0340
>> #define UVC_CMD_PIN_PAGE_SHARED 0x0341
>> #define UVC_CMD_UNPIN_PAGE_SHARED 0x0342
>> #define UVC_CMD_SET_SHARED_ACCESS 0x1000
>> @@ -56,6 +58,8 @@ enum uv_cmds_inst {
>> BIT_UVC_CMD_SET_SEC_PARMS = 11,
>> BIT_UVC_CMD_UNPACK_IMG = 13,
>> BIT_UVC_CMD_VERIFY_IMG = 14,
>> + BIT_UVC_CMD_PREPARE_RESET = 18,
>> + BIT_UVC_CMD_UNSHARE_ALL = 20,
>> BIT_UVC_CMD_PIN_PAGE_SHARED = 21,
>> BIT_UVC_CMD_UNPIN_PAGE_SHARED = 22,
>> };
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index f96c1f530cc2..ad84c1144908 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2285,6 +2285,28 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>> cmd->rrc);
>> break;
>> }
>> + case KVM_PV_VM_PREP_RESET: {
>> + r = -EINVAL;
>> + if (!kvm_s390_pv_is_protected(kvm))
>> + break;
>> +
>> + r = uv_cmd_nodata(kvm_s390_pv_handle(kvm),
>> + UVC_CMD_PREPARE_RESET, &cmd->rc, &cmd->rrc);
>> + KVM_UV_EVENT(kvm, 3, "PROTVIRT PREP RESET: rc %x rrc %x",
>> + cmd->rc, cmd->rrc);
>> + break;
>> + }
>> + case KVM_PV_VM_UNSHARE_ALL: {
>> + r = -EINVAL;
>> + if (!kvm_s390_pv_is_protected(kvm))
>> + break;
>> +
>> + r = uv_cmd_nodata(kvm_s390_pv_handle(kvm),
>> + UVC_CMD_SET_UNSHARE_ALL, &cmd->rc, &cmd->rrc);
>> + KVM_UV_EVENT(kvm, 3, "PROTVIRT UNSHARE: rc %x rrc %x",
>> + cmd->rc, cmd->rrc);
>
> I do wonder if that has any possible races with CPUs currently running,
> which try to mark pages shared. That no other VCPUs are running is not
> enforced here AFAIKs.
The only concern would be that the newly bootet guest (e.g. via kexec/kdump) will
have pages shared that it dont want to share. As it is the responsiblity of the
ultravisor to prevent data leakage, it is enforced that we do an UNSHARE all on
diag 308 subcode 0 and 1. At the same time the ultravisor will enforce that no
CPU will run before we have finished the full dance of reset activity. (The
ultravisor must guarantee guest secrecy and integrity no matter what KVM does).
>
> Apart from that, looks good to me.
>
