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

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

Ray, any comments on the below?

Regards,

Leif

On Wed, Nov 30, 2016 at 02:06:55PM +0000, Leif Lindholm wrote:
> 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
> >