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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Dec 02, 2016 at 03:07:39PM +0000, Ard Biesheuvel wrote:
> 
> > 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

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 <shift>:
   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.<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
> >>>>