[PATCH v2 0/4] tools: propagate bridge MTU to vif frontends

[PATCH v2 0/4] tools: propagate bridge MTU to vif frontends

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Paul Durrant (4):
  tools/hotplug: add remove_from_bridge() and improve debug output
  tools/hotplug: combine add/online and remove/offline in vif-bridge...
  public/io/netif: specify MTU override node
  tools/hotplug: modify set_mtu() to inform the frontend via xenstore

 docs/misc/xenstore-paths.pandoc           |  3 ++
 tools/hotplug/Linux/vif-bridge            | 20 +++------
 tools/hotplug/Linux/xen-network-common.sh | 51 +++++++++++++++++++----
 xen/include/public/io/netif.h             | 12 ++++++
 4 files changed, 63 insertions(+), 23 deletions(-)
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
-- 
2.20.1

[PATCH v2 1/4] tools/hotplug: add remove_from_bridge() and improve debug output

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This patch adds a remove_from_bridge() function into xen-network-common.sh
to partner with the existing add_to_bridge() function. The code in
add_to_bridge() is also slightly re-arranged to avoid duplication calls of
'ip link'.

Both add_to_bridge() and remove_from_bridge() will check if their bridge
manipulation operations are necessary and emit a log message if they are not.

NOTE: A call to remove_from_bridge() will be added by a subsequent patch.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 tools/hotplug/Linux/xen-network-common.sh | 37 ++++++++++++++++++-----
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 8dd3a62068..37e71cfa9c 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -126,19 +126,40 @@ add_to_bridge () {
     local bridge=$1
     local dev=$2
 
-    # Don't add $dev to $bridge if it's already on a bridge.
-    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
-	ip link set dev ${dev} up || true
-	return
-    fi
-    if which brctl >&/dev/null; then
-        brctl addif ${bridge} ${dev}
+    # Don't add $dev to $bridge if it's already on the bridge.
+    if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
+	log debug "adding $dev to bridge $bridge"
+	if which brctl >&/dev/null; then
+            brctl addif ${bridge} ${dev}
+	else
+            ip link set ${dev} master ${bridge}
+	fi
     else
-        ip link set ${dev} master ${bridge}
+	log debug "$dev already on bridge $bridge"
     fi
+
     ip link set dev ${dev} up
 }
 
+remove_from_bridge () {
+    local bridge=$1
+    local dev=$2
+
+    ip link set dev ${dev} down || :
+
+    # Don't remove $dev from $bridge if it's not on the bridge.
+    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
+	log debug "removing $dev from bridge $bridge"
+	if which brctl >&/dev/null; then
+            brctl delif ${bridge} ${dev}
+	else
+            ip link set ${dev} nomaster
+	fi
+    else
+	log debug "$dev not on bridge $bridge"
+    fi
+}
+
 # Usage: set_mtu bridge dev
 set_mtu () {
     local bridge=$1
-- 
2.20.1

Re: [PATCH v2 1/4] tools/hotplug: add remove_from_bridge() and improve debug output

[Ian Jackson]

Paul Durrant writes ("[PATCH v2 1/4] tools/hotplug: add remove_from_bridge() and improve debug output"):
> From: Paul Durrant <pdurrant@amazon.com>
> 
> This patch adds a remove_from_bridge() function into xen-network-common.sh
> to partner with the existing add_to_bridge() function. The code in
> add_to_bridge() is also slightly re-arranged to avoid duplication calls of
> 'ip link'.
> 
> Both add_to_bridge() and remove_from_bridge() will check if their bridge
> manipulation operations are necessary and emit a log message if they are not.
> 
> NOTE: A call to remove_from_bridge() will be added by a subsequent patch.

I think there is another semantic change here which is that now it
executes the "ip link set up" even if the device is already on the
bridge.

I think this is correct, but it probably ought to be mentioned in the
commit message.

I hesitate to suggest this, but: my personal preference would have
been to split that refactoring (in particular, the inversion of the
early exit if approach) into yet another commit.  I find tiny commits
easier to review.  But this commit is already quite small so if you
prefer to keep it this way I think that is fine.

> +remove_from_bridge () {
> +    local bridge=$1
> +    local dev=$2
> +
> +    ip link set dev ${dev} down || :
> +
> +    # Don't remove $dev from $bridge if it's not on the bridge.
> +    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
> +	log debug "removing $dev from bridge $bridge"
> +	if which brctl >&/dev/null; then
> +            brctl delif ${bridge} ${dev}
> +	else
> +            ip link set ${dev} nomaster
> +	fi
> +    else
> +	log debug "$dev not on bridge $bridge"
> +    fi
> +}

I think this is code motion split into two patches - here the added
code and in 2/, the other copy is removed.  Could you please shuffle
this addition into patch 2 ?

Thanks,
Ian.

[PATCH v2 2/4] tools/hotplug: combine add/online and remove/offline in vif-bridge...

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

... as they are in vif-route.

The script is invoked with online/offline for vifs and add/remove for taps.
The operations that are necessary, however, are the same in both cases. This
patch therefore combines the cases.

The open-coded bridge removal code is also replaced with call to
remove_from_bridge().

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 tools/hotplug/Linux/vif-bridge | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
index e722090ca8..e1d7c49788 100644
--- a/tools/hotplug/Linux/vif-bridge
+++ b/tools/hotplug/Linux/vif-bridge
@@ -77,25 +77,17 @@ then
 fi
 
 case "$command" in
+    add)
+        ;&
     online)
         setup_virtual_bridge_port "$dev"
         set_mtu "$bridge" "$dev"
         add_to_bridge "$bridge" "$dev"
         ;;
