From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ehD2P-0006E4-M9 for qemu-devel@nongnu.org; Thu, 01 Feb 2018 06:28:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ehD2K-0003XW-Nz for qemu-devel@nongnu.org; Thu, 01 Feb 2018 06:28:17 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:36442 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ehD2K-0003Wf-Ib for qemu-devel@nongnu.org; Thu, 01 Feb 2018 06:28:12 -0500 Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w11BOViO079930 for ; Thu, 1 Feb 2018 06:28:10 -0500 Received: from e14.ny.us.ibm.com (e14.ny.us.ibm.com [129.33.205.204]) by mx0b-001b2d01.pphosted.com with ESMTP id 2fv0fjcn9a-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 01 Feb 2018 06:28:09 -0500 Received: from localhost by e14.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 1 Feb 2018 06:28:09 -0500 References: <20180130094715.11578-1-zyimin@linux.vnet.ibm.com> <20180130094715.11578-2-zyimin@linux.vnet.ibm.com> <20180131115824.7329b8ee.cohuck@redhat.com> From: Yi Min Zhao Date: Thu, 1 Feb 2018 19:28:02 +0800 MIME-Version: 1.0 In-Reply-To: <20180131115824.7329b8ee.cohuck@redhat.com> Content-Type: text/plain; charset=gbk; format=flowed Message-Id: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: qemu-devel@nongnu.org, borntraeger@de.ibm.com, pasic@linux.vnet.ibm.com, pmorel@linux.vnet.ibm.com, alex.williamson@redhat.com, marcel@redhat.com =D4=DA 2018/1/31 =CF=C2=CE=E76:58, Cornelia Huck =D0=B4=B5=C0: > On Tue, 30 Jan 2018 10:47:13 +0100 > Yi Min Zhao wrote: > >> Current s390x PCI IOMMU code is lack of flags' checking, including: >> 1) protection bit >> 2) table length >> 3) table offset >> 4) intermediate tables' invalid bit >> 5) format control bit >> >> This patch introduces a new struct named S390IOTLBEntry, and makes up >> these missed checkings. At the same time, inform the guest with the >> corresponding error number when the check fails. > There are a lot of things in this patch I cannot review due to -ENODOC, > but some comments below. > >> Reviewed-by: Pierre Morel >> Signed-off-by: Yi Min Zhao >> --- >> hw/s390x/s390-pci-bus.c | 223 +++++++++++++++++++++++++++++++++++++= +--------- >> hw/s390x/s390-pci-bus.h | 10 +++ >> hw/s390x/s390-pci-inst.c | 10 --- >> 3 files changed, 190 insertions(+), 53 deletions(-) > (...) > >> +/* ett is expected table type, -1 page table, 0 segment table, 1 regi= on table */ >> +static uint64_t get_table_index(uint64_t iova, int8_t ett) >> +{ >> + switch (ett) { >> + case -1: >> + return calc_px(iova); >> + case 0: >> + return calc_sx(iova); >> + case 1: >> + return calc_rtx(iova); >> + } >> + >> + return -1; > You use ett to differentiate between the three table types a lot. Is > this an architectured value, or an internal construct? It's an architectured value to some degree, because it's used to descript the translation more clearly in the doc. > > If you introduced it yourself, it might make sense to switch to an enum > instead. Otherwise, using some #defines would improve readability of > the code. OK. I will add macros in next version. > >> +} > (...) > >> +/** >> + * table_translate: do translation within one table and return the fo= llowing >> + * table origin >> + * >> + * @entry: the entry being traslated, the result is stored in this. > s/traslated/translated/ OK. > >> + * @to: the address of table origin. >> + * @ett: expected table type, 1 region table, 0 segment table and -1 = page table. >> + * @error: error code >> + */ >> +static uint64_t table_translate(S390IOTLBEntry *entry, uint64_t to, i= nt8_t ett, >> + uint16_t *error) > (...) > >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >> index be449210d9..63fa06fb97 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, = uint8_t r2, uintptr_t ra) >> =20 >> while (start < end) { >> entry =3D imrc->translate(iommu_mr, start, IOMMU_NONE); >> - >> - if (!entry.translated_addr) { >> - pbdev->state =3D ZPCI_FS_ERROR; >> - setcc(cpu, ZPCI_PCI_LS_ERR); >> - s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES); >> - s390_pci_generate_error_event(ERR_EVENT_SERR, pbdev->fh, = pbdev->fid, >> - start, ERR_EVENT_Q_BIT); >> - goto out; >> - } >> - >> memory_region_notify_iommu(iommu_mr, entry); >> start +=3D entry.addr_mask + 1; > You're now progressing even though you might have generated an error > event. Is that what's intended? Yes, this is wrong. The right thing is only delete the code generating=20 error event, and keep the if check here in this patch. > >> } >