[Qemu-devel] [patch qemu 0/3] rocker: prepare for easy addition of other worlds

[Qemu-devel] [patch qemu 0/3] rocker: prepare for easy addition of other worlds

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

This patchset does couple of small changes in order to prepare for smooth
addition of other worlds, like P4 and BPF. qemu user will be able to request
desired rocker world by "world=worldname" property.

Jiri Pirko (3):
  rocker: forbid to change world type
  rocker: add name field into WorldOps ale let world specify its name
  rocker: allow user to specify rocker world by property

 hw/net/rocker/rocker.c        | 35 +++++++++++++++++++++++++++++++++--
 hw/net/rocker/rocker_fp.c     |  5 +++++
 hw/net/rocker/rocker_fp.h     |  1 +
 hw/net/rocker/rocker_of_dpa.c |  1 +
 hw/net/rocker/rocker_world.c  |  7 +------
 hw/net/rocker/rocker_world.h  |  1 +
 6 files changed, 42 insertions(+), 8 deletions(-)

-- 
2.4.3

[Qemu-devel] [patch qemu 1/3] rocker: forbid to change world type

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Port to world assignment should be permitted only by qemu user. Driver
should not be able to do it, so forbid that possibility.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 hw/net/rocker/rocker.c    | 8 +++++++-
 hw/net/rocker/rocker_fp.c | 5 +++++
 hw/net/rocker/rocker_fp.h | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index f3e994d..a1d921d 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -400,7 +400,13 @@ static int cmd_set_port_settings(Rocker *r,
 
     if (tlvs[ROCKER_TLV_CMD_PORT_SETTINGS_MODE]) {
         mode = rocker_tlv_get_u8(tlvs[ROCKER_TLV_CMD_PORT_SETTINGS_MODE]);
-        fp_port_set_world(fp_port, r->worlds[mode]);
+        if (mode >= ROCKER_WORLD_TYPE_MAX) {
+            return -ROCKER_EINVAL;
+        }
+        /* We don't support world change. */
+        if (!fp_port_check_world(fp_port, r->worlds[mode])) {
+            return -ROCKER_EINVAL;
+        }
     }
 
     if (tlvs[ROCKER_TLV_CMD_PORT_SETTINGS_LEARNING]) {
diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
index af37fef..0149899 100644
--- a/hw/net/rocker/rocker_fp.c
+++ b/hw/net/rocker/rocker_fp.c
@@ -186,6 +186,11 @@ void fp_port_set_world(FpPort *port, World *world)
     port->world = world;
 }
 
+bool fp_port_check_world(FpPort *port, World *world)
+{
+    return port->world == world;
+}
+
 bool fp_port_enabled(FpPort *port)
 {
     return port->enabled;
diff --git a/hw/net/rocker/rocker_fp.h b/hw/net/rocker/rocker_fp.h
index ab80fd8..04592bb 100644
--- a/hw/net/rocker/rocker_fp.h
+++ b/hw/net/rocker/rocker_fp.h
@@ -40,6 +40,7 @@ int fp_port_set_settings(FpPort *port, uint32_t speed,
 bool fp_port_from_pport(uint32_t pport, uint32_t *port);
 World *fp_port_get_world(FpPort *port);
 void fp_port_set_world(FpPort *port, World *world);
+bool fp_port_check_world(FpPort *port, World *world);
 bool fp_port_enabled(FpPort *port);
 void fp_port_enable(FpPort *port);
 void fp_port_disable(FpPort *port);
-- 
2.4.3

[Qemu-devel] [patch qemu 2/3] rocker: add name field into WorldOps ale let world specify its name

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Also use this in world_name getter function.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 hw/net/rocker/rocker_of_dpa.c | 1 +
 hw/net/rocker/rocker_world.c  | 7 +------
 hw/net/rocker/rocker_world.h  | 1 +
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
index da3fc54..0a134eb 100644
--- a/hw/net/rocker/rocker_of_dpa.c
+++ b/hw/net/rocker/rocker_of_dpa.c
@@ -2614,6 +2614,7 @@ RockerOfDpaGroupList *qmp_query_rocker_of_dpa_groups(const char *name,
 }
 
 static WorldOps of_dpa_ops = {
+    .name = "ofdpa",
     .init = of_dpa_init,
     .uninit = of_dpa_uninit,
     .ig = of_dpa_ig,
diff --git a/hw/net/rocker/rocker_world.c b/hw/net/rocker/rocker_world.c
index 1ed0fcd..89777e9 100644
--- a/hw/net/rocker/rocker_world.c
+++ b/hw/net/rocker/rocker_world.c
@@ -98,10 +98,5 @@ enum rocker_world_type world_type(World *world)
 
 const char *world_name(World *world)
 {
-    switch (world->type) {
-    case ROCKER_WORLD_TYPE_OF_DPA:
-        return "OF_DPA";
-    default:
-        return "unknown";
-    }
+    return world->ops->name;
 }
diff --git a/hw/net/rocker/rocker_world.h b/hw/net/rocker/rocker_world.h
index 18d277b..58ade47 100644
--- a/hw/net/rocker/rocker_world.h
+++ b/hw/net/rocker/rocker_world.h
@@ -33,6 +33,7 @@ typedef int (world_cmd)(World *world, DescInfo *info,
                         RockerTlv *cmd_info_tlv);
 
 typedef struct world_ops {
+    const char *name;
     world_init *init;
     world_uninit *uninit;
     world_ig *ig;
-- 
2.4.3

[Qemu-devel] [patch qemu 3/3] rocker: allow user to specify rocker world by property

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Add property to specify rocker world. All ports will be assigned to this
world.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 hw/net/rocker/rocker.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index a1d921d..d0bdfcb 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -43,6 +43,7 @@ struct rocker {
 
     /* switch configuration */
     char *name;                  /* switch name */
+    char *world_name;            /* world name */
     uint32_t fp_ports;           /* front-panel port count */
     NICPeers *fp_ports_peers;
     MACAddr fp_start_macaddr;    /* front-panel port 0 mac addr */
@@ -1286,6 +1287,18 @@ static void rocker_msix_uninit(Rocker *r)
     rocker_msix_vectors_unuse(r, ROCKER_MSIX_VEC_COUNT(r->fp_ports));
 }
 
+static World *rocker_world_type_by_name(Rocker *r, const char *name)
+{
+    int i;
+
+    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
+        if (strcmp(name, world_name(r->worlds[i])) == 0) {
+            return r->worlds[i];
+	}
+    }
+    return NULL;
+}
+
 static int pci_rocker_init(PCIDevice *dev)
 {
     Rocker *r = to_rocker(dev);
@@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
     /* allocate worlds */
 
     r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
-    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
+
+    if (!r->world_name) {
+        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
+    }
+
+    r->world_dflt = rocker_world_type_by_name(r, r->world_name);
+    if (!r->world_dflt) {
+        fprintf(stderr,
+                "rocker: requested world \"%s\" does not exist\n",
+                r->world_name);
+        return -EINVAL;
+    }
 
     for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
         if (!r->worlds[i]) {
@@ -1509,6 +1533,7 @@ static void rocker_reset(DeviceState *dev)
 
 static Property rocker_properties[] = {
     DEFINE_PROP_STRING("name", Rocker, name),
+    DEFINE_PROP_STRING("world", Rocker, world_name),
     DEFINE_PROP_MACADDR("fp_start_macaddr", Rocker,
                         fp_start_macaddr),
     DEFINE_PROP_UINT64("switch_id", Rocker,
-- 
2.4.3

Re: [Qemu-devel] [patch qemu 3/3] rocker: allow user to specify rocker world by property

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
>      /* allocate worlds */
>  
>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
> +
> +    if (!r->world_name) {
> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
> +    }
> +
> +    r->world_dflt = rocker_world_type_by_name(r, r->world_name);
> +    if (!r->world_dflt) {
> +        fprintf(stderr,
> +                "rocker: requested world \"%s\" does not exist\n",
> +                r->world_name);
> +        return -EINVAL;
> +    }

world_name is leaked here.  Please use goto to run the appropriate
cleanup code instead of returning directly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [patch qemu 3/3] rocker: allow user to specify rocker world by property

[Jiri Pirko]

Mon, Feb 22, 2016 at 06:51:50PM CET, stefanha@gmail.com wrote:
>On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
>> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
>>      /* allocate worlds */
>>  
>>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
>> +
>> +    if (!r->world_name) {
>> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>> +    }
>> +
>> +    r->world_dflt = rocker_world_type_by_name(r, r->world_name);
>> +    if (!r->world_dflt) {
>> +        fprintf(stderr,
>> +                "rocker: requested world \"%s\" does not exist\n",
>> +                r->world_name);
>> +        return -EINVAL;
>> +    }
>
>world_name is leaked here.  Please use goto to run the appropriate
>cleanup code instead of returning directly.

I did the same what is done with "r->name = g_strdup(ROCKER)"

I assumed since this is a property, the caller core will take care of
that.

Re: [Qemu-devel] [patch qemu 3/3] rocker: allow user to specify rocker world by property

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Mon, Feb 22, 2016 at 07:06:34PM +0100, Jiri Pirko wrote:
> Mon, Feb 22, 2016 at 06:51:50PM CET, stefanha@gmail.com wrote:
> >On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
> >> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
> >>      /* allocate worlds */
> >>  
> >>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
> >> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
> >> +
> >> +    if (!r->world_name) {
> >> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
> >> +    }
> >> +
> >> +    r->world_dflt = rocker_world_type_by_name(r, r->world_name);
> >> +    if (!r->world_dflt) {
> >> +        fprintf(stderr,
> >> +                "rocker: requested world \"%s\" does not exist\n",
> >> +                r->world_name);
> >> +        return -EINVAL;
> >> +    }
> >
> >world_name is leaked here.  Please use goto to run the appropriate
> >cleanup code instead of returning directly.
> 
> I did the same what is done with "r->name = g_strdup(ROCKER)"
> 
> I assumed since this is a property, the caller core will take care of
> that.

You are right, the string properties will be freed by qdev property
release.

The return statement added by this patch still leaks
r->worlds[ROCKER_WORLD_TYPE_OF_DPA] so goto err_world_alloc is needed.

(pci_rocker_uninit() is not called if pci_rocker_init() fails.)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [patch qemu 3/3] rocker: allow user to specify rocker world by property

[Jiri Pirko]

Thu, Feb 25, 2016 at 12:31:58PM CET, stefanha@gmail.com wrote:
>On Mon, Feb 22, 2016 at 07:06:34PM +0100, Jiri Pirko wrote:
>> Mon, Feb 22, 2016 at 06:51:50PM CET, stefanha@gmail.com wrote:
>> >On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
>> >> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
>> >>      /* allocate worlds */
>> >>  
>> >>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>> >> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
>> >> +
>> >> +    if (!r->world_name) {
>> >> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>> >> +    }
>> >> +
>> >> +    r->world_dflt = rocker_world_type_by_name(r, r->world_name);
>> >> +    if (!r->world_dflt) {
>> >> +        fprintf(stderr,
>> >> +                "rocker: requested world \"%s\" does not exist\n",
>> >> +                r->world_name);
>> >> +        return -EINVAL;
>> >> +    }
>> >
>> >world_name is leaked here.  Please use goto to run the appropriate
>> >cleanup code instead of returning directly.
>> 
>> I did the same what is done with "r->name = g_strdup(ROCKER)"
>> 
>> I assumed since this is a property, the caller core will take care of
>> that.
>
>You are right, the string properties will be freed by qdev property
>release.
>
>The return statement added by this patch still leaks
>r->worlds[ROCKER_WORLD_TYPE_OF_DPA] so goto err_world_alloc is needed.
>
>(pci_rocker_uninit() is not called if pci_rocker_init() fails.)

Will fix and send v2. Thanks.