-
+    remove)
+        ;&
     offline)
-        if which brctl >&/dev/null; then
-            do_without_error brctl delif "$bridge" "$dev"
-        else
-            do_without_error ip link set "$dev" nomaster
-        fi
-        do_without_error ifconfig "$dev" down
-        ;;
-
-    add)
-        setup_virtual_bridge_port "$dev"
-        set_mtu "$bridge" "$dev"
-        add_to_bridge "$bridge" "$dev"
+        remove_from_bridge "$bridge" "$dev"
         ;;
 esac
 
-- 
2.20.1

Re: [PATCH v2 2/4] tools/hotplug: combine add/online and remove/offline in vif-bridge...

[Ian Jackson]

Paul Durrant writes ("[PATCH v2 2/4] tools/hotplug: combine add/online and remove/offline in vif-bridge..."):
> From: Paul Durrant <pdurrant@amazon.com>
> 
> ... as they are in vif-route.
> 
> The script is invoked with online/offline for vifs and add/remove for taps.
> The operations that are necessary, however, are the same in both cases. This
> patch therefore combines the cases.

This seems to newly add a "remove" case.  Previously "remove" was a
no-op here.  Is that right ?  If so it needs to be discussed in the
commit message.  We're not talking about a simple refactoring here!

Perhaps it would be best to move this bit

> +    remove)
> +        ;&
>      offline)

which I think is the relevant change, into its own commit ?

Ian.

[PATCH v2 3/4] public/io/netif: specify MTU override node

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

There is currently no documentation to state what MTU a frontend should
adertise to its network stack. It has however long been assumed that the
default value of 1500 is correct.

This patch specifies a mechanism to allow the tools to set the MTU via a
xenstore node in the frontend area and states that the absence of that node
means the frontend should assume an MTU of 1500 octets.

NOTE: The Windows PV frontend has used an MTU sampled from the xenstore
      node specified in this patch.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>

v2:
 - Add a note in xenstore-paths highlighting the new xenstore node
---
 docs/misc/xenstore-paths.pandoc |  3 +++
 xen/include/public/io/netif.h   | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/docs/misc/xenstore-paths.pandoc b/docs/misc/xenstore-paths.pandoc
index 766e8008dc..5cd5c8a3b9 100644
--- a/docs/misc/xenstore-paths.pandoc
+++ b/docs/misc/xenstore-paths.pandoc
@@ -298,6 +298,9 @@ A virtual keyboard device frontend. Described by
 A virtual network device frontend. Described by
 [xen/include/public/io/netif.h][NETIF]
 
+NOTE: ~/device/vif/$DEVID/mtu can be used to inform the frontend of an
+      increased MTU. (The default MTU is 1500 octets).
+
 #### ~/device/vscsi/$DEVID/* []
 
 A virtual scsi device frontend. Described by
diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 9fcf91a2fe..00dd258712 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -204,6 +204,18 @@
  * present).
  */
 
