Re: [PATCH v3 1/1] Add pci_hole_min_size

[Don Slutz <dslutz@verizon.com>]

On 06/27/14 04:34, Jan Beulich wrote:
>>>> On 26.06.14 at 19:47, <dslutz@verizon.com> 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.

Will do.

>
>> @@ -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.

I think it is better to have at most 2 names.  So either
s/pci_hole_min_size/mmio_hole_min_size/ or switch to
max_ram_below_4g.

>> +
>> +        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?

Sure.

>> +        }
>> +        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.

I am fine with disabling the auto-growing (since as far as I know
this is bug #28 (


  #28 - support PCI hole resize in qemu-xen

)

Which this change is a kind of work around.

I would also drop the _min part of the name (i.e. use mmio_hole_size
or max_ram_below_4g).

     -Don Slutz


> Jan
>