Re: [edk2-devel] [PATCH] OvmfPkg/XenPlatformPei: Relocate shared_info page mapping

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

On 06/29/21 12:35, Andrew Cooper wrote:
> On 29/06/2021 11:07, Jan Beulich wrote:
>> On 29.06.2021 11:20, Laszlo Ersek wrote:
>>> On 06/28/21 15:23, Anthony PERARD via groups.io wrote:
>>>> From: Anthony PERARD <anthony.perard@citrix.com>
>>>>
>>>> Unfortunately, Xen isn't ready to deal with mapping at the top of the
>>>> physical address space, so we relocate the mapping after the LAPIC
>>>> location.
>>>>
>>>> See this thread about the issue with the mapping:
>>>> - https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/
>>>>
>>>> The PhysicalAddressIdentityMapping() call isn't going to do anything
>>>> anymore since everything under 4GB is already mapped, but there is no
>>>> need to remove the call.
>>>>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>> ---
>>>>  OvmfPkg/XenPlatformPei/Xen.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
>>>> index a4e82b356936..9c6641895970 100644
>>>> --- a/OvmfPkg/XenPlatformPei/Xen.c
>>>> +++ b/OvmfPkg/XenPlatformPei/Xen.c
>>>> @@ -569,7 +569,7 @@ CalibrateLapicTimer (
>>>>    EFI_STATUS            Status;
>>>>  
>>>>  
>>>> -  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
>>>> +  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
>>>>    Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
>>>>    if (EFI_ERROR (Status)) {
>>>>      DEBUG ((DEBUG_ERROR,
>>>>
>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> I guess I should merge this after Jan and/or Andrew ack it.
>> Well, I can informally ack that a move like this is needed, but I can't
>> really give an Acked-by on the change, as I know next to nothing about
>> ovmf, and hence I cannot, for example, tell whether the chosen new
>> location is okay to use there.
> 
> That bit is easy, although it probably does warrant a code comment.  The
> choice of location doesn't matter specifically (so long as it's not too
> high for 32bit toolstacks to cope with), but wants to be something which
> isn't RAM, and ideally doesn't shatter a superpage.
> 
> The various MMIO ranges (IO-APIC, TPM, LAPIC) are 2M aligned, so LAPIC +
> 1M is firmly in the middle of a range not used by anything else, and
> with APICV/AVIC, the LAPIC does have a 4k mapping to the sink page.

I'm OK with informal ACKs of course, just please let me know when you
guys feel it's OK for me to merge the patch. I saw the CCs and so I
didn't want to hasten and merge the patch before you could object.

Thanks
Laszlo