Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v1] KVM: s390: store DXC/VXC in fpc on
 DATA/Vector-processing exceptions
To:     David Hildenbrand <david@redhat.com>, linux-kernel@vger.kernel.org
Cc:     linux-s390@vger.kernel.org,
        Heiko Carstens <heiko.carstens@de.ibm.com>,
        Martin Schwidefsky <schwidefsky@de.ibm.com>,
        Cornelia Huck <cohuck@redhat.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
References: <20180822095310.29145-1-david@redhat.com>
 <7c7e52aa-7b07-d146-ac4e-2b1afea6a83d@de.ibm.com>
 <b613c011-67f0-317d-c430-cdccfbded46f@redhat.com>
From:   Christian Borntraeger <borntraeger@de.ibm.com>
Openpgp: preference=signencrypt
Autocrypt: addr=borntraeger@de.ibm.com; prefer-encrypt=mutual; keydata=
 xsFNBE6cPPgBEAC2VpALY0UJjGmgAmavkL/iAdqul2/F9ONz42K6NrwmT+SI9CylKHIX+fdf
 J34pLNJDmDVEdeb+brtpwC9JEZOLVE0nb+SR83CsAINJYKG3V1b3Kfs0hydseYKsBYqJTN2j
 CmUXDYq9J7uOyQQ7TNVoQejmpp5ifR4EzwIFfmYDekxRVZDJygD0wL/EzUr8Je3/j548NLyL
 4Uhv6CIPf3TY3/aLVKXdxz/ntbLgMcfZsDoHgDk3lY3r1iwbWwEM2+eYRdSZaR4VD+JRD7p8
 0FBadNwWnBce1fmQp3EklodGi5y7TNZ/CKdJ+jRPAAnw7SINhSd7PhJMruDAJaUlbYaIm23A
 +82g+IGe4z9tRGQ9TAflezVMhT5J3ccu6cpIjjvwDlbxucSmtVi5VtPAMTLmfjYp7VY2Tgr+
 T92v7+V96jAfE3Zy2nq52e8RDdUo/F6faxcumdl+aLhhKLXgrozpoe2nL0Nyc2uqFjkjwXXI
 OBQiaqGeWtxeKJP+O8MIpjyGuHUGzvjNx5S/592TQO3phpT5IFWfMgbu4OreZ9yekDhf7Cvn
 /fkYsiLDz9W6Clihd/xlpm79+jlhm4E3xBPiQOPCZowmHjx57mXVAypOP2Eu+i2nyQrkapaY
 IdisDQfWPdNeHNOiPnPS3+GhVlPcqSJAIWnuO7Ofw1ZVOyg/jwARAQABzTRDaHJpc3RpYW4g
 Qm9ybnRyYWVnZXIgKElCTSkgPGJvcm50cmFlZ2VyQGRlLmlibS5jb20+wsF4BBMBAgAiBQJO
 nDz4AhsDBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgAAKCRARe7yAtaYcfOYVD/9sqc6ZdYKD
 bmDIvc2/1LL0g7OgiA8pHJlYN2WHvIhUoZUIqy8Sw2EFny/nlpPVWfG290JizNS2LZ0mCeGZ
 80yt0EpQNR8tLVzLSSr0GgoY0lwsKhAnx3p3AOrA8WXsPL6prLAu3yJI5D0ym4MJ6KlYVIjU
 ppi4NLWz7ncA2nDwiIqk8PBGxsjdc/W767zOOv7117rwhaGHgrJ2tLxoGWj0uoH3ZVhITP1z
 gqHXYaehPEELDV36WrSKidTarfThCWW0T3y4bH/mjvqi4ji9emp1/pOWs5/fmd4HpKW+44tD
 Yt4rSJRSa8lsXnZaEPaeY3nkbWPcy3vX6qafIey5d8dc8Uyaan39WslnJFNEx8cCqJrC77kI
 vcnl65HaW3y48DezrMDH34t3FsNrSVv5fRQ0mbEed8hbn4jguFAjPt4az1xawSp0YvhzwATJ
 YmZWRMa3LPx/fAxoolq9cNa0UB3D3jmikWktm+Jnp6aPeQ2Db3C0cDyxcOQY/GASYHY3KNra
 z8iwS7vULyq1lVhOXg1EeSm+lXQ1Ciz3ub3AhzE4c0ASqRrIHloVHBmh4favY4DEFN19Xw1p
 76vBu6QjlsJGjvROW3GRKpLGogQTLslbjCdIYyp3AJq2KkoKxqdeQYm0LZXjtAwtRDbDo71C
 FxS7i/qfvWJv8ie7bE9A6Wsjn87BTQROnDz4ARAAmPI1e8xB0k23TsEg8O1sBCTXkV8HSEq7
 JlWz7SWyM8oFkJqYAB7E1GTXV5UZcr9iurCMKGSTrSu3ermLja4+k0w71pLxws859V+3z1jr
 nhB3dGzVZEUhCr3EuN0t8eHSLSMyrlPL5qJ11JelnuhToT6535cLOzeTlECc51bp5Xf6/XSx
 SMQaIU1nDM31R13o98oRPQnvSqOeljc25aflKnVkSfqWSrZmb4b0bcWUFFUKVPfQ5Z6JEcJg
 Hp7qPXHW7+tJTgmI1iM/BIkDwQ8qe3Wz8R6rfupde+T70NiId1M9w5rdo0JJsjKAPePKOSDo
 RX1kseJsTZH88wyJ30WuqEqH9zBxif0WtPQUTjz/YgFbmZ8OkB1i+lrBCVHPdcmvathknAxS
 bXL7j37VmYNyVoXez11zPYm+7LA2rvzP9WxR8bPhJvHLhKGk2kZESiNFzP/E4r4Wo24GT4eh
 YrDo7GBHN82V4O9JxWZtjpxBBl8bH9PvGWBmOXky7/bP6h96jFu9ZYzVgIkBP3UYW+Pb1a+b
 w4A83/5ImPwtBrN324bNUxPPqUWNW0ftiR5b81ms/rOcDC/k/VoN1B+IHkXrcBf742VOLID4
 YP+CB9GXrwuF5KyQ5zEPCAjlOqZoq1fX/xGSsumfM7d6/OR8lvUPmqHfAzW3s9n4lZOW5Jfx
 bbkAEQEAAcLBXwQYAQIACQUCTpw8+AIbDAAKCRARe7yAtaYcfPzbD/9WNGVf60oXezNzSVCL
 hfS36l/zy4iy9H9rUZFmmmlBufWOATjiGAXnn0rr/Jh6Zy9NHuvpe3tyNYZLjB9pHT6mRZX7
 Z1vDxeLgMjTv983TQ2hUSlhRSc6e6kGDJyG1WnGQaqymUllCmeC/p9q5m3IRxQrd0skfdN1V
 AMttRwvipmnMduy5SdNayY2YbhWLQ2wS3XHJ39a7D7SQz+gUQfXgE3pf3FlwbwZhRtVR3z5u
 aKjxqjybS3Ojimx4NkWjidwOaUVZTqEecBV+QCzi2oDr9+XtEs0m5YGI4v+Y/kHocNBP0myd
 pF3OoXvcWdTb5atk+OKcc8t4TviKy1WCNujC+yBSq3OM8gbmk6NwCwqhHQzXCibMlVF9hq5a
 FiJb8p4QKSVyLhM8EM3HtiFqFJSV7F+h+2W0kDyzBGyE0D8z3T+L3MOj3JJJkfCwbEbTpk4f
 n8zMboekuNruDw1OADRMPlhoWb+g6exBWx/YN4AY9LbE2KuaScONqph5/HvJDsUldcRN3a5V
 RGIN40QWFVlZvkKIEkzlzqpAyGaRLhXJPv/6tpoQaCQQoSAc5Z9kM/wEd9e2zMeojcWjUXgg
 oWj8A/wY4UXExGBu+UCzzP/6sQRpBiPFgmqPTytrDo/gsUGqjOudLiHQcMU+uunULYQxVghC
 syiRa+UVlsKmx1hsEg==
