Re: [Xen-devel] [v10][PATCH 16/16] tools: parse to enable new rdm policy parameters

2015-07-20 Thread Chen, Tiejun

For clarity:

I am not acking this patch, primarily because I am not happy with the
code in xlu_rdm_parse which is (a) the result of repeated
clone-and-hack and (b) consists of ad-hoc string pointer fiddling.


Yes, I knew you mentioned this previously but I also remember our last 
deal was something as follows:


">>> Really I would prefer that this parsing was done with a miniature flex
>>> parser, rather than ad-hoc pointer arithmetic and use of strtok.
>>
>> Sorry, could you show this explicitly?
>
> Something like what was done for disk devices.  See libxlu_disk_l.l
> for an example.  In this case your code would be a lot less
> complicated than what you see there.
>
> After the codefreeze I would probably have some time to write it for

Sounds yourself would do this so currently I just keep the original, right?

Thanks
Tiejun

> you.  (I think that would be valuable because libxlu_disk_l.l is a
> very complicated example, and I want be able to point future
> submitters at something simpler.)
>
> Ian."

Then I didn't receive any response again so I thought yourself made this 
promise.


Thanks
Tiejun



If I had been able to review this patch earlier in the release cycle I
would be explictly nacking this patch.  It is true that maybe someone
will have some time to clean this up later; but in practice it often
turns out that they don't - which is why we usually try not to accept
patches on the basis of promises to do further cleanup.

However, I am late to this party.  I first made this complaint in
response to v7 on the 9th of July.  Under the circumstances I am going
to stand aside and neither ack nor nack this patch.

The rules then say that the patch may be committed given that it has
Wei's ack as a tools maintainer.

Ian.



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


Re: [Xen-devel] [v10][PATCH 16/16] tools: parse to enable new rdm policy parameters

2015-07-20 Thread Ian Jackson
Tiejun Chen writes ("[v10][PATCH 16/16] tools: parse to enable new rdm policy 
parameters"):
> This patch parses to enable user configurable parameters to specify
> RDM resource and according policies which are defined previously,
> 
> Global RDM parameter:
> rdm = "strategy=host,policy=strict/relaxed"
> Per-device RDM parameter:
> pci = [ 'sbdf, rdm_policy=strict/relaxed' ]
> 
> Default per-device RDM policy is same as default global RDM policy as being
> 'relaxed'. And the per-device policy would override the global policy like
> others.
> 
> CC: Ian Jackson 
> CC: Stefano Stabellini 
> CC: Ian Campbell 
> CC: Wei Liu 
> Acked-by: Wei Liu 

For clarity:

I am not acking this patch, primarily because I am not happy with the
code in xlu_rdm_parse which is (a) the result of repeated
clone-and-hack and (b) consists of ad-hoc string pointer fiddling.

If I had been able to review this patch earlier in the release cycle I
would be explictly nacking this patch.  It is true that maybe someone
will have some time to clean this up later; but in practice it often
turns out that they don't - which is why we usually try not to accept
patches on the basis of promises to do further cleanup.

However, I am late to this party.  I first made this complaint in
response to v7 on the 9th of July.  Under the circumstances I am going
to stand aside and neither ack nor nack this patch.

The rules then say that the patch may be committed given that it has
Wei's ack as a tools maintainer.

Ian.

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


[Xen-devel] [v10][PATCH 16/16] tools: parse to enable new rdm policy parameters

2015-07-19 Thread Tiejun Chen
This patch parses to enable user configurable parameters to specify
RDM resource and according policies which are defined previously,

Global RDM parameter:
rdm = "strategy=host,policy=strict/relaxed"
Per-device RDM parameter:
pci = [ 'sbdf, rdm_policy=strict/relaxed' ]

Default per-device RDM policy is same as default global RDM policy as being
'relaxed'. And the per-device policy would override the global policy like
others.

CC: Ian Jackson 
CC: Stefano Stabellini 
CC: Ian Campbell 
CC: Wei Liu 
Acked-by: Wei Liu 
Signed-off-by: Tiejun Chen 
---
v9 ~ v10:

* Nothing is changed.

v8:

* Clean some codes style issues.

v7:

* Just sync with the fallout of renaming parameters from patch #10.

v6:

* Just sync those renames introduced by patch #10.

v5:

* Need a rebase after we make all rdm variables specific to .hvm.
* Like other pci option, the per-device policy always follows
  the global policy by default.

v4:

* Separated from current patch #11 to parse/enable our rdm policy parameters
  since its make a lot sense and these stuffs are specific to xl/libxlu.

 tools/libxl/libxlu_pci.c | 92 +++-
 tools/libxl/libxlutil.h  |  4 +++
 tools/libxl/xl_cmdimpl.c | 13 +++
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxlu_pci.c b/tools/libxl/libxlu_pci.c
index 26fb143..026413b 100644
--- a/tools/libxl/libxlu_pci.c
+++ b/tools/libxl/libxlu_pci.c
@@ -42,6 +42,9 @@ 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_RDM_STRATEGY  10
+#define STATE_RESERVE_POLICY11
 int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char 
*str)
 {
 unsigned state = STATE_DOMAIN;
@@ -143,7 +146,18 @@ 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{
+} else if (!strcmp(optkey, "rdm_policy")) {
+if (!strcmp(tok, "strict")) {
+pcidev->rdm_policy = LIBXL_RDM_RESERVE_POLICY_STRICT;
+} else if (!strcmp(tok, "relaxed")) {
+pcidev->rdm_policy = LIBXL_RDM_RESERVE_POLICY_RELAXED;
+} else {
+XLU__PCI_ERR(cfg, "%s is not an valid PCI RDM property"
+  " policy: 'strict' or 'relaxed'.",
+ tok);
+goto parse_error;
+}
+} else {
 XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", optkey);
 }
 tok = ptr + 1;
@@ -167,6 +181,82 @@ 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) {
+case STATE_TYPE:
+if (*ptr == '=') {
+state = STATE_RDM_STRATEGY;
+*ptr = '\0';
+if (strcmp(tok, "strategy")) {
+XLU__PCI_ERR(cfg, "Unknown RDM state option: %s", tok);
+goto parse_error;
+}
+tok = ptr + 1;
+}
+break;
+case STATE_RDM_STRATEGY:
+if (*ptr == '\0' || *ptr == ',') {
+state = STATE_RESERVE_POLICY;
+*ptr = '\0';
+if (!strcmp(tok, "host")) {
+rdm->strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST;
+} else {
+XLU__PCI_ERR(cfg, "Unknown RDM strategy option: %s", tok);
+goto parse_error;
+}
+tok = ptr + 1;
+}
+break;
+case STATE_RESERVE_POLICY:
+if (*ptr == '=') {
+state = STATE_OPTIONS_V;
+*ptr = '\0';
+if (strcmp(tok, "policy")) {
+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, "strict")) {
+rdm->policy = LIBXL_RDM_RESERVE_POLICY_STRICT;
+} else if (!strcmp(tok, "relaxed")) {
+rdm->policy = LIBXL_RDM_RESERVE_POLICY_RELAXED;
+} else {