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

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

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?

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
>