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

[Jan Beulich <jbeulich@suse.com>]

On 29.06.2021 13:29, Laszlo Ersek wrote:
> 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.

I think you are fine going ahead as long as from the ovmf side all is
good here.

Jan