| Message ID | 20190729153944.24239-24-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Specific platform to run OVMF in Xen PVH and HVM guests | expand |
On 07/29/19 17:39, Anthony PERARD wrote: > When running as a Xen PVH guest, there is no CMOS to read the memory > size from. Rework GetSystemMemorySize(Below|Above)4gb() so they can > work without CMOS by reading the e820 table. > > Rework XenPublishRamRegions to also care for the reserved and ACPI > entry in the e820 table. The region that was added by InitializeXen() > isn't needed as that same entry is in the e820 table provided by > hvmloader. > > MTRR settings aren't modified anymore, on HVM it's already done by > hvmloader, on PVH it is supposed to have sane default. MTRR will need > to be done properly but keeping what's already been done by programmes > that has runned before OVMF will do for now. (1) s/programmes that has runned/programs that have run/ > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689 > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > Acked-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > v4: > - some coding style > - Added AddReservedMemoryRangeHob, and using it. > - this patch now replace "OvmfPkg/XenPlatformPei: Reserve hvmloader's memory only when it has run" > from v3. hvmloader have added an entry in the e820 table, there is no > need for a special case. > - now, everything that is in the e820 table is added to OVMF's memory > map, no more skipping ACPI entries or hvmloader's reserved entries. > Instead, we look for the local APIC region and avoid it if it is > present in the e820. > - rework commit message > > OvmfPkg/XenPlatformPei/Platform.h | 13 ++++++ > OvmfPkg/XenPlatformPei/MemDetect.c | 69 +++++++++++++++++++++++++++++ > OvmfPkg/XenPlatformPei/Platform.c | 11 +++++ > OvmfPkg/XenPlatformPei/Xen.c | 70 +++++++++++++++++++----------- > 4 files changed, 137 insertions(+), 26 deletions(-) > > diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h > index db9a62572f..7661f4a8de 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.h > +++ b/OvmfPkg/XenPlatformPei/Platform.h > @@ -44,6 +44,13 @@ AddReservedMemoryBaseSizeHob ( > BOOLEAN Cacheable > ); > > +VOID > +AddReservedMemoryRangeHob ( > + EFI_PHYSICAL_ADDRESS MemoryBase, > + EFI_PHYSICAL_ADDRESS MemoryLimit, > + BOOLEAN Cacheable > + ); > + > VOID > AddressWidthInitialization ( > VOID > @@ -114,6 +121,12 @@ XenPublishRamRegions ( > VOID > ); > > +EFI_STATUS > +XenGetE820Map ( > + EFI_E820_ENTRY64 **Entries, > + UINT32 *Count > + ); > + > extern EFI_BOOT_MODE mBootMode; > > extern UINT8 mPhysMemAddressWidth; > diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c > index cf95f9c474..1f81eee407 100644 > --- a/OvmfPkg/XenPlatformPei/MemDetect.c > +++ b/OvmfPkg/XenPlatformPei/MemDetect.c > @@ -96,6 +96,45 @@ Q35TsegMbytesInitialization ( > mQ35TsegMbytes = ExtendedTsegMbytes; > } > > +STATIC > +UINT64 > +GetHighestSystemMemoryAddress ( > + BOOLEAN Below4gb > + ) > +{ > + EFI_E820_ENTRY64 *E820Map; > + UINT32 E820EntriesCount; > + EFI_E820_ENTRY64 *Entry; > + EFI_STATUS Status; > + UINT32 Loop; > + UINT64 HighestAddress; > + UINT64 EntryEnd; > + > + HighestAddress = 0; > + > + Status = XenGetE820Map (&E820Map, &E820EntriesCount); > + ASSERT_EFI_ERROR (Status); > + > + for (Loop = 0; Loop < E820EntriesCount; Loop++) { > + Entry = E820Map + Loop; > + EntryEnd = Entry->BaseAddr + Entry->Length; > + > + if (Entry->Type == EfiAcpiAddressRangeMemory && > + EntryEnd > HighestAddress) { > + > + if (Below4gb && (EntryEnd <= BASE_4GB)) { > + HighestAddress = EntryEnd; > + } else if (!Below4gb && (EntryEnd >= BASE_4GB)) { > + HighestAddress = EntryEnd; > + } > + } > + } > + > + // > + // Round down the end address. > + // > + return HighestAddress & ~(UINT64)EFI_PAGE_MASK; > +} > > UINT32 > GetSystemMemorySizeBelow4gb ( > @@ -105,6 +144,19 @@ GetSystemMemorySizeBelow4gb ( > UINT8 Cmos0x34; > UINT8 Cmos0x35; > > + // > + // In PVH case, there is no CMOS, we have to calculate the memory size > + // from parsing the E820 > + // > + if (XenPvhDetected ()) { > + UINT64 HighestAddress; > + > + HighestAddress = GetHighestSystemMemoryAddress (TRUE); > + ASSERT (HighestAddress > 0 && HighestAddress <= BASE_4GB); > + > + return HighestAddress; > + } > + > // > // CMOS 0x34/0x35 specifies the system memory above 16 MB. > // * CMOS(0x35) is the high byte > @@ -129,6 +181,23 @@ GetSystemMemorySizeAbove4gb ( > UINT32 Size; > UINTN CmosIndex; > > + // > + // In PVH case, there is no CMOS, we have to calculate the memory size > + // from parsing the E820 > + // > + if (XenPvhDetected ()) { > + UINT64 HighestAddress; > + > + HighestAddress = GetHighestSystemMemoryAddress (FALSE); > + ASSERT (HighestAddress == 0 || HighestAddress >= BASE_4GB); > + > + if (HighestAddress >= BASE_4GB) { > + HighestAddress -= BASE_4GB; > + } > + > + return HighestAddress; > + } > + > // > // CMOS 0x5b-0x5d specifies the system memory above 4GB MB. > // * CMOS(0x5d) is the most significant size byte > diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c > index 6aaafc3ee9..2f42ca6ccd 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.c > +++ b/OvmfPkg/XenPlatformPei/Platform.c > @@ -102,6 +102,17 @@ AddReservedMemoryBaseSizeHob ( > ); > } > > +VOID > +AddReservedMemoryRangeHob ( > + EFI_PHYSICAL_ADDRESS MemoryBase, > + EFI_PHYSICAL_ADDRESS MemoryLimit, > + BOOLEAN Cacheable > + ) > +{ > + AddReservedMemoryBaseSizeHob (MemoryBase, > + (UINT64)(MemoryLimit - MemoryBase), Cacheable); > +} > + > VOID > AddIoMemoryRangeHob ( > EFI_PHYSICAL_ADDRESS MemoryBase, > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > index a21d657357..182e96cc5b 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -276,9 +276,12 @@ XenPublishRamRegions ( > VOID > ) > { > - EFI_E820_ENTRY64 *E820Map; > - UINT32 E820EntriesCount; > - EFI_STATUS Status; > + EFI_E820_ENTRY64 *E820Map; > + UINT32 E820EntriesCount; > + EFI_STATUS Status; > + EFI_E820_ENTRY64 *Entry; > + UINTN Index; > + EFI_PHYSICAL_ADDRESS LocalApic; > > DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n")); > > @@ -287,26 +290,47 @@ XenPublishRamRegions ( > // > E820EntriesCount = 0; > Status = XenGetE820Map (&E820Map, &E820EntriesCount); > - > ASSERT_EFI_ERROR (Status); > > - if (E820EntriesCount > 0) { > - EFI_E820_ENTRY64 *Entry; > - UINT32 Loop; > - > - for (Loop = 0; Loop < E820EntriesCount; Loop++) { > - Entry = E820Map + Loop; > - > - // > - // Only care about RAM > - // > - if (Entry->Type != EfiAcpiAddressRangeMemory) { > - continue; > + LocalApic = PcdGet32(PcdCpuLocalApicBaseAddress); (2) missing space before "(" -- but, actually, please see below > + AddIoMemoryBaseSizeHob (LocalApic, SIZE_1MB); > + > + for (Index = 0; Index < E820EntriesCount; Index++) { > + UINT64 Base; > + UINT64 End; > + > + Entry = &E820Map[Index]; > + > + // > + // Round up the start address, and round down the end address. > + // > + Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE); > + End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK; > + > + switch (Entry->Type) { > + case EfiAcpiAddressRangeMemory: > + AddMemoryRangeHob (Base, End); > + break; > + case EfiAcpiAddressRangeACPI: > + AddReservedMemoryRangeHob (Base, End, FALSE); > + break; > + case EfiAcpiAddressRangeReserved: > + if (Base < LocalApic && LocalApic < End) { > + // > + // hvmloader marks a range that overlaps with the local APIC memory > + // mapped region as reserved, but CpuDxe wants it as mapped IO. We > + // have already added it as mapped IO, so skip it here. > + // > + AddReservedMemoryRangeHob (Base, LocalApic, FALSE); > + if (End > (LocalApic + SIZE_1MB)) { > + AddReservedMemoryRangeHob (LocalApic + SIZE_1MB, End, FALSE); > + } > + } else { > + AddReservedMemoryRangeHob (Base, End, FALSE); > } (3) Not sure how general you want to be here, but I don't think this is fully general. How about: (3a) before the loop, drop "LocalApic", and add: UINT64 LapicBase; UINT64 LapicEnd; LapicBase = PcdGet32 (PcdCpuLocalApicBaseAddress); LapicEnd = LapicBase + SIZE_1MB; (3b) inside the loop body, near the top of the block, add: UINT64 ReservedBase; UINT64 ReservedEnd; (3c) under the EfiAcpiAddressRangeReserved case label: // // add LAPIC predecessor range, if any // ReservedBase = Base; ReservedEnd = MIN (End, LapicBase); if (ReservedBase < ReservedEnd) { AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); } // // add LAPIC successor range, if any // ReservedBase = MAX (Base, LapicEnd); ReservedEnd = End; if (ReservedBase < ReservedEnd) { AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); } This will cover all possible constellations between the [LapicBase, LapicEnd) and [Base, End) intervals. --*-- Regarding the approach itself, I'll defer to Roger. Thanks Laszlo > - > - AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length); > - > - MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack); > + break; > + default: > + break; > } > } > } > @@ -326,12 +350,6 @@ InitializeXen ( > { > RETURN_STATUS PcdStatus; > > - // > - // Reserve away HVMLOADER reserved memory [0xFC000000,0xFD000000). > - // This needs to match HVMLOADER RESERVED_MEMBASE/RESERVED_MEMSIZE. > - // > - AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE); > - > PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE); > ASSERT_RETURN_ERROR (PcdStatus); > >
On 07/30/19 13:45, Laszlo Ersek wrote: > On 07/29/19 17:39, Anthony PERARD wrote: >> When running as a Xen PVH guest, there is no CMOS to read the memory >> size from. Rework GetSystemMemorySize(Below|Above)4gb() so they can >> work without CMOS by reading the e820 table. >> >> Rework XenPublishRamRegions to also care for the reserved and ACPI >> entry in the e820 table. The region that was added by InitializeXen() >> isn't needed as that same entry is in the e820 table provided by >> hvmloader. >> >> MTRR settings aren't modified anymore, on HVM it's already done by >> hvmloader, on PVH it is supposed to have sane default. MTRR will need >> to be done properly but keeping what's already been done by programmes >> that has runned before OVMF will do for now. > > (1) s/programmes that has runned/programs that have run/ > >> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689 >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >> Acked-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> v4: >> - some coding style >> - Added AddReservedMemoryRangeHob, and using it. >> - this patch now replace "OvmfPkg/XenPlatformPei: Reserve hvmloader's memory only when it has run" >> from v3. hvmloader have added an entry in the e820 table, there is no >> need for a special case. >> - now, everything that is in the e820 table is added to OVMF's memory >> map, no more skipping ACPI entries or hvmloader's reserved entries. >> Instead, we look for the local APIC region and avoid it if it is >> present in the e820. >> - rework commit message >> >> OvmfPkg/XenPlatformPei/Platform.h | 13 ++++++ >> OvmfPkg/XenPlatformPei/MemDetect.c | 69 +++++++++++++++++++++++++++++ >> OvmfPkg/XenPlatformPei/Platform.c | 11 +++++ >> OvmfPkg/XenPlatformPei/Xen.c | 70 +++++++++++++++++++----------- >> 4 files changed, 137 insertions(+), 26 deletions(-) >> >> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h >> index db9a62572f..7661f4a8de 100644 >> --- a/OvmfPkg/XenPlatformPei/Platform.h >> +++ b/OvmfPkg/XenPlatformPei/Platform.h >> @@ -44,6 +44,13 @@ AddReservedMemoryBaseSizeHob ( >> BOOLEAN Cacheable >> ); >> >> +VOID >> +AddReservedMemoryRangeHob ( >> + EFI_PHYSICAL_ADDRESS MemoryBase, >> + EFI_PHYSICAL_ADDRESS MemoryLimit, >> + BOOLEAN Cacheable >> + ); >> + >> VOID >> AddressWidthInitialization ( >> VOID >> @@ -114,6 +121,12 @@ XenPublishRamRegions ( >> VOID >> ); >> >> +EFI_STATUS >> +XenGetE820Map ( >> + EFI_E820_ENTRY64 **Entries, >> + UINT32 *Count >> + ); >> + >> extern EFI_BOOT_MODE mBootMode; >> >> extern UINT8 mPhysMemAddressWidth; >> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c >> index cf95f9c474..1f81eee407 100644 >> --- a/OvmfPkg/XenPlatformPei/MemDetect.c >> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c >> @@ -96,6 +96,45 @@ Q35TsegMbytesInitialization ( >> mQ35TsegMbytes = ExtendedTsegMbytes; >> } >> >> +STATIC >> +UINT64 >> +GetHighestSystemMemoryAddress ( >> + BOOLEAN Below4gb >> + ) >> +{ >> + EFI_E820_ENTRY64 *E820Map; >> + UINT32 E820EntriesCount; >> + EFI_E820_ENTRY64 *Entry; >> + EFI_STATUS Status; >> + UINT32 Loop; >> + UINT64 HighestAddress; >> + UINT64 EntryEnd; >> + >> + HighestAddress = 0; >> + >> + Status = XenGetE820Map (&E820Map, &E820EntriesCount); >> + ASSERT_EFI_ERROR (Status); >> + >> + for (Loop = 0; Loop < E820EntriesCount; Loop++) { >> + Entry = E820Map + Loop; >> + EntryEnd = Entry->BaseAddr + Entry->Length; >> + >> + if (Entry->Type == EfiAcpiAddressRangeMemory && >> + EntryEnd > HighestAddress) { >> + >> + if (Below4gb && (EntryEnd <= BASE_4GB)) { >> + HighestAddress = EntryEnd; >> + } else if (!Below4gb && (EntryEnd >= BASE_4GB)) { >> + HighestAddress = EntryEnd; >> + } >> + } >> + } >> + >> + // >> + // Round down the end address. >> + // >> + return HighestAddress & ~(UINT64)EFI_PAGE_MASK; >> +} >> >> UINT32 >> GetSystemMemorySizeBelow4gb ( >> @@ -105,6 +144,19 @@ GetSystemMemorySizeBelow4gb ( >> UINT8 Cmos0x34; >> UINT8 Cmos0x35; >> >> + // >> + // In PVH case, there is no CMOS, we have to calculate the memory size >> + // from parsing the E820 >> + // >> + if (XenPvhDetected ()) { >> + UINT64 HighestAddress; >> + >> + HighestAddress = GetHighestSystemMemoryAddress (TRUE); >> + ASSERT (HighestAddress > 0 && HighestAddress <= BASE_4GB); >> + >> + return HighestAddress; >> + } >> + >> // >> // CMOS 0x34/0x35 specifies the system memory above 16 MB. >> // * CMOS(0x35) is the high byte >> @@ -129,6 +181,23 @@ GetSystemMemorySizeAbove4gb ( >> UINT32 Size; >> UINTN CmosIndex; >> >> + // >> + // In PVH case, there is no CMOS, we have to calculate the memory size >> + // from parsing the E820 >> + // >> + if (XenPvhDetected ()) { >> + UINT64 HighestAddress; >> + >> + HighestAddress = GetHighestSystemMemoryAddress (FALSE); >> + ASSERT (HighestAddress == 0 || HighestAddress >= BASE_4GB); >> + >> + if (HighestAddress >= BASE_4GB) { >> + HighestAddress -= BASE_4GB; >> + } >> + >> + return HighestAddress; >> + } >> + >> // >> // CMOS 0x5b-0x5d specifies the system memory above 4GB MB. >> // * CMOS(0x5d) is the most significant size byte >> diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c >> index 6aaafc3ee9..2f42ca6ccd 100644 >> --- a/OvmfPkg/XenPlatformPei/Platform.c >> +++ b/OvmfPkg/XenPlatformPei/Platform.c >> @@ -102,6 +102,17 @@ AddReservedMemoryBaseSizeHob ( >> ); >> } >> >> +VOID >> +AddReservedMemoryRangeHob ( >> + EFI_PHYSICAL_ADDRESS MemoryBase, >> + EFI_PHYSICAL_ADDRESS MemoryLimit, >> + BOOLEAN Cacheable >> + ) >> +{ >> + AddReservedMemoryBaseSizeHob (MemoryBase, >> + (UINT64)(MemoryLimit - MemoryBase), Cacheable); >> +} >> + >> VOID >> AddIoMemoryRangeHob ( >> EFI_PHYSICAL_ADDRESS MemoryBase, >> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c >> index a21d657357..182e96cc5b 100644 >> --- a/OvmfPkg/XenPlatformPei/Xen.c >> +++ b/OvmfPkg/XenPlatformPei/Xen.c >> @@ -276,9 +276,12 @@ XenPublishRamRegions ( >> VOID >> ) >> { >> - EFI_E820_ENTRY64 *E820Map; >> - UINT32 E820EntriesCount; >> - EFI_STATUS Status; >> + EFI_E820_ENTRY64 *E820Map; >> + UINT32 E820EntriesCount; >> + EFI_STATUS Status; >> + EFI_E820_ENTRY64 *Entry; >> + UINTN Index; >> + EFI_PHYSICAL_ADDRESS LocalApic; >> >> DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n")); >> >> @@ -287,26 +290,47 @@ XenPublishRamRegions ( >> // >> E820EntriesCount = 0; >> Status = XenGetE820Map (&E820Map, &E820EntriesCount); >> - >> ASSERT_EFI_ERROR (Status); >> >> - if (E820EntriesCount > 0) { >> - EFI_E820_ENTRY64 *Entry; >> - UINT32 Loop; >> - >> - for (Loop = 0; Loop < E820EntriesCount; Loop++) { >> - Entry = E820Map + Loop; >> - >> - // >> - // Only care about RAM >> - // >> - if (Entry->Type != EfiAcpiAddressRangeMemory) { >> - continue; >> + LocalApic = PcdGet32(PcdCpuLocalApicBaseAddress); > > (2) missing space before "(" -- but, actually, please see below > > >> + AddIoMemoryBaseSizeHob (LocalApic, SIZE_1MB); >> + >> + for (Index = 0; Index < E820EntriesCount; Index++) { >> + UINT64 Base; >> + UINT64 End; >> + >> + Entry = &E820Map[Index]; >> + >> + // >> + // Round up the start address, and round down the end address. >> + // >> + Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE); >> + End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK; >> + >> + switch (Entry->Type) { >> + case EfiAcpiAddressRangeMemory: >> + AddMemoryRangeHob (Base, End); >> + break; >> + case EfiAcpiAddressRangeACPI: >> + AddReservedMemoryRangeHob (Base, End, FALSE); >> + break; >> + case EfiAcpiAddressRangeReserved: >> + if (Base < LocalApic && LocalApic < End) { >> + // >> + // hvmloader marks a range that overlaps with the local APIC memory >> + // mapped region as reserved, but CpuDxe wants it as mapped IO. We >> + // have already added it as mapped IO, so skip it here. >> + // >> + AddReservedMemoryRangeHob (Base, LocalApic, FALSE); >> + if (End > (LocalApic + SIZE_1MB)) { >> + AddReservedMemoryRangeHob (LocalApic + SIZE_1MB, End, FALSE); >> + } >> + } else { >> + AddReservedMemoryRangeHob (Base, End, FALSE); >> } > > (3) Not sure how general you want to be here, but I don't think this is > fully general. How about: > > (3a) before the loop, drop "LocalApic", and add: > > UINT64 LapicBase; > UINT64 LapicEnd; > > LapicBase = PcdGet32 (PcdCpuLocalApicBaseAddress); > LapicEnd = LapicBase + SIZE_1MB; > > (3b) inside the loop body, near the top of the block, add: > > UINT64 ReservedBase; > UINT64 ReservedEnd; > > (3c) under the EfiAcpiAddressRangeReserved case label: > > // > // add LAPIC predecessor range, if any > // > ReservedBase = Base; > ReservedEnd = MIN (End, LapicBase); > if (ReservedBase < ReservedEnd) { > AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); > } > > // > // add LAPIC successor range, if any > // > ReservedBase = MAX (Base, LapicEnd); > ReservedEnd = End; > if (ReservedBase < ReservedEnd) { > AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); > } > > This will cover all possible constellations between the [LapicBase, > LapicEnd) and [Base, End) intervals. > > --*-- > > Regarding the approach itself, I'll defer to Roger. If you and Roger both think that the present patch is good enough (because there is no issue in practice), I'm OK to ACK the patch -- I can fix the typos (1) and (2) when pushing. (What I propose under (3) could be posted as an incremental improvement, if you agree with it. It could be ignored too, if you think the suggested generality is speculative and would never be exercised in practice.) Thanks Laszlo >> - >> - AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length); >> - >> - MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack); >> + break; >> + default: >> + break; >> } >> } >> } >> @@ -326,12 +350,6 @@ InitializeXen ( >> { >> RETURN_STATUS PcdStatus; >> >> - // >> - // Reserve away HVMLOADER reserved memory [0xFC000000,0xFD000000). >> - // This needs to match HVMLOADER RESERVED_MEMBASE/RESERVED_MEMSIZE. >> - // >> - AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE); >> - >> PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE); >> ASSERT_RETURN_ERROR (PcdStatus); >> >> >
On Mon, Jul 29, 2019 at 04:39:32PM +0100, Anthony PERARD wrote: > When running as a Xen PVH guest, there is no CMOS to read the memory > size from. Rework GetSystemMemorySize(Below|Above)4gb() so they can > work without CMOS by reading the e820 table. > > Rework XenPublishRamRegions to also care for the reserved and ACPI > entry in the e820 table. The region that was added by InitializeXen() > isn't needed as that same entry is in the e820 table provided by > hvmloader. > > MTRR settings aren't modified anymore, on HVM it's already done by > hvmloader, on PVH it is supposed to have sane default. MTRR will need > to be done properly but keeping what's already been done by programmes ^ firmware > that has runned before OVMF will do for now. ^ ran > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > index a21d657357..182e96cc5b 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -276,9 +276,12 @@ XenPublishRamRegions ( > VOID > ) > { > - EFI_E820_ENTRY64 *E820Map; > - UINT32 E820EntriesCount; > - EFI_STATUS Status; > + EFI_E820_ENTRY64 *E820Map; > + UINT32 E820EntriesCount; > + EFI_STATUS Status; > + EFI_E820_ENTRY64 *Entry; > + UINTN Index; > + EFI_PHYSICAL_ADDRESS LocalApic; > > DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n")); > > @@ -287,26 +290,47 @@ XenPublishRamRegions ( > // > E820EntriesCount = 0; > Status = XenGetE820Map (&E820Map, &E820EntriesCount); > - > ASSERT_EFI_ERROR (Status); > > - if (E820EntriesCount > 0) { > - EFI_E820_ENTRY64 *Entry; > - UINT32 Loop; > - > - for (Loop = 0; Loop < E820EntriesCount; Loop++) { > - Entry = E820Map + Loop; > - > - // > - // Only care about RAM > - // > - if (Entry->Type != EfiAcpiAddressRangeMemory) { > - continue; > + LocalApic = PcdGet32(PcdCpuLocalApicBaseAddress); > + AddIoMemoryBaseSizeHob (LocalApic, SIZE_1MB); > + > + for (Index = 0; Index < E820EntriesCount; Index++) { > + UINT64 Base; > + UINT64 End; > + > + Entry = &E820Map[Index]; > + > + // > + // Round up the start address, and round down the end address. > + // > + Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE); > + End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK; > + > + switch (Entry->Type) { > + case EfiAcpiAddressRangeMemory: > + AddMemoryRangeHob (Base, End); > + break; > + case EfiAcpiAddressRangeACPI: > + AddReservedMemoryRangeHob (Base, End, FALSE); > + break; > + case EfiAcpiAddressRangeReserved: > + if (Base < LocalApic && LocalApic < End) { Don't you also need to check for equality? In case such region starts at Base == LocalApic? I guess it doesn't matter that much since this is to workaround a specific issue with hvmloader, but I would like to see this sorted out in hvmloader so that there's no clash anymore. Thanks, Roger.
On Wed, Aug 07, 2019 at 05:34:32PM +0200, Roger Pau Monné wrote: > On Mon, Jul 29, 2019 at 04:39:32PM +0100, Anthony PERARD wrote: > > When running as a Xen PVH guest, there is no CMOS to read the memory > > size from. Rework GetSystemMemorySize(Below|Above)4gb() so they can > > work without CMOS by reading the e820 table. > > > > Rework XenPublishRamRegions to also care for the reserved and ACPI > > entry in the e820 table. The region that was added by InitializeXen() > > isn't needed as that same entry is in the e820 table provided by > > hvmloader. > > > > MTRR settings aren't modified anymore, on HVM it's already done by > > hvmloader, on PVH it is supposed to have sane default. MTRR will need > > to be done properly but keeping what's already been done by programmes > ^ firmware I've used programmes instead of firmware because in case of PVH, OVMF is the first firmware to run, libxl (and what ever it called) is what causes an MTRR to be setup, no firmware are involved in that. > > + // > > + // Round up the start address, and round down the end address. > > + // > > + Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE); > > + End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK; > > + > > + switch (Entry->Type) { > > + case EfiAcpiAddressRangeMemory: > > + AddMemoryRangeHob (Base, End); > > + break; > > + case EfiAcpiAddressRangeACPI: > > + AddReservedMemoryRangeHob (Base, End, FALSE); > > + break; > > + case EfiAcpiAddressRangeReserved: > > + if (Base < LocalApic && LocalApic < End) { > > Don't you also need to check for equality? In case such region starts > at Base == LocalApic? > > I guess it doesn't matter that much since this is to workaround a > specific issue with hvmloader, but I would like to see this sorted out > in hvmloader so that there's no clash anymore. Indeed, it doesn't matter to much, so I've been lazy. But Laszlo have pointed that out as well, so there were going to be a patch to make the workaround better. But I feel like I'm going to need to repost the series, so I'm probably going to fix that as well. Thanks,
diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h index db9a62572f..7661f4a8de 100644 --- a/OvmfPkg/XenPlatformPei/Platform.h +++ b/OvmfPkg/XenPlatformPei/Platform.h @@ -44,6 +44,13 @@ AddReservedMemoryBaseSizeHob ( BOOLEAN Cacheable ); +VOID +AddReservedMemoryRangeHob ( + EFI_PHYSICAL_ADDRESS MemoryBase, + EFI_PHYSICAL_ADDRESS MemoryLimit, + BOOLEAN Cacheable + ); + VOID AddressWidthInitialization ( VOID @@ -114,6 +121,12 @@ XenPublishRamRegions ( VOID ); +EFI_STATUS +XenGetE820Map ( + EFI_E820_ENTRY64 **Entries, + UINT32 *Count + ); + extern EFI_BOOT_MODE mBootMode; extern UINT8 mPhysMemAddressWidth; diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c index cf95f9c474..1f81eee407 100644 --- a/OvmfPkg/XenPlatformPei/MemDetect.c +++ b/OvmfPkg/XenPlatformPei/MemDetect.c @@ -96,6 +96,45 @@ Q35TsegMbytesInitialization ( mQ35TsegMbytes = ExtendedTsegMbytes; } +STATIC +UINT64 +GetHighestSystemMemoryAddress ( + BOOLEAN Below4gb + ) +{ + EFI_E820_ENTRY64 *E820Map; + UINT32 E820EntriesCount; + EFI_E820_ENTRY64 *Entry; + EFI_STATUS Status; + UINT32 Loop; + UINT64 HighestAddress; + UINT64 EntryEnd; + + HighestAddress = 0; + + Status = XenGetE820Map (&E820Map, &E820EntriesCount); + ASSERT_EFI_ERROR (Status); + + for (Loop = 0; Loop < E820EntriesCount; Loop++) { + Entry = E820Map + Loop; + EntryEnd = Entry->BaseAddr + Entry->Length; + + if (Entry->Type == EfiAcpiAddressRangeMemory && + EntryEnd > HighestAddress) { + + if (Below4gb && (EntryEnd <= BASE_4GB)) { + HighestAddress = EntryEnd; + } else if (!Below4gb && (EntryEnd >= BASE_4GB)) { + HighestAddress = EntryEnd; + } + } + } + + // + // Round down the end address. + // + return HighestAddress & ~(UINT64)EFI_PAGE_MASK; +} UINT32 GetSystemMemorySizeBelow4gb ( @@ -105,6 +144,19 @@ GetSystemMemorySizeBelow4gb ( UINT8 Cmos0x34; UINT8 Cmos0x35; + // + // In PVH case, there is no CMOS, we have to calculate the memory size + // from parsing the E820 + // + if (XenPvhDetected ()) { + UINT64 HighestAddress; + + HighestAddress = GetHighestSystemMemoryAddress (TRUE); + ASSERT (HighestAddress > 0 && HighestAddress <= BASE_4GB); + + return HighestAddress; + } + // // CMOS 0x34/0x35 specifies the system memory above 16 MB. // * CMOS(0x35) is the high byte @@ -129,6 +181,23 @@ GetSystemMemorySizeAbove4gb ( UINT32 Size; UINTN CmosIndex; + // + // In PVH case, there is no CMOS, we have to calculate the memory size + // from parsing the E820 + // + if (XenPvhDetected ()) { + UINT64 HighestAddress; + + HighestAddress = GetHighestSystemMemoryAddress (FALSE); + ASSERT (HighestAddress == 0 || HighestAddress >= BASE_4GB); + + if (HighestAddress >= BASE_4GB) { + HighestAddress -= BASE_4GB; + } + + return HighestAddress; + } + // // CMOS 0x5b-0x5d specifies the system memory above 4GB MB. // * CMOS(0x5d) is the most significant size byte diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c index 6aaafc3ee9..2f42ca6ccd 100644 --- a/OvmfPkg/XenPlatformPei/Platform.c +++ b/OvmfPkg/XenPlatformPei/Platform.c @@ -102,6 +102,17 @@ AddReservedMemoryBaseSizeHob ( ); } +VOID +AddReservedMemoryRangeHob ( + EFI_PHYSICAL_ADDRESS MemoryBase, + EFI_PHYSICAL_ADDRESS MemoryLimit, + BOOLEAN Cacheable + ) +{ + AddReservedMemoryBaseSizeHob (MemoryBase, + (UINT64)(MemoryLimit - MemoryBase), Cacheable); +} + VOID AddIoMemoryRangeHob ( EFI_PHYSICAL_ADDRESS MemoryBase, diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c index a21d657357..182e96cc5b 100644 --- a/OvmfPkg/XenPlatformPei/Xen.c +++ b/OvmfPkg/XenPlatformPei/Xen.c @@ -276,9 +276,12 @@ XenPublishRamRegions ( VOID ) { - EFI_E820_ENTRY64 *E820Map; - UINT32 E820EntriesCount; - EFI_STATUS Status; + EFI_E820_ENTRY64 *E820Map; + UINT32 E820EntriesCount; + EFI_STATUS Status; + EFI_E820_ENTRY64 *Entry; + UINTN Index; + EFI_PHYSICAL_ADDRESS LocalApic; DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n")); @@ -287,26 +290,47 @@ XenPublishRamRegions ( // E820EntriesCount = 0; Status = XenGetE820Map (&E820Map, &E820EntriesCount); - ASSERT_EFI_ERROR (Status); - if (E820EntriesCount > 0) { - EFI_E820_ENTRY64 *Entry; - UINT32 Loop; - - for (Loop = 0; Loop < E820EntriesCount; Loop++) { - Entry = E820Map + Loop; - - // - // Only care about RAM - // - if (Entry->Type != EfiAcpiAddressRangeMemory) { - continue; + LocalApic = PcdGet32(PcdCpuLocalApicBaseAddress); + AddIoMemoryBaseSizeHob (LocalApic, SIZE_1MB); + + for (Index = 0; Index < E820EntriesCount; Index++) { + UINT64 Base; + UINT64 End; + + Entry = &E820Map[Index]; + + // + // Round up the start address, and round down the end address. + // + Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE); + End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK; + + switch (Entry->Type) { + case EfiAcpiAddressRangeMemory: + AddMemoryRangeHob (Base, End); + break; + case EfiAcpiAddressRangeACPI: + AddReservedMemoryRangeHob (Base, End, FALSE); + break; + case EfiAcpiAddressRangeReserved: + if (Base < LocalApic && LocalApic < End) { + // + // hvmloader marks a range that overlaps with the local APIC memory + // mapped region as reserved, but CpuDxe wants it as mapped IO. We + // have already added it as mapped IO, so skip it here. + // + AddReservedMemoryRangeHob (Base, LocalApic, FALSE); + if (End > (LocalApic + SIZE_1MB)) { + AddReservedMemoryRangeHob (LocalApic + SIZE_1MB, End, FALSE); + } + } else { + AddReservedMemoryRangeHob (Base, End, FALSE); } - - AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length); - - MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack); + break; + default: + break; } } } @@ -326,12 +350,6 @@ InitializeXen ( { RETURN_STATUS PcdStatus; - // - // Reserve away HVMLOADER reserved memory [0xFC000000,0xFD000000). - // This needs to match HVMLOADER RESERVED_MEMBASE/RESERVED_MEMSIZE. - // - AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE); - PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE); ASSERT_RETURN_ERROR (PcdStatus);