From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-x231.google.com (mail-wj0-x231.google.com [IPv6:2a00:1450:400c:c01::231]) (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 9C64081F51 for ; Fri, 2 Dec 2016 07:09:12 -0800 (PST) Received: by mail-wj0-x231.google.com with SMTP id v7so234271151wjy.2 for ; Fri, 02 Dec 2016 07:09:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=P7UirjZ9ujDy7JCpB1xxQXx31mHxr37d6nqdBO2dPvM=; b=kVEI9RPv/MjxRW2xf/7V8YQKNulcwbvlK0PTKdXcosrCYy59BQtTKdKyfVTrN0g0qm UFeCo+miu2tdE9+oc2HbYnSFAt//L/1XjyskMhcmpxffT4iVmSl7IeOD8z58pt6ROEre OI+S/hG/FPKpU7JMBMLFC5Ofxla0rNJv3cn7M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=P7UirjZ9ujDy7JCpB1xxQXx31mHxr37d6nqdBO2dPvM=; b=RfHeg6v+UNSG3J29DB34zBIZozKuiCw1bK4E8Wzwls+DHm0FFB2pE5sHKF+E7PEu9e Y7mGjnxk/QGS92/VcL+OCr4/qBEpHtbCtX0KCQXpFdxhyLyTtluENV5Pohlzy4FXh4pI vagPwR5OseKsIa7DCP1brQvIwBwsgtYn7So/s+hDMRJ1xpidL1AXFSYrhxUk/kHb1MPS +v1wvVYdAU0CPPVhaypHJtUQkBS+TmqM5YFi1i2QYSR+faV3iA61DlwbVP2Z728cDBFb WqXVOpPlcsUHvCwVogO5R1/AkNl/Z1yZmY2LlgI+p/wqF07fH5GyYQqQLnwPQNRf0G4w bSjg== X-Gm-Message-State: AKaTC02muGatM955DtoApI8afdzgcx+rdwOZVhokJWrgzGWjDfiQC8PxHnPqNk7X11wBztNl X-Received: by 10.194.203.5 with SMTP id km5mr44248696wjc.230.1480691350856; Fri, 02 Dec 2016 07:09:10 -0800 (PST) Received: from [196.72.181.3] ([196.72.181.3]) by smtp.gmail.com with ESMTPSA id c133sm3662445wme.12.2016.12.02.07.09.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 Dec 2016 07:09:10 -0800 (PST) Mime-Version: 1.0 (1.0) From: Ard Biesheuvel X-Mailer: iPhone Mail (14B100) In-Reply-To: <20161202144050.GV27069@bivouac.eciton.net> Date: Fri, 2 Dec 2016 15:07:39 +0000 Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "Gao, Liming" , "afish@apple.com" , "Kinney, Michael D" , "mw@semihalf.com" , "Tian, Feng" Message-Id: 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> To: Leif Lindholm 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:09:13 -0000 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable > On 2 Dec 2016, at 14:40, Leif Lindholm wrote: >=20 > On Fri, Dec 02, 2016 at 12:54:45PM +0000, Ni, Ruiyu wrote: >>>>> + 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); >>>>=20 >>>> 1. You need to use RShiftU64 otherwise the above code will break the >>>> build process in 32bit. >>>=20 >>> I don't understand how that could cause breakage (and experiments with >>> IA32 and ARM fail to reproduce it with GCC). >>>=20 >>> - 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]). >>>=20 >>> Is the problem related to the shift value being signed? >>> Or does some other compiler refuse to cast a UINT64 to a UINT32? >>=20 >> Desc->AddrRangeMin is UINT64. My experience tells me that >> LShiftU64 or RShiftU64 should be used for shifting operation >> on UINT64. >=20 > 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. >=20 >> 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? >=20 > There is no cleverness required, it's just a defined operation on a > base type. But sure, for example: >=20 > unsigned int shift(long long val) > { > return (val >> 33); > } >=20 > 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) >=20 > The only thing that changes if the shift value is modified is that the > 0x21 is changed accordingly. >=20 > Change it to an inline constant, not much changes: >=20 > unsigned int shiftconst(void) > { > unsigned long long var =3D 0xaaaabbbbccccddddULL; > return (var >> 31); > } >=20 > 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) >=20 > Both also compile correctly for IA32 with -m32. >=20 > 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. >=20 > 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=3Darmv4t onwards) That's a 32-bit operand. Shifting a 64-bit value will result in a call to a c= ompiler intrinsic, which is kind of the point of Riuyu's remark > 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. >=20 > But I don't want to replace functional C code with abstraction > functions without a proper understanding of where it would be > problematic. >=20 > Regards, >=20 > Leif >=20 >>=20 >>>=20 >>> Regards, >>>=20 >>> Leif >>>=20 >>>>> + } >>>>> + } >>>>> +} >>>>> 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 >>>>>=20 >>>>> MdeModulePkg/Core/Dxe/DxeMain.inf { >>>>> >>>>> -- >>>>> 2.7.4 >>>>=20