Re: [RFC PATCH 1/1] Add pci_hole_min_size

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/03/2014 03:43 PM, Don Slutz wrote:
> On 03/03/14 11:07, Boris Ostrovsky wrote:
>> On 03/03/2014 10:30 AM, Don Slutz wrote:
>>> On 02/28/14 17:07, Boris Ostrovsky wrote:
>>>
>>>
>>>> ...
>>>>
>>>>
>>>>> 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?
>>
>>
>> What I meant (but apparently not what I wrote) was that if config 
>> file doesn't have "pci_hole_min_size" then 
>> b_info->u.hvm.pci_hole_min_size will be left uninitialized and I 
>> don't know whether we can assume that it will be zero.
>>
>
> As expected, changing the .idl to have an init:
>
> - ("pci_hole_min_size",uint64),
> + ("pci_hole_min_size",UInt(64, init_val = 1)),
>
> The 1 is setup in the generated code routine 
> libxl_domain_build_info_init_type().  I plan to change this to 0.
>
> Since libxl__domain_build_info_setdefault() is called after 
> parse_config_data() it is not simple to code.
>
> If you do not like the .idl to have a 0, (i.e. like 
> LIBXL_MEMKB_DEFAULT) then I will need to add a 
> LIBXL_PCI_HOLE_MIN_DEFAULT define (like #define 
> LIBXL_PCI_HOLE_MIN_DEFAULT ~0ULL). And then use it.


As long as we know that pci_hole_min_size is initialized either way 
should be fine.

Thanks.
-boris