kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Huth <thuth@redhat.com>
To: Janosch Frank <frankja@linux.ibm.com>,
	Collin Walling <walling@linux.ibm.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org
Cc: pbonzini@redhat.com, borntraeger@de.ibm.com, david@redhat.com,
	cohuck@redhat.com, imbrenda@linux.ibm.com,
	heiko.carstens@de.ibm.com, gor@linux.ibm.com
Subject: Re: [PATCH v6 2/2] s390/kvm: diagnose 318 handling
Date: Thu, 14 May 2020 20:40:04 +0200	[thread overview]
Message-ID: <55f9b4a8-61a1-aab8-40a2-cb992be72fef@redhat.com> (raw)
In-Reply-To: <de4e4416-5158-b60f-e2a8-fb6d3d48d516@linux.ibm.com>


[-- Attachment #1.1: Type: text/plain, Size: 4099 bytes --]

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@linux.ibm.com>
>>> ---
>>>  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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  parent reply	other threads:[~2020-05-14 18:40 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 22:15 [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
2020-05-13 22:15 ` [PATCH v6 1/2] s390/setup: diag318: refactor struct Collin Walling
2020-05-14  7:15   ` Thomas Huth
2020-05-14  8:33   ` Janosch Frank
2020-05-13 22:15 ` [PATCH v6 2/2] s390/kvm: diagnose 318 handling Collin Walling
2020-05-14  2:22   ` kbuild test robot
2020-05-14  7:53   ` Thomas Huth
2020-05-14  8:52     ` Janosch Frank
2020-05-14  9:37       ` David Hildenbrand
2020-05-14  9:49         ` Janosch Frank
2020-05-14  9:53           ` David Hildenbrand
2020-05-14 17:20             ` Collin Walling
2020-05-14 18:37               ` Thomas Huth
2020-05-14 18:53                 ` Collin Walling
2020-05-15  6:16                   ` Cornelia Huck
2020-05-14 18:40       ` Thomas Huth [this message]
2020-05-14  9:05   ` Cornelia Huck
2020-05-14 15:24     ` Collin Walling
2020-05-14 15:49       ` Collin Walling

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55f9b4a8-61a1-aab8-40a2-cb992be72fef@redhat.com \
    --to=thuth@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=frankja@linux.ibm.com \
    --cc=gor@linux.ibm.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=imbrenda@linux.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=walling@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).