Re: [Xen-devel] [PATCH v3 1/1] Add pci_hole_min_size

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.06.14 at 19:47, <dslutz@xxxxxxxxxxx> wrote:
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -53,7 +53,6 @@ extern struct bios_config ovmf_config;
>  #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
>  
>  /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
> -#define PCI_MEM_START       0xf0000000
>  #define PCI_MEM_END         0xfc000000

This pair already shows that ... [skip next comment for continuation]

> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -28,9 +28,10 @@
>  #include <xen/memory.h>
>  #include <xen/hvm/ioreq.h>
>  #include <xen/hvm/hvm_xs_strings.h>
> +#include <xen/hvm/e820.h>
>  #include <stdbool.h>
>  
> -unsigned long pci_mem_start = PCI_MEM_START;
> +unsigned long pci_mem_start = HVM_BELOW_4G_RAM_END;

Perhaps (also related to the comment below) better to use
HVM_BELOW_4G_MMIO_START here, even if right now both evaluate
to the same number.

> @@ -237,6 +243,26 @@ void pci_setup(void)
>          pci_writew(devfn, PCI_COMMAND, cmd);
>      }
>  
> +    if ( pci_hole_min_size )
> +    {
> +        uint64_t max_ram_below_4g = (1ULL << 32) - pci_hole_min_size;

... this isn't really correct: The space immediately below 4G is used for
MMIO unrelated to PCI. Hence, to not confuse people, I'd really like
the variable to be properly named (whether the xl config option should
also get renamed I'm not sure - the xl maintainers will know), i.e.
mmio_hole_min_size.

> +
> +        if ( max_ram_below_4g >= HVM_BELOW_4G_RAM_END )
> +        {
> +            printf("max_ram_below_4g=0x"PRIllx
> +                   " too big for pci_hole_min_size=0x"PRIllx"\n",
> +                   PRIllx_arg(max_ram_below_4g),
> +                   PRIllx_arg(pci_hole_min_size));

Probably worth making clear in the logged message that the option
therefore is being ignored?

> +        }
> +        else
> +        {
> +            pci_mem_start = max_ram_below_4g;
> +            printf("pci_mem_start=0x%lx (was 0x%x) for 
> pci_hole_min_size=%lu\n",
> +                   pci_mem_start, HVM_BELOW_4G_RAM_END,
> +                   (long)pci_hole_min_size);
> +        }
> +    }

Furthermore the whole concept collides with the auto-growing of
the hole immediately following this code. Both logically (both having
a similar purpose) and, what's worse, functionally in that the
shifting done there requires that ((1ULL << 32) - pci_mem_start)
be a power of 2. An option might be to skip this auto-sizing if the
option was given.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel