From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 728EC81D68 for ; Fri, 2 Dec 2016 04:54:51 -0800 (PST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP; 02 Dec 2016 04:54:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,729,1477983600"; d="scan'208";a="36482975" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga004.jf.intel.com with ESMTP; 02 Dec 2016 04:54:50 -0800 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 2 Dec 2016 04:54:50 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 2 Dec 2016 04:54:49 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.142]) by shsmsx102.ccr.corp.intel.com ([169.254.2.239]) with mapi id 14.03.0248.002; Fri, 2 Dec 2016 20:54:46 +0800 From: "Ni, Ruiyu" To: Leif Lindholm CC: Ard Biesheuvel , "edk2-devel@lists.01.org" , "Gao, Liming" , "afish@apple.com" , "Kinney, Michael D" , "mw@semihalf.com" , "Tian, Feng" Thread-Topic: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices Thread-Index: AQHSRzKX75V+6GjrZE+UJ1Wz2vrVWaDtphnAgANraICAA5VTAA== Date: Fri, 2 Dec 2016 12:54:45 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D58E926A5@SHSMSX104.ccr.corp.intel.com> References: <1480088538-21834-1-git-send-email-ard.biesheuvel@linaro.org> <1480088538-21834-4-git-send-email-ard.biesheuvel@linaro.org> <734D49CCEBEEF84792F5B80ED585239D58E8CFA1@SHSMSX104.ccr.corp.intel.com> <20161130140655.GG27069@bivouac.eciton.net> In-Reply-To: <20161130140655.GG27069@bivouac.eciton.net> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjNkYTNmZDgtMzlmOS00MmFmLWJlMjgtMGYwOTMzZDZlYmVjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InJKMExrWHpnNlh6c09lbFd2eFN6WXFvVzJaeGs3NnNONVlyNklzWml5d0E9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Dec 2016 12:54:51 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Regards, Ray >-----Original Message----- >From: Leif Lindholm [mailto:leif.lindholm@linaro.org] >Sent: Wednesday, November 30, 2016 10:07 PM >To: Ni, Ruiyu >Cc: Ard Biesheuvel ; edk2-devel@lists.01.org; G= ao, Liming ; >afish@apple.com; Kinney, Michael D ; mw@semiha= lf.com; Tian, Feng >Subject: Re: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver= for non-discoverable devices > >Hi Ray, > >Ard is on holiday this week, and mostly managing to stay off the >email... > >Anyway, I could really use this support in order to bring the Marvell >Armada 70x0 support (worked on by the Semihalf guys) into >OpenPlatformPkg, so I will pick this up while he is away - at least >parts 1-3. > >4/5 is required for 5/5, so I will not be looking to push the final >one myself. > >Would you OK with pushing just 1-3 once we get this one resolved? > >On Mon, Nov 28, 2016 at 01:56:21AM +0000, Ni, Ruiyu wrote: >> > diff --git >> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable >> > PciDeviceIo.c >> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable >> > PciDeviceIo.c >> > new file mode 100644 >> > index 000000000000..fd59267a8d66 >> > --- /dev/null >> > +++ >> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable >> > Pc >> > +++ iDeviceIo.c >> > @@ -0,0 +1,797 @@ >... >> > +VOID >> > +InitializePciIoProtocol ( >> > + NON_DISCOVERABLE_PCI_DEVICE *Dev >> > + ) >> > +{ >> > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; >> > + INTN Idx; >> > + >> > + InitializeListHead (&Dev->UncachedAllocationList); > >The above line has crept in here, but belongs to 4/5 (causes >compilation error). I will fix that and resend - is it OK if I resend >this patch only? > >> > + // Iterate over the resources to populate the virtual BARs >> > + // >> > + Idx =3D Dev->BarOffset; >> > + for (Desc =3D Dev->Device->Resources, Dev->BarCount =3D 0; >> > + Desc->Desc !=3D ACPI_END_TAG_DESCRIPTOR; >> > + Desc =3D (VOID *)((UINT8 *)Desc + Desc->Len + 3)) { >> > + >> > + ASSERT (Desc->Desc =3D=3D ACPI_ADDRESS_SPACE_DESCRIPTOR); >> > + ASSERT (Desc->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_MEM); >> > + >> > + if (Idx >=3D PCI_MAX_BARS || >> > + (Idx =3D=3D PCI_MAX_BARS - 1 && Desc->AddrSpaceGranularity = =3D=3D 64)) { >> > + DEBUG ((DEBUG_ERROR, >> > + "%a: resource count exceeds number of emulated BARs\n", >> > + __FUNCTION__)); >> > + ASSERT (FALSE); >> > + break; >> > + } >> > + >> > + Dev->ConfigSpace.Device.Bar[Idx] =3D (UINT32)Desc->AddrRangeMin; >> > + Dev->BarCount++; >> > + >> > + if (Desc->AddrSpaceGranularity =3D=3D 64) { >> > + Dev->ConfigSpace.Device.Bar[Idx] |=3D 0x4; >> > + Dev->ConfigSpace.Device.Bar[++Idx] =3D (UINT32)(Desc->AddrRange= Min >> 32); >> >> 1. You need to use RShiftU64 otherwise the above code will break the >> build process in 32bit. > >I don't understand how that could cause breakage (and experiments with >IA32 and ARM fail to reproduce it with GCC). > >- Bar[x] is an array of UINT32 >- Desc->AddrRangeMin is UINT64 >- The result of (Desc->AddrRangeMin >> 32) is explicitly cast to > UINT32 (the type of Bar[x]). > >Is the problem related to the shift value being signed? >Or does some other compiler refuse to cast a UINT64 to a UINT32? Desc->AddrRangeMin is UINT64. My experience tells me that LShiftU64 or RShiftU64 should be used for shifting operation on UINT64. I guess the GCC might cleverly recognizes the ">>32" actually picks up the high-32 bits so in 32bit environment, it just uses value of the register which stores the high-32 bits. Can you check the assembly code GCC generates? or simply can you change ">>32" to ">>31" or ">>33" to see what happens? > >Regards, > >Leif > >> > + } >> > + } >> > +} >> > diff --git >> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable >> > PciDeviceIo.h >> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable >> > PciDeviceIo.h >> > new file mode 100644 >> > index 000000000000..bc0a3d3258f9 >> > --- /dev/null >> > +++ >> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable >> > Pc >> > +++ iDeviceIo.h >> > @@ -0,0 +1,84 @@ >> > +/** @file >> > + >> > + Copyright (C) 2016, Linaro Ltd. All rights reserved.
>> > + >> > + This program and the accompanying materials are licensed and made >> > + available under the terms and conditions of the BSD License which >> > + accompanies this distribution. The full text of the license may be >> > + found at http://opensource.org/licenses/bsd-license.php >> > + >> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" >> > BASIS, >> > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >> > EXPRESS OR IMPLIED. >> > + >> > +**/ >> > + >> > +#ifndef __NON_DISCOVERABLE_PCI_DEVICE_IO_H__ >> > +#define __NON_DISCOVERABLE_PCI_DEVICE_IO_H__ >> > + >> > +#include >> > +#include >> > +#include #include >> > + >> > +#include >> > + >> > +#include >> > + >> > +#include >> > +#include >> > +#include >> > + >> > +#define NON_DISCOVERABLE_PCI_DEVICE_SIG SIGNATURE_32 ('P', 'P', 'I', >> > +'D') >> > + >> > +#define NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(PciIoPointer) \ >> > + CR (PciIoPointer, NON_DISCOVERABLE_PCI_DEVICE, PciIo, \ >> > + NON_DISCOVERABLE_PCI_DEVICE_SIG) >> > + >> > +#define PCI_ID_VENDOR_UNKNOWN 0xffff >> > +#define PCI_ID_DEVICE_DONTCARE 0x0000 >> > + >> > +#define PCI_MAX_BARS 6 >> > + >> > +typedef struct { >> > + UINT32 Signature; >> > + // >> > + // The bound non-discoverable device protocol instance >> > + // >> > + NON_DISCOVERABLE_DEVICE *Device; >> > + // >> > + // The exposed PCI I/O protocol instance. >> > + // >> > + EFI_PCI_IO_PROTOCOL PciIo; >> > + // >> > + // The emulated PCI config space of the device. Only the minimally >> > +required >> > + // items are assigned. >> > + // >> > + PCI_TYPE00 ConfigSpace; >> > + // >> > + // The first virtual BAR to assign based on the resources described >> > + // by the non-discoverable device. >> > + // >> > + UINT32 BarOffset; >> > + // >> > + // The number of virtual BARs we expose based on the number of >> > + // resources >> > + // >> > + UINT32 BarCount; >> > + // >> > + // The PCI I/O attributes for this device >> > + // >> > + UINT64 Attributes; >> > + // >> > + // Whether this device has been enabled >> > + // >> > + BOOLEAN Enabled; >> > +} NON_DISCOVERABLE_PCI_DEVICE; >> > + >> > +VOID >> > +InitializePciIoProtocol ( >> > + NON_DISCOVERABLE_PCI_DEVICE *Device >> > + ); >> > + >> > +extern EFI_COMPONENT_NAME_PROTOCOL gComponentName; extern >> > +EFI_COMPONENT_NAME2_PROTOCOL gComponentName2; >> > + >> > +#endif >> > diff --git a/MdeModulePkg/MdeModulePkg.dsc >> > b/MdeModulePkg/MdeModulePkg.dsc index 20e5e8c78be0..5dd30556cb3c >> > 100644 >> > --- a/MdeModulePkg/MdeModulePkg.dsc >> > +++ b/MdeModulePkg/MdeModulePkg.dsc >> > @@ -265,6 +265,7 @@ [Components] >> > MdeModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf >> > MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf >> > MdeModulePkg/Bus/Isa/Ps2MouseDxe/Ps2MouseDxe.inf >> > + >> > + >> > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci >> > Dev >> > + iceDxe.inf >> > >> > MdeModulePkg/Core/Dxe/DxeMain.inf { >> > >> > -- >> > 2.7.4 >>