Date:   Fri, 24 Aug 2018 08:34:43 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <b613c011-67f0-317d-c430-cdccfbded46f@redhat.com>
Content-Language: en-US
Message-Id: <d8ab5075-c589-8a0b-516c-9cff79522abb@de.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 08/23/2018 07:44 PM, David Hildenbrand wrote:
> On 23.08.2018 17:43, Christian Borntraeger wrote:
>>
>>
>> On 08/22/2018 11:53 AM, David Hildenbrand wrote:
>>> When DATA exceptions and vector-processing exceptions (program interrupts)
>>> are injected, the DXC/VXC is also to be stored in the fpc, if AFP is
>>> enabled in CR0.
>>>
>>> This can happen inside KVM when reinjecting an interrupt during program
>>> interrupt intercepts. These are triggered for example when debugging the
>>> guest (concurrent PER events result in an intercept instead of an
>>> injection of such interrupts).
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>
>>> Only compile-tested.
>>
>> I checked the Linux code (arch/s390/kernel/traps.c) and Linux uses the FPC (and
>> not the lowcore field) to decide about the signal (SIGFPE) and si_code. So we want
>> to have the correct DXC/VXC value.
>>
>> Now, I wrote a short test program that does
>> feenableexcept(FE_DIVBYZERO);
>> and a division by zero.
>> and attached gdb to that guest together with a breakpoint on the divide (and the instruction
>> after).
>> I get the pint exit for the instruction after (as it is suppressing) and at this point in
>> time the guest fpc already contains the correct DXC value. So you patch will certainly not
>> hurt, but it seems not necessary.
> 
> Thanks for trying. Wonder if that is documented behavior or just works
> by pure luck.


