Re: [PATCH 02/10] KVM: s390: add the GIB and its related life-cyle functions

Re: [PATCH 02/10] KVM: s390: add the GIB and its related life-cyle functions

[Janosch Frank]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: multipart/mixed; boundary="--XDjQN55Lte2ZufrDHe7XirFdE1xIorgNl", Size: 3593 bytes --]

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--XDjQN55Lte2ZufrDHe7XirFdE1xIorgNl
Content-Type: multipart/mixed; boundary="SjT5KeSPysEhjjRofzUUrumk4oX3hRDaq";
 protected-headers="v1"
From: Janosch Frank <frankja@linux.ibm.com>
To: Michael Mueller <mimu@linux.ibm.com>, linux-s390@vger.kernel.org
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>,
 Heiko Carstens <heiko.carstens@de.ibm.com>,
 Christian Borntraeger <borntraeger@de.ibm.com>,
 David Hildenbrand <david@redhat.com>, Cornelia Huck <cohuck@redhat.com>,
 Sebastian Ott <sebott@linux.ibm.com>,
 Peter Oberparleiter <oberpar@linux.ibm.com>,
 Vasily Gorbik <gor@linux.ibm.com>, Julian Wiedmann <jwi@linux.vnet.ibm.com>,
 Tony Krowiak <akrowiak@linux.vnet.ibm.com>,
 Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>,
 Halil Pasic <pasic@linux.ibm.com>, Thomas Huth <thuth@redhat.com>,
 Pierre Morel <pmorel@linux.ibm.com>
Message-ID: <95e038f1-8d7b-bc9d-c553-bacbcc6d4410@linux.ibm.com>
Subject: Re: [PATCH 02/10] KVM: s390: add the GIB and its related life-cyle
 functions
References: <20181025123751.48809-1-mimu@linux.ibm.com>
 <20181025123751.48809-3-mimu@linux.ibm.com>
In-Reply-To: <20181025123751.48809-3-mimu@linux.ibm.com>

--SjT5KeSPysEhjjRofzUUrumk4oX3hRDaq
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

On 25.10.18 14:37, Michael Mueller wrote:
> The GIB (Guest Information Block) links the GISA of all guests
> that have adapter interrupts pending. These interrupts cannot be
> deliverd because no vcpu of these guests is currently running in
> SIE context. Instead, a GIB alert is issued on the host to schedule
> these guests to run.
>=20
> This mechanism allows to process adapter interrupts for currently
> not running guests.
>=20
> The GIB is created during host initialization and associated with
> the Adapter Interruption Facility in case an Adapter Interruption
> Virtualization Facility is available.
>=20
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---

>  /*
>   * sie_page2 has to be allocated as DMA because fac_list, crycb and
>   * gisa need 31bit addresses in the sie control block.
> @@ -829,6 +838,7 @@ struct kvm_arch{
>  	/* subset of available cpu features enabled by user space */
>  	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>  	struct kvm_s390_gisa *gisa;
> +	int gib_in_use;
>  };

That's a kvm wide value, right? But here we save it per VM.
Is that really necessary or am I missing something?


--SjT5KeSPysEhjjRofzUUrumk4oX3hRDaq--

--XDjQN55Lte2ZufrDHe7XirFdE1xIorgNl
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJb2W41AAoJEONU5rjiOLn47IoP/A6WyM3+0TFkzBHrvbX7n/gI
NbaV3+ifH65fIk2vbo1jskoW/Er4AcnhC+3vcMJwkIpKUWfq/8EaoRWzdFUM3xvg
1Jydb0n9RZ9RPnH2mKxTmIZOm5SUrxScicTI0yc0bEO1ktySnJRxU44ZULEQSbrE
pLGVJScg+kfJrM653d0njL0LctanPoxvzE6C1ktWIK9LiL1nsxPkjoVH3YQQDoms
++s/V2QuY4ue2qCBuGBOc5WXt+fNqfvdn52zplTFm07kmVSasApSZ2nwa7Vfbnb5
yoyop1QO68Hd5QAvwYNFX2dL3Qk8uqcnxlx5tV8ejArxfGv3M/wuDQaul7muPQTN
RG7F7zCALDIRmYZK2/CXu85bIINnI9D4P1Lw8NnIxGUBSkw7dYHwoDPPZ7LRZmBb
BhicHtSmOnlJDyHCpMuUPj7Qh55j/UraoblZ5igP2OJMr2OyA04QYyKWNWq0aOIX
nNugzb7KzHDM0OWEyAtfNl5eQNGiE7iJu0xMBLgPKh+WwIHXTzezcLalxDUjX+K6
l1gmyf/E7B5nSlHo4F+O/wGOcS3dHZiO0ua72J7rF8u03tK2QgX4H2KRSdTrOVob
C1uAb0jGWiBQ7dGmafldoH8i8ncsQ+6D56caf0ocX/0DNXMpuLoZS8Rk9NHU3Huc
j80zWt3YOE1djDIvYzbN
=MBPT
-----END PGP SIGNATURE-----

