Re: [edk2-devel] [PATCH 2/5] UefiCpuPkg: Contiguous memory allocation and code clean-up.

["Yuanhao Xie" <yuanhao.xie@intel.com>]

Hi Gerd,

1) You convert size to pages, pages to size, size to pages again.
Agree. I will update it.

2) Also you don't want stack and code being on the same page
Patch 5 ensures that stack and code are in different pages and also ensure alignment. I will update it patch2 as well in v2.

3) The special case you have to handle is not running on a AMD Processor, but AmdSev being active (i.e. UseSevEsAPMethod == True).  Otherwise it should be just standard
Ia32 and X64, there should be no need to check whenever you are running on a AMD processor.

I understand your point, but for both cases (check AmdSev, standard Ia32 and X64), AMD related code will be changed. We would like to keep the original implementation as much as possible.

Best regards,
Yuanhao 


-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com> 
Sent: Wednesday, February 8, 2023 7:10 PM
To: devel@edk2.groups.io; Xie, Yuanhao <yuanhao.xie@intel.com>
Cc: Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Rhodes, Sean <sean@starlabs.systems>; Lu, James <james.lu@intel.com>; Guo, Gua <gua.guo@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/5] UefiCpuPkg: Contiguous memory allocation and code clean-up.

> +  AllocSize = EFI_PAGES_TO_SIZE (
> +                EFI_SIZE_TO_PAGES (
> +                  CpuMpData->CpuCount * AP_SAFE_STACK_SIZE + ApLoopFuncSize
> +                  )
> +                );
> +  Status = gBS->AllocatePages (
> +                  AllocateMaxAddress,
> +                  EfiReservedMemoryType,
> +                  EFI_SIZE_TO_PAGES (AllocSize),
> +                  &Address
> +                  );

Hmm?  You convert size to pages, pages to size, size to pages again.

Also you don't want stack and code being on the same page, so I guess the logic you actually need is this:

StackPages = EFI_SIZE_TO_PAGES(CpuMpData->CpuCount * AP_SAFE_STACK_SIZE); FuncPages  = EFI_SIZE_TO_PAGES(ApLoopFuncSize)
gBS->AllocatePages(..., StackPages + FuncPages, ...);

> +//
> +// Union holds the relocate APs loop entries for different cases // 
> +typedef union {
> +  VOID                          *Data;
> +  ASM_RELOCATE_AP_LOOP_AMD64    Amd64Entry;   // 64-bit AMD Processor
> +  ASM_RELOCATE_AP_LOOP          GenericEntry; // Intel Processor (32-bit or 64-bit), or 32-bit AMD Processor
> +} RELOCATE_AP_LOOP_ENTRY;

I'm sure I've mentioned this before.  The special case you have to handle is not running on a AMD Processor, but AmdSev being active (i.e. UseSevEsAPMethod == True).  Otherwise it should be just standard
Ia32 and X64, there should be no need to check whenever you are running on a AMD processor.

take care,
  Gerd