This reverts commit c22bd567ce22f6ad9bd93318ad0d7fd1c2eadb0d.

While its description is correct from an abstract or real hardware pov,
the range is special inside HVM guests. The range being UC in particular
gets in the way of OVMF, which places itself at [FFE00000,FFFFFFFF].
While this is benign to epte_get_entry_emt() as long as the IOMMU isn't
enabled for a guest, it becomes a very noticable problem otherwise: It
takes about half a minute for OVMF to decompress itself into its
designated address range.

And even beyond OVMF there's no reason to have e.g. the ACPI memory
range marked UC.

Fixes: c22bd567ce22 ("hvmloader: PA range 0xfc000000-0xffffffff should be UC")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/cacheattr.c
+++ b/tools/firmware/hvmloader/cacheattr.c
@@ -98,12 +98,12 @@ void cacheattr_init(void)
     {
         uint64_t base = pci_mem_start, size;
 
-        for ( i = 0; !(base >> 32) && (i < nr_var_ranges); i++ )
+        for ( i = 0; (base != pci_mem_end) && (i < nr_var_ranges); i++ )
         {
             size = PAGE_SIZE;
             while ( !(base & size) )
                 size <<= 1;
-            while ( ((base + size) < base) || ((base + size - 1) >> 32) )
+            while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
                 size >>= 1;
 
             wrmsr(MSR_MTRRphysBase(i), base);

On Tue, Dec 14, 2021 at 09:21:22AM +0100, Jan Beulich wrote:
> This reverts commit c22bd567ce22f6ad9bd93318ad0d7fd1c2eadb0d.
> 
> While its description is correct from an abstract or real hardware pov,
> the range is special inside HVM guests. The range being UC in particular
> gets in the way of OVMF, which places itself at [FFE00000,FFFFFFFF].

I would assume this range to be unpopulated? Does hvmloader populate
it in order to place ovmf?

> While this is benign to epte_get_entry_emt() as long as the IOMMU isn't
> enabled for a guest, it becomes a very noticable problem otherwise: It
> takes about half a minute for OVMF to decompress itself into its
> designated address range.
> 
> And even beyond OVMF there's no reason to have e.g. the ACPI memory
> range marked UC.
> 
> Fixes: c22bd567ce22 ("hvmloader: PA range 0xfc000000-0xffffffff should be UC")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

This is just a hole from hvmloader PoV, so it's fine to not apply any
specific MTRR attributes.

Thanks, Roger.

On 16/12/2021 09:54, Roger Pau Monné wrote:
> On Tue, Dec 14, 2021 at 09:21:22AM +0100, Jan Beulich wrote:
>> This reverts commit c22bd567ce22f6ad9bd93318ad0d7fd1c2eadb0d.
>>
>> While its description is correct from an abstract or real hardware pov,
>> the range is special inside HVM guests. The range being UC in particular
>> gets in the way of OVMF, which places itself at [FFE00000,FFFFFFFF].
> I would assume this range to be unpopulated? Does hvmloader populate
> it in order to place ovmf?

It's generally not unpopulated.  The video RAM lives there until the VGA
BAR is reprogrammed.

The reason OVMF places itself there is because it is where the real SPI
ROM is mapped in address space on a real system.

>> While this is benign to epte_get_entry_emt() as long as the IOMMU isn't
>> enabled for a guest, it becomes a very noticable problem otherwise: It
>> takes about half a minute for OVMF to decompress itself into its
>> designated address range.
>>
>> And even beyond OVMF there's no reason to have e.g. the ACPI memory
>> range marked UC.
>>
>> Fixes: c22bd567ce22 ("hvmloader: PA range 0xfc000000-0xffffffff should be UC")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

On Thu, Dec 16, 2021 at 11:00:17AM +0000, Andrew Cooper wrote:
> On 16/12/2021 09:54, Roger Pau Monné wrote:
> > On Tue, Dec 14, 2021 at 09:21:22AM +0100, Jan Beulich wrote:
> >> This reverts commit c22bd567ce22f6ad9bd93318ad0d7fd1c2eadb0d.
> >>
> >> While its description is correct from an abstract or real hardware pov,
> >> the range is special inside HVM guests. The range being UC in particular
> >> gets in the way of OVMF, which places itself at [FFE00000,FFFFFFFF].
> > I would assume this range to be unpopulated? Does hvmloader populate
> > it in order to place ovmf?
> 
> It's generally not unpopulated.  The video RAM lives there until the VGA
> BAR is reprogrammed.

Right, but that's an MMIO area from guests PoV, even if in our
implementation is backed by RAM pages.

> The reason OVMF places itself there is because it is where the real SPI
> ROM is mapped in address space on a real system.

Just to clarify my understanding, this is not reported as a RAM region
to guests? So hvmloader or the domain builder populates this with RAM
to place OVMF, even if not reported as a RAM region in the memory map
(much like with ACPI tables for example).

I wonder whether we should have some kind of document or code comment
about the guest memory layout (maybe there's one and I'm missing it).

Thanks, Roger.

On 16/12/2021 11:09, Roger Pau Monné wrote:
> On Thu, Dec 16, 2021 at 11:00:17AM +0000, Andrew Cooper wrote:
>> On 16/12/2021 09:54, Roger Pau Monné wrote:
>>> On Tue, Dec 14, 2021 at 09:21:22AM +0100, Jan Beulich wrote:
>>>> This reverts commit c22bd567ce22f6ad9bd93318ad0d7fd1c2eadb0d.
>>>>
>>>> While its description is correct from an abstract or real hardware pov,
>>>> the range is special inside HVM guests. The range being UC in particular
>>>> gets in the way of OVMF, which places itself at [FFE00000,FFFFFFFF].
>>> I would assume this range to be unpopulated? Does hvmloader populate
>>> it in order to place ovmf?
>> It's generally not unpopulated.  The video RAM lives there until the VGA
>> BAR is reprogrammed.
> Right, but that's an MMIO area from guests PoV, even if in our
> implementation is backed by RAM pages.
>
>> The reason OVMF places itself there is because it is where the real SPI
>> ROM is mapped in address space on a real system.
> Just to clarify my understanding, this is not reported as a RAM region
> to guests? So hvmloader or the domain builder populates this with RAM
> to place OVMF, even if not reported as a RAM region in the memory map
> (much like with ACPI tables for example).
>
> I wonder whether we should have some kind of document or code comment
> about the guest memory layout (maybe there's one and I'm missing it).

Like most things with guest physmap handling, it's broken and held
together with hope.

Yes, it's logically an MMIO region, but actually "just plain RAM" under
the hood.

What should happen is the domain builder place OVMF there directly. 
OVMF is linked to run there, and there ought to be no hvmloader involved
for UEFI guests, and there shouldn't be any copying into place
required.  That was a very short sited move on behalf of the OVMF
support logic, and I guarantee you that it creates more problems than it
allegedly solves.

~Andrew

On Thu, Dec 16, 2021 at 11:17:12AM +0000, Andrew Cooper wrote:
> What should happen is the domain builder place OVMF there directly. 
> OVMF is linked to run there, and there ought to be no hvmloader involved
> for UEFI guests, and there shouldn't be any copying into place
> required.  That was a very short sited move on behalf of the OVMF
> support logic, and I guarantee you that it creates more problems than it
> allegedly solves.

FYI, OVMF been copied there is due to how KVM/QEMU make use of OVMF, so
it was probably the easiest thing to do at the time.

Now, we have OvmfXen with support for PVH (still a few thing to fix),
which doesn't need to be loaded from hvmloader, and doesn't need to run
from that address (~4GB). Most of OVMF is linked to run at 8MB anyway (I
think, if I remember the address).

We aren't far from been able to run UEFI HVM guests without hvmloader,
we need to fix the logic in OVMF to take care of the PCI bars, and maybe
one or two other things, then load OVMF as a PVH kernel and hopefully no
more hvmloader on HVM guests.

Cheers,

-- 
Anthony PERARD