From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-x22a.google.com (mail-wj0-x22a.google.com [IPv6:2a00:1450:400c:c01::22a]) (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 CF06781F44 for ; Fri, 2 Dec 2016 06:40:54 -0800 (PST) Received: by mail-wj0-x22a.google.com with SMTP id v7so233571412wjy.2 for ; Fri, 02 Dec 2016 06:40:54 -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=VW6Dzr1/Y2+GOt0ow+gXQ4YuzD4fq2LA22Gx0rc/wjE=; b=dHf7A+DS2dQ72zG8cMmsVdYcgQmGjw4XedntjTxgBb8lrLUdsE/E7dnC7tVJuGPhw7 0dv4jmNTiPG28zmNfoVSinO4iTIR557Nm3in+Gv2Kv7cXWgKJXKTtgCnHxqkWskFpryI 6WJEV4LkVCHpEuhDXTgUoxDaWcGdE0xEH4mnw= 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=VW6Dzr1/Y2+GOt0ow+gXQ4YuzD4fq2LA22Gx0rc/wjE=; b=IAtroTxNXB1KH6b7+Ml7yOZ3N+3C1Y/bLbgMu488FuAOSmnkGNixNNxQYp0hQybiEg Yt8k4wubqKAH/laU1BjOx862AkokR+9wk1tvuM5qArAGMhj23pkfOv5yl2iAY7MG3rln saJ9LH+zsRtOaIK2MRqFHbnRSB/pwPE8iKR8G/62cANxikuO6569rc3GsgAQijLXVbTi +RPDULV4Pjy48Ctza1sIpamSHaxQwvSPM5i2hozMhsK+RXrJeI7HgF/xzXRifwhjMR96 1mszjzZFsSI4YbmbqtjTLskZG9H9OSY1cJ+UIr1E1LySfDr065/a4nCFPXJKO+3GEtdN eKKw== X-Gm-Message-State: AKaTC03e0DOQy83/g9X6TqUruBhd1Uu55Xv5YrK0z4Jsg2bKRh4eaXFtG1MY2A3hmA9wcQmV X-Received: by 10.194.0.167 with SMTP id 7mr37055164wjf.80.1480689652729; Fri, 02 Dec 2016 06:40:52 -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 dj5sm5880968wjb.34.2016.12.02.06.40.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 Dec 2016 06:40:52 -0800 (PST) Date: Fri, 2 Dec 2016 14:40:50 +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: <20161202144050.GV27069@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> <734D49CCEBEEF84792F5B80ED585239D58E926A5@SHSMSX104.ccr.corp.intel.com> MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D58E926A5@SHSMSX104.ccr.corp.intel.com> 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 14:40:55 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Dec 02, 2016 at 12:54:45PM +0000, Ni, Ruiyu wrote: > >> > + 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? > > Desc->AddrRangeMin is UINT64. My experience tells me that > LShiftU64 or RShiftU64 should be used for shifting operation > on UINT64. This is the bit that confuses me. There is no such automatic limitation in the C language (if long long is supported at all). So if we're working around some bug in a specific compiler - I am happy to do so. I just prefer knowing why rather than cargo-culting. > 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? There is no cleverness required, it's just a defined operation on a base type. But sure, for example: unsigned int shift(long long val) { return (val >> 33); } decodes as --- 0000000000000000 : 0: 48 89 f8 mov %rdi,%rax 3: 48 c1 f8 21 sar $0x21,%rax 7: c3 retq --- (in x86_64, using gcc -O2 to get rid of some redundant stack operations) The only thing that changes if the shift value is modified is that the 0x21 is changed accordingly. Change it to an inline constant, not much changes: unsigned int shiftconst(void) { unsigned long long var = 0xaaaabbbbccccddddULL; return (var >> 31); } decodes as --- 0000000000000000 : 0: 55 push %rbp 1: 48 89 e5 mov %rsp,%rbp 4: 48 b8 dd dd cc cc bb movabs $0xaaaabbbbccccdddd,%rax b: bb aa aa e: 48 89 45 f8 mov %rax,-0x8(%rbp) 12: 48 8b 45 f8 mov -0x8(%rbp),%rax 16: 48 c1 e8 1f shr $0x1f,%rax 1a: 5d pop %rbp 1b: c3 retq --- (with -O0, to prevent the compiler from just doing the arithmetic compile time) Both also compile correctly for IA32 with -m32. The point is that there is nothing clever here for the compiler to do: casting a 64-bit int to a 32-bit int is an operation that discards the top 32 bits - but the behaviour is entirely defined. In fact, all architectures other than IA32/ARM use the Math64.c implementation of RShiftU64, which simply does the shift. And ARM does it only for GCC. I'm not sure why, I can't imagine a GCC version that would do anything more complicated than --- 00000000 : 0: e1a000c1 asr r0, r1, #1 4: e12fff1e bx lr --- (and that's with -march=armv4t onwards). So if we are saying that there are certain specific compilers that do not support 64-bit arithmetic for 32-bit architectures, and that those compilers are still supported by BaseTools - then I'm fine with that. But I don't want to replace functional C code with abstraction functions without a proper understanding of where it would be problematic. Regards, Leif > > > > >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 > >>