My guess is, that this is works as designed. There is the interruption
parameter block that is used instead of the guest lowcore for program
interrupt exits. To me it looks like that everything is "prepared" except
for the psw swap itself and the data in the lowcore. The data is written 
to the interruption parameter block instead. So that the hypervisor then
just has to move the data and do the psw swap. 

> 
> E.g. it would be interesting to see what other instructions do that
> usually don't touch the DXC, except when injecting an exception. E.g. CRT.
> 
> But if you believe this is not needed, we can also drop it. (if ever
> somebody would want to inject from QEMU, he could also just set the fpc
> directly)

The (unlikely to ever happen) inject from QEMU is indeed a thing where this
patch would simplify things.

I will talk to some hardware folks to verify my assumption but for the time
being, lets drop this patch.


> 
>>
>> Still trying to look further if I missed something.
>>
>>>
>>>  arch/s390/include/asm/ctl_reg.h | 1 +
>>>  arch/s390/kvm/interrupt.c       | 8 ++++++++
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
>>> index 4600453536c2..88f3f14baee9 100644
>>> --- a/arch/s390/include/asm/ctl_reg.h
>>> +++ b/arch/s390/include/asm/ctl_reg.h
>>> @@ -11,6 +11,7 @@
>>>  #include <linux/const.h>
>>>  
>>>  #define CR0_CLOCK_COMPARATOR_SIGN	_BITUL(63 - 10)
>>> +#define CR0_AFP_REGISTER_CONTROL	_BITUL(63 - 45)
>>>  #define CR0_EMERGENCY_SIGNAL_SUBMASK	_BITUL(63 - 49)
>>>  #define CR0_EXTERNAL_CALL_SUBMASK	_BITUL(63 - 50)
>>>  #define CR0_CLOCK_COMPARATOR_SUBMASK	_BITUL(63 - 52)
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index fcb55b02990e..5b5754d8f460 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -765,6 +765,14 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)
>>>  		break;
>>>  	case PGM_VECTOR_PROCESSING:
>>>  	case PGM_DATA:
>>> +		if (vcpu->arch.sie_block->gcr[0] & CR0_AFP_REGISTER_CONTROL) {
>>> +			/* make sure the new fpc will be lazily loaded */
>>> +			save_fpu_regs();
>>> +			/* the DXC/VXC cannot make the fpc invalid */
>>> +			current->thread.fpu.fpc &= ~0xff00u;
>>> +			current->thread.fpu.fpc |= (pgm_info.data_exc_code << 8)
>>> +						   & 0xff00u;
>>
>> maybe reuse  FPC_DXC_MASK instead of 0xff00 ?
>>
> 
> Sure, didn't know about that.
> 
>