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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 03/03/2014 10:30 AM, Don Slutz wrote:
> On 02/28/14 17:07, Boris Ostrovsky wrote:
> > > 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.
If you decide to do this I think the better place may be in 
libxl__build_post().
> > ...
> >
> >
> > > 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.
-boris


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

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