Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, January 20, 2023 4:21 PM
> To: Ni, Ray <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Zimmer, Vincent
> <vincent.zimmer@intel.com>
> Subject: Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base
> HOB Data
> 
> On 1/18/23 16:06, Ni, Ray wrote:
> 
> > #pragma pack(1)
> > typedef struct {
> >   UINT32    CpuIndex;
> >   UINT32    NumberOfCpus;  // align to
> EFI_SEC_PLATFORM_INFORMATION_RECORD2.NumberOfCpus
> >   UINT64    SmBase[];
> > } SMM_BASE_HOB_DATA;
> > #pragma pack()
> >
> > For system with less than 8K CPUs, one HOB is produced. CpuIndex is set to
> 0 indicating
> > the HOB describes the CPU from 0 to NumberOfCpus-1.
> >
> > The HOB list may contains multiple such HOB instances each describing the
> information for
> > CPU from CpuIndex to CpuIndex + NumberOfCpus - 1.
> > The instance order in the HOB list is random so consumer cannot assume
> the CpuIndex
> > of first instance is 0.
> 
> When using discontiguous blocks that are limited to ~64KB each:
> 
> - The consumer won't be able to access elements of the "conceptual" big
> array in a truly random (= random-access) fashion. There won't be a
> single contiguous domain of valid subscripts. It's "bank switching", and
> switching banks should be avoided IMO. It effectively replaces the
> vector data structure with a linked list. The consequence is that the
> consumer will have to either (a) build a new (temporary, or permanent)
> index table of sorts, for implementing the "conceptual" big array as a
> factual contiguous array, or (b) traverse the HOB list multiple times.

I agree. My recommendation is the consumer queries the total cpu count and
allocate a big-enough temp buffer, then it traverse the HOB and copies
each instance of the SMM information HOB to the proper location in the buffer.
After the SMM rebase is done, the temp buffer can be freed to reduce
SMM memory consumption.

Multi-traverse of the HOB can avoid the temp buffer allocation. But the code
logic will be a bit more complicated. I don't prefer.


> 
> - If the element size of the array increases (which is otherwise
> possible to do compatibly, e.g. by placing a GUID and/or revision# in
> the HOB), the number of elements that fit in a single HOB decreases. I
> think that's an artifact that needlessly complicates debugging, and
> maybe performance too (it might increase the number of full-list
> traversals).

TRUE that multi-instance of the HOBs make the debugging harder.
That's why I propose to have "CpuIndex" field to tell which range CPU the
specific HOB describes.

> 
> - With relatively many elements fitting into a single HOB, on most
> platforms, just one HOB is going to be used. While that may be good for
> performance, it is not good for code coverage (testing). The quirky
> indexing method will not be exercised by most platforms.

TRUE so I propose that the first version of the code change only expects
the HOB.NumberOfCpus equals to the NumberOfCpus returned from MP
service, meaning the code logic only supports single instance of the HOB.
When a platform that contains >8000 cpu threads resulting in multiple HOBs
produced, the expectation will break and remind us that the CpuSmm driver
needs to update to handle multiple HOBs.

> 
> What's wrong with:
> 
> - restricting the creation of all such HOBs after
> "gEfiPeiMemoryDiscoveredPpiGuid"
> 
> - using a page allocation, for representing the array contiguously
> 
> - in the HOB, only storing the address of the page allocation.
> 
> Page allocations performed after gEfiPeiMemoryDiscoveredPpiGuid will not
> be moved, so the address in the HOB would be stable.

It can work. I agree.
However, please think about future standalone MM case. The StandaloneMmIpl
builds HOB list and pass to StandaloneMmCore. The HOB list shall contain the
SMM base information HOB. If we only store a pointer in the HOB, the pointer will
points to a SMRAM carved out from SMRAM by StandaloneMmIpl. Memory service
in StandaloneMmCore cannot be used because StandaloneMmCore hasn't been loaded.
I don't prefer to introduce extra memory management logic in StandaloneMmIpl though
the logic is very simple to carve out some pages from SMRAM.

Thanks,
Ray