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 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).

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
> >>