From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124])
 by mx.groups.io with SMTP id smtpd.web09.8280.1610026319765240623
 for <devel@edk2.groups.io>;
 Thu, 07 Jan 2021 05:32:00 -0800
Authentication-Results: mx.groups.io;
 dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WRd9K85z;
 spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
	s=mimecast20190719; t=1610026319;
	h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
	 to:to:cc:mime-version:mime-version:content-type:content-type:
	 content-transfer-encoding:content-transfer-encoding:
	 in-reply-to:in-reply-to:references:references;
	bh=O4+K+CSiM2U3M/fs3xT9sqkXdiFul82kteV1FF+AfcY=;
	b=WRd9K85zuTv/iVd1SViO7J3QJBxkon1P3b86tCzvMeob5kMf6jS0iUmXxjR32jiYkLNEtZ
	R+7sHWfiuYwZmdtJGpkzjMUrOe2z+QB3td65NoVRiLpoY91xt6NF0QS1a6lHK3YC9xikgz
	XldaIB91csJbb4Qqglztor+6PI8czjA=
Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com
 [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id
 us-mta-546-_6vViRmXPU2z2xAWB4l7Sg-1; Thu, 07 Jan 2021 08:31:52 -0500
X-MC-Unique: _6vViRmXPU2z2xAWB4l7Sg-1
Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mimecast-mx01.redhat.com (Postfix) with ESMTPS id ACEA41005D44;
	Thu,  7 Jan 2021 13:31:51 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-119.ams2.redhat.com [10.36.114.119])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 1128A5D746;
	Thu,  7 Jan 2021 13:31:50 +0000 (UTC)
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
To: devel@edk2.groups.io, sumana.venur@intel.com
References: <20210107062959.25388-1-sumana.venur@intel.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <0b1f805e-6dad-219d-7989-43be64ffa376@redhat.com>
Date: Thu, 7 Jan 2021 14:31:50 +0100
MIME-Version: 1.0
In-Reply-To: <20210107062959.25388-1-sumana.venur@intel.com>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15
Authentication-Results: relay.mimecast.com;
	auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 01/07/21 07:29, Sumana Venur wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989
> 
> Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
> This needs to be wider, at least 32bit (even 64bit) reads to reduce the time for oprom shadow
> 
> Signed-off-by: Sumana Venur <sumana.venur@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index c994ed5fe3..fe6c7db417 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -521,7 +521,7 @@ LoadOpRomImage (
>      //
>      PciDevice->PciRootBridgeIo->Mem.Read (
>                                        PciDevice->PciRootBridgeIo,
> -                                      EfiPciWidthUint8,
> +                                      EfiPciWidthUint64,
>                                        RomBar,
>                                        (UINT32) RomImageSize,
>                                        Image
> 

This has three bugs:

(1) From the UEFI specification v2.8B,
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Mem.Read():

    The caller is responsible for satisfying any alignment and memory
    width restrictions that a PCI Root Bridge on a platform might
    require. For example on some platforms, width requests of
    EfiPciWidthUint64 do not work.

I don't think the patch ensures that EfiPciWidthUint64 is safe on the
particular platform.

(2) RomImageSize (the Count parameter) is expressed in bytes, currently.
If you scale up the access width, you have to scale down the access count.

(3) If you use an access size that is larger than 1 byte, you have to
handle the tail separately. For example, if the access width is
EfiPciWidthUint32, and the ROM image size is (4*k+3) bytes, then the
tail needs to be handled separately.

Laszlo