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: Wed, 20 May 2015 13:27:56 +0800 Message-ID: <555C1B5C.7070401@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> <20150518191743.GJ9503@zion.uk.xensource.com> <555AAB11.3080105@intel.com> <20150519094203.GC9893@zion.uk.xensource.com> <555B1563.5000502@intel.com> <20150519110041.GB21998@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150519110041.GB21998@zion.uk.xensource.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 On 2015/5/19 19:00, Wei Liu wrote: > On Tue, May 19, 2015 at 06:50:11PM +0800, Chen, Tiejun wrote: >> On 2015/5/19 17:42, Wei Liu wrote: > > [...] > >>>>>>>>> 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") >>>>>> + >>>>>> >>>>> >>> >>> Yet another question about this feature. The current setup suggests that >>> we must choose a policy, either "strict" or "relaxed", i.e. there is no >>> way to disable this feature, as in there is no "none" policy to skip >>> checking rdm conflict. >>> >>> AIUI this feature is more like a bug fix to existing problem, so we >>> always want to enable it. And the default relaxed policy makes sure we >>> don't break guest that was working before this feature. Do I understand >>> this correctly? >>> >>> If we risk breaking existing guests, we might want to consider adding a >>> "none" (name subject to improvement) policy to just skip RDM all >>> together. >> >> We have this definition, >> >> +libxl_rdm_reserve_type = Enumeration("rdm_reserve_type", [ >> + (0, "none"), >> + (1, "host"), >> + ]) >> >> If we set 'type=none', this means we would do nothing actually since we >> don't expose any rdms to guest. This behavior will ensue we go back the >> existing scenario without our patch. >> > > But this only works with global configuration and individual > configuration in PCI spec trumps this, right? You're right. > > Think about how an old configuration migrated to newer version of Xen > should work. For example, I don't have rdm= but have pci=['xxxx']. Do we > need to make sure this still work? I guess the answer is if it already Definitely. > works before RDM it should continue to work as there is really no > conflict before. In this case whether we enable RDM or not doesn't make > a difference to a guest that's already working before. Am I right? I think we can set the default 'type' to NONE, libxl__rdm_setdefault() { b_info->rdm.type = LIBXL_RDM_RESERVE_TYPE_NONE; and then, libxl__domain_device_construct_rdm() { ... /* Might not expose rdm. */ if (type == LIBXL_RDM_RESERVE_TYPE_NONE) return 0; This means we don't expose any rdm so we would never concern any policy anymore. Thanks Tiejun > >>> >>>>> Yes, and then don't forget to set the value to appropriate value in the >>>>> _setdefault functions for different types. >>>> > > [...] > >>> Spaces. >>> >>>>>>>>>> + if ( NULL == (buf2 = ptr = strdup(str)) ) >>>>>>>>>> + return ERROR_NOMEM; >>>>>>>>>> + >>>>>>>>>> + for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { >>>>>>>>>> + switch(state) { >>>> >>>> I thought initially you let me to follow that previous "if" :) >>>> >>> >>> Just be consistent with other part of the source code. >> >> I just refer to that existing xlu_pci_parse_bdf()... >> > > Sorry I didn't mean to blame you for something that's not your fault. > >> Anyway I guess you mean I should do something like this, >> >> if (NULL == (buf2 = ptr = strdup(str))) >> return ERROR_NOMEM; >> >> for (tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { >> switch(state) { >> case STATE_TYPE: >> if (*ptr == '=') { >> ... >> > > Fair enough. I prefer consistency. > > Wei. > >> Thanks >> Tiejun >> > >