Re: [PATCH] hvmloader: pass PCI MMIO layout to OVMF as an info table

[Jan Beulich <jbeulich@suse.com>]

On 11.01.2021 05:53, Igor Druzhinin wrote:
> We faced a problem with passing through a PCI device with 64GB BAR to
> UEFI guest. The BAR is expectedly programmed into 64-bit PCI aperture at
> 64G address which pushes physical address space to 37 bits. OVMF uses
> address width early in PEI phase to make DXE identity pages covering
> the whole addressable space so it needs to know the last address it needs
> to cover but at the same time not overdo the mappings.
> 
> As there is seemingly no other way to pass or get this information in
> OVMF at this early phase (ACPI is not yet available, PCI is not yet enumerated,
> xenstore is not yet initialized) - extend the info structure with a new
> table. Since the structure was initially created to be extendable -
> the change is backward compatible.

How does UEFI handle the same situation on baremetal? I'd guess it is
in even more trouble there, as it couldn't even read addresses from
BARs, but would first need to assign them (or at least calculate
their intended positions).

> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -61,6 +61,14 @@ struct ovmf_info {
>      uint32_t e820_nr;
>  } __attribute__ ((packed));
>  
> +#define OVMF_INFO_PCI_TABLE 0
> +struct ovmf_pci_info {
> +    uint64_t low_start;
> +    uint64_t low_end;
> +    uint64_t hi_start;
> +    uint64_t hi_end;
> +} __attribute__ ((packed));

Forming part of ABI, I believe this belongs in a public header,
which consumers could at least in principle use verbatim if
they wanted to.

> @@ -74,9 +82,21 @@ static void ovmf_setup_bios_info(void)
>  static void ovmf_finish_bios_info(void)
>  {
>      struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> +    struct ovmf_pci_info *pci_info;
> +    uint64_t *tables = scratch_alloc(sizeof(uint64_t)*OVMF_INFO_MAX_TABLES, 0);

I wasn't able to locate OVMF_INFO_MAX_TABLES in either
xen/include/public/ or tools/firmware/. Where does it get
defined?

Also (nit) missing blanks around * .

>      uint32_t i;
>      uint8_t checksum;
>  
> +    pci_info = scratch_alloc(sizeof(struct ovmf_pci_info), 0);

Is "scratch" correct here and above? I guess intended usage /
scope will want spelling out somewhere.

> +    pci_info->low_start = pci_mem_start;
> +    pci_info->low_end = pci_mem_end;
> +    pci_info->hi_start = pci_hi_mem_start;
> +    pci_info->hi_end = pci_hi_mem_end;
> +
> +    tables[OVMF_INFO_PCI_TABLE] = (uint32_t)pci_info;
> +    info->tables = (uint32_t)tables;
> +    info->tables_nr = 1;

In how far is this problem (and hence solution / workaround) OVMF
specific? IOW don't we need a more generic approach here?

Jan