From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-x236.google.com (mail-wj0-x236.google.com [IPv6:2a00:1450:400c:c01::236]) (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 1705381F50 for ; Fri, 2 Dec 2016 07:56:42 -0800 (PST) Received: by mail-wj0-x236.google.com with SMTP id mp19so236169389wjc.1 for ; Fri, 02 Dec 2016 07:56:41 -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=c58kb1IZ2nEku7yiwRP4UBCvq142P4V083R8ejlbOkk=; b=J3hQE/rW2tJHoYpEuyEefFL8VfCeCx1uc1E2EEtq+juLxOoetHNatnFg6Q6kyRVtZS G+Ytsh1ZBibSQ2StQGHR9uCrlW5SiTNftd6UzVY+pax2RL1vrhZSu2z6zDaus/Gg3OPl vpOUtHAEFN2RPTj/XvUl1c0CiV+lDryRI1eXo= 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=c58kb1IZ2nEku7yiwRP4UBCvq142P4V083R8ejlbOkk=; b=lcqd6J/cJG/WBUkX8tvxP3gt4VgBP/kXNAniEJhvP52kE+/AUvhl1bdVXrmdw9vZ8Y L3ROXRn6ZXRhUuuQW2blecD1E5j6F60fSALsyux63DeaNlC22xWv8wswha1gMJ/u2KKI 18ytvxnXi7rh/pOsq57S/Z3zdDfEaEpkUMJUISBKIfJxIJmN9jNO1DMAPWLkysJO/DLq bZdmB6QoLFxCjr0uO6UpHT+wFHMLrMZEwe307bWDj30uT7FOdyWhL8blqVNKgoW0zbQy 6X9ZqD8af2Ct9w4Ed4Qg93FSAkK9t8kLYidyk/FxQhbzQI4hlzOhH3poI0pFLGmNhpZY urqw== X-Gm-Message-State: AKaTC02l1z3wiTIOu/fpUmEIc2GMa0QocNvJ+PWM++EoWNRQDPM7VTHfAJowu3/HuxZyG+SN X-Received: by 10.194.171.200 with SMTP id aw8mr37958899wjc.79.1480694200349; Fri, 02 Dec 2016 07:56:40 -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 m145sm3859954wma.3.2016.12.02.07.56.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 Dec 2016 07:56:39 -0800 (PST) Date: Fri, 2 Dec 2016 15:56:37 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "Gao, Liming" , "afish@apple.com" , "Kinney, Michael D" , "mw@semihalf.com" , "Tian, Feng" Message-ID: <20161202155637.GB27069@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> <20161202144050.GV27069@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: 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 15:56:42 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Dec 02, 2016 at 03:07:39PM +0000, Ard Biesheuvel wrote: > > > On 2 Dec 2016, at 14:40, Leif Lindholm wrote: > > > > 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) > > That's a 32-bit operand. Shifting a 64-bit value will result in a > call to a compiler intrinsic, which is kind of the point of Riuyu's > remark No, it's not. As stated, that is the output from compiling: unsigned int shift(long long val) { return (val >> 33); } Unless you are claiming long long is 32-bit on ARM. Although I did mess up and make it signed :) When fixed, the code generation is identical but with an lsr instead. The compiler just operates directly on the top 32-bits (in r1), since the ones in r0 are irrelevant to the result. Change the shift to 31 and the compiler doesn't do anything cute, so the code is slightly more obvious: --- 00000000 : 0: e1a00fa0 lsr r0, r0, #31 4: e1800081 orr r0, r0, r1, lsl #1 8: e12fff1e bx lr --- But even if there were intrinsics generated, shouldn't we support the intrinsic instead of massaging the C code and hope we can prevent it from being generated. / Leif > > > > 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 > >>>>