From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR04-HE1-obe.outbound.protection.outlook.com (EUR04-HE1-obe.outbound.protection.outlook.com [40.107.7.57]) by mx.groups.io with SMTP id smtpd.web12.17693.1582527843582549619 for ; Sun, 23 Feb 2020 23:04:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nxp.com header.s=selector2 header.b=JXKHQkHr; spf=pass (domain: nxp.com, ip: 40.107.7.57, mailfrom: gaurav.jain@nxp.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=P9HSUWxVQ+KiQ10vQFRZvPs9bH5tJA9bzkaFIXBa6ZwG0/hwwsm4z4UiWG8gSOOinHFE+8b+1PNSfXJvMFOU03qnXrmcozb7TjQVBwTzwCF3GjXHvttVkZ7DbcGRgge7MBygc4bIzPItu2502hI0oaXWnd7zFkKYxsqGigmDUTKQgSLA3k2QBitGlz3aVbTNjFOGpEFDkCg6gqHtRUzDngh+RhKjafD8qYSKAzKFldl/MCfNU7V3kkrMnofJRfP1P0DLLZ4xIYIG86ATm6zlNgAnXml+37tdC7eEvui/OOTKIN7YRYVvjVeJfl6XKgIthwzXl4IZd/i72wl9KXD8fw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=LMGxh28IPfT8LTX32B0oiJ8GHb4eDt+j9TOXAiA2E58=; b=ldTOV6s1q8JdS6BoIp/2fQPy9zi0kaA2T3wsGFGaRt1kBL3k0/NiLVDsIMR+0r3w5X892xofziW64vvBNVfOayp7Amj7sfNInFc0ypV52uiTbEHpuKz5q/IwUdmYbqRRBjxmCJC67aeqB/0IsVYCKAxCORu7xGYJI1gdvOmScTyIZGpMERrV+Cn/O0h7SYdAGlyMKkiC03RJQ3ixxxQpkvshXnGqqmhJ+DNyG2Mgjj/2sJUzI32E5hDH5OtAVn+eOq5ltv+x6XWdHuOf7iwf75od7/DbmC4E5pOQngmAFJi/yDbfoAanvpGYsVLHzJYSQO7zpdIxFnP/Kyh+dZhSqw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com; dkim=pass header.d=nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=LMGxh28IPfT8LTX32B0oiJ8GHb4eDt+j9TOXAiA2E58=; b=JXKHQkHrPlBGlrQF876AI7z4cXrfIs+r8tOtT48Ah8YgE6VlWVc0INaCxrHJfD4gui0WPv4ymFKWT4YxyaDzQMfsm8KWGuGAvkLaoo0qo0tEbbanT25fqnnXtOQmntAJPVmz8XbufGymj65/xvX/ybQVjCVgmgzq0iqP/OJBRo4= Received: from DB7PR04MB4091.eurprd04.prod.outlook.com (52.134.110.144) by DB7PR04MB4106.eurprd04.prod.outlook.com (52.134.108.13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2750.22; Mon, 24 Feb 2020 07:04:01 +0000 Received: from DB7PR04MB4091.eurprd04.prod.outlook.com ([fe80::183:969:2508:3c92]) by DB7PR04MB4091.eurprd04.prod.outlook.com ([fe80::183:969:2508:3c92%6]) with mapi id 15.20.2750.021; Mon, 24 Feb 2020 07:04:00 +0000 From: "Gaurav Jain" To: "Wu, Hao A" , "devel@edk2.groups.io" , "Gao, Liming" , "afish@apple.com" , "lersek@redhat.com" , "leif@nuviainc.com" , "Kinney, Michael D" CC: "Wang, Jian J" , "Ni, Ray" , Ard Biesheuvel , Pankaj Bansal Subject: Re: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. Thread-Topic: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. Thread-Index: AdXoVX1UfqTUDarfQqSjaIalQXdrhgCiUCDg Date: Mon, 24 Feb 2020 07:04:00 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=gaurav.jain@nxp.com; x-originating-ip: [92.120.1.65] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: bbe3ff48-28b3-453b-c407-08d7b8f7b9bf x-ms-traffictypediagnostic: DB7PR04MB4106:|DB7PR04MB4106: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 032334F434 x-forefront-antispam-report: SFV:NSPM;SFS:(10001)(10009020)(4636009)(136003)(376002)(366004)(39860400002)(396003)(346002)(199004)(189003)(2906002)(33656002)(66476007)(86362001)(45080400002)(316002)(52536014)(5660300002)(478600001)(110136005)(54906003)(7416002)(76116006)(66946007)(71200400001)(966005)(9686003)(4326008)(186003)(30864003)(66556008)(53546011)(6506007)(8936002)(81156014)(26005)(81166006)(55016002)(7696005)(44832011)(64756008)(66446008)(8676002)(921003)(1121003);DIR:OUT;SFP:1101;SCL:1;SRVR:DB7PR04MB4106;H:DB7PR04MB4091.eurprd04.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 50klzzuMmLROJcu3dMjcv3RHkYLaQGd8vh3dlLBXiL44DX0pt/ma/mhX19f1plrv/LKb1ZgDXdI6qSJ0FhDhonLSr4rT4LPZjkBlJXoyGfGfUWsqrcZL/MRLf9Ls/86oTTC38EbIZ0o+0YMw8104+62opwrp63YngPwVXO3s9YllV4mWyHeZWNXHFyemthHqCc3fMw7l5H1ZT4GAjzCB2lZxXUcKglzsVV66Qg0Kx96EvfJtZkjRYlVttb8UKffeIbB48od+YtS1lzvhu7lX/EIYYPkxO8FMcZ5knbSlr/JrUDUv/zYkSuRGzuIAx4JDg1yse2pujSkLjU38pDZQiOw37eAXqF+cKDworVaf+/ecFFTnpxrC+wfcAvWkJuaCbzIuTh8w0y+dQ8JpmxP4yNpQK6HUdOZBzP6XOWubqaADy52BiS9buOti7g4zc6droApIx4YBltZUSu3zujlq5N/sm3MYzcrHq2OUzR92tK1ACWHPHUUInmuK9HsZOhXJDvXESK7BAAYeshZqieVZvZMNswQqfDlGBn0Ygecwl/bjfhR4m8PWSNaaZjej2+0IkdN1zYjc1+nUnrKlPeYfyEmRUGI8xXZMsLHRZpVefgLLMJfmD3ZaFTzTekfKiqFDOIGkaOa0K/W4U+U2LP8vvYfh9HrlguTm+niwROTbc6YCXsjuS42b/5dkOOu832RT x-ms-exchange-antispam-messagedata: z8JSjoUxRhadp2AqJ6SqEHWHBQ1e6KCcVnipSy4Y/EYDumIalMyaTHyP5/L2hTlDdUp84ufVrp0NoTU7R54nrrT9PmpKtLBowRhcwElme7naNqhb+QVuGBPjUJu5B3Lpyn9Ie0vdeVEGBQ3Qe64VzA== MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: bbe3ff48-28b3-453b-c407-08d7b8f7b9bf X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Feb 2020 07:04:00.7600 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: W4fNki/6IqwL8poAtHnlcoxImhzMTk6pPPvShBOIuAaFuU/pbbmEHsQg4+2XsUhahyBwlhxb02UAPHSXoqnY8w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR04MB4106 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > I think the above check for 'Attributes' can be dropped. > I found that the implementation of the PciIoGetBarAttributes() function = does not > expose any configurable attributes. So the logic can fall through to the= ASSERT > (for DEBUG images) and then returns EFI_UNSUPPORTED. I agree that PciIoGetBarAttributes() function sets *Supports as 0. But In SCT Test for SetBarAttributes, there is a test case for Unsupported= Attribute which expects EFI_UNSUPPORTED. If I drop this check, ASSERT will= come, which is not expected. Can we keep check for 'Attributes'? > -----Original Message----- > From: Wu, Hao A > Sent: Friday, February 21, 2020 6:53 AM > To: devel@edk2.groups.io; Gaurav Jain ; Gao, Liming > ; afish@apple.com; lersek@redhat.com; > leif@nuviainc.com; Kinney, Michael D > Cc: Wang, Jian J ; Ni, Ray ; Ar= d > Biesheuvel ; Pankaj Bansal > > Subject: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] > MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. >=20 > Caution: EXT Email >=20 > A couple of inline comments below. Please help to handle them in the nex= t > version of patch. > With them addressed, > Reviewed-by: Hao A Wu >=20 >=20 > Hello Liming and Stewards, >=20 > I would like to confirm with you for whether the patch should catch the > upcoming stable tag. >=20 > My personal take is that the patch is more like a code refinement rather= than a > bug fix. >=20 > Could you help to make a final call for this one? Thanks in advance. >=20 > Best Regards, > Hao Wu >=20 >=20 > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Gaurav Jain > > Sent: Thursday, February 20, 2020 11:40 PM > > To: devel@edk2.groups.io > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Ard Biesheuvel; Pankaj Bansal; > > Gaurav Jain > > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts > > in SCT PCIIO Protocol Test. > > > > ASSERT in PollMem_Conf, CopyMem_Conf, SetBarAttributes_Conf > > Conformance Test. > > SCT Test expect return as Invalid Parameter or Unsupported. > > Added Checks for Function Parameters. > > return Invalid or Unsupported if Check fails. > > > > Added Checks in PciIoPollIo(), PciIoIoRead() > > PciIoIoWrite() > > > > Signed-off-by: Gaurav Jain > > --- > > > > Notes: > > v2 > > - Reverted ASSERT(FALSE) code. > > - Added Checks for Width, BarIndex, Buffer, > > Address range in PciIoIoRead, PciIoIoWrite. > > - Added Checks for Width, BarIndex, Result, > > Address range in PciIoPollIo, PciIoPollMem, > > PciIoCopyMem. > > - Added Checks for Attributes, BarIndex, > > Address range in PciIoSetBarAttributes. > > > > .../NonDiscoverablePciDeviceIo.c | 180 +++++++++++++++++= + > > 1 file changed, 180 insertions(+) > > > > diff --git > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > index 2d55c9699322..4dd804356021 100644 > > --- > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > +++ > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > @@ -93,6 +93,35 @@ PciIoPollMem ( > > OUT UINT64 *Result > > ) > > { > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > + UINTN Count; > > + EFI_STATUS Status; > > + > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (BarIndex >=3D PCI_MAX_BAR) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + if (Result =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > + Count =3D 1; > > + > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > + (Status)) { > > + return Status; > > + } > > + > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > + return EFI_UNSUPPORTED; > > + } > > + > > ASSERT (FALSE); > > return EFI_UNSUPPORTED; > > } > > @@ -126,6 +155,35 @@ PciIoPollIo ( > > OUT UINT64 *Result > > ) > > { > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > + UINTN Count; > > + EFI_STATUS Status; > > + > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (BarIndex >=3D PCI_MAX_BAR) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + if (Result =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > + Count =3D 1; > > + > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > + (Status)) { > > + return Status; > > + } > > + > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > + return EFI_UNSUPPORTED; > > + } > > + > > ASSERT (FALSE); > > return EFI_UNSUPPORTED; > > } > > @@ -396,6 +454,33 @@ PciIoIoRead ( > > IN OUT VOID *Buffer > > ) > > { > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > + EFI_STATUS Status; > > + > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > + return EFI_INVALID_PARAMETER; > > + } >=20 >=20 > For PciIoIoRead(), I think enum values smaller than EfiPciIoWidthMaximum= are > all valid. The above check seems to strict. Will address this in v3. >=20 >=20 > > + > > + if (BarIndex >=3D PCI_MAX_BAR) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + if (Buffer =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > + > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > + (Status)) { > > + return Status; > > + } > > + > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > + return EFI_UNSUPPORTED; > > + } > > + > > ASSERT (FALSE); > > return EFI_UNSUPPORTED; > > } > > @@ -425,6 +510,33 @@ PciIoIoWrite ( > > IN OUT VOID *Buffer > > ) > > { > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > + EFI_STATUS Status; > > + > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > + return EFI_INVALID_PARAMETER; > > + } >=20 >=20 > For PciIoIoWrite(), I think enum values smaller than EfiPciIoWidthMaximu= m are > all valid. The above check seems to strict. Will address this in v3. >=20 >=20 > > + > > + if (BarIndex >=3D PCI_MAX_BAR) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + if (Buffer =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > + > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > + (Status)) { > > + return Status; > > + } > > + > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > + return EFI_UNSUPPORTED; > > + } > > + > > ASSERT (FALSE); > > return EFI_UNSUPPORTED; > > } > > @@ -556,6 +668,40 @@ PciIoCopyMem ( > > IN UINTN Count > > ) > > { > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *DestDesc; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *SrcDesc; > > + EFI_STATUS Status; > > + > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (DestBarIndex >=3D PCI_MAX_BAR || > > + SrcBarIndex >=3D PCI_MAX_BAR) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > + > > + Status =3D GetBarResource (Dev, DestBarIndex, &DestDesc); if > > + (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + if (DestOffset + (Count << (Width & 0x3)) > DestDesc->AddrLen) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + Status =3D GetBarResource (Dev, SrcBarIndex, &SrcDesc); if > > + (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + if (SrcOffset + (Count << (Width & 0x3)) > SrcDesc->AddrLen) { > > + return EFI_UNSUPPORTED; > > + } > > + > > ASSERT (FALSE); > > return EFI_UNSUPPORTED; > > } > > @@ -1414,6 +1560,40 @@ PciIoSetBarAttributes ( > > IN OUT UINT64 *Length > > ) > > { > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > + EFI_PCI_IO_PROTOCOL_WIDTH Width; > > + UINTN Count; > > + EFI_STATUS Status; > > + > > + #define DEV_SUPPORTED_ATTRIBUTES \ > > + (EFI_PCI_DEVICE_ENABLE | > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) > > + > > + if ((Attributes & (~DEV_SUPPORTED_ATTRIBUTES)) !=3D 0) { > > + return EFI_UNSUPPORTED; > > + } >=20 >=20 > I think the above check for 'Attributes' can be dropped. > I found that the implementation of the PciIoGetBarAttributes() function = does not > expose any configurable attributes. So the logic can fall through to the= ASSERT > (for DEBUG images) and then returns EFI_UNSUPPORTED. >=20 > Best Regards, > HaoWu >=20 >=20 > > + > > + if (BarIndex >=3D PCI_MAX_BAR) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + if (Offset =3D=3D NULL || Length =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > + Width =3D EfiPciIoWidthUint8; > > + Count =3D (UINT32) *Length; > > + > > + Status =3D GetBarResource(Dev, BarIndex, &Desc); if (EFI_ERROR > > + (Status)) { > > + return Status; > > + } > > + > > + if (*Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > + return EFI_UNSUPPORTED; > > + } > > + > > ASSERT (FALSE); > > return EFI_UNSUPPORTED; > > } > > -- > > 2.17.1 > > > > > >=20