+/*
+ * MTU
+ * ===
+ *
+ * The toolstack may set a value of MTU for the frontend by setting the
+ * /local/domain/<domid>/device/vif/<vif>/mtu node with the MTU value in
+ * octets. If this node is absent the frontend should assume an MTU value
+ * of 1500 octets. A frontend is also at liberty to ignore this value so
+ * it is only suitable for informing the frontend that a packet payload
+ * >1500 octets is permitted.
+ */
+
 /*
  * Hash types
  * ==========
-- 
2.20.1

Re: [PATCH v2 3/4] public/io/netif: specify MTU override node

[Ian Jackson]

Paul Durrant writes ("[PATCH v2 3/4] public/io/netif: specify MTU override node"):
> From: Paul Durrant <pdurrant@amazon.com>
> 
> There is currently no documentation to state what MTU a frontend should
> adertise to its network stack. It has however long been assumed that the
> default value of 1500 is correct.
> 
> This patch specifies a mechanism to allow the tools to set the MTU via a
> xenstore node in the frontend area and states that the absence of that node
> means the frontend should assume an MTU of 1500 octets.
> 
> NOTE: The Windows PV frontend has used an MTU sampled from the xenstore
>       node specified in this patch.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

> +/*
> + * MTU
> + * ===
> + *
> + * The toolstack may set a value of MTU for the frontend by setting the
> + * /local/domain/<domid>/device/vif/<vif>/mtu node with the MTU value in
> + * octets. If this node is absent the frontend should assume an MTU value
> + * of 1500 octets. A frontend is also at liberty to ignore this value so
> + * it is only suitable for informing the frontend that a packet payload
> + * >1500 octets is permitted.
> + */

I find this wording a bit clumsy - a more formal treatment might be
better - but in my reading the overall semantics are correct.

Thanks,X
Ian.

[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

set_mtu() currently sets the backend vif MTU but does not inform the frontend
what it is. This patch adds code to write the MTU into a xenstore node. See
netif.h for a specification of the node.

NOTE: There is also a small modification replacing '$mtu' with '${mtu}'
      for style consistency.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 tools/hotplug/Linux/vif-bridge            |  2 +-
 tools/hotplug/Linux/xen-network-common.sh | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
index e1d7c49788..b99cc82a21 100644
--- a/tools/hotplug/Linux/vif-bridge
+++ b/tools/hotplug/Linux/vif-bridge
@@ -81,7 +81,7 @@ case "$command" in
         ;&
     online)
         setup_virtual_bridge_port "$dev"
-        set_mtu "$bridge" "$dev"
+        set_mtu "$bridge" "$dev" "$type_if"
         add_to_bridge "$bridge" "$dev"
         ;;
     remove)
diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 37e71cfa9c..24fc42d9cf 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -164,9 +164,21 @@ remove_from_bridge () {
 set_mtu () {
     local bridge=$1
     local dev=$2
+    local type_if=$3
+
     mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
     if [ -n "$mtu" ] && [ "$mtu" -gt 0 ]
     then
-            ip link set dev ${dev} mtu $mtu || :
+            ip link set dev ${dev} mtu ${mtu} || :
+    fi
+
+    if [ ${type_if} = vif ]
+    then
+       dev_=${dev#vif}
+       domid=${dev_%.*}
+       devid=${dev_#*.}
+
+       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
+       xenstore_write "$XENBUS_PATH/mtu" ${mtu}
     fi
 }
-- 
2.20.1

Re: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Ian Jackson]

Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> From: Paul Durrant <pdurrant@amazon.com>
> 
> set_mtu() currently sets the backend vif MTU but does not inform the frontend
> what it is. This patch adds code to write the MTU into a xenstore node. See
> netif.h for a specification of the node.
> 
> NOTE: There is also a small modification replacing '$mtu' with '${mtu}'
>       for style consistency.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

> diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
> index 37e71cfa9c..24fc42d9cf 100644
> --- a/tools/hotplug/Linux/xen-network-common.sh
> +++ b/tools/hotplug/Linux/xen-network-common.sh
> @@ -164,9 +164,21 @@ remove_from_bridge () {
>  set_mtu () {
>      local bridge=$1
>      local dev=$2
> +    local type_if=$3
> +
>      mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
>      if [ -n "$mtu" ] && [ "$mtu" -gt 0 ]
>      then
> -            ip link set dev ${dev} mtu $mtu || :
> +            ip link set dev ${dev} mtu ${mtu} || :
> +    fi
> +
> +    if [ ${type_if} = vif ]
> +    then
> +       dev_=${dev#vif}
> +       domid=${dev_%.*}
> +       devid=${dev_#*.}
> +
> +       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
> +       xenstore_write "$XENBUS_PATH/mtu" ${mtu}

It's surprising to me that this code doesn't have the xenbus path
already in some variable.  But I guess from the fact that you've added
this code, that it doesn't.

Ian.

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 04 August 2020 12:14
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> 
> Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > set_mtu() currently sets the backend vif MTU but does not inform the frontend
> > what it is. This patch adds code to write the MTU into a xenstore node. See
> > netif.h for a specification of the node.
> >
> > NOTE: There is also a small modification replacing '$mtu' with '${mtu}'
> >       for style consistency.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 

Thanks.

> > diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
> > index 37e71cfa9c..24fc42d9cf 100644
> > --- a/tools/hotplug/Linux/xen-network-common.sh
> > +++ b/tools/hotplug/Linux/xen-network-common.sh
> > @@ -164,9 +164,21 @@ remove_from_bridge () {
> >  set_mtu () {
> >      local bridge=$1
> >      local dev=$2
> > +    local type_if=$3
> > +
> >      mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
> >      if [ -n "$mtu" ] && [ "$mtu" -gt 0 ]
> >      then
> > -            ip link set dev ${dev} mtu $mtu || :
> > +            ip link set dev ${dev} mtu ${mtu} || :
> > +    fi
> > +
> > +    if [ ${type_if} = vif ]
> > +    then
> > +       dev_=${dev#vif}
> > +       domid=${dev_%.*}
> > +       devid=${dev_#*.}
> > +
> > +       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
> > +       xenstore_write "$XENBUS_PATH/mtu" ${mtu}
> 
> It's surprising to me that this code doesn't have the xenbus path
> already in some variable.  But I guess from the fact that you've added
> this code, that it doesn't.
> 

It is set, but set to the backend path. For safety I guess it's probably best if I use a local in this instance. Can I keep your R-b
with such a change?

  Paul

> Ian.

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Ian Jackson]

Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> > -----Original Message-----
> > From: Ian Jackson <ian.jackson@citrix.com>
> > Sent: 04 August 2020 12:14
> > To: Paul Durrant <paul@xen.org>
> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>
> > Subject: Re: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> > 
> > Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> > xenstore"):
> > > +       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
> > > +       xenstore_write "$XENBUS_PATH/mtu" ${mtu}
> > 
> > It's surprising to me that this code doesn't have the xenbus path
> > already in some variable.  But I guess from the fact that you've added
> > this code, that it doesn't.
> 
> It is set, but set to the backend path. For safety I guess it's probably best if I use a local in this instance. Can I keep your R-b
> with such a change?

Oh, wow.  I hadn't realised that.  I take back my earlier R-b :-).

Can you please use a different variable name for the frontend path ?

...

Actually.

This shouldn't be in the frontend at all, should it ?  In general the
backend writes to the backend and the frontend to the frontend.

So maybe I need to take back my R-b of
  [PATCH v2 3/4] public/io/netif: specify MTU override node

Sorry for the confusion.  I seem rather undercaffienated today.

Ian.

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 04 August 2020 12:35
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Wei Liu' <wl@xen.org>
> Subject: RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> 
> Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > > -----Original Message-----
> > > From: Ian Jackson <ian.jackson@citrix.com>
> > > Sent: 04 August 2020 12:14
> > > To: Paul Durrant <paul@xen.org>
> > > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>
> > > Subject: Re: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> > >
> > > Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> > > xenstore"):
> > > > +       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
> > > > +       xenstore_write "$XENBUS_PATH/mtu" ${mtu}
> > >
> > > It's surprising to me that this code doesn't have the xenbus path
> > > already in some variable.  But I guess from the fact that you've added
> > > this code, that it doesn't.
> >
> > It is set, but set to the backend path. For safety I guess it's probably best if I use a local in
> this instance. Can I keep your R-b
> > with such a change?
> 
> Oh, wow.  I hadn't realised that.  I take back my earlier R-b :-).
> 
> Can you please use a different variable name for the frontend path ?
> 

OK.

> ...
> 
> Actually.
> 
> This shouldn't be in the frontend at all, should it ?  In general the
> backend writes to the backend and the frontend to the frontend.
> 
> So maybe I need to take back my R-b of
>   [PATCH v2 3/4] public/io/netif: specify MTU override node
> 
> Sorry for the confusion.  I seem rather undercaffienated today.
> 

Too late. The xenstore node has been used by Windows frontends for the best part of a decade so we can't practically change the
path. Another way would be to also modify netback to simply echo the value from backend into frontend, but that seems rather
pointless.

Interestingly libxl does define an 'mtu' field for libxl_device_nic, which it sets to 1492 in libxl__device_nic_setdefault() but
never writes it into xenstore. There is even a comment:

/* nic->mtu = */

in libxl__nic_from_xenstore() which implies it should have been there, but isn't.
I still think picking up the MTU from the bridge is the better way though. 

  Paul

> Ian.

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Ian Jackson]

Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> > -----Original Message-----
> > From: Ian Jackson <ian.jackson@citrix.com>
...
> > Actually.
> > 
> > This shouldn't be in the frontend at all, should it ?  In general the
> > backend writes to the backend and the frontend to the frontend.
> > 
> > So maybe I need to take back my R-b of
> >   [PATCH v2 3/4] public/io/netif: specify MTU override node
> > 
> > Sorry for the confusion.  I seem rather undercaffienated today.
> 
> Too late. The xenstore node has been used by Windows frontends for the best part of a decade so we can't practically change the
> path. Another way would be to also modify netback to simply echo the value from backend into frontend, but that seems rather
> pointless.

Hmm.  How does this interact with driver domains ?  I think a driver
domain might not have write access to this node.

Is there a value we can store in it that won't break these Windows
frontends, that libxl in the toolstack domain could write, before the
hotplug script runs in the driver domain ?

> Interestingly libxl does define an 'mtu' field for libxl_device_nic, which it sets to 1492 in libxl__device_nic_setdefault() but
> never writes it into xenstore. There is even a comment:
> 
> /* nic->mtu = */
> 
> in libxl__nic_from_xenstore() which implies it should have been there, but isn't.
> I still think picking up the MTU from the bridge is the better way though. 

I agree that the default should come from the bridge.  Ideally there
would be a way to override it in the config.

Thanks,
Ian.

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Durrant, Paul]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 05 August 2020 10:31
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; 'Wei Liu' <wl@xen.org>
> Subject: RE: [EXTERNAL] [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > > -----Original Message-----
> > > From: Ian Jackson <ian.jackson@citrix.com>
> ...
> > > Actually.
> > >
> > > This shouldn't be in the frontend at all, should it ?  In general the
> > > backend writes to the backend and the frontend to the frontend.
> > >
> > > So maybe I need to take back my R-b of
> > >   [PATCH v2 3/4] public/io/netif: specify MTU override node
> > >
> > > Sorry for the confusion.  I seem rather undercaffienated today.
> >
> > Too late. The xenstore node has been used by Windows frontends for the best part of a decade so we
> can't practically change the
> > path. Another way would be to also modify netback to simply echo the value from backend into
> frontend, but that seems rather
> > pointless.
> 
> Hmm.  How does this interact with driver domains ?  I think a driver
> domain might not have write access to this node.
> 

That's a good point; I think we will also need to actually write it from libxl first in that case.

> Is there a value we can store in it that won't break these Windows
> frontends, that libxl in the toolstack domain could write, before the
> hotplug script runs in the driver domain ?
> 
> > Interestingly libxl does define an 'mtu' field for libxl_device_nic, which it sets to 1492 in
> libxl__device_nic_setdefault() but
> > never writes it into xenstore. There is even a comment:
> >
> > /* nic->mtu = */
> >
> > in libxl__nic_from_xenstore() which implies it should have been there, but isn't.
> > I still think picking up the MTU from the bridge is the better way though.
> 
> I agree that the default should come from the bridge.  Ideally there
> would be a way to override it in the config.
> 

Well, I guess we address the driver domain issue in this way too... I will add a patch to libxl to write the libxl_device_nic mtu value into xenstore, in both backend (where it should always have been) and frontend. I think the current setting of 1492 can be changed to 1500 safely (since nothing appears to currently use that value). The hotplug script should then have sufficient access to update, and a subsequent patch can add a mechanism to set the value from the config.

  Paul

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Ian Jackson]

Durrant, Paul writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> > -----Original Message-----
> > From: Ian Jackson <ian.jackson@citrix.com>
...
> Well, I guess we address the driver domain issue in this way
> too... I will add a patch to libxl to write the libxl_device_nic mtu
> value into xenstore,

Do you mean libxl in dom0 or libxl in the driver domain ?

libxl contains code that runs in both contexts.

See device_hotplug in libxl_device.c, in particular notice
    if (aodev->dev->backend_domid != domid) {

If you want the mtu to be read from the bridge, it can only be
determined by the driver domain, because the bridge is in the driver
domain.

In v2 of this series you arrange for the hotplug script to copy the
mtu from the bridge into the frontend path.  That won't work because
the hotplug script can't write to that xenstore node because (unlike a
domo0 backend) a driver domain backend doesn't have write access to
the frontend so can't create a new node there.

ISTM that it is correct that it is the hotplug script that does this
interface setup.  If it weren't for this erroneous use of the frontend
path I think the right design would be:
  * toolstack libxl reads the config file to find whether there is an MTU
  * toolstack libxl writes mtu node in backend iff one was in config
    (and leaves the node absent otherwise)
  * driver domain libxl runs hotplug script
  * driver domain hotplug script looks for mtu in backend; if there
    isn't one, it gets the value from the bridge and writes it to
    the backend in xenstore
  * driver domain backend driver reads mtu value from backend path
  * guest domain frontend driver reads mtu value from backend path
  * on domain save/migrate, toolstack libxl will record the mtu
    value as the actual configuration so that the migrated domain
    will get the same mtu

I don't think I understand what (in these kind of terms) you are
proposing, in order to support the frontends that want to read the mtu
from the frontend.

>  I think the current setting of 1492 can be changed to 1500 safely
> (since nothing appears to currently use that value).

Right, that seems correct to me.

> The hotplug script should then have sufficient access to update, and
> a subsequent patch can add a mechanism to set the value from the
> config.

I think what I am missing is how this "subsequent patch" would work ?
Ie what design are we aiming for, that we are now implementing part
of ?

Ian.

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Durrant, Paul]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 05 August 2020 11:13
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: paul@xen.org; xen-devel@lists.xenproject.org; 'Wei Liu' <wl@xen.org>
> Subject: RE: [EXTERNAL] [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Durrant, Paul writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > > -----Original Message-----
> > > From: Ian Jackson <ian.jackson@citrix.com>
> ...
> > Well, I guess we address the driver domain issue in this way
> > too... I will add a patch to libxl to write the libxl_device_nic mtu
> > value into xenstore,
> 
> Do you mean libxl in dom0 or libxl in the driver domain ?
> 
> libxl contains code that runs in both contexts.
> 
> See device_hotplug in libxl_device.c, in particular notice
>     if (aodev->dev->backend_domid != domid) {
> 
> If you want the mtu to be read from the bridge, it can only be
> determined by the driver domain, because the bridge is in the driver
> domain.
> 
> In v2 of this series you arrange for the hotplug script to copy the
> mtu from the bridge into the frontend path.  That won't work because
> the hotplug script can't write to that xenstore node because (unlike a
> domo0 backend) a driver domain backend doesn't have write access to
> the frontend so can't create a new node there.
> 
> ISTM that it is correct that it is the hotplug script that does this
> interface setup.  If it weren't for this erroneous use of the frontend
> path I think the right design would be:
>   * toolstack libxl reads the config file to find whether there is an MTU
>   * toolstack libxl writes mtu node in backend iff one was in config
>     (and leaves the node absent otherwise)

This is where the 'subsequent patch' comes in (see the end of the email)...

>   * driver domain libxl runs hotplug script
>   * driver domain hotplug script looks for mtu in backend; if there
>     isn't one, it gets the value from the bridge and writes it to
>     the backend in xenstore
>   * driver domain backend driver reads mtu value from backend path
>   * guest domain frontend driver reads mtu value from backend path
>   * on domain save/migrate, toolstack libxl will record the mtu
>     value as the actual configuration so that the migrated domain
>     will get the same mtu
> 

That sounds right.

> I don't think I understand what (in these kind of terms) you are
> proposing, in order to support the frontends that want to read the mtu
> from the frontend.

We need some way for creating the frontend node such that the driver domain has write access. Presumably there is a suitable place in the toolstack instance of libxl to do this. This would mean we either need to write a sentinel 'invalid' value or write the default value. In the default case we could still leave the backend node absent so the hotplug script will still know whether or not a value was set in the cfg.

> 
> >  I think the current setting of 1492 can be changed to 1500 safely
> > (since nothing appears to currently use that value).
> 
> Right, that seems correct to me.
> 
> > The hotplug script should then have sufficient access to update, and
> > a subsequent patch can add a mechanism to set the value from the
> > config.
> 
> I think what I am missing is how this "subsequent patch" would work ?
> Ie what design are we aiming for, that we are now implementing part
> of ?

The subsequent patch would be one that actually acquires the mtu value from the vif config, and adds documentation to xl-network-configuration.5.pod, since this is currently missing.

  Paul

> 
> Ian.