Re: [edk2-devel] [PATCH v2 5/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

["Laszlo Ersek" <lersek@redhat.com>]

On 2/21/24 11:24, Gerd Hoffmann wrote:
> On Wed, Feb 21, 2024 at 03:48:25AM +0000, Ni, Ray wrote:
>>>
>>> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
>>> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>>
>> Above formula assumes the maximum HOB length could be 0xFFFF.
> 
> Which is IMHO correct.
> 
>> But actually the maximum HOB length could be only 0xFFF8 because
>> PeiCore::PeiCreateHob() contains following logic:
>>
>>   if (0x10000 - Length <= 0x7) {
>>     return EFI_INVALID_PARAMETER;
>>   }
> 
> That Length is the *data* size, the HOB header is not included.
> 
> The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
> space needed for HOB header and GUID.


From the patch:

  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
  ...
  CpusInHob     = MIN (CpuMpData->CpuCount - HobBase, MaxCpusPerHob);
  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpusInHob;
  MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);

Assuming that the division is exact, and that the MIN selects MaxCpusPerHob for CpusInHob, we get:

  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

the multiplication and the division cancel out (again, assuming exact division):

  MpHandOffSize = sizeof (MP_HAND_OFF) + (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF));

the sizeof (MP_HAND_OFF) addition and subtraction cancel out:

  MpHandOffSize = MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE);

then we get:

  MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE));

Inside BuildGuidHob() [MdePkg/Library/PeiHobLib/HobLib.c], we get

  DataLength = MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE);

and further

  ASSERT (DataLength <= (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)));

Substituting for DataLength,

  ASSERT (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) <= (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)));

which is

  ASSERT (MAX_UINT16  <= 0xFFF8);

which fails.

So, as Ray says, and as I wrote under v1, you need to use 0xFFF8 here, not MAX_UINT16.

... Under v1, I wrote

  CpusPerHob = (0xFFE0 - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

If you want to make "sizeof (EFI_HOB_GUID_TYPE)" -- 24 bytes -- explicit in that subtraction, that is fine, but then we just get back to 0xFFF8:

  CpusPerHob = (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115766): https://edk2.groups.io/g/devel/message/115766
Mute This Topic: https://groups.io/mt/104472313/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-