From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web12.7782.1646140188839995482 for ; Tue, 01 Mar 2022 05:09:49 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=We9H6FBJ; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: kraxel@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1646140188; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=MEDq3e6LhmTq2Pk7UXTQp+u/8ZZeK7bhRJBuR9/3yAI=; b=We9H6FBJzkc+3yHJAHjTIEazz7bqFc/Zta+yUowMz7iEOXBE+OfmznVrSWvEfM0R3d3khL Tils5FWFTLpkuL3FVRZ9f3b0e45Ya5U+HcAJyYdNOEhcdrnuWbALwutd//a11kgY+fmZhQ B2IeYwnOsiS/JoeHCE8/FBK6xV6ZrZE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-319-xhx4hmCGMMyM7OMg3ghGFg-1; Tue, 01 Mar 2022 08:09:44 -0500 X-MC-Unique: xhx4hmCGMMyM7OMg3ghGFg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2C6E71006AA6; Tue, 1 Mar 2022 13:09:43 +0000 (UTC) Received: from sirius.home.kraxel.org (unknown [10.39.195.81]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C61B684961; Tue, 1 Mar 2022 13:09:42 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id 357CE180091B; Tue, 1 Mar 2022 14:09:41 +0100 (CET) Date: Tue, 1 Mar 2022 14:09:41 +0100 From: "Gerd Hoffmann" To: Min Xu Cc: devel@edk2.groups.io, Ard Biesheuvel , Jordan Justen , Brijesh Singh , Erdem Aktas , James Bottomley , Jiewen Yao , Tom Lendacky Subject: Re: [PATCH V7 19/37] OvmfPkg/PlatformInitLib: Add memory functions Message-ID: <20220301130941.dw6cdc5nvh7c63u2@sirius.home.kraxel.org> References: <46e0f662050779ec3ce3caf17196403266e73269.1646031164.git.min.m.xu@intel.com> MIME-Version: 1.0 In-Reply-To: <46e0f662050779ec3ce3caf17196403266e73269.1646031164.git.min.m.xu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=kraxel@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Feb 28, 2022 at 03:20:51PM +0800, Min Xu wrote: > Below functions are introduced in PlatformInitLib: > - PlatformGetFirstNonAddress > - PlatformAddressWidthInitialization > - PlatformGetSystemMemorySizeBelow4gb > - PlatformQemuUc32BaseInitialization > - PlatformInitializeRamRegions > > They correspond to the below functions in OvmfPkg/PlatformPei: > - GetFirstNonAddress > - AddressWidthInitialization > - GetSystemMemorySizeBelow4gb > - QemuUc32BaseInitialization > - InitializeRamRegions > > After that OvmfPkg/PlatformPei is refactored with this library. > > Note: PlatformInitLib will not determine whether SMM or S3 is supported > or not. Instead the caller of these functions should input SMM / S3 > support as the IN parameter by themselves. This is to reduce the > complexity of PlatformInitLib. Another reason is that some PCDs cannot > be declared as FixedAtBuild while PlatformInitLib is designed to be used > in both SEC and PEI phase. Hmm. Unlike patches 17+18 which are pure code motion (except the function renaming but that doesn't change the workflow) this patch mixes code changes and code moving which makes it hard to review. It should be splitted into one (or more) patches changing the functions as needed (and keeping the code in PlatformPei), and one patch moving things over to PlatformInitLib without functional changes. > +PlatformGetFirstNonAddress ( > + OUT UINT64 *Pci64Base, > + OUT UINT64 *Pci64Size, > + IN UINT64 DefaultPciMmio64Size > + ) > +VOID > +QemuInitializeRam ( > + UINT32 Uc32Base, > + UINT16 HostBridgeDevId, > + EFI_BOOT_MODE BootMode, > + BOOLEAN SmmSmramRequire, > + UINT32 LowerMemorySize, > + UINT16 Q35TsegMbytes > + ) > +VOID > +EFIAPI > +PlatformInitializeRamRegions ( > + IN UINT32 Uc32Base, > + IN UINT16 HostBridgeDevId, > + IN BOOLEAN SmmSmramRequire, > + IN EFI_BOOT_MODE BootMode, > + IN BOOLEAN S3Supported, > + IN UINT32 LowerMemorySize, > + IN UINT16 Q35TsegMbytes > + ) I think we should add all those parameters to the PLATFORM_INFO struct, then simply pass around a pointer to that struct instead of having tons of parameters for each function. Due to this patch needing an update anyway it might be easier to do it right away instead of an incremental cleanup after merging this series. > @@ -85,7 +87,7 @@ MemMapInitialization ( > return; > } > > - TopOfLowRam = GetSystemMemorySizeBelow4gb (); > + TopOfLowRam = PlatformGetSystemMemorySizeBelow4gb (); > PciExBarBase = 0; > if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) { > // > @@ -736,6 +738,11 @@ InitializePlatform ( > Q35SmramAtDefaultSmbaseInitialization (); > } > > + // > + // Fetch the lower memory size (Below 4G) > + // > + mLowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (); Can't you just use TopOfLowRam here? take care, Gerd