From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 371AFC3F2D1 for ; Thu, 5 Mar 2020 14:12:55 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 092BB20709 for ; Thu, 5 Mar 2020 14:12:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 092BB20709 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.ibm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:49828 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j9rF8-0001qg-8B for qemu-devel@archiver.kernel.org; Thu, 05 Mar 2020 09:12:54 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:59717) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j9rEA-00015x-7K for qemu-devel@nongnu.org; Thu, 05 Mar 2020 09:11:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j9rE8-00009w-V4 for qemu-devel@nongnu.org; Thu, 05 Mar 2020 09:11:54 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:50890) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1j9rE8-0007ul-MJ for qemu-devel@nongnu.org; Thu, 05 Mar 2020 09:11:52 -0500 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 025E9nP5141306 for ; Thu, 5 Mar 2020 09:11:28 -0500 Received: from e06smtp07.uk.ibm.com (e06smtp07.uk.ibm.com [195.75.94.103]) by mx0a-001b2d01.pphosted.com with ESMTP id 2yhryewaj9-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 05 Mar 2020 09:11:27 -0500 Received: from localhost by e06smtp07.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 5 Mar 2020 14:11:24 -0000 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp07.uk.ibm.com (192.168.101.137) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Thu, 5 Mar 2020 14:11:22 -0000 Received: from d06av23.portsmouth.uk.ibm.com (d06av23.portsmouth.uk.ibm.com [9.149.105.59]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 025EBLNd45613084 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 5 Mar 2020 14:11:21 GMT Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 616BBA4051; Thu, 5 Mar 2020 14:11:21 +0000 (GMT) Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 0BE3DA4053; Thu, 5 Mar 2020 14:11:21 +0000 (GMT) Received: from oc2783563651 (unknown [9.152.224.114]) by d06av23.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 5 Mar 2020 14:11:20 +0000 (GMT) Date: Thu, 5 Mar 2020 15:11:19 +0100 From: Halil Pasic To: Christian Borntraeger Subject: Re: [PATCH 1/1] s390/ipl: sync back loadparm In-Reply-To: References: <20200224150213.21253-1-pasic@linux.ibm.com> <05f7dcf7-a0c7-8811-6b88-df86d5fa0974@redhat.com> <20200225125641.72e8cc86.pasic@linux.ibm.com> <853387e3-4425-731b-bb09-a7210ea6b299@linux.ibm.com> Organization: IBM X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 x-cbid: 20030514-0028-0000-0000-000003E12E9A X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 20030514-0029-0000-0000-000024A6639E Message-Id: <20200305151119.6cd63e96.pasic@linux.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.572 definitions=2020-03-05_04:2020-03-05, 2020-03-05 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 malwarescore=0 bulkscore=0 phishscore=0 impostorscore=0 spamscore=0 adultscore=0 clxscore=1015 mlxlogscore=999 priorityscore=1501 mlxscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2003050092 Content-Transfer-Encoding: quoted-printable X-MIME-Autoconverted: from 8bit to quoted-printable by mx0a-001b2d01.pphosted.com id 025E9nP5141306 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 148.163.156.1 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Huth , Janosch Frank , David Hildenbrand , Cornelia Huck , qemu-devel@nongnu.org, qemu-s390x@nongnu.org, Marc Hartmayer , Viktor Mihajlovski , Richard Henderson Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Thu, 5 Mar 2020 13:44:31 +0100 Christian Borntraeger wrote: >=20 >=20 > On 25.02.20 15:35, Viktor Mihajlovski wrote: > >=20 > >=20 > > On 2/25/20 12:56 PM, Halil Pasic wrote: > >> On Tue, 25 Feb 2020 10:39:40 +0100 > >> David Hildenbrand wrote: > >> > >>> On 24.02.20 16:02, Halil Pasic wrote: > >>>> We expose loadparm as a r/w machine property, but if loadparm is s= et by > >>>> the guest via DIAG 308, we don't update the property. Having a > >>>> disconnect between the guest view and the QEMU property is not nic= e in > >>>> itself, but things get even worse for SCSI, where under certain > >>>> circumstances (see 789b5a401b "s390: Ensure IPL from SCSI works as > >>>> expected" for details) we call s390_gen_initial_iplb() on resets > >>>> effectively overwriting the guest/user supplied loadparm with the = stale > >>>> value. > >>>> > >>>> Signed-off-by: Halil Pasic > >>>> Fixes: 7104bae9de "hw/s390x: provide loadparm property for the mac= hine" > >>>> Reported-by: Marc Hartmayer > >>>> Reviewed-by: Janosch Frank > >>>> Reviewed-by: Viktor Mihajlovski > >>>> Tested-by: Marc Hartmayer > >>>> --- > >>>> =C2=A0 hw/s390x/ipl.c | 21 +++++++++++++++++++++ > >>>> =C2=A0 1 file changed, 21 insertions(+) > >>>> > >>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > > [...] > >>>> + > >>>> +=C2=A0=C2=A0=C2=A0 /* Sync loadparm */ > >>>> +=C2=A0=C2=A0=C2=A0 if (iplb->flags & DIAG308_FLAGS_LP_VALID) { > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 char ascii_loadparm[8]= ; > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint8_t *ebcdic_loadpa= rm =3D iplb->loadparm; > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int i; > >>>> + > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for (i =3D 0; i < 8 &&= ebcdic_loadparm[i]; i++) { > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= ascii_loadparm[i] =3D ebcdic2ascii[(uint8_t) ebcdic_loadparm[i]]; > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ascii_loadparm[i] =3D = 0; > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 object_property_set_st= r(mo, ascii_loadparm, "loadparm", NULL); > >>>> +=C2=A0=C2=A0=C2=A0 } else { > >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 object_property_set_st= r(mo, "", "loadparm", NULL); > >>>> +=C2=A0=C2=A0=C2=A0 } > >>> > >>> &error_abort instead of NULL, we certainly want to know if this wou= ld > >>> ever surprisingly fail. > >> > >> IMHO this is a typical assert() situation where one would like to ha= ve > >> a fast and obvious failure when testing, but not in production. > >> > >> AFAIU the guest can trigger this code at any time, and crashing the > >> whole (production) system seems a bit heavy handed to me. The setter > >> should only fail if something is buggy. > >> > >> But if the majority says &error_abort I can certainly do. Other opin= ions? > >> > > We might consider to return 0x0402 (invalid parameter) from the diag3= 08 "set", which is less drastic and would allow the OS to do whatever it = finds appropriate to deal with the failure. Not that Linux would care abo= ut that today :-). >=20 > I think it is not an error. It is perfectly fine for a guest to not set= DIAG308_FLAGS_LP_VALID if the guest does not want to set it. The LOADPAR= M is supposed to be ignored then. >=20 I believe David's concern was not the else branch, but the last parameter of object_property_set_str(), which tells us what to do if the validation/normalization done by the setter of the loadparm qemu property fails the set operation. > So we have two options: > a. leave the patch as-is. This means that we replace the loadparm with = an empty string > b. remove the else. THis means that we leave the global loadparm unchan= ged if the guest does not specify one (but it specifies a new IPLB). >=20 > I will double check what LPAR does. >=20 Thanks! BTW my reading of the architecture and understanding how we expose it via qemu interfaces makes me to lean towards option a). In my understanding we represent invalid loadparm with an empty string in the context of the qemu property. That is we don't expose the garbage-value. Regards, Halil