From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from alexa-out.qualcomm.com (alexa-out.qualcomm.com [129.46.98.28]) by mx.groups.io with SMTP id smtpd.web10.10098.1645708823576920748 for ; Thu, 24 Feb 2022 05:20:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@quicinc.com header.s=qcdkim header.b=F0/fCCnF; spf=pass (domain: quicinc.com, ip: 129.46.98.28, mailfrom: quic_tpilar@quicinc.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; i=@quicinc.com; q=dns/txt; s=qcdkim; t=1645708824; x=1677244824; h=message-id:date:mime-version:subject:from:to:cc:reply-to: references:in-reply-to:content-transfer-encoding; bh=sDz1rjD81sRGC8+e+KqBvCJGfq5KhXOioqubfxQTGlo=; b=F0/fCCnF5L8uWnewnrVUmRWYHN/RcHz6os7sRCkaaGc94f4BkygJmDcJ pZcfpzZMCFCDw9ZKPtfEQQxgGBEh7f0gAbuIsbDXxpPu5gR1p9gOKB9HZ sIT7awCwl9TtCDo90UnJJygF2QdeY1j6P83lLZhCaTl4u75yr4sckIrN8 k=; Received: from ironmsg08-lv.qualcomm.com ([10.47.202.152]) by alexa-out.qualcomm.com with ESMTP; 24 Feb 2022 05:20:23 -0800 X-QCInternal: smtphost Received: from nasanex01c.na.qualcomm.com ([10.47.97.222]) by ironmsg08-lv.qualcomm.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Feb 2022 05:20:23 -0800 Received: from nalasex01a.na.qualcomm.com (10.47.209.196) by nasanex01c.na.qualcomm.com (10.47.97.222) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.15; Thu, 24 Feb 2022 05:20:22 -0800 Received: from [10.111.134.175] (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.15; Thu, 24 Feb 2022 05:20:21 -0800 Message-ID: <5ecf2400-ae0d-d472-8bd6-2d7b5fa3dfaf@quicinc.com> Date: Thu, 24 Feb 2022 13:20:19 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe From: "Tomas Pilar (tpilar)" To: Ard Biesheuvel , edk2-devel-groups-io CC: Ray Ni , Ard Biesheuvel , Leif Lindholm Reply-To: , References: <20220224125753.2021633-1-quic_tpilar@quicinc.com> <1c5ee62c-251a-af20-e277-4534976f397b@quicinc.com> In-Reply-To: Return-Path: quic_tpilar@quicinc.com X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nalasex01a.na.qualcomm.com (10.47.209.196) Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit On 24/02/2022 13:19, Tomas Pilar (tpilar) wrote: > > > On 24/02/2022 13:14, Tomas Pilar (tpilar) wrote: >> >> >> On 24/02/2022 13:13, Ard Biesheuvel wrote: >>> On Thu, 24 Feb 2022 at 13:58, Tomas Pilar (tpilar) >>> wrote: >>>> Move the logic that sets EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE Pci >>>> >>>> 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. >>>> >>>> >>>> >>>> Cc: Ray Ni >>>> >>>> Cc: Ard Biesheuvel >>>> >>>> Cc: Leif Lindholm >>>> >>>> Signed-off-by: Tomas Pilar >>> Ehm, nope, that is not exactly what I meant. >>> >>> The existing code stores the original PCI attributes in the controller >>> private data, enables MMIO/IO decoding and bus mastering, and only >>> then sets the dual address cycle attribute. >>> >>> All of that needs to move, so that the captured attributes are >>> accurate. >>> >>> >> Okay, I was wondering. My thought was that we probably want to >> re-enable bus mastering >> on reset so I kept that bit of code in the original location. >> > > Also, doesn't this code: > >   if (!EFI_ERROR (Status)) { >     Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; >     Status    = PciIo->Attributes ( >                          PciIo, >                          EfiPciIoAttributeOperationEnable, >                          Supports, >                          NULL >                          ); >   } > > *strip* the PCI_DEVICE_ENABLE set of attributes rather than add them? > I am somewhat confused about this. > Wait no. I just cannot read code, ignore that last bit. I'll prepare a patch.