From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][PATCH 01/13] tools: introduce some new parameters to set rdm policy Date: Mon, 18 May 2015 09:06:29 +0800 Message-ID: <55593B15.6050600@intel.com> References: <1428657724-3498-1-git-send-email-tiejun.chen@intel.com> <1428657724-3498-2-git-send-email-tiejun.chen@intel.com> <20150508130429.GL3848@zion.uk.xensource.com> <55503F8A.5070709@intel.com> <20150511145432.GD30997@zion.uk.xensource.com> <55555157.3000604@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55555157.3000604@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: kevin.tian@intel.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, JBeulich@suse.com, yang.z.zhang@intel.com, Ian.Jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Wei, Ping... :) Thanks Tiejun On 2015/5/15 9:52, Chen, Tiejun wrote: > On 2015/5/11 22:54, Wei Liu wrote: >> On Mon, May 11, 2015 at 01:35:06PM +0800, Chen, Tiejun wrote: >>> On 2015/5/8 21:04, Wei Liu wrote: >>>> Sorry for the late review. >>>> >>> >>> Really thanks for taking your time :) >>> >>>> On Fri, Apr 10, 2015 at 05:21:52PM +0800, Tiejun Chen wrote: >>>>> This patch introduces user configurable parameters to specify RDM >>>>> resource and according policies, >>>>> >>>>> Global RDM parameter: >>>>> rdm = [ 'host, reserve=force/try' ] >>>>> Per-device RDM parameter: >>>>> pci = [ 'sbdf, rdm_reserve=force/try' ] >>>>> >>>>> Global RDM parameter allows user to specify reserved regions >>>>> explicitly, >>>>> e.g. using 'host' to include all reserved regions reported on this >>>>> platform >>>>> which is good to handle hotplug scenario. In the future this parameter >>>>> may be further extended to allow specifying random regions, e.g. even >>>>> those belonging to another platform as a preparation for live >>>>> migration >>>>> with passthrough devices. >>>>> >>>>> 'force/try' policy decides how to handle conflict when reserving RDM >>>>> regions in pfn space. If conflict exists, 'force' means an >>>>> immediate error >>>>> so VM will be killed, while 'try' allows moving forward with a warning >>>>> message thrown out. >>>>> >>>>> Default per-device RDM policy is 'force', while default global RDM >>>>> policy >>>>> is 'try'. When both policies are specified on a given region, >>>>> 'force' is >>>>> always preferred. >>>>> >>>>> Signed-off-by: Tiejun Chen >>>>> --- >>>>> docs/man/xl.cfg.pod.5 | 44 +++++++++++++++++++++++++ >>>>> docs/misc/vtd.txt | 34 ++++++++++++++++++++ >>>>> tools/libxl/libxl_create.c | 5 +++ >>>>> tools/libxl/libxl_types.idl | 18 +++++++++++ >>>>> tools/libxl/libxlu_pci.c | 78 >>>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>>> tools/libxl/libxlutil.h | 4 +++ >>>>> tools/libxl/xl_cmdimpl.c | 21 +++++++++++- >>>>> 7 files changed, 203 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 >>>>> index 408653f..9ed3055 100644 >>>>> --- a/docs/man/xl.cfg.pod.5 >>>>> +++ b/docs/man/xl.cfg.pod.5 >>>>> @@ -583,6 +583,36 @@ assigned slave device. >>>>> >>>>> =back >>>>> >>>>> +=item B >>>>> + >>>> >>>> Shouldn't this be "TYPE,RDM_RESERVE_STRIGN" according to your commit >>>> message? If the only available config is just one string, you probably >>>> don't need a list for this? >>> >>> Yes, based on that design we don't need a list. So >>> >>> =item B >>> >> >> Note that this is still a list (enclosed by "[]"). Maybe you mean >> >> rdm = "RDM_RESERVE_STRING" >> >> ? > > Yes, I'll do this. > >> >>>> >>>>> +(HVM/x86 only) Specifies the information about Reserved Device >>>>> Memory (RDM), >>>>> +which is necessary to enable robust device passthrough usage. One >>>>> example of >>>>> +RDM is reported through ACPI Reserved Memory Region Reporting (RMRR) >>>>> +structure on x86 platform. >>>>> +Each B has the form >>>>> C<["TYPE",KEY=VALUE,KEY=VALUE,...> where: >>>>> + >>>> >>>> RDM_CHECK_STRING? >>> >>> And here should be corrected like this, >>> >>> B has the form ... >>> >>>> >>>>> +=over 4 >>>>> + >>>>> +=item B<"TYPE"> >>>>> + >>>>> +Currently we just have one type. 'host' means all reserved device >>>>> memory on >>>>> +this platform should be reserved in this VM's pfn space. >>>>> + >>>> >>>> What are other possible types? If there is only one type then we can >>> >>> Currently we just have one type and looks that design doesn't make this >>> clear. >>> >>>> simply ignore the type? >>> >>> I just think we may introduce something else specific to live >>> migration in >>> the future... But I'm really not sure right now. >>> >> >> Fair enough. I was just wondering if there would be any other types. If >> so we do need provisioning. >> >> In any case, the "type" argument you proposed is a positional argument >> (you require it to be the first element of the spec string"). >> I think you can just make it a key-value pair to make parsing easier. > > Do you mean this statement? > > =item B > > ... > > B has the form C<[KEY=VALUE,KEY=VALUE,...> where: > > =over 4 > > =item B > > Possible Bs are: > > =over 4 > > =item B > > Currently we just have one type. "host" means all reserved device memory on > this platform should be reserved in this VM's pfn space. > > =over 4 > > =item B > ... > > >> >>>> >>>>> +=item B >>>>> + >>>>> +Possible Bs are: >>>>> + >>>>> +=over 4 >>>>> + >>>>> +=item B >>>>> + >>>>> +Conflict may be detected when reserving reserved device memory in >>>>> gfn space. >>>>> +'force' means an unsolved conflict leads to immediate VM destroy, >>>>> while >>>> >>>> Do you mean "immediate VM crash"? >>> >>> Yes. So I guess I should replace this. >>> >>>> >>>>> +'try' allows VM moving forward with a warning message thrown out. >>>>> 'try' >>>>> +is default. >>>> >>>> Can you please your double quotes for "force", "try" etc. >>> >>> Sure. Just note we'd like to use "strict"/"relaxed" to replace >>> "force"/"try" >>> from next revision according to Jan's suggestion. >>> >> >> No problem. >> >>>> >>>>> + >>>>> +Note this may be overrided by another sub item, rdm_reserve, in >>>>> pci device. >>>>> + >>>>> =item B >>>>> >>>>> Specifies the host PCI devices to passthrough to this guest. Each >>>>> B >>>>> @@ -645,6 +675,20 @@ dom0 without confirmation. Please use with care. >>>>> D0-D3hot power management states for the PCI device. False (0) by >>>>> default. >>>>> >>>>> +=item B >>>>> + >>>>> +(HVM/x86 only) Specifies the information about Reserved Device >>>>> Memory (RDM), >>>>> +which is necessary to enable robust device passthrough usage. One >>>>> example of >>>>> +RDM is reported through ACPI Reserved Memory Region Reporting (RMRR) >>>>> +structure on x86 platform. >>>>> + >>>>> +Conflict may be detected when reserving reserved device memory in >>>>> gfn space. >>>>> +'force' means an unsolved conflict leads to immediate VM destroy, >>>>> while >>>>> +'try' allows VM moving forward with a warning message thrown out. >>>>> 'force' >>>>> +is default. >>>>> + >>>>> +Note this would override another global item, rdm = ['']. >>>>> + >>>> >>>> Note this would override global B option. >>> >>> Fixed. >>> >>>> >>>>> =back >>>>> >>>>> =back >>>>> diff --git a/docs/misc/vtd.txt b/docs/misc/vtd.txt >>>>> index 9af0e99..d7434d6 100644 >>>>> --- a/docs/misc/vtd.txt >>>>> +++ b/docs/misc/vtd.txt >>>>> @@ -111,6 +111,40 @@ in the config file: >>>>> To override for a specific device: >>>>> pci = [ '01:00.0,msitranslate=0', '03:00.0' ] >>>>> >>>>> +RDM, 'reserved device memory', for PCI Device Passthrough >>>>> +--------------------------------------------------------- >>>>> + >>>>> +There are some devices the BIOS controls, for e.g. USB devices to >>>>> perform >>>>> +PS2 emulation. The regions of memory used for these devices are >>>>> marked >>>>> +reserved in the e820 map. When we turn on DMA translation, DMA to >>>>> those >>>>> +regions will fail. Hence BIOS uses RMRR to specify these regions >>>>> along with >>>>> +devices that need to access these regions. OS is expected to setup >>>>> +identity mappings for these regions for these devices to access >>>>> these regions. >>>>> + >>>>> +While creating a VM we should reserve them in advance, and avoid >>>>> any conflicts. >>>>> +So we introduce user configurable parameters to specify RDM >>>>> resource and >>>>> +according policies, >>>>> + >>>>> +To enable this globally, add "rdm" in the config file: >>>>> + >>>>> + rdm = [ 'host, reserve=force/try' ] >>>>> + >>>> >>>> The "force/try" should be called "policy". And then you explain what >>>> policies we have. >>> >>> Do you mean we should rename this? >>> >>> rdm = [ 'host, policy=force/try' ] >>> >> >> No, I didn't ask you to rename that. >> >> The above line is an example which should reflect the correct syntax. >> "force/try" is not the *actual syntax*, i.e. you won't write that in >> your config file. >> >> I meant to changes it to "reserve=POLICY". Then you explain the possible >> values of POLICY. >> > > Understood so what about this, > > To enable this globally, add "rdm" in the config file: > > rdm = [ 'host, reserve=' ] > > Or just for a specific device: > > pci = [ '01:00.0,rdm_reserve=', '03:00.0' ] > > Global RDM parameter allows user to specify reserved regions explicitly. > Using "host" to include all reserved regions reported on this platform > which is good to handle hotplug scenario. In the future this parameter > may be further extended to allow specifying random regions, e.g. even > those belonging to another platform as a preparation for live migration > with passthrough devices. > > Currently "POLICY" includes two options, "strict" and "relaxed". It > decides how to handle conflict when reserving RDM regions in pfn space. > If conflict ... > >>> This is really a policy but 'reserve' may can reflect our action >>> explicitly, >>> right? >>> >>>> >>>>> +Or just for a specific device: >>>>> + >>>>> + pci = [ '01:00.0,rdm_reserve=force/try', '03:00.0' ] >>> >>> And you also can see this. >>> >>> But anyway, if you're really stick to rename this, I'm going to be >>> fine as >>> well but we should ping every one to check this point since this name is >>> from our previous discussion. >>> >>>>> + >>>>> +Global RDM parameter allows user to specify reserved regions >>>>> explicitly. >>>>> +Using 'host' to include all reserved regions reported on this >>>>> platform >>>>> +which is good to handle hotplug scenario. In the future this >>>>> parameter >>>>> +may be further extended to allow specifying random regions, e.g. even >>>>> +those belonging to another platform as a preparation for live >>>>> migration >>>>> +with passthrough devices. >>>>> + >>>>> +'force/try' policy decides how to handle conflict when reserving RDM >>>>> +regions in pfn space. If conflict exists, 'force' means an >>>>> immediate error >>>>> +so VM will be killed, while 'try' allows moving forward with a >>>>> warning >>>> >>>> Be killed by whom? I think it's hvmloader crashes voluntarily, right? >>> >>> s/VM will be kille/hvmloader crashes voluntarily >>> >>>> >>>>> +message thrown out. >>>>> + >>>>> >>>>> Caveat on Conventional PCI Device Passthrough >>>>> --------------------------------------------- >>>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >>>>> index 98687bd..9ed40d4 100644 >>>>> --- a/tools/libxl/libxl_create.c >>>>> +++ b/tools/libxl/libxl_create.c >>>>> @@ -1407,6 +1407,11 @@ static void domcreate_attach_pci(libxl__egc >>>>> *egc, libxl__multidev *multidev, >>>>> } >>>>> >>>>> for (i = 0; i < d_config->num_pcidevs; i++) { >>>>> + /* >>>>> + * If the rdm global policy is 'force' we should override >>>>> each device. >>>>> + */ >>>>> + if (d_config->b_info.rdm.reserve == >>>>> LIBXL_RDM_RESERVE_FLAG_FORCE) >>>>> + d_config->pcidevs[i].rdm_reserve = >>>>> LIBXL_RDM_RESERVE_FLAG_FORCE; >>>>> ret = libxl__device_pci_add(gc, domid, >>>>> &d_config->pcidevs[i], 1); >>>>> if (ret < 0) { >>>>> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, >>>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >>>>> index 47af340..5786455 100644 >>>>> --- a/tools/libxl/libxl_types.idl >>>>> +++ b/tools/libxl/libxl_types.idl >>>>> @@ -71,6 +71,17 @@ libxl_domain_type = Enumeration("domain_type", [ >>>>> (2, "PV"), >>>>> ], init_val = "LIBXL_DOMAIN_TYPE_INVALID") >>>>> >>>>> +libxl_rdm_reserve_type = Enumeration("rdm_reserve_type", [ >>>>> + (0, "none"), >>>>> + (1, "host"), >>>>> + ]) >>>>> + >>>>> +libxl_rdm_reserve_flag = Enumeration("rdm_reserve_flag", [ >>>>> + (-1, "invalid"), >>>>> + (0, "force"), >>>>> + (1, "try"), >>>>> + ]) >>>> >>>> If you don't set init_val, the default value would be "force" (0), >>>> is this >>> >>> Yes. >>> >>>> want you want? >>> >>> We have a little bit of complexity here, >>> >>> "Default per-device RDM policy is 'force', while default global RDM >>> policy >>> is 'try'. When both policies are specified on a given region, 'force' is >>> always preferred." >>> >> >> This is going to be done in actual code anyway. >> >> This type is used both in global and per-device setting, so I envisage > > Yes. > >> this to have an invalid value to start with. Appropriate default values > > Sounds I should set this, > > +libxl_rdm_reserve_flag = Enumeration("rdm_reserve_flag", [ > + (-1, "invalid"), > + (0, "strict"), > + (1, "relaxed"), > + ], init_val = "LIBXL_RDM_RESERVE_FLAG_INVALID") > + > > >> should be done in libxl_TYPE_setdefault functions. And the logic to >> detect conflict and preferences done in your construct function. >> >> What do you think? >> >>>> >>>>> + >>>>> libxl_channel_connection = Enumeration("channel_connection", [ >>>>> (0, "UNKNOWN"), >>>>> (1, "PTY"), >>>>> @@ -356,6 +367,11 @@ libxl_domain_sched_params = >>>>> Struct("domain_sched_params",[ >>>>> ("budget", integer, {'init_val': >>>>> 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}), >>>>> ]) >>>>> >>>>> +libxl_rdm_reserve = Struct("rdm_reserve", [ >>>>> + ("type", libxl_rdm_reserve_type), >>>>> + ("reserve", libxl_rdm_reserve_flag), >>>>> + ]) >>>>> + >>>>> libxl_domain_build_info = Struct("domain_build_info",[ >>>>> ("max_vcpus", integer), >>>>> ("avail_vcpus", libxl_bitmap), >>>>> @@ -401,6 +417,7 @@ libxl_domain_build_info = >>>>> Struct("domain_build_info",[ >>>>> ("kernel", string), >>>>> ("cmdline", string), >>>>> ("ramdisk", string), >>>>> + ("rdm", libxl_rdm_reserve), >>>>> ("u", KeyedUnion(None, libxl_domain_type, "type", >>>>> [("hvm", Struct(None, [("firmware", string), >>>>> ("bios", >>>>> libxl_bios_type), >>>>> @@ -521,6 +538,7 @@ libxl_device_pci = Struct("device_pci", [ >>>>> ("power_mgmt", bool), >>>>> ("permissive", bool), >>>>> ("seize", bool), >>>>> + ("rdm_reserve", libxl_rdm_reserve_flag), >>>>> ]) >>>>> >>>>> libxl_device_vtpm = Struct("device_vtpm", [ >>>>> diff --git a/tools/libxl/libxlu_pci.c b/tools/libxl/libxlu_pci.c >>>>> index 26fb143..45be0d9 100644 >>>>> --- a/tools/libxl/libxlu_pci.c >>>>> +++ b/tools/libxl/libxlu_pci.c >>>>> @@ -42,6 +42,8 @@ static int pcidev_struct_fill(libxl_device_pci >>>>> *pcidev, unsigned int domain, >>>>> #define STATE_OPTIONS_K 6 >>>>> #define STATE_OPTIONS_V 7 >>>>> #define STATE_TERMINAL 8 >>>>> +#define STATE_TYPE 9 >>>>> +#define STATE_CHECK_FLAG 10 >>>>> int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, >>>>> const char *str) >>>>> { >>>>> unsigned state = STATE_DOMAIN; >>>>> @@ -143,6 +145,17 @@ int xlu_pci_parse_bdf(XLU_Config *cfg, >>>>> libxl_device_pci *pcidev, const char *str >>>>> pcidev->permissive = atoi(tok); >>>>> }else if ( !strcmp(optkey, "seize") ) { >>>>> pcidev->seize = atoi(tok); >>>>> + }else if ( !strcmp(optkey, "rdm_reserve") ) { >>>>> + if ( !strcmp(tok, "force") ) { >>>>> + pcidev->rdm_reserve = >>>>> LIBXL_RDM_RESERVE_FLAG_FORCE; >>>>> + } else if ( !strcmp(tok, "try") ) { >>>>> + pcidev->rdm_reserve = >>>>> LIBXL_RDM_RESERVE_FLAG_TRY; >>>>> + } else { >>>>> + pcidev->rdm_reserve = >>>>> LIBXL_RDM_RESERVE_FLAG_FORCE; >>>>> + XLU__PCI_ERR(cfg, "Unknown PCI RDM >>>>> property flag value:" >>>>> + " %s, so goes 'force' by >>>>> default.", >>>> >>>> If this is not an error, you don't need XLU__PCI_ERR. >>>> >>>> But I would say we should just treat this as an error and >>>> abort/exit/report (whatever the parser should do in this case). >>> >>> In our case we just want to post a message to set a appropriate flag to >>> recover this behavior like we write here, >>> >>> XLU__PCI_ERR(cfg, "Unknown PCI RDM property >>> flag >>> value:" >>> " %s, so goes 'strict' by >>> default.", >>> tok); >> >> I suggest we just abort in this case and not second guess what the admin >> wants. > > Okay, > } else { > XLU__PCI_ERR(cfg, "%s is not an valid PCI RDM > property" > " flag: 'strict' or 'relaxed'.", > tok); > abort(); > > >> >>> >>> This may just be a warning? But I don't we have this sort of definition, >>> XLU__PCI_WARN, ... >>> >>> So what LOG format can be adopted here? >> >> Feel free to introduce XLU__PCI_WARN if it turns out to be necessary. > > If it goes to abort(), I think XLU__PCI_ERR() should be good. > >> >>> >>>> >>>>> + tok); >>>>> + } >>>>> }else{ >>>>> XLU__PCI_ERR(cfg, "Unknown PCI BDF option: >>>>> %s", optkey); >>>>> } >>>>> @@ -167,6 +180,71 @@ parse_error: >>>>> return ERROR_INVAL; >>>>> } >>>>> >>>>> +int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const >>>>> char *str) >>>>> +{ >>>>> + unsigned state = STATE_TYPE; >>>>> + char *buf2, *tok, *ptr, *end; >>>>> + >>>>> + if ( NULL == (buf2 = ptr = strdup(str)) ) >>>>> + return ERROR_NOMEM; >>>>> + >>>>> + for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { >>>>> + switch(state) { >> >> Coding style. I haven't checked what actual style this file uses, but >> there is inconsistency in this function by itself. > > I just refer to xlu_pci_parse_bdf() to generate xlu_rdm_parse(), and > they are in the same file... > > Anyway, I should change this line, > > for ( tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++ ) { > >> >>>>> + case STATE_TYPE: >>>>> + if ( *ptr == '\0' || *ptr == ',' ) { >>>>> + state = STATE_CHECK_FLAG; >>>>> + *ptr = '\0'; >>>>> + if ( !strcmp(tok, "host") ) { >>>>> + rdm->type = LIBXL_RDM_RESERVE_TYPE_HOST; >>>>> + } else { >>>>> + XLU__PCI_ERR(cfg, "Unknown RDM state option: >>>>> %s", tok); >>>>> + goto parse_error; >>>>> + } >>>>> + tok = ptr + 1; >>>>> + } >>>>> + break; >>>>> + case STATE_CHECK_FLAG: >>>>> + if ( *ptr == '=' ) { >>>>> + state = STATE_OPTIONS_V; >>>>> + *ptr = '\0'; >>>>> + if ( strcmp(tok, "reserve") ) { >>>>> + XLU__PCI_ERR(cfg, "Unknown RDM property value: >>>>> %s", tok); >>>>> + goto parse_error; >>>>> + } >>>>> + tok = ptr + 1; >>>>> + } >>>>> + break; >>>>> + case STATE_OPTIONS_V: >>>>> + if ( *ptr == ',' || *ptr == '\0' ) { >>>>> + state = STATE_TERMINAL; >>>>> + *ptr = '\0'; >>>>> + if ( !strcmp(tok, "force") ) { >>>>> + rdm->reserve = LIBXL_RDM_RESERVE_FLAG_FORCE; >>>>> + } else if ( !strcmp(tok, "try") ) { >>>>> + rdm->reserve = LIBXL_RDM_RESERVE_FLAG_TRY; >>>>> + } else { >>>>> + XLU__PCI_ERR(cfg, "Unknown RDM property flag >>>>> value: %s", >>>>> + tok); >>>>> + goto parse_error; >>>>> + } >>>>> + tok = ptr + 1; >>>>> + } >>>>> + default: >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + free(buf2); >>>>> + >>>>> + if ( tok != ptr || state != STATE_TERMINAL ) >>>>> + goto parse_error; >>>>> + >>>>> + return 0; >>>>> + >>>>> +parse_error: >>>>> + return ERROR_INVAL; >>>>> +} >>>>> + >>>>> /* >>>>> * Local variables: >>>>> * mode: C >>>>> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h >>>>> index 0333e55..80497f8 100644 >>>>> --- a/tools/libxl/libxlutil.h >>>>> +++ b/tools/libxl/libxlutil.h >>>>> @@ -93,6 +93,10 @@ int xlu_disk_parse(XLU_Config *cfg, int nspecs, >>>>> const char *const *specs, >>>>> */ >>>>> int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, >>>>> const char *str); >>>>> >>>>> +/* >>>>> + * RDM parsing >>>>> + */ >>>>> +int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const >>>>> char *str); >>>>> >>>>> /* >>>>> * Vif rate parsing. >>>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >>>>> index 04faf98..9a58464 100644 >>>>> --- a/tools/libxl/xl_cmdimpl.c >>>>> +++ b/tools/libxl/xl_cmdimpl.c >>>>> @@ -988,7 +988,7 @@ static void parse_config_data(const char >>>>> *config_source, >>>>> const char *buf; >>>>> long l; >>>>> XLU_Config *config; >>>>> - XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, >>>>> *vtpms; >>>>> + XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, >>>>> *vtpms, *rdms; >>>>> XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian; >>>>> int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian; >>>>> int pci_power_mgmt = 0; >>>>> @@ -1727,6 +1727,23 @@ skip_vfb: >>>>> xlu_cfg_get_defbool(config, "e820_host", >>>>> &b_info->u.pv.e820_host, 0); >>>>> } >>>>> >>>>> + /* >>>>> + * By default our global policy is to query all rdm entries, and >>>>> + * force reserve them. >>>>> + */ >>>>> + b_info->rdm.type = LIBXL_RDM_RESERVE_TYPE_HOST; >>>>> + b_info->rdm.reserve = LIBXL_RDM_RESERVE_FLAG_TRY; >>>> >>>> This should probably to into the _setdefault function of >>>> libxl_domain_build_info. >>> >>> Sorry, I just see this >>> >>> libxl_domain_build_info_init() >>> | >>> + libxl_rdm_reserve_init(&p->rdm); >>> | >>> + memset(p, '\0', sizeof(*p)); >>> >>> But this should be generated automatically, right? So how to >>> implement your >>> idea? Could you give me a show? >>> >> >> Check libxl_domain_build_info_setdefault. >> >> To use libxl types. You normally do: >> >> libxl_TYPE_init >> libxl_TYPE_setdefault >> >> DO STUFF >> >> libxl_TYPE_dispose >> >> _init and _dispose are auto-generated. _setdefault is not. > > So in our case, maybe we can do this, > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index f0da7dc..461606c 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -100,6 +100,17 @@ static int sched_params_valid(libxl__gc *gc, > return 1; > } > > +void libxl__device_rdm_setdefault(libxl__gc *gc, > + libxl_domain_build_info *b_info) > +{ > + /* > + * By default our global policy is to query all rdm entries, and > + * force reserve them. > + */ > + b_info->rdm.type = LIBXL_RDM_RESERVE_TYPE_HOST; > + b_info->rdm.reserve = LIBXL_RDM_RESERVE_FLAG_STRICT; > +} > + > int libxl__domain_build_info_setdefault(libxl__gc *gc, > libxl_domain_build_info *b_info) > { > @@ -410,6 +421,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > libxl_domain_type_to_string(b_info->type)); > return ERROR_INVAL; > } > + > + libxl__device_rdm_setdefault(gc, b_info); > return 0; > } > > >> >>>> >>>>> + if (!xlu_cfg_get_list (config, "rdm", &rdms, 0, 0)) { >>>>> + if ((buf = xlu_cfg_get_listitem (rdms, 0)) != NULL ) { >>>>> + libxl_rdm_reserve rdm; >>>>> + if (!xlu_rdm_parse(config, &rdm, buf)) >>>>> + { >>>>> + b_info->rdm.type = rdm.type; >>>>> + b_info->rdm.reserve = rdm.reserve; >>>>> + } >>>> >>>> You only have one rdm in b_info, so there is no need to use a list for >>>> it in config file. >>>> >>> >>> Is this fine? >>> >>> if (!xlu_cfg_get_string(config, "rdm", &buf, 0)) { >>> >>> libxl_rdm_reserve rdm; >>> >>> if (!xlu_rdm_parse(config, &rdm, buf)) { >>> b_info->rdm.type = rdm.type; >>> >>> b_info->rdm.reserve = rdm.reserve; >>> >>> } >>> } >>> >> >> I think it is fine. But you'd better wait a little bit for other people >> to voice their opinions. >> > > Sure. > > Thanks > Tiejun > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >