From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::141; helo=mail-it1-x141.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x141.google.com (mail-it1-x141.google.com [IPv6:2607:f8b0:4864:20::141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2968521962301 for ; Wed, 10 Oct 2018 01:16:14 -0700 (PDT) Received: by mail-it1-x141.google.com with SMTP id p64-v6so6765873itp.0 for ; Wed, 10 Oct 2018 01:16:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=3KNjFKE6f2pxvJcTtyeJ7H/jOxdLvysjsb/PdhxSeKU=; b=cXFeR7nddMN+O1YFiVm0PARfEwCFsqZ5eQshg9fyXP8gpfKJCc+yrpMpwizGbhYkYW n1uHVWA9TB88Qe37P7Uv9f7j4FQ3KutcFkIeP02kbS9fC+e1i77uHTyXWF+vTELyLniX z/rmGPzmwwZNcntrHPf7MOi6DbFNUF/6TrZlU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=3KNjFKE6f2pxvJcTtyeJ7H/jOxdLvysjsb/PdhxSeKU=; b=b6VMJ8Yb6h/RpM7pKNIaCDBliqQz9S5ocXRz2RrZnc9P0pyYy4u6Cft5xYBuDimPU1 A4zvI9iODpTu4VBy5e6ySNrScWpPzwbW6JfVfbZZ56DrRXKXaTCqid05EpQj8v4thdbF ozpu9K0PX+mVAIHVrLQWjLMMif9cGhI/62JQ66Xh2t8zmLT5cW4G7Jl7oG+yyZnBmI4j ifGSwG0rV6d8HejGyhz86h/JuzkJcf3D+WLhIKiMApC5RfAHjbt5RXz4xH9fqmOrn8cP neGyNvJ6dZGRmb0NrGnMTM3aPP7ySBgusacWqI5g4V9UUrptrn9MQPyW62oGEbAdj41F moNw== X-Gm-Message-State: ABuFfoioFkoFpEEBxPHzJGTDXqyvG1xjhOH4RDNnWwuQ/OJvXOdAXtD8 5ljmijLM20LhoSJpFTfVd7KXDOfWUjEq/gzX0HgjbQ== X-Google-Smtp-Source: ACcGV63lHFPiBCIh4vvunhKvG9kxfcN+sPnu/T3HmJvRx/kYfPEw85Bh2V9BJ5I4c5xx02IZrNj3FyR2hXeUyhLjCzw= X-Received: by 2002:a24:4ac5:: with SMTP id k188-v6mr4875824itb.158.1539159373744; Wed, 10 Oct 2018 01:16:13 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:5910:0:0:0:0:0 with HTTP; Wed, 10 Oct 2018 01:16:13 -0700 (PDT) In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BC16DE7@shsmsx102.ccr.corp.intel.com> References: <20181009140128.21378-1-ard.biesheuvel@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103BC16DE7@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Wed, 10 Oct 2018 10:16:13 +0200 Message-ID: To: "Zeng, Star" Cc: "edk2-devel@lists.01.org" , "Ni, Ruiyu" Subject: Re: [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: expose unique B/D/F identifiers X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Oct 2018 08:16:15 -0000 Content-Type: text/plain; charset="UTF-8" Hi Star, On 10 October 2018 at 10:08, Zeng, Star wrote: > Two minor comments. > 1. Is it better to initialize mUniqueIdCounter to 0? The C standard guarantees that it is initialized to 0 so this is unnecessary > 2. With this patch, the Bus value returned may be > 0xFF, right? Should the code restrict the Bus value to be <= 0xFF? > That will happen if we register more than 32 * 256 == 8192 non-discoverable PCI devices. That is unlikely ever to occur, but it makes sense to add an ASSERT for it and return an error, I suppose. I will change that. > > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel > Sent: Tuesday, October 9, 2018 10:01 PM > To: edk2-devel@lists.01.org > Cc: Ni, Ruiyu ; Zeng, Star > Subject: [edk2] [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: expose unique B/D/F identifiers > > Currently, the implementation of EFI_PCI_IO_PROTOCOL::GetLocation() > in NonDiscoverablePciDeviceDxe returns the same set of dummy values > for each instance of the NON_DISCOVERABLE_DEVICE protocol that it > attaches itself to. However, this turns out to be causing problems > in cases where software (such as the ARM Compliance Test Suite [ACS]) > attempts to use these values to uniquely identify controllers, since > the collisions create ambiguity in this regard. > > So let's modify GetLocation() to return an arbitrary bus/device tuple > on segment 0xff instead. This is guaranteed not to clash with other > non-discoverable PCI devices, and highly unlikely to clash with real > PCIe devices. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 4 ++++ > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 10 +++++++--- > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h | 6 ++++++ > 3 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > index 3e9ff6620d8d..ed5256f3c7ea 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > @@ -18,6 +18,8 @@ > > EFI_CPU_ARCH_PROTOCOL *mCpu; > > +STATIC UINTN mUniqueIdCounter; > + > // > // We only support the following device types > // > @@ -167,6 +169,8 @@ NonDiscoverablePciDeviceStart ( > goto CloseProtocol; > } > > + Dev->UniqueId = mUniqueIdCounter++; > + > return EFI_SUCCESS; > > CloseProtocol: > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c > index 0e42ae4bf6ec..58cb5d8b1fc5 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c > @@ -1181,6 +1181,8 @@ PciIoGetLocation ( > OUT UINTN *FunctionNumber > ) > { > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + > if (SegmentNumber == NULL || > BusNumber == NULL || > DeviceNumber == NULL || > @@ -1188,9 +1190,11 @@ PciIoGetLocation ( > return EFI_INVALID_PARAMETER; > } > > - *SegmentNumber = 0; > - *BusNumber = 0xff; > - *DeviceNumber = 0; > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + *SegmentNumber = 0xff; > + *BusNumber = Dev->UniqueId >> 5; > + *DeviceNumber = Dev->UniqueId & 0x1f; > *FunctionNumber = 0; > > return EFI_SUCCESS; > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h > index e641189267ee..5b4c57fa2a8e 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h > @@ -100,6 +100,12 @@ typedef struct { > // on behalf of this device > // > LIST_ENTRY UncachedAllocationList; > + // > + // Unique ID for this device instance: needed so that we can report unique > + // segment/bus/device number for each device instance. Note that this number > + // may change when disconnecting/reconnecting the driver. > + // > + UINTN UniqueId; > } NON_DISCOVERABLE_PCI_DEVICE; > > /** > -- > 2.17.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel