From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.10778.1645712400736264221 for ; Thu, 24 Feb 2022 06:20:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=qxy+3Im5; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id A46EEB82605 for ; Thu, 24 Feb 2022 14:19:58 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4B42CC340EC for ; Thu, 24 Feb 2022 14:19:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1645712397; bh=IHOdPnbY4A5KBZ05nIchxq3AtPohToyrSVEjOLex+5Y=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=qxy+3Im5PWS8uR/iERzHKRpIX6yRKhFwSKJPW9JT21ss18rqJTmfduuizPEZGmVRG qRiaNMe2L7d/iO7z8DcZ5GFKelln0Z+uLWbx+2R22O6AO5QHfN35or17i4YQIMVKSA dqL8jX8ayDxNn1GLCbp6RYz7ZsxMs2yLmVq2LyV3IB5ZJd86FKJ+fZ6584I9/Yq+nO aXMaAY1FG9aRS1X3PMVVB5FFya9lfVVtbrHKynpLPhpMFlMeeExp0juiD12Af8D87l RB5VNtG06vsG9n6NflF7OKCxsV2PZ/3E+gP3mQBZ3LFUvDn/fSS/aby7UlbAMQz2AC bHIjrkNbgs4FA== Received: by mail-yb1-f175.google.com with SMTP id d21so4060212yba.11 for ; Thu, 24 Feb 2022 06:19:57 -0800 (PST) X-Gm-Message-State: AOAM533tzfR5FdAeRaYZAZi43avNLwSy2vAAOgjIUG8Gk3dsHAAEpjsB 5dnROS3lpBYJ8L/bTRX4xREpc7t2CjqYjmJMfeI= X-Google-Smtp-Source: ABdhPJxkLhnlaYkEF1TE+wVpBRN8FHeYygmspsU7HArQkAA6pW76fG7RMVt0IBv/IDCyDuKLdE7Tkz5WutSkI91/ZpI= X-Received: by 2002:a25:24ce:0:b0:61e:1276:bfcf with SMTP id k197-20020a2524ce000000b0061e1276bfcfmr2538824ybk.299.1645712396352; Thu, 24 Feb 2022 06:19:56 -0800 (PST) MIME-Version: 1.0 References: <20220224132929.2052626-1-quic_tpilar@quicinc.com> In-Reply-To: <20220224132929.2052626-1-quic_tpilar@quicinc.com> From: "Ard Biesheuvel" Date: Thu, 24 Feb 2022 15:19:44 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v3] MdeModulePkg: Correct high-memory use in NvmExpressDxe To: edk2-devel-groups-io , Tomas Pilar , Hao A Wu Cc: Ray Ni , Ard Biesheuvel , Leif Lindholm Content-Type: text/plain; charset="UTF-8" (+ Hao Wu) On Thu, 24 Feb 2022 at 14:29, Tomas Pilar (tpilar) wrote: > > Move the logic that stores starting PCI attributes and sets the > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute to > DriverBindingStart() before the memory that backs the > DMA engine is allocated. > > This ensures that the DMA-backing memory is not forcibly allocated > below 4G in system address map. Otherwise the allocation fails on > platforms that do not have any memory below the 4G mark and the drive > initialisation fails. > > Leave the PCI device enabling attribute logic in NvmeControllerInit() > to ensure that the device is re-enabled on reset in case it was > disabled via PCI attributes. > > Cc: Ray Ni > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Signed-off-by: Tomas Pilar This is still not exactly what I had in mind, but I think it's actually better: the manipulation of the MMIO/IO decode and bus master attributes occurs in the same place as before (and therefore potentially multiple times), whereas recording the original value of the attributes now occurs only once. In that sense, this patch fixes more than one bug. Reviewed-by: Ard Biesheuvel > --- > .../Bus/Pci/NvmExpressDxe/NvmExpress.c | 27 +++++++++++++++++++ > .../Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 26 +----------------- > 2 files changed, 28 insertions(+), 25 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c > index 9d40f67e8e..b70499e3be 100644 > --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c > +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c > @@ -959,6 +959,33 @@ NvmExpressDriverBindingStart ( > goto Exit; > } > > + // > + // Save original PCI attributes > + // > + Status = PciIo->Attributes ( > + PciIo, > + EfiPciIoAttributeOperationGet, > + 0, > + &Private->PciAttributes > + ); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Enable 64-bit DMA support in the PCI layer. > + // > + Status = PciIo->Attributes ( > + PciIo, > + EfiPciIoAttributeOperationEnable, > + EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "NvmeControllerInit: failed to enable 64-bit DMA (%r)\n", Status)); > + } > + > // > // 6 x 4kB aligned buffers will be carved out of this buffer. > // 1st 4kB boundary is the start of the admin submission queue. > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c > index ac77afe113..d87212ffb2 100644 > --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c > +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c > @@ -728,20 +728,9 @@ NvmeControllerInit ( > UINT8 Mn[41]; > > // > - // Save original PCI attributes and enable this controller. > + // Enable this controller. > // > PciIo = Private->PciIo; > - Status = PciIo->Attributes ( > - PciIo, > - EfiPciIoAttributeOperationGet, > - 0, > - &Private->PciAttributes > - ); > - > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > Status = PciIo->Attributes ( > PciIo, > EfiPciIoAttributeOperationSupported, > @@ -764,19 +753,6 @@ NvmeControllerInit ( > return Status; > } > > - // > - // Enable 64-bit DMA support in the PCI layer. > - // > - Status = PciIo->Attributes ( > - PciIo, > - EfiPciIoAttributeOperationEnable, > - EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, > - NULL > - ); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_WARN, "NvmeControllerInit: failed to enable 64-bit DMA (%r)\n", Status)); > - } > - > // > // Read the Controller Capabilities register and verify that the NVM command set is supported > // > -- > 2.30.2 > > > > >