Re: [PATCH v4 2/2] s390/kvm: diagnose 318 handling

[Collin Walling <walling@linux.ibm.com>]

On 5/2/19 11:39 AM, David Hildenbrand wrote:
> On 02.05.19 17:25, Collin Walling wrote:
>> On 5/2/19 8:59 AM, David Hildenbrand wrote:
>>> On 02.05.19 00:51, 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 (also known
>>>> as the "Control Program Code" or "CPC"), as well as check the system
>>>> if KVM supports handling this instruction.
>>>>
>>>> This information can help with diagnosing the OS the VM is running
>>>> in (Linux, z/VM, etc) if the OS calls this instruction.
>>>>
>>>> The get/set functions are introduced primarily for VM migration and
>>>> reset, though no harm could be done to the system if a userspace
>>>> program decides to alter this data (this is highly discouraged).
>>>>
>>>> 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) retains a copy in each VCPU as well.
>>>>
>>>> At this time, the CPVC is not reported as its format is yet to be
>>>> defined.
>>>>
>>>> Note that the CPNC is set in the SIE block iff the host hardware
>>>> supports it.
>>>
>>> For vSIE and SIE you only configure the CPNC. Is that sufficient?
>>> Shouldn't diag318 allow the guest to set both? (especially regarding vSIE)
>>>
>>
>> The SIE block only stores the CPNC. The CPVC is not designed to be
>> stored in the SIE block, so we store it in guest memory only.
> 
> How can the cpvc value be used? Who will access it? Right now, it is
> only written to some location in KVM, and only read/written during
> migration.
> 

Guest dump, ring dump, and call home are events where this data
would we observed to assist with debugging efforts ("what environment
/ OS is the guest running?")

> You mention "The Control Program Version Code (CPVC) retains a copy in
> each VCPU as well", this is wrong, no?
> 

The parent struct kvm_arch retains a copy of the CPVC, not the VCPUs
themselves. The commit message should be changed to reflect that.

>>
>>> [...]
>>>>
>>>> diff --git a/Documentation/virtual/kvm/devices/vm.txt b/Documentation/virtual/kvm/devices/vm.txt
>>>> index 95ca68d..9a8d934 100644
>>>> --- a/Documentation/virtual/kvm/devices/vm.txt
>>>> +++ b/Documentation/virtual/kvm/devices/vm.txt
>>>> @@ -267,3 +267,17 @@ Parameters: address of a buffer in user space to store the data (u64) to;
>>>>    	    if it is enabled
>>>>    Returns:    -EFAULT if the given address is not accessible from kernel space
>>>>    	    0 in case of success.
>>>> +
>>>> +6. GROUP: KVM_S390_VM_MISC
>>>> +Architectures: s390
>>>> +
>>>> +6.1. KVM_S390_VM_MISC_CPC (r/w)
>>>> +
>>>> +Allows userspace to access the "Control Program Code" which consists of a
>>>> +1-byte "Control Program Name Code" and a 7-byte "Control Program Version Code".
>>>> +This information is initialized during IPL and must be preserved during
>>>> +migration.
>>>
>>> Your implementation does not match this description. User space can only
>>> get/set the cpnc effectively for the HW to see it, not the CPVC, no?
>>>
>>
>> We retrieve the entire CPNC + CPVC. User space (i.e. QEMU) can retrieve
>> this 64-bit value and save / load it during live guest migration.
>>
>> I figured it would be best to set / get this entire value now, so that
>> we don't need to add extra handling for the version code later when its
>> format is properly decided.
>>
>>> Shouldn't you transparently forward that data to the SCB for vSIE/SIE,
>>> because we really don't care what the target format will be?
>>>
>>
>> Sorry, I'm not fully understanding what you mean by "we really don't
>> care what the target format will be?"
>>
>> Do you mean to shadow the CPNC without checking if diag318 is supported?
>> I imagine that would be harmless.
> 
> No, I was rather wondering about the CPVC format. But I think I am
> missing how that one will be used at all.
>