Re: [RFC PATCH 1/1] Add pci_hole_min_size

[Don Slutz <dslutz@verizon.com>]

On 02/28/14 17:07, Boris Ostrovsky wrote:
> On 02/28/2014 03:15 PM, Don Slutz wrote:
>> This allows growing the pci_hole to the size needed.
>>
>> Signed-off-by: Don Slutz<dslutz@verizon.com>
>> ---
>>   tools/firmware/hvmloader/pci.c | 10 ++++++++++
>>   tools/libxc/xc_hvm_build_x86.c | 22 ++++++++++++++++++++++
>>   tools/libxc/xenguest.h         | 11 +++++++++++
>>   tools/libxl/libxl_create.c     |  4 +++-
>>   tools/libxl/libxl_dm.c         | 13 +++++++++++--
>>   tools/libxl/libxl_dom.c        |  3 ++-
>>   tools/libxl/libxl_types.idl    |  1 +
>>   tools/libxl/xl_cmdimpl.c       |  6 ++++++
>>   8 files changed, 66 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
>> index 627e8cb..b36d00b 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -58,6 +58,7 @@ void pci_setup(void)
>>           uint64_t bar_sz;
>>       } *bars = (struct bars *)scratch_start;
>>       unsigned int i, nr_bars = 0;
>> +    uint64_t pci_hole_min_size;
>>         const char *s;
>>       /*
>> @@ -85,6 +86,9 @@ void pci_setup(void)
>>       printf("Relocating guest memory for lowmem MMIO space %s\n",
>>              allow_memory_relocate?"enabled":"disabled");
>>   +    s = xenstore_read("platform/pci_hole_min_size", "0");
>
> 'if (s)'
>
Ok, will switch to:

     s = xenstore_read("platform/pci_hole_min_size", NULL);
     if ( s )
>> +    pci_hole_min_size = strtoll(s, NULL, 0);
>> +
>>       /* Program PCI-ISA bridge with appropriate link routes. */
>>       isa_irq = 0;
>>       for ( link = 0; link < 4; link++ )
>> @@ -236,6 +240,12 @@ void pci_setup(void)
>>           pci_writew(devfn, PCI_COMMAND, cmd);
>>       }
>>   +    if (pci_hole_min_size) {
>> +        pci_mem_start = (1ull << 32) - pci_hole_min_size;
>> +        printf("pci_mem_start=0x%lx (was 0x%x) for pci_hole_min_size=%llu\n",
>> +               pci_mem_start, PCI_MEM_START, pci_hole_min_size);
>> +    }
>> +
>>       /*
>>        * At the moment qemu-xen can't deal with relocated memory regions.
>>        * It's too close to the release to make a proper fix; for now,
>
> ...
>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index a604cd8..24ceac6 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -388,13 +388,15 @@ int libxl__domain_build(libxl__gc *gc,
>>           vments[4] = "start_time";
>>           vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
>>   -        localents = libxl__calloc(gc, 7, sizeof(char *));
>> +        localents = libxl__calloc(gc, 9, sizeof(char *));
>>           localents[0] = "platform/acpi";
>>           localents[1] = libxl_defbool_val(info->u.hvm.acpi) ? "1" : "0";
>>           localents[2] = "platform/acpi_s3";
>>           localents[3] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0";
>>           localents[4] = "platform/acpi_s4";
>>           localents[5] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0";
>> +        localents[6] = "platform/pci_hole_min_size";
>> +        localents[7] = libxl__sprintf(gc, "%llu", (unsigned long long)info->u.hvm.pci_hole_min_size);
>
> Do you want to always store this parameter? There is a default already (HVM_BELOW_4G_MMIO_LENGTH) so if it's not set in the config file it may be safe to omit it.
>

I do not always need to store it.  Since none of the rest of these are conditional stores, I just followed them.  Since this is the minimum size, I can add a check on pci_hole_min_size > HVM_BELOW_4G_MMIO_LENGTH here and skip the xenstore write if you want.

> ...
>
>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 4fc46eb..fe247ee 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1025,6 +1025,12 @@ static void parse_config_data(const char *config_source,
>>           xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
>>           xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
>>   +        if (!xlu_cfg_get_long(config, "pci_hole_min_size", &l, 0)) {
>> +            b_info->u.hvm.pci_hole_min_size = (uint64_t) l;
>> +            if (dom_info->debug)
>> +                fprintf(stderr, "pci_hole_min_size: %llu\n", (unsigned long long) b_info->u.hvm.pci_hole_min_size);
>> +        }
>
> You probably want to set b_info->u.hvm.pci_hole_min_size to 0 (or HVM_BELOW_4G_MMIO_LENGTH?) in case it's not specified in libxl__domain_build_info_setdefault().

Since 0 is a valid value, I do not think that libxl__domain_build_info_setdefault() needs to do any thing.  Should I be
specifying a default in the idl of 0?

    -Don Slutz
>
> -boris
>
>
>> +
>>           if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
>>               const char *s = libxl_timer_mode_to_string(l);
>>               fprintf(stderr, "WARNING: specifying \"timer_mode\" as an integer is deprecated. "
>