From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-x234.google.com (mail-wj0-x234.google.com [IPv6:2a00:1450:400c:c01::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E1CCC81F4C for ; Fri, 2 Dec 2016 02:09:04 -0800 (PST) Received: by mail-wj0-x234.google.com with SMTP id qp4so227790547wjc.3 for ; Fri, 02 Dec 2016 02:09:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=FeEMhr1sb0ikem18YRwCw14Rojl8oeP7WgTpKQHvwis=; b=U8q7IYVnsIp9PBWNLpFdY5DncY9TpFcAyb7ZBSY1eHXD0gQtuUwx8KaJhHiWtrRFY5 I+qpObStYLyi8c3QLicmeJOhFBCf3EZ2yF89dsXqLVF+hign2Sc5ft7m9eNAUYqMrEEv kFf0W7GQj+7KdlA9VzqzjT8PMDXA5yggbKgSs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=FeEMhr1sb0ikem18YRwCw14Rojl8oeP7WgTpKQHvwis=; b=kq+0P9maHzpGolo5vwYe8MFKrcMR5pyXm4n7kXGDqpRX23S9JTjbRXcK4YTnyNoSTk ILLSyskP0ETpPO+m1M+k3/keCJ9ACZkozUMZWnVEsb+P4mv7LOwciuSimhSV7mjDdSPx WNsJPlrMZ6XYml/WavAz88zGyrztfXnXrwUwDUnKTTgOX17JLGBQ8ZQw0rbhE3wbsPBK P2paQWyIT0cq0hwnUIT2d3oBN9jgPjtimjo+d9RXSKnd0tHGxPuaWblUo7umWe24daYR iyxcYcu1Un8ktctVX6oSM4xGdeR04NM80iYx2xqyYdenebpYpJIK2L9jf03vRI5cc3Yt iTag== X-Gm-Message-State: AKaTC03yT059My1YD946jdeF3afOCiEcc1VBlGr6HlZB8P3Z1HL72JHp9U/Tq7Za3AWazYKt X-Received: by 10.194.109.168 with SMTP id ht8mr37570674wjb.36.1480673343059; Fri, 02 Dec 2016 02:09:03 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id f126sm2345814wme.22.2016.12.02.02.09.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 Dec 2016 02:09:02 -0800 (PST) Date: Fri, 2 Dec 2016 10:09:00 +0000 From: Leif Lindholm To: "Ni, Ruiyu" Cc: Ard Biesheuvel , "edk2-devel@lists.01.org" , "Gao, Liming" , "afish@apple.com" , "Kinney, Michael D" , "mw@semihalf.com" , "Tian, Feng" Message-ID: <20161202100900.GP27069@bivouac.eciton.net> 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> MIME-Version: 1.0 In-Reply-To: <20161130140655.GG27069@bivouac.eciton.net> User-Agent: Mutt/1.5.23 (2014-03-12) 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 10:09:05 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Ray, any comments on the below? Regards, Leif On Wed, Nov 30, 2016 at 02:06:55PM +0000, Leif Lindholm wrote: > 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 = Dev->BarOffset; > > > + for (Desc = Dev->Device->Resources, Dev->BarCount = 0; > > > + Desc->Desc != ACPI_END_TAG_DESCRIPTOR; > > > + Desc = (VOID *)((UINT8 *)Desc + Desc->Len + 3)) { > > > + > > > + ASSERT (Desc->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR); > > > + ASSERT (Desc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM); > > > + > > > + if (Idx >= PCI_MAX_BARS || > > > + (Idx == PCI_MAX_BARS - 1 && Desc->AddrSpaceGranularity == 64)) { > > > + DEBUG ((DEBUG_ERROR, > > > + "%a: resource count exceeds number of emulated BARs\n", > > > + __FUNCTION__)); > > > + ASSERT (FALSE); > > > + break; > > > + } > > > + > > > + Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin; > > > + Dev->BarCount++; > > > + > > > + if (Desc->AddrSpaceGranularity == 64) { > > > + Dev->ConfigSpace.Device.Bar[Idx] |= 0x4; > > > + Dev->ConfigSpace.Device.Bar[++Idx] = (UINT32)(Desc->AddrRangeMin >> 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? > > 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 > >