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::d42; helo=mail-io1-xd42.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) (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 97E8821A09130 for ; Wed, 10 Oct 2018 02:21:09 -0700 (PDT) Received: by mail-io1-xd42.google.com with SMTP id n18-v6so3337591ioa.9 for ; Wed, 10 Oct 2018 02:21:09 -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=ZzF38t3UBzfiUvltRQ6IHprqOrCllciwI2k+LMt59l8=; b=DbCsKHfyJQwEDZexACnO6hyP7tjyYCqFOlkWFnmmlBdXPM+5RL4ouL4t7BjWzKVuFk QmskNqSwVPnQ+qV8LSHsfveaMEpGTHd5lFzXs2OaBq7KNcjn0H3mgvmd5molyO/olV7C oWmSmRMtoIE5zAMhS6W9EhSDClTWPVKdBtQc8= 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=ZzF38t3UBzfiUvltRQ6IHprqOrCllciwI2k+LMt59l8=; b=FwEjHseudsEj0qxDu2TfdgEwbWdesf9amXJg7xrgd3mne7fbQPnKZOM4dy3LvbjZr6 cQq8OT46oz7Yn4gZ+qcqLkHKxaUmilrGPKnxi8TnxI1r9J3U+xobhLlGP8+h+g/b7Fpe bhqbBo3a10zBLkIOOyRVmlewJMBM9e37+D4cNG/4e7+nCTgu8ypyOPM4u3R5/f4HJGiy SFcEXSvhe3W8Nmyez9AVZpQtsA8W6I7AW16P8LYdbcfam/MdS9tx/CYxvJv2D+KtrZEm 5Bz3SAZPn5S+7K0bSiagc95acM5gAd7TGJlYbMK+Ps/OG9ltwDIgjLP1Fqz/RymEU1rM BHiQ== X-Gm-Message-State: ABuFfogOwMAWGSiCTVhk2t8t2KBo/1t3asU6ZfRsCdiYtaTblSRdzFUP qgzmIrRCJ1ZJIVG4xZqjhkXKptGXTIWXuc8qKMbKyg== X-Google-Smtp-Source: ACcGV60xAjW2YsVz3ySKwrEoge8GsllwDLS6Nsr7PytJUDu74zJ9pnVkPUA95Vsg6EYEhS3a6RqX2+oarjlhWCUG+Y8= X-Received: by 2002:a6b:3787:: with SMTP id e129-v6mr22312763ioa.60.1539163267992; Wed, 10 Oct 2018 02:21:07 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:5910:0:0:0:0:0 with HTTP; Wed, 10 Oct 2018 02:21:07 -0700 (PDT) In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BC17EAF@shsmsx102.ccr.corp.intel.com> References: <20181010083450.22298-1-ard.biesheuvel@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103BC17EAF@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Wed, 10 Oct 2018 11:21:07 +0200 Message-ID: To: "Zeng, Star" Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH v2] 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 09:21:09 -0000 Content-Type: text/plain; charset="UTF-8" On 10 October 2018 at 10:55, Zeng, Star wrote: > Reviewed-by: Star Zeng > Thanks. Pushed as 2730470f9d3b..1a3bee20820c > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel > Sent: Wednesday, October 10, 2018 4:35 PM > To: edk2-devel@lists.01.org > Cc: Zeng, Star > Subject: [edk2] [PATCH v2] 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 > Tested-by: Marcin Wojtas > --- > v2: add sanity check on id value so we don't exceed 256 buses > initialize counter to 0 explicitly - this isn't strictly necessary but clarifies > the code > > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 10 ++++++++++ > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 10 +++++++--- > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h | 6 ++++++ > 3 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > index 3e9ff6620d8d..2483c7296912 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > @@ -16,6 +16,9 @@ > > #include > > +#define MAX_NON_DISCOVERABLE_PCI_DEVICE_ID (32 * 256) > + > +STATIC UINTN mUniqueIdCounter = 0; > EFI_CPU_ARCH_PROTOCOL *mCpu; > > // > @@ -141,6 +144,11 @@ NonDiscoverablePciDeviceStart ( > NON_DISCOVERABLE_PCI_DEVICE *Dev; > EFI_STATUS Status; > > + ASSERT (mUniqueIdCounter < MAX_NON_DISCOVERABLE_PCI_DEVICE_ID); > + if (mUniqueIdCounter >= MAX_NON_DISCOVERABLE_PCI_DEVICE_ID) { > + return EFI_OUT_OF_RESOURCES; > + } > + > Dev = AllocateZeroPool (sizeof *Dev); > if (Dev == NULL) { > return EFI_OUT_OF_RESOURCES; > @@ -167,6 +175,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.19.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel