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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Regards,
Ray

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: Wednesday, November 30, 2016 10:07 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>;
>afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; mw@semihalf.com; Tian, Feng <feng.tian@intel.com>
>Subject: Re: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices
>
>Hi Ray,
>
>Ard is on holiday this week, and mostly managing to stay off the
>email...
>
>Anyway, I could really use this support in order to bring the Marvell
>Armada 70x0 support (worked on by the Semihalf guys) into
>OpenPlatformPkg, so I will pick this up while he is away - at least
>parts 1-3.
>
>4/5 is required for 5/5, so I will not be looking to push the final
>one myself.
>
>Would you OK with pushing just 1-3 once we get this one resolved?
>
>On Mon, Nov 28, 2016 at 01:56:21AM +0000, Ni, Ruiyu wrote:
>> > diff --git
>> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > PciDeviceIo.c
>> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > PciDeviceIo.c
>> > new file mode 100644
>> > index 000000000000..fd59267a8d66
>> > --- /dev/null
>> > +++
>> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > Pc
>> > +++ iDeviceIo.c
>> > @@ -0,0 +1,797 @@
>...
>> > +VOID
>> > +InitializePciIoProtocol (
>> > +  NON_DISCOVERABLE_PCI_DEVICE     *Dev
>> > +  )
>> > +{
>> > +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *Desc;
>> > +  INTN                                Idx;
>> > +
>> > +  InitializeListHead (&Dev->UncachedAllocationList);
>
>The above line has crept in here, but belongs to 4/5 (causes
>compilation error). I will fix that and resend - is it OK if I resend
>this patch only?
>
>> > +  // Iterate over the resources to populate the virtual BARs
>> > +  //
>> > +  Idx = Dev->BarOffset;
>> > +  for (Desc = Dev->Device->Resources, Dev->BarCount = 0;
>> > +       Desc->Desc != ACPI_END_TAG_DESCRIPTOR;
>> > +       Desc = (VOID *)((UINT8 *)Desc + Desc->Len + 3)) {
>> > +
>> > +    ASSERT (Desc->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR);
>> > +    ASSERT (Desc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM);
>> > +
>> > +    if (Idx >= PCI_MAX_BARS ||
>> > +        (Idx == PCI_MAX_BARS - 1 && Desc->AddrSpaceGranularity == 64)) {
>> > +      DEBUG ((DEBUG_ERROR,
>> > +        "%a: resource count exceeds number of emulated BARs\n",
>> > +        __FUNCTION__));
>> > +      ASSERT (FALSE);
>> > +      break;
>> > +    }
>> > +
>> > +    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.
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?

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