Re: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 2 Dec 2016, at 14:40, Leif Lindholm <leif.lindholm@linaro.org> 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 <shift>:
>   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 <shiftconst>:
>   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 <shift>:
>   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


> 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.<BR>
>>>>> +
>>>>> +  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 <Library/BaseMemoryLib.h>
>>>>> +#include <Library/DebugLib.h>
>>>>> +#include <Library/MemoryAllocationLib.h> #include
>>>>> +<Library/UefiBootServicesTableLib.h>
>>>>> +#include <Library/UefiLib.h>
>>>>> +
>>>>> +#include <IndustryStandard/Pci.h>
>>>>> +
>>>>> +#include <Protocol/ComponentName.h>
>>>>> +#include <Protocol/NonDiscoverableDevice.h>
>>>>> +#include <Protocol/PciIo.h>
>>>>> +
>>>>> +#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 {
>>>>>     <LibraryClasses>
>>>>> --
>>>>> 2.7.4
>>>>