--XDjQN55Lte2ZufrDHe7XirFdE1xIorgNl--

Re: [PATCH 02/10] KVM: s390: add the GIB and its related life-cyle functions

[Pierre Morel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

On 06/11/2018 13:06, Michael Mueller wrote:
> 
> 
> On 05.11.18 16:44, Janosch Frank wrote:
>> On 05.11.18 16:36, Halil Pasic wrote:
>>>
>>> On 11/05/2018 04:14 PM, Michael Mueller wrote:
>>>>
>>>> On 05.11.18 15:51, Pierre Morel wrote:
>>>>> On 25/10/2018 14:37, Michael Mueller wrote:
>>> [..]
>>>>>> +
>>>>>> +void kvm_s390_gib_init(u8 nisc)
>>>>>> +{
>>>>>> +��� int rc;
>>>>>> +
>>>>>> +��� if (gib)
>>>>>> +������� return;
>>>>>> +
>>>>>> +��� if (!css_general_characteristics.aiv) {
>>>>>> +������� KVM_EVENT(3, "%s", "gib not initialized, no AIV facility");
>>>>>> +������� return;
>>>>>> +��� }
>>>>>> +
>>>>>> +��� gib = (struct kvm_s390_gib *)get_zeroed_page(GFP_KERNEL | 
>>>>>> GFP_DMA);
>>>>>> +��� if (!gib) {
>>>>>> +������� KVM_EVENT(3, "%s", "gib memory allocation failed");
>>>>>> +������� return;
>>>>>> +��� }
>>>>> Why should we survive this error?
>>>> Design question.
>>>>
>>> I'm with Pierre on this one. We should fail starting the guest
>>> if this allocation fails.
>>>
>> Halil, the GIB is global to KVM, we would need to fail loading/init of
>> KVM, which I'd like to avoid.
>>
>> Neither GIB nor gisa is necessary for a VM to run, so we shouldn't stop
>> on an error.
> 
> I changed the design such that kvm_s390_gib_init() fails and thus 
> kvm_arch_init() as well
> in all error cases except facility AIV is *not* available. This is also 
> a success case but no GIB
> is initialized.

OK, seems right.

> 
>>
>>>>>
>>>>>> +
>>>>>> +��� gib->nisc = nisc;
>>>>>> +��� rc = chsc_sgib((u32)(u64)gib);
>>>>>> +��� if (rc) {
>>>>>> +������� KVM_EVENT(3, "gib 0x%pK AIV association failed rc: %d",
>>>>>> +������������� gib, rc);
>>>>>> +������� free_page((unsigned long)gib);
>>>>>> +������� gib = NULL;
>>>>>> +������� return;
>>>>>> +��� }
>>>>> or this failure ?
>>>>>
>>>>> shouldn't we better return an error and abort loading KVM?
>>>> Design question.
>>>>
>>> Same here I guess. This would be only either due to a guest or
>>> a host bug, or?
>>>
>>> I would prefer being fairly deterministic about whether we do have
>>> a gib or not.
>> Same as above
> 
> see above
> 
>>> Regards,
>>> Halil
>>>
>>
> 


-- 
Pierre Morel
Linux/KVM/QEMU in B�blingen - Germany