From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [RFC][PATCH 01/13] tools: introduce some new
 parameters to set rdm policy
Date: Tue, 19 May 2015 18:50:11 +0800
Message-ID: <555B1563.5000502@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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20150519094203.GC9893@zion.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Wei Liu <wei.liu2@citrix.com>
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 17:42, Wei Liu wrote:
> On Tue, May 19, 2015 at 11:16:33AM +0800, Chen, Tiejun wrote:
>>
>> On 2015/5/19 3:17, Wei Liu wrote:
>
> [...]
>
>>>>      rdm = [ 'host, reserve=<POLICY>' ]
>>>>
>>>
>>> OK, so this is a specific example in vtd.txt. Last time I misread it as
>>> part of the manpage.
>>>
>>> I think you meant in this specific example (with other suggestions
>>> incorporated):
>>>
>>>       rdm = "type=host, reserve=force"
>>>
>>> Then you point user to xl.cfg manpage.
>>>
>>>> Or just for a specific device:
>>>>
>>>>      pci = [ '01:00.0,rdm_reserve=<POLICY>', '03:00.0' ]
>>>>
>>>
>>> Same here.
>>>
>>
>> In order to be compatible with vtd.txt, could this work for you?
>>
>> To enable this globally, add "rdm" in the config file:
>>
>>      rdm = [ 'type=host, reserve=<policy>' ]     (default policy is
>> "relaxed")
>>
>> Or just for a specific device:
>>
>>      pci = [ '01:00.0,rdm_reserve=<policy>' ]    (default policy is "strict")
>>
>
> In my last reply I meant to ask you to have specific example in vtd.txt
> and point user to xl.cfg manpage (see below). Previously I thought the
> examples you proposed were for manpages, that's why I had asked you to
> "list all values of that option". But now this is in vtd.txt, I would
> just write something like:
>
>
>      rdm = "type=host, reserve=relaxed"   (default policy is "relaxed")
>
>      pci = [ '01:00.0,rdm_reserve=try', '03:00.0,reserve=strict' ]
>
>      For all the options available to RDM, see xl.cfg(5).
>
> And of course you would need to list all values of these options in
> xl.cfg(5), as I suggested in my first reply to this patch.

This really make me clear. Sorry for my previous misunderstanding.

>
>>
>>> Just don't write "force/try" or "strcit/relax" because that's not the
>>> exact syntax you would use in a real config file.
>>
>> Yeah.
>>
>>>
>
> [...]
>
>>>>>>> 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.

>
>>> Yes, and then don't forget to set the value to appropriate value in the
>>> _setdefault functions for different types.
>>
>> Currently "type" is not associated to "policy" so we can do this if
>> necessary in the future.
>>
>>>
>>>>
>>>>> 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?
>>>>>
>>>>>>>
>>>>>>>> +
>>>
>>> [...]
>>>
>>>>>>>>                       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();
>>>>
>>>
>>> No, not calling the "abort" function. I meant returning appropriate error
>>> value and let the caller handles this situation.
>>
>> Okay, just call "goto parse_error".
>>
>
> Yes, that would work.
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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++ ) {
>>>>
>>>
>>>    for (tok = ptr, end...)
>>>
>>>    switch (state) {
>>
>> But what is the difference to compare the initial code?
>>
>
> 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()...

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 == '=') {
	    ...

Thanks
Tiejun

>
>>>
>>>
>>>>>
>>>>>>>> +        case STATE_TYPE:
>>>>>>>> +            if ( *ptr == '\0' || *ptr == ',' ) {
>>>>>>>> +                state = STATE_CHECK_FLAG;
>>>>>>>> +                *ptr = '\0';
>>>
>
> [...]
>
>>>                   [("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),
>>>       ])
>>>
>>> Do you actually need libxl_rdm_reserve type? I.e. do you envisage that
>>> structure to change a lot? Can you not just use libxl_rdm_reserve_type
>>> and libxl_rdm_reserve_flag in build_info.
>>>
>>
>> We'd like to introduce this type, libxl_rdm_reserve, to combine "type" and
>> "flag". From my point of view, this sole structure can represent a holistic
>> approach to rdm because,
>>
>> #1. Obviously its easy to get all;
>> #2. It will probably be extended since like this name, rdm, reserved device
>> memory, this should not be restricted to RMRR currently. So I just feel its
>> flexible to support others in the future, or much more properties :)
>>
>
> Fair enough.
>
> Wei.
>
>> Thanks
>> Tiejun
>
>