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.133.124]) by mx.groups.io with SMTP id smtpd.web10.3096.1676368629421823852 for ; Tue, 14 Feb 2023 01:57:09 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LqDGSmHo; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: kraxel@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676368628; 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=eopMUVToyxTtPW7SDrMX3bP/C/dxhG+sJp6HiRcy7/U=; b=LqDGSmHojUpO8aHuJNuUTI1yRCNnskTwiJ69IGmCFS6eCljp/3SFKuyL0r5cnQeXLSu1WA tEx2awgOjuffCq54T20F+YWtBlV+n9kKNSrXlCWju6mKgnB28AQDmP8+v/SE9RnBIQrSMe Gw8KUrWLUb5CAJjTq7ochPWgqlCxS+A= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-508-0Yes4-doPG2zz3AQQot40w-1; Tue, 14 Feb 2023 04:57:02 -0500 X-MC-Unique: 0Yes4-doPG2zz3AQQot40w-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9EA082817231; Tue, 14 Feb 2023 09:57:01 +0000 (UTC) Received: from sirius.home.kraxel.org (ovpn-195-41.brq.redhat.com [10.40.195.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 588B4492B15; Tue, 14 Feb 2023 09:57:01 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id 191101802381; Tue, 14 Feb 2023 10:56:59 +0100 (CET) Date: Tue, 14 Feb 2023 10:56:59 +0100 From: "Gerd Hoffmann" To: "Wu, Jiaxin" Cc: "devel@edk2.groups.io" , "Dong, Eric" , "Ni, Ray" , "Zeng, Star" , Laszlo Ersek , "Kumar, Rahul R" Subject: Re: [edk2-devel] [PATCH v6 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info Message-ID: <20230214095659.sxvmwxmitwbp7qbq@sirius.home.kraxel.org> References: <20230213084417.9232-1-jiaxin.wu@intel.com> <20230213084417.9232-5-jiaxin.wu@intel.com> <20230213131614.3vw4q233wtkqkpmv@sirius.home.kraxel.org> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi, > In PEI module, it also has such assumption, so we don't pass in the > HOB for the resolved smbase mem size, because we have avoided the > possibility of error in the reference pi smm cpu driver. So you essentially are hoping this will never ever change and hard-code the 8k in both PEI module and PiSmmCpuDxeSmm. I'd suggest to add a field to the HOB struct instead. If you want stick to the hardcoded 8k please add a note saying so to the HOB struct description. > > > + BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * > > (mMaxNumberOfCpus - 1)); > > > > I think correct is: > > BufferPages = EFI_SIZE_TO_PAGES(TileSize * mMaxNumberOfCpus); > > > > This is the existing code logic & it's correct, not wrong, I don't change it. To understand that, we need understand the algorithm of smbase: > > The SIZE_32KB covers the *several* SMI Entry and Save State of CPU 0, while TileSize * (mMaxNumberOfCpus - 1) to cover Save State of CPU 1+, not include the cpu0, so, it's the mMaxNumberOfCpus - 1. Ok, there is a longish comment in the source code explaining the tiling (starting at line 639). smram is 64k (16 pages), with pages 0-7 being unused, page 8 being the smi handler, 9-14 unused again, page 15 holding cpu state. Due to the smi handler having an even page index and the cpu state page having a odd page index you can use that tiling trick so you need only 8k not 32k per additional cpu. I agree, the existing code is correct. > > > + if ((FamilyId == 4) || (FamilyId == 5)) { > > > + Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB); > > > > Does that actually matter still? I'm pretty sure we can safely use > > "ASSERT(FamilyId > 5)" here. Pentium processors have been built in > > the last century, predating x64. > > This is the existing code logic. I don't change it. If you think we don't need it, please file Bugzilla for change. > > > Beside that the code is broken for SMP, only cpu0 will get a properly > > aligned smbase. Not sure penium processors support SMP in the first > > place though ... > > I don't understand why "only cpu0 will get a properly aligned smbase" When the cpu expects the smbase being 32k-aligned (as the comment in the code explains) the tiling trick just doesn't work. The whole buffer and the smbase for cpu0 are properly aligned to 32k, but the smbase for cpu1 is not. take care, Gerd