On 14/05/2020 10.52, Janosch Frank wrote:
> On 5/14/20 9:53 AM, Thomas Huth wrote:
>> On 14/05/2020 00.15, Collin Walling wrote:
>>> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
>>> be intercepted by SIE and handled via KVM. Let's introduce some
>>> functions to communicate between userspace and KVM via ioctls. These
>>> will be used to get/set the diag318 related information, as well as
>>> check the system if KVM supports handling this instruction.
>>>
>>> This information can help with diagnosing the environment the VM is
>>> running in (Linux, z/VM, etc) if the OS calls this instruction.
>>>
>>> By default, this feature is disabled and can only be enabled if a
>>> user space program (such as QEMU) explicitly requests it.
>>>
>>> The Control Program Name Code (CPNC) is stored in the SIE block
>>> and a copy is retained in each VCPU. The Control Program Version
>>> Code (CPVC) is not designed to be stored in the SIE block, so we
>>> retain a copy in each VCPU next to the CPNC.
>>>
>>> Signed-off-by: Collin Walling <walling@xxxxxxxxxxxxx>
>>> ---
>>> Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
>>> arch/s390/include/asm/kvm_host.h | 6 +-
>>> arch/s390/include/uapi/asm/kvm.h | 5 ++
>>> arch/s390/kvm/diag.c | 20 ++++++
>>> arch/s390/kvm/kvm-s390.c | 89 +++++++++++++++++++++++++++
>>> arch/s390/kvm/kvm-s390.h | 1 +
>>> arch/s390/kvm/vsie.c | 2 +
>>> 7 files changed, 151 insertions(+), 1 deletion(-)
>> [...]
>>> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
>>> index 563429dece03..3caed4b880c8 100644
>>> --- a/arch/s390/kvm/diag.c
>>> +++ b/arch/s390/kvm/diag.c
>>> @@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
>>> return ret < 0 ? ret : 0;
>>> }
>>>
>>> +static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
>>> +{
>>> + unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
>>> + u64 info = vcpu->run->s.regs.gprs[reg];
>>> +
>>> + if (!vcpu->kvm->arch.use_diag318)
>>> + return -EOPNOTSUPP;
>>> +
>>> + vcpu->stat.diagnose_318++;
>>> + kvm_s390_set_diag318_info(vcpu->kvm, info);
>>> +
>>> + VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
>>> + vcpu->kvm->arch.diag318_info.cpnc,
>>> + (u64)vcpu->kvm->arch.diag318_info.cpvc);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>>> {
>>> int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;
>>> @@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>>> return __diag_page_ref_service(vcpu);
>>> case 0x308:
>>> return __diag_ipl_functions(vcpu);
>>> + case 0x318:
>>> + return __diag_set_diag318_info(vcpu);
>>> case 0x500:
>>> return __diag_virtio_hypercall(vcpu);
>>
>> I wonder whether it would make more sense to simply drop to userspace
>> and handle the diag 318 call there? That way the userspace would always
>> be up-to-date, and as we've seen in the past (e.g. with the various SIGP
>> handling), it's better if the userspace is in control... e.g. userspace
>> could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the
>> guest just executed the diag 318 instruction.
>>
>> And you need the kvm_s390_vm_get/set_misc functions anyway, so these
>> could also be simply used by the diag 318 handler in userspace?
[...]
>> What about a reset of the guest VM? If a user first boots into a Linux
>> kernel that supports diag 318, then reboots and selects a Linux kernel
>> that does not support diag 318? I'd expect that the cpnc / cpnv values
>> need to be cleared here somewhere? Otherwise the information might not
>> be accurate anymore?
>
> He resets via QEMU on a machine reset.
Ah, thanks for the pointer, makes sense! ... and actually, I think that
is another indication that QEMU should rather be in control, thus the
kernel code should drop to userspace instead of handling the diag 318
call in diag.c in the kernel above.
Thomas
Attachment:
signature.asc
Description: OpenPGP digital signature
