[RFC PATCH v2 0/5] Add bridge VLAN support

[RFC PATCH v2 0/5] Add bridge VLAN support

[Leigh Brown]

Hello all,

I realised over the weekend that there is a valid use case for providing
a VIF to a domain that has access to multiple VLANs, e.g. a router. Yes,
you can create a VIF per VLAN, but if you start having several VLANs (as
I do), it would be nicer to create a single interface that has access to
all the relevant VLANs (e.g. enX0.10, enX0.20, etc.).

So, version 2 changes the name and type of the parameter from an integer
called `vid' to a string called `vlan'. The vlan parameter is then
parsed by the vif-bridge script (actually, the functions called by it in
xen-network-common.sh).

As it quite a common practice to allocate VLANs in round numbers, I also
implemented the ability to specify contiguous or non-contiguous ranges.
You can specify whether a VLAN is tagged or untagged, and which VLAN is
the PVID (only one PVID is allowed).  For example,

vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10p/20-29' ]

will setup the VIF so that 10 is the PVID and VLAN IDs 20 through 29
are permitted with tags. Another example:

vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=1p/10+10x9' ]

will setup the bridge to set 1 as the PVID and permit access with
tags for VLAN IDs 10, 20, 30, 40, 50, 60, 70, 80 and 90.

This patch set enables this capability as follows:

1. Adds `vlan' as a new member of the libxl_device_nic structure;
2. Adds support to read and write the vlan parameter from the xenstore;
3. Adds `vlan' as a new keyword for the vif configuration option;
4. Adds support to assign the bridge VLANs in the Linux hotplug scripts;
5. Updated xl-network-configuration(5) manpage and example configs.

Original blurb below:

For many years I have been configuring VLANs on my Linux Dom0 by
creating VLAN interfaces for each VLAN I wanted to connect a domain
to and then a corresponding bridge. So I would tend to have things
like:

enp0s0    -> br0     -> vif1, vif2
enp0s0.10 -> br0vl10 -> vif3, vif4
enp0s0.20 -> br0vl20 -> vif5
dummy0    -> br1     -> vif6

I recently discovered that iproute2 supports creating bridge VLANs that
allows you to assign a VLAN to each of the interfaces associated to a
bridge. This allows a greatly simplified configuration where a single
bridge can support all the domains, and the iproute2 bridge command can
assign each VIF to the required VLAN.  This looks like this:

# bridge vlan
port              vlan-id  
enp0s0            1 PVID Egress Untagged
                  10
                  20
br0               1 PVID Egress Untagged
vif1.0            1 PVID Egress Untagged
vif2.0            1 PVID Egress Untagged
vif3.0            10 PVID Egress Untagged
vif4.0            10 PVID Egress Untagged
vif5.0            20 PVID Egress Untagged
vif6.0            30 PVID Egress Untagged

This patch set enables this capability as follows:

1. Adds `vid' as a new member of the libxl_device_nic structure;
2. Adds support to read and write vid from the xenstore;
3. Adds `vid' as a new keyword for the vif configuration option;
4. Adds support for assign the bridge VLAN in the Linux hotplug scripts.

I don't believe NetBSD or FreeBSD support this capability, but if they
do please point me in the direction of some documentation and/or examples.

NB: I'm not very familiar with Xen code base so may have missed
something important, although I have tested it and it is working well
for me.

Cheers,

Leigh.


Leigh Brown (5):
  tools/libs/light: Add vlan field to libxl_device_nic
  tools/xl: add vlan keyword to vif option
  tools/hotplug/Linux: Add bridge VLAN support
  docs/man: document VIF vlan keyword
  tools/examples: Example Linux bridge VLAN config

 docs/man/xl-network-configuration.5.pod.in    |  38 ++++++
 tools/examples/linux-bridge-vlan/README       |  68 +++++++++++
 tools/examples/linux-bridge-vlan/br0.netdev   |   7 ++
 tools/examples/linux-bridge-vlan/br0.network  |   8 ++
 .../examples/linux-bridge-vlan/enp0s0.network |  16 +++
 tools/hotplug/Linux/xen-network-common.sh     | 111 ++++++++++++++++++
 tools/libs/light/libxl_nic.c                  |  10 ++
 tools/libs/light/libxl_types.idl              |   1 +
 tools/xl/xl_parse.c                           |   2 +
 9 files changed, 261 insertions(+)
 create mode 100644 tools/examples/linux-bridge-vlan/README
 create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev
 create mode 100644 tools/examples/linux-bridge-vlan/br0.network
 create mode 100644 tools/examples/linux-bridge-vlan/enp0s0.network

-- 
2.39.2

[RFC PATCH v2 1/5] tools/libs/light: Add vlan field to libxl_device_nic

[Leigh Brown]

Add `vlan' string field to libxl_device_nic, to allow a VLAN
configuration to be specified for the VIF when adding it to the
bridge device.

Update libxl_nic.c to read and write the vlan field from the
xenstore.

This provides the capability for supported operating systems (e.g.
Linux) to perform VLAN filtering on bridge ports.  The Xen
hotplug scripts need to be updated to read this information from
the xenstore and perform the required configuration.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 tools/libs/light/libxl_nic.c     | 10 ++++++++++
 tools/libs/light/libxl_types.idl |  1 +
 2 files changed, 11 insertions(+)

diff --git a/tools/libs/light/libxl_nic.c b/tools/libs/light/libxl_nic.c
index d6bf06fc34..d861e3726d 100644
--- a/tools/libs/light/libxl_nic.c
+++ b/tools/libs/light/libxl_nic.c
@@ -233,6 +233,11 @@ static int libxl__set_xenstore_nic(libxl__gc *gc, uint32_t domid,
         flexarray_append(back, GCSPRINTF("%u", nic->mtu));
     }
     
+    if (nic->vlan) {
+        flexarray_append(back, "vlan");
+        flexarray_append(back, libxl__strdup(gc, nic->vlan));
+    }
+
     flexarray_append(back, "bridge");
     flexarray_append(back, libxl__strdup(gc, nic->bridge));
     flexarray_append(back, "handle");
@@ -313,6 +318,11 @@ static int libxl__nic_from_xenstore(libxl__gc *gc, const char *libxl_path,
         nic->mtu = LIBXL_DEVICE_NIC_MTU_DEFAULT;
     }
 
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                GCSPRINTF("%s/vlan", libxl_path),
+				(const char **)(&nic->vlan));
+    if (rc) goto out;
+
     rc = libxl__xs_read_checked(gc, XBT_NULL,
                                 GCSPRINTF("%s/mac", libxl_path), &tmp);
     if (rc) goto out;
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 7d8bd5d216..5c510dc272 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -809,6 +809,7 @@ libxl_device_nic = Struct("device_nic", [
     ("backend_domname", string),
     ("devid", libxl_devid),
     ("mtu", integer),
+    ("vlan", string),
     ("model", string),
     ("mac", libxl_mac),
     ("ip", string),
-- 
2.39.2

[RFC PATCH v2 2/5] tools/xl: add vlan keyword to vif option

[Leigh Brown]

Update parse_nic_config() to support a new `vlan' keyword. This
keyword specifies the VLAN configuration to assign to the VIF when
attaching it to the bridge port, on operating systems that support
the capability (e.g. Linux). The vlan keyword will allow one or
more VLANs to be configured on the VIF when adding it to the bridge
port. This will be done by the vif-bridge script and functions.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 tools/xl/xl_parse.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index ed983200c3..7546fe7e7a 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -565,6 +565,8 @@ int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token)
         nic->devid = parse_ulong(oparg);
     } else if (MATCH_OPTION("mtu", token, oparg)) {
         nic->mtu = parse_ulong(oparg);
+    } else if (MATCH_OPTION("vlan", token, oparg)) {
+        replace_string(&nic->vlan, oparg);
     } else if (!strcmp("trusted", token)) {
         libxl_defbool_set(&nic->trusted, true);
     } else if (!strcmp("untrusted", token)) {
-- 
2.39.2

[RFC PATCH v2 3/5] tools/hotplug/Linux: Add bridge VLAN support

[Leigh Brown]

Update add_to_bridge shell function to read the vlan parameter
from xenstore and set the bridge VLAN configuration for the VID.

Add additional helper functions to parse the vlan specification,
which consists of one or more of the follow:

a) single VLAN (e.g. 10).
b) contiguous range of VLANs (e.g. 10-15).
c) discontiguous range with base, increment and count
   (e.g. 100+10x9 which gives VLAN IDs 100, 110, ... 190).

A single VLAN can be suffixed with "p" to indicate the PVID, or
"u" to indicate untagged. A range of VLANs can be suffixed with
"u" to indicate untagged.  A complex example would be:

   vlan=1p/10-15/20-25u

This capability only works when using the iproute2 bridge command,
so a warning is issued if the vlan parameter is set and the bridge
command is not available, as it will be ignored.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 tools/hotplug/Linux/xen-network-common.sh | 111 ++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 42fa704e8d..d9fb4f7355 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -121,10 +121,113 @@ create_bridge () {
     fi
 }
 
+_vif_vlan_add() {
+    # References vlans, pvid and error variables from the calling function
+    local -i vid=$1
+    local flag=${2:-}
+
+    if (( vid < 1 || vid > 4094 )) ;then
+        error="vlan id $vid not between 1 and 4094"
+        return
+    fi
+    if [[ -n "${vlans[$vid]}" ]] ;then
+        error="vlan id $vid specified more than once"
+        return
+    fi
+    case $flag in
+     p) if (( pvid != 0 )) ;then
+            error="more than one pvid specified ($vid and $pvid)"
+            return
+        fi
+        pvid=$vid
+        vlans[$vid]=p ;;
+     u) vlans[$vid]=u ;;
+     *) vlans[$vid]=t ;;
+    esac
+}
+
+_vif_vlan_parse_term() {
+    # References error variable from the calling function
+    local vid incr last term=${1:-}
+
+    if [[ $term =~ ^([0-9]+)([pu])?$ ]] ;then
+        _vif_vlan_add ${BASH_REMATCH[1]} ${BASH_REMATCH[2]}
+    elif [[ $term =~ ^([0-9]+)-([0-9]+)(u)?$ ]] ;then
+        vid=${BASH_REMATCH[1]}
+        last=${BASH_REMATCH[2]}
+        if (( last >= vid )) ;then
+            for (( ; vid<=last; vid++ )) ;do
+                _vif_vlan_add $vid ${BASH_REMATCH[3]}
+            done
+        else
+	    error="invalid vlan id range: $term"
+        fi
+    elif [[ $term =~ ^([0-9]+)\+([0-9]+)x([0-9]+)(u)?$ ]] ;then
+        vid=${BASH_REMATCH[1]}
+        incr=${BASH_REMATCH[2]}
+        for (( j=${BASH_REMATCH[3]}; j>0; --j, vid+=incr ))
+        do
+            _vif_vlan_add $vid ${BASH_REMATCH[4]}
+        done
+    else
+        error="invalid vlan specification: $term"
+    fi
+}
+
+_vif_vlan_validate_pvid() {
+    # References vlans and pvid variables from the calling function
+    if (( pvid == 0 )) ;then
+        if (( ${#vlans[@]} == 1 )) ;then
+            vlans[${!vlans[*]}]=p
+        else
+            error="pvid required for multiple vlan ids"
+        fi
+    fi
+}
+
+_vif_vlan_setup() {
+    # References vlans and dev variable from the calling function
+    local vid cmd
+
+    bridge vlan del dev "$dev" vid 1
+    for vid in ${!vlans[@]} ;do
+        cmd="bridge vlan add dev '$dev' vid $vid"
+        case ${vlans[$vid]} in
+             p) cmd="$cmd pvid untagged" ;;
+             u) cmd="$cmd untagged" ;;
+             t) ;;
+        esac
+        eval "$cmd"
+    done
+}
+
+_vif_vlan_membership() {
+    # The vlans, pvid, dev and error variables are used by sub-functions
+    local -A vlans=()
+    local -a terms=()
+    local -i i pvid=0
+    local dev=$1 error=""
+
+    # Split the vlan specification string into its terms
+    readarray -d / -t terms <<<$2
+    for (( i=0; i<${#terms[@]}; ++i )) ;do
+        _vif_vlan_parse_term ${terms[$i]%%[[:space:]]}
+        [[ -n "$error" ]] && break
+    done
+
+    [[ -z "$error" ]] && _vif_vlan_validate_pvid
+    [[ -z "$error" ]] && _vif_vlan_setup
+    [[ -z "$error" ]] && return 0
+
+    log error "$error"
+    return 1
+}
+
 # Usage: add_to_bridge bridge dev
 add_to_bridge () {
     local bridge=$1
     local dev=$2
+    local vlan=$(xenstore_read_default "$XENBUS_PATH/vlan" "")
 
     # Don't add $dev to $bridge if it's already on the bridge.
     if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
@@ -134,6 +237,14 @@ add_to_bridge () {
         else
             ip link set ${dev} master ${bridge}
         fi
+        if [ -n "${vlan}" ] ;then
+            if which bridge >&/dev/null; then
+                log debug "configuring VLANs for ${dev} on ${bridge}"
+                _vif_vlan_membership "${dev}" "${vlan}"
+            else
+                log warning "bridge command not available, ignoring vlan config"
+            fi
+        fi
     else
         log debug "$dev already on bridge $bridge"
     fi
-- 
2.39.2

[RFC PATCH v2 4/5] docs/man: document VIF vlan keyword

[Leigh Brown]

Document the new `vlan' keyword in xl-network-configuration(5).

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 docs/man/xl-network-configuration.5.pod.in | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/docs/man/xl-network-configuration.5.pod.in b/docs/man/xl-network-configuration.5.pod.in
index f3e379bcf8..c35c0922b3 100644
--- a/docs/man/xl-network-configuration.5.pod.in
+++ b/docs/man/xl-network-configuration.5.pod.in
@@ -259,6 +259,44 @@ Specifies the MTU (i.e. the maximum size of an IP payload, exclusing headers). T
 default value is 1500 but, if the VIF is attached to a bridge, it will be set to match
 unless overridden by this parameter.
 
+=head2 vlan
+
+Specifies the VLAN configuration. The format of this parameter is one or more
+VLAN IDs or ranges separated by forward slashes. Each term can be:
+
+=over
+
+=item *
+
+B<vlan> - a single VLAN ID in the range 1 to 4094. This can optionally followed
+by a B<p> to indicate the PVID or by a B<u> to indicate an untagged VLAN. C<p>
+implies B<u>.
+
+=item *
+
+B<vlan1>-B<vlan2> - a range of VLAN IDs from B<vlan1> to B<vlan2>, both between
+1 and 4094 and B<vlan1> being less than or equal to B<vlan2>. This can be
+optionally followed by a B<u> to indicate that the range of VLANs are untagged.
+
+=item *
+
+B<vlan>+B<offset>xB<count> - describing a range of VLAN IDs starting at B<vlan>
+with B<count> additional entries, each incremented by B<offset>. This can be 
+optionally followed by a B<u> to indicate that the range of VLANs are untagged.
+
+=back
+
+Note, one VLAN ID must be marked as the PVID. In the case of a vlan 
+specification consisting of a single VLAN ID (e.g. C<vlan=10>), the B<p> suffix
+may be omitted. Specifying more than one untagged VLAN ID is an advanced 
+configuration - use with caution.
+
+For example:
+
+        'vlan=10' -- meaning a single VLAN that is the PVID.
+	'vlan=10p/20' -- VLAN 10 is the PVID and VLAN 20 is tagged.
+	'vlan=10p/100+10x4' -- VLANs 10, 100, 110, 120, 130, 140, 150.
+
 =head2 trusted / untrusted
 
 An advisory setting for the frontend driver on whether the backend should be
-- 
2.39.2

[RFC PATCH v2 5/5] tools/examples: Example Linux bridge VLAN config

[Leigh Brown]

Add a new directory linux-bridge-vlan with examples files showing
how to configure systemd-networkd to support a bridge VLAN
configuration.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 tools/examples/linux-bridge-vlan/README       | 68 +++++++++++++++++++
 tools/examples/linux-bridge-vlan/br0.netdev   |  7 ++
 tools/examples/linux-bridge-vlan/br0.network  |  8 +++
 .../examples/linux-bridge-vlan/enp0s0.network | 16 +++++
 4 files changed, 99 insertions(+)
 create mode 100644 tools/examples/linux-bridge-vlan/README
 create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev
 create mode 100644 tools/examples/linux-bridge-vlan/br0.network
 create mode 100644 tools/examples/linux-bridge-vlan/enp0s0.network

diff --git a/tools/examples/linux-bridge-vlan/README b/tools/examples/linux-bridge-vlan/README
new file mode 100644
index 0000000000..83b9fa3fd6
--- /dev/null
+++ b/tools/examples/linux-bridge-vlan/README
@@ -0,0 +1,68 @@
+Linux Xen Dom0 single bridge multiple VLAN configuration with systemd
+=====================================================================
+
+Introduction
+------------
+
+This directory contains example files to be placed in /etc/systemd/network
+to enable a single bridge with multiple VLAN support.
+
+The example is to support the scenario where the Xen host network interface
+is connected to an Ethernet switch configured as a trunk port. Each domain
+VIF can then be configured with one or more VLAN IDs, one of which will be
+the PVID.
+
+The example files create a bridge device called br0, with a physical interface 
+called enp0s0. You will need to update this with your system's device name.
+
+Key points of the configuration are:
+
+1. In br0.netdev, VLANFiltering=on is set. This is required to ensure the
+   VLAN tags are handled correctly.  If it is not set then the packets
+   from the VIF interfaces will not have the correct VLAN tags set.
+
+2. In br0.network, a system IPv4 address is configured that can be updated
+   according to your local network settings.
+
+3. In enp0s0.network, Bridge=br0 sets the bridge device to connect to. There
+   is also a [BridgeVLAN] section for each VLAN you want to give access
+   to the switch. Note, if you want to create an internal VLAN private to
+   the host, do not include its VLAN ID in this file.
+
+
+Domain configuration
+--------------------
+
+Add the vlan= keyword to the vif definition in the domain. The simplest
+and most common example is a domain that wishes to connect to a single VLAN:
+
+vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10' ]
+
+If you wish to configure a domain to route between two VLANs, you have two
+options. Option 1 is to create multiple interfaces on different VLANs:
+
+vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10',
+	'max=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=20' ]
+
+Alternatively, you can create single interface:
+
+vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10p/20' ]
+
+In the domain, you would, for example, use enX0 for VLAN 10 and enX0.20 for 
+VLAN 20.
+
+
+Hints and tips
+--------------
+
+You can run the following commands on dom0 or a driver domain:
+
+1. To check if vlan_filtering is enabled:
+   # cat /sys/devices/virtual/net/<name>/bridge/vlan_filtering
+
+2. To check the bridge port VLAN assignments:
+   # bridge vlan
+
+3. To check the vlan setting in the xenstore (dom0 only):
+   # xenstore-ls -f | grep 'vlan ='
+
diff --git a/tools/examples/linux-bridge-vlan/br0.netdev b/tools/examples/linux-bridge-vlan/br0.netdev
new file mode 100644
index 0000000000..ae1fe487c3
--- /dev/null
+++ b/tools/examples/linux-bridge-vlan/br0.netdev
@@ -0,0 +1,7 @@
+[NetDev]
+Name=br0
+Kind=bridge
+MACAddress=xx:xx:xx:xx:xx:xx
+
+[Bridge]
+VLANFiltering=on
diff --git a/tools/examples/linux-bridge-vlan/br0.network b/tools/examples/linux-bridge-vlan/br0.network
new file mode 100644
index 0000000000..b56203b66a
--- /dev/null
+++ b/tools/examples/linux-bridge-vlan/br0.network
@@ -0,0 +1,8 @@
+[Match]
+Name=br0
+
+[Network]
+DNS=8.8.8.8
+#Domains=example.com
+Address=10.1.1.10/24
+Gateway=10.1.1.1
diff --git a/tools/examples/linux-bridge-vlan/enp0s0.network b/tools/examples/linux-bridge-vlan/enp0s0.network
new file mode 100644
index 0000000000..6ee3154dfc
--- /dev/null
+++ b/tools/examples/linux-bridge-vlan/enp0s0.network
@@ -0,0 +1,16 @@
+[Match]
+Name=enp0s0
+
+[Network]
+Bridge=br0
+
+# If Jumbo frames are required
+#[Link]
+#MTUBytes=9000
+
+[BridgeVLAN]
+VLAN=10
+
+[BridgeVLAN]
+VLAN=20
+
-- 
2.39.2

Re: [RFC PATCH v2 0/5] Add bridge VLAN support

[Andrew Cooper]

On 08/05/2024 10:38 pm, Leigh Brown wrote:
> Hello all,
>
> I realised over the weekend that there is a valid use case for providing
> a VIF to a domain that has access to multiple VLANs, e.g. a router. Yes,
> you can create a VIF per VLAN, but if you start having several VLANs (as
> I do), it would be nicer to create a single interface that has access to
> all the relevant VLANs (e.g. enX0.10, enX0.20, etc.).
>
> So, version 2 changes the name and type of the parameter from an integer
> called `vid' to a string called `vlan'. The vlan parameter is then
> parsed by the vif-bridge script (actually, the functions called by it in
> xen-network-common.sh).
>
> As it quite a common practice to allocate VLANs in round numbers, I also
> implemented the ability to specify contiguous or non-contiguous ranges.
> You can specify whether a VLAN is tagged or untagged, and which VLAN is
> the PVID (only one PVID is allowed).  For example,
>
> vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10p/20-29' ]
>
> will setup the VIF so that 10 is the PVID and VLAN IDs 20 through 29
> are permitted with tags. Another example:
>
> vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=1p/10+10x9' ]
>
> will setup the bridge to set 1 as the PVID and permit access with
> tags for VLAN IDs 10, 20, 30, 40, 50, 60, 70, 80 and 90.
>
> This patch set enables this capability as follows:
>
> 1. Adds `vlan' as a new member of the libxl_device_nic structure;
> 2. Adds support to read and write the vlan parameter from the xenstore;
> 3. Adds `vlan' as a new keyword for the vif configuration option;
> 4. Adds support to assign the bridge VLANs in the Linux hotplug scripts;
> 5. Updated xl-network-configuration(5) manpage and example configs.
>
> Original blurb below:
>
> For many years I have been configuring VLANs on my Linux Dom0 by
> creating VLAN interfaces for each VLAN I wanted to connect a domain
> to and then a corresponding bridge. So I would tend to have things
> like:
>
> enp0s0    -> br0     -> vif1, vif2
> enp0s0.10 -> br0vl10 -> vif3, vif4
> enp0s0.20 -> br0vl20 -> vif5
> dummy0    -> br1     -> vif6
>
> I recently discovered that iproute2 supports creating bridge VLANs that
> allows you to assign a VLAN to each of the interfaces associated to a
> bridge. This allows a greatly simplified configuration where a single
> bridge can support all the domains, and the iproute2 bridge command can
> assign each VIF to the required VLAN.  This looks like this:
>
> # bridge vlan
> port              vlan-id  
> enp0s0            1 PVID Egress Untagged
>                   10
>                   20
> br0               1 PVID Egress Untagged
> vif1.0            1 PVID Egress Untagged
> vif2.0            1 PVID Egress Untagged
> vif3.0            10 PVID Egress Untagged
> vif4.0            10 PVID Egress Untagged
> vif5.0            20 PVID Egress Untagged
> vif6.0            30 PVID Egress Untagged
>
> This patch set enables this capability as follows:
>
> 1. Adds `vid' as a new member of the libxl_device_nic structure;
> 2. Adds support to read and write vid from the xenstore;
> 3. Adds `vid' as a new keyword for the vif configuration option;
> 4. Adds support for assign the bridge VLAN in the Linux hotplug scripts.
>
> I don't believe NetBSD or FreeBSD support this capability, but if they
> do please point me in the direction of some documentation and/or examples.
>
> NB: I'm not very familiar with Xen code base so may have missed
> something important, although I have tested it and it is working well
> for me.
>
> Cheers,
>
> Leigh.
>
>
> Leigh Brown (5):
>   tools/libs/light: Add vlan field to libxl_device_nic
>   tools/xl: add vlan keyword to vif option
>   tools/hotplug/Linux: Add bridge VLAN support
>   docs/man: document VIF vlan keyword
>   tools/examples: Example Linux bridge VLAN config
>
>  docs/man/xl-network-configuration.5.pod.in    |  38 ++++++
>  tools/examples/linux-bridge-vlan/README       |  68 +++++++++++
>  tools/examples/linux-bridge-vlan/br0.netdev   |   7 ++
>  tools/examples/linux-bridge-vlan/br0.network  |   8 ++
>  .../examples/linux-bridge-vlan/enp0s0.network |  16 +++
>  tools/hotplug/Linux/xen-network-common.sh     | 111 ++++++++++++++++++
>  tools/libs/light/libxl_nic.c                  |  10 ++
>  tools/libs/light/libxl_types.idl              |   1 +
>  tools/xl/xl_parse.c                           |   2 +
>  9 files changed, 261 insertions(+)
>  create mode 100644 tools/examples/linux-bridge-vlan/README
>  create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev
>  create mode 100644 tools/examples/linux-bridge-vlan/br0.network
>  create mode 100644 tools/examples/linux-bridge-vlan/enp0s0.network
>

This is past the last-post date, so Oleksii will need to decide whether
he's happy to make an exception for it.

Anthony is OoO for a month now, so if this is to get in for 4.19, then
it will need reviewing by others.  I've CC'd a few plausible candidates...

~Andrew

Re: [RFC PATCH v2 0/5] Add bridge VLAN support

[Leigh Brown]

Hi Andrew,

On 2024-05-09 16:53, Andrew Cooper wrote:
> On 08/05/2024 10:38 pm, Leigh Brown wrote:
>> Hello all,
>> 
>> I realised over the weekend that there is a valid use case for 
>> providing
>> a VIF to a domain that has access to multiple VLANs, e.g. a router. 
>> Yes,
>> you can create a VIF per VLAN, but if you start having several VLANs 
>> (as
>> I do), it would be nicer to create a single interface that has access 
>> to
>> all the relevant VLANs (e.g. enX0.10, enX0.20, etc.).
>> 
>> So, version 2 changes the name and type of the parameter from an 
>> integer
>> called `vid' to a string called `vlan'. The vlan parameter is then
>> parsed by the vif-bridge script (actually, the functions called by it 
>> in
>> xen-network-common.sh).
>> 
>> As it quite a common practice to allocate VLANs in round numbers, I 
>> also
>> implemented the ability to specify contiguous or non-contiguous 
>> ranges.
>> You can specify whether a VLAN is tagged or untagged, and which VLAN 
>> is
>> the PVID (only one PVID is allowed).  For example,
>> 
>> vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10p/20-29' ]
>> 
>> will setup the VIF so that 10 is the PVID and VLAN IDs 20 through 29
>> are permitted with tags. Another example:
>> 
>> vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=1p/10+10x9' ]
>> 
>> will setup the bridge to set 1 as the PVID and permit access with
>> tags for VLAN IDs 10, 20, 30, 40, 50, 60, 70, 80 and 90.
>> 
>> This patch set enables this capability as follows:
>> 
>> 1. Adds `vlan' as a new member of the libxl_device_nic structure;
>> 2. Adds support to read and write the vlan parameter from the 
>> xenstore;
>> 3. Adds `vlan' as a new keyword for the vif configuration option;
>> 4. Adds support to assign the bridge VLANs in the Linux hotplug 
>> scripts;
>> 5. Updated xl-network-configuration(5) manpage and example configs.
>> 
>> Original blurb below:
>> 
>> For many years I have been configuring VLANs on my Linux Dom0 by
>> creating VLAN interfaces for each VLAN I wanted to connect a domain
>> to and then a corresponding bridge. So I would tend to have things
>> like:
>> 
>> enp0s0    -> br0     -> vif1, vif2
>> enp0s0.10 -> br0vl10 -> vif3, vif4
>> enp0s0.20 -> br0vl20 -> vif5
>> dummy0    -> br1     -> vif6
>> 
>> I recently discovered that iproute2 supports creating bridge VLANs 
>> that
>> allows you to assign a VLAN to each of the interfaces associated to a
>> bridge. This allows a greatly simplified configuration where a single
>> bridge can support all the domains, and the iproute2 bridge command 
>> can
>> assign each VIF to the required VLAN.  This looks like this:
>> 
>> # bridge vlan
>> port              vlan-id
>> enp0s0            1 PVID Egress Untagged
>>                   10
>>                   20
>> br0               1 PVID Egress Untagged
>> vif1.0            1 PVID Egress Untagged
>> vif2.0            1 PVID Egress Untagged
>> vif3.0            10 PVID Egress Untagged
>> vif4.0            10 PVID Egress Untagged
>> vif5.0            20 PVID Egress Untagged
>> vif6.0            30 PVID Egress Untagged
>> 
>> This patch set enables this capability as follows:
>> 
>> 1. Adds `vid' as a new member of the libxl_device_nic structure;
>> 2. Adds support to read and write vid from the xenstore;
>> 3. Adds `vid' as a new keyword for the vif configuration option;
>> 4. Adds support for assign the bridge VLAN in the Linux hotplug 
>> scripts.
>> 
>> I don't believe NetBSD or FreeBSD support this capability, but if they
>> do please point me in the direction of some documentation and/or 
>> examples.
>> 
>> NB: I'm not very familiar with Xen code base so may have missed
>> something important, although I have tested it and it is working well
>> for me.
>> 
>> Cheers,
>> 
>> Leigh.
>> 
>> 
>> Leigh Brown (5):
>>   tools/libs/light: Add vlan field to libxl_device_nic
>>   tools/xl: add vlan keyword to vif option
>>   tools/hotplug/Linux: Add bridge VLAN support
>>   docs/man: document VIF vlan keyword
>>   tools/examples: Example Linux bridge VLAN config
>> 
>>  docs/man/xl-network-configuration.5.pod.in    |  38 ++++++
>>  tools/examples/linux-bridge-vlan/README       |  68 +++++++++++
>>  tools/examples/linux-bridge-vlan/br0.netdev   |   7 ++
>>  tools/examples/linux-bridge-vlan/br0.network  |   8 ++
>>  .../examples/linux-bridge-vlan/enp0s0.network |  16 +++
>>  tools/hotplug/Linux/xen-network-common.sh     | 111 
>> ++++++++++++++++++
>>  tools/libs/light/libxl_nic.c                  |  10 ++
>>  tools/libs/light/libxl_types.idl              |   1 +
>>  tools/xl/xl_parse.c                           |   2 +
>>  9 files changed, 261 insertions(+)
>>  create mode 100644 tools/examples/linux-bridge-vlan/README
>>  create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev
>>  create mode 100644 tools/examples/linux-bridge-vlan/br0.network
>>  create mode 100644 tools/examples/linux-bridge-vlan/enp0s0.network
>> 
> 
> This is past the last-post date, so Oleksii will need to decide whether
> he's happy to make an exception for it.

 From my own perspective, I know this is an enhancement and am more than
happy to maintain it locally for the time being, so no problem if it has
to wait.  Still happy for any feedback though :-)

> Anthony is OoO for a month now, so if this is to get in for 4.19, then
> it will need reviewing by others.  I've CC'd a few plausible 
> candidates...
> 
> ~Andrew

Regards,

Leigh,

Re: [RFC PATCH v2 0/5] Add bridge VLAN support

[Jason Andryuk]

On Thu, May 9, 2024 at 12:10 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>
> Hi Andrew,
>
> On 2024-05-09 16:53, Andrew Cooper wrote:
> > On 08/05/2024 10:38 pm, Leigh Brown wrote:
> > This is past the last-post date, so Oleksii will need to decide whether
> > he's happy to make an exception for it.
>
>  From my own perspective, I know this is an enhancement and am more than
> happy to maintain it locally for the time being, so no problem if it has
> to wait.  Still happy for any feedback though :-)
>
> > Anthony is OoO for a month now, so if this is to get in for 4.19, then
> > it will need reviewing by others.  I've CC'd a few plausible
> > candidates...

I'll try to review them in the next few days.

Regards,
Jason

Re: [RFC PATCH v2 1/5] tools/libs/light: Add vlan field to libxl_device_nic

[Jason Andryuk]

On Wed, May 8, 2024 at 7:43 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>
> Add `vlan' string field to libxl_device_nic, to allow a VLAN
> configuration to be specified for the VIF when adding it to the
> bridge device.
>
> Update libxl_nic.c to read and write the vlan field from the
> xenstore.
>
> This provides the capability for supported operating systems (e.g.
> Linux) to perform VLAN filtering on bridge ports.  The Xen
> hotplug scripts need to be updated to read this information from
> the xenstore and perform the required configuration.
>
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

I think vlan is a better name than vid for the field.

Thanks,
Jason

Re: [RFC PATCH v2 2/5] tools/xl: add vlan keyword to vif option

[Jason Andryuk]

On Wed, May 8, 2024 at 6:13 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>
> Update parse_nic_config() to support a new `vlan' keyword. This
> keyword specifies the VLAN configuration to assign to the VIF when
> attaching it to the bridge port, on operating systems that support
> the capability (e.g. Linux). The vlan keyword will allow one or
> more VLANs to be configured on the VIF when adding it to the bridge
> port. This will be done by the vif-bridge script and functions.
>
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Re: [RFC PATCH v2 0/5] Add bridge VLAN support

[Oleksii K.]

On Thu, 2024-05-09 at 16:53 +0100, Andrew Cooper wrote:
> On 08/05/2024 10:38 pm, Leigh Brown wrote:
> > Hello all,
> > 
> > I realised over the weekend that there is a valid use case for
> > providing
> > a VIF to a domain that has access to multiple VLANs, e.g. a router.
> > Yes,
> > you can create a VIF per VLAN, but if you start having several
> > VLANs (as
> > I do), it would be nicer to create a single interface that has
> > access to
> > all the relevant VLANs (e.g. enX0.10, enX0.20, etc.).
> > 
> > So, version 2 changes the name and type of the parameter from an
> > integer
> > called `vid' to a string called `vlan'. The vlan parameter is then
> > parsed by the vif-bridge script (actually, the functions called by
> > it in
> > xen-network-common.sh).
> > 
> > As it quite a common practice to allocate VLANs in round numbers, I
> > also
> > implemented the ability to specify contiguous or non-contiguous
> > ranges.
> > You can specify whether a VLAN is tagged or untagged, and which
> > VLAN is
> > the PVID (only one PVID is allowed).  For example,
> > 
> > vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10p/20-29' ]
> > 
> > will setup the VIF so that 10 is the PVID and VLAN IDs 20 through
> > 29
> > are permitted with tags. Another example:
> > 
> > vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=1p/10+10x9' ]
> > 
> > will setup the bridge to set 1 as the PVID and permit access with
> > tags for VLAN IDs 10, 20, 30, 40, 50, 60, 70, 80 and 90.
> > 
> > This patch set enables this capability as follows:
> > 
> > 1. Adds `vlan' as a new member of the libxl_device_nic structure;
> > 2. Adds support to read and write the vlan parameter from the
> > xenstore;
> > 3. Adds `vlan' as a new keyword for the vif configuration option;
> > 4. Adds support to assign the bridge VLANs in the Linux hotplug
> > scripts;
> > 5. Updated xl-network-configuration(5) manpage and example configs.
> > 
> > Original blurb below:
> > 
> > For many years I have been configuring VLANs on my Linux Dom0 by
> > creating VLAN interfaces for each VLAN I wanted to connect a domain
> > to and then a corresponding bridge. So I would tend to have things
> > like:
> > 
> > enp0s0    -> br0     -> vif1, vif2
> > enp0s0.10 -> br0vl10 -> vif3, vif4
> > enp0s0.20 -> br0vl20 -> vif5
> > dummy0    -> br1     -> vif6
> > 
> > I recently discovered that iproute2 supports creating bridge VLANs
> > that
> > allows you to assign a VLAN to each of the interfaces associated to
> > a
> > bridge. This allows a greatly simplified configuration where a
> > single
> > bridge can support all the domains, and the iproute2 bridge command
> > can
> > assign each VIF to the required VLAN.  This looks like this:
> > 
> > # bridge vlan
> > port              vlan-id  
> > enp0s0            1 PVID Egress Untagged
> >                   10
> >                   20
> > br0               1 PVID Egress Untagged
> > vif1.0            1 PVID Egress Untagged
> > vif2.0            1 PVID Egress Untagged
> > vif3.0            10 PVID Egress Untagged
> > vif4.0            10 PVID Egress Untagged
> > vif5.0            20 PVID Egress Untagged
> > vif6.0            30 PVID Egress Untagged
> > 
> > This patch set enables this capability as follows:
> > 
> > 1. Adds `vid' as a new member of the libxl_device_nic structure;
> > 2. Adds support to read and write vid from the xenstore;
> > 3. Adds `vid' as a new keyword for the vif configuration option;
> > 4. Adds support for assign the bridge VLAN in the Linux hotplug
> > scripts.
> > 
> > I don't believe NetBSD or FreeBSD support this capability, but if
> > they
> > do please point me in the direction of some documentation and/or
> > examples.
> > 
> > NB: I'm not very familiar with Xen code base so may have missed
> > something important, although I have tested it and it is working
> > well
> > for me.
> > 
> > Cheers,
> > 
> > Leigh.
> > 
> > 
> > Leigh Brown (5):
> >   tools/libs/light: Add vlan field to libxl_device_nic
> >   tools/xl: add vlan keyword to vif option
> >   tools/hotplug/Linux: Add bridge VLAN support
> >   docs/man: document VIF vlan keyword
> >   tools/examples: Example Linux bridge VLAN config
> > 
> >  docs/man/xl-network-configuration.5.pod.in    |  38 ++++++
> >  tools/examples/linux-bridge-vlan/README       |  68 +++++++++++
> >  tools/examples/linux-bridge-vlan/br0.netdev   |   7 ++
> >  tools/examples/linux-bridge-vlan/br0.network  |   8 ++
> >  .../examples/linux-bridge-vlan/enp0s0.network |  16 +++
> >  tools/hotplug/Linux/xen-network-common.sh     | 111
> > ++++++++++++++++++
> >  tools/libs/light/libxl_nic.c                  |  10 ++
> >  tools/libs/light/libxl_types.idl              |   1 +
> >  tools/xl/xl_parse.c                           |   2 +
> >  9 files changed, 261 insertions(+)
> >  create mode 100644 tools/examples/linux-bridge-vlan/README
> >  create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev
> >  create mode 100644 tools/examples/linux-bridge-vlan/br0.network
> >  create mode 100644 tools/examples/linux-bridge-vlan/enp0s0.network
> > 
> 
> This is past the last-post date, so Oleksii will need to decide
> whether
> he's happy to make an exception for it.
We also have Feature Freeze this week. But if maintainers think they
have additional/enough time for reviewing this patch series then we can
consider to have in Xen 4.19.

~ Oleksii

> 
> Anthony is OoO for a month now, so if this is to get in for 4.19,
> then
> it will need reviewing by others.  I've CC'd a few plausible
> candidates...
> 
> ~Andrew

Re: [RFC PATCH v2 3/5] tools/hotplug/Linux: Add bridge VLAN support

[Jason Andryuk]

On Wed, May 8, 2024 at 6:55 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>
> Update add_to_bridge shell function to read the vlan parameter
> from xenstore and set the bridge VLAN configuration for the VID.
>
> Add additional helper functions to parse the vlan specification,
> which consists of one or more of the follow:
>
> a) single VLAN (e.g. 10).
> b) contiguous range of VLANs (e.g. 10-15).
> c) discontiguous range with base, increment and count
>    (e.g. 100+10x9 which gives VLAN IDs 100, 110, ... 190).
>
> A single VLAN can be suffixed with "p" to indicate the PVID, or
> "u" to indicate untagged. A range of VLANs can be suffixed with
> "u" to indicate untagged.  A complex example would be:
>
>    vlan=1p/10-15/20-25u
>
> This capability only works when using the iproute2 bridge command,
> so a warning is issued if the vlan parameter is set and the bridge
> command is not available, as it will be ignored.
>
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
> ---
>  tools/hotplug/Linux/xen-network-common.sh | 111 ++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
>
> diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
> index 42fa704e8d..d9fb4f7355 100644
> --- a/tools/hotplug/Linux/xen-network-common.sh
> +++ b/tools/hotplug/Linux/xen-network-common.sh
> @@ -121,10 +121,113 @@ create_bridge () {
>      fi
>  }
>
> +_vif_vlan_add() {
> +    # References vlans, pvid and error variables from the calling function
> +    local -i vid=$1
> +    local flag=${2:-}
> +
> +    if (( vid < 1 || vid > 4094 )) ;then
> +        error="vlan id $vid not between 1 and 4094"
> +        return
> +    fi
> +    if [[ -n "${vlans[$vid]}" ]] ;then
> +        error="vlan id $vid specified more than once"
> +        return

You could do `fatal "vlan id $vid specified more than once"` and just
terminate the script at this point.  It would simplify your later code
if you use fatal more.

> +    fi
> +    case $flag in
> +     p) if (( pvid != 0 )) ;then
> +            error="more than one pvid specified ($vid and $pvid)"
> +            return
> +        fi
> +        pvid=$vid
> +        vlans[$vid]=p ;;
> +     u) vlans[$vid]=u ;;
> +     *) vlans[$vid]=t ;;
> +    esac
> +}
> +
> +_vif_vlan_parse_term() {
> +    # References error variable from the calling function
> +    local vid incr last term=${1:-}
> +
> +    if [[ $term =~ ^([0-9]+)([pu])?$ ]] ;then

I like that you split the different cases into multiple REs.

> +        _vif_vlan_add ${BASH_REMATCH[1]} ${BASH_REMATCH[2]}
> +    elif [[ $term =~ ^([0-9]+)-([0-9]+)(u)?$ ]] ;then
> +        vid=${BASH_REMATCH[1]}
> +        last=${BASH_REMATCH[2]}
> +        if (( last >= vid )) ;then
> +            for (( ; vid<=last; vid++ )) ;do
> +                _vif_vlan_add $vid ${BASH_REMATCH[3]}
> +            done
> +        else
> +           error="invalid vlan id range: $term"
> +        fi
> +    elif [[ $term =~ ^([0-9]+)\+([0-9]+)x([0-9]+)(u)?$ ]] ;then
> +        vid=${BASH_REMATCH[1]}
> +        incr=${BASH_REMATCH[2]}
> +        for (( j=${BASH_REMATCH[3]}; j>0; --j, vid+=incr ))
> +        do
> +            _vif_vlan_add $vid ${BASH_REMATCH[4]}
> +        done
> +    else
> +        error="invalid vlan specification: $term"
> +    fi
> +}
> +
> +_vif_vlan_validate_pvid() {
> +    # References vlans and pvid variables from the calling function
> +    if (( pvid == 0 )) ;then
> +        if (( ${#vlans[@]} == 1 )) ;then
> +            vlans[${!vlans[*]}]=p
> +        else
> +            error="pvid required for multiple vlan ids"
> +        fi
> +    fi
> +}
> +
> +_vif_vlan_setup() {
> +    # References vlans and dev variable from the calling function
> +    local vid cmd
> +
> +    bridge vlan del dev "$dev" vid 1
> +    for vid in ${!vlans[@]} ;do
> +        cmd="bridge vlan add dev '$dev' vid $vid"
> +        case ${vlans[$vid]} in
> +             p) cmd="$cmd pvid untagged" ;;
> +             u) cmd="$cmd untagged" ;;
> +             t) ;;
> +        esac
> +        eval "$cmd"
> +    done
> +}
> +
> +_vif_vlan_membership() {
> +    # The vlans, pvid, dev and error variables are used by sub-functions
> +    local -A vlans=()
> +    local -a terms=()
> +    local -i i pvid=0
> +    local dev=$1 error=""
> +
> +    # Split the vlan specification string into its terms
> +    readarray -d / -t terms <<<$2
> +    for (( i=0; i<${#terms[@]}; ++i )) ;do
> +        _vif_vlan_parse_term ${terms[$i]%%[[:space:]]}
> +        [[ -n "$error" ]] && break
> +    done
> +
> +    [[ -z "$error" ]] && _vif_vlan_validate_pvid
> +    [[ -z "$error" ]] && _vif_vlan_setup
> +    [[ -z "$error" ]] && return 0
> +
> +    log error "$error"
> +    return 1
> +}
> +
>  # Usage: add_to_bridge bridge dev
>  add_to_bridge () {
>      local bridge=$1
>      local dev=$2
> +    local vlan=$(xenstore_read_default "$XENBUS_PATH/vlan" "")
>
>      # Don't add $dev to $bridge if it's already on the bridge.
>      if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
> @@ -134,6 +237,14 @@ add_to_bridge () {
>          else
>              ip link set ${dev} master ${bridge}
>          fi
> +        if [ -n "${vlan}" ] ;then
> +            if which bridge >&/dev/null; then
> +                log debug "configuring VLANs for ${dev} on ${bridge}"
> +                _vif_vlan_membership "${dev}" "${vlan}"
> +            else
> +                log warning "bridge command not available, ignoring vlan config"

Do you think this should be fatal?  It seems to me that setting up the
connection but not applying the vlans could be a security issue.

This file before your patch was very close to posix sh.  Afterwards,
it definitely needs bash.  vif-bridge is /bin/bash, so it is fine.

Regards,
Jason

Re: [RFC PATCH v2 4/5] docs/man: document VIF vlan keyword

[Jason Andryuk]

On Wed, May 8, 2024 at 5:39 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>
> Document the new `vlan' keyword in xl-network-configuration(5).
>
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

One nit below

> ---
>  docs/man/xl-network-configuration.5.pod.in | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/docs/man/xl-network-configuration.5.pod.in b/docs/man/xl-network-configuration.5.pod.in
> index f3e379bcf8..c35c0922b3 100644
> --- a/docs/man/xl-network-configuration.5.pod.in
> +++ b/docs/man/xl-network-configuration.5.pod.in
> @@ -259,6 +259,44 @@ Specifies the MTU (i.e. the maximum size of an IP payload, exclusing headers). T

> +Note, one VLAN ID must be marked as the PVID. In the case of a vlan
> +specification consisting of a single VLAN ID (e.g. C<vlan=10>), the B<p> suffix
> +may be omitted. Specifying more than one untagged VLAN ID is an advanced
> +configuration - use with caution.
> +
> +For example:
> +
> +        'vlan=10' -- meaning a single VLAN that is the PVID.
> +       'vlan=10p/20' -- VLAN 10 is the PVID and VLAN 20 is tagged.
> +       'vlan=10p/100+10x4' -- VLANs 10, 100, 110, 120, 130, 140, 150.

Indent mismatch between 7 and 8 spaces.

> +
>  =head2 trusted / untrusted
>
>  An advisory setting for the frontend driver on whether the backend should be
> --
> 2.39.2
>
>

Re: [RFC PATCH v2 5/5] tools/examples: Example Linux bridge VLAN config

[Jason Andryuk]

On Wed, May 8, 2024 at 6:08 PM Leigh Brown <leigh@solinno.co.uk> wrote:>
> Add a new directory linux-bridge-vlan with examples files showing
> how to configure systemd-networkd to support a bridge VLAN
> configuration.
>
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
> ---
>  tools/examples/linux-bridge-vlan/README       | 68 +++++++++++++++++++
>  tools/examples/linux-bridge-vlan/br0.netdev   |  7 ++
>  tools/examples/linux-bridge-vlan/br0.network  |  8 +++
>  .../examples/linux-bridge-vlan/enp0s0.network | 16 +++++
>  4 files changed, 99 insertions(+)
>  create mode 100644 tools/examples/linux-bridge-vlan/README
>  create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev
>  create mode 100644 tools/examples/linux-bridge-vlan/br0.network
>  create mode 100644 tools/examples/linux-bridge-vlan/enp0s0.network

I think putting these in docs/misc/linux-bridge-vlan/ might be a
better location.

> diff --git a/tools/examples/linux-bridge-vlan/README b/tools/examples/linux-bridge-vlan/README
> new file mode 100644
> index 0000000000..83b9fa3fd6
> --- /dev/null
> +++ b/tools/examples/linux-bridge-vlan/README
> @@ -0,0 +1,68 @@
> +Linux Xen Dom0 single bridge multiple VLAN configuration with systemd
> +=====================================================================
> +
> +Introduction
> +------------
> +
> +This directory contains example files to be placed in /etc/systemd/network
> +to enable a single bridge with multiple VLAN support.
> +
> +The example is to support the scenario where the Xen host network interface
> +is connected to an Ethernet switch configured as a trunk port. Each domain
> +VIF can then be configured with one or more VLAN IDs, one of which will be
> +the PVID.
> +
> +The example files create a bridge device called br0, with a physical interface
> +called enp0s0. You will need to update this with your system's device name.
> +
> +Key points of the configuration are:
> +
> +1. In br0.netdev, VLANFiltering=on is set. This is required to ensure the
> +   VLAN tags are handled correctly.  If it is not set then the packets
> +   from the VIF interfaces will not have the correct VLAN tags set.
> +
> +2. In br0.network, a system IPv4 address is configured that can be updated
> +   according to your local network settings.
> +
> +3. In enp0s0.network, Bridge=br0 sets the bridge device to connect to. There
> +   is also a [BridgeVLAN] section for each VLAN you want to give access
> +   to the switch. Note, if you want to create an internal VLAN private to

For
"for each VLAN you want to give access to the switch"
do you mean:
"for each VLAN you want connected with the external network"
or
"for each VLAN you want accessible on the external network"
?

The "access to the switch" part I find unclear.

> +   the host, do not include its VLAN ID in this file.


> +Domain configuration
> +--------------------
> +
> +Add the vlan= keyword to the vif definition in the domain. The simplest
> +and most common example is a domain that wishes to connect to a single VLAN:
> +
> +vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10' ]
> +
> +If you wish to configure a domain to route between two VLANs, you have two
> +options. Option 1 is to create multiple interfaces on different VLANs:
> +
> +vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10',
> +       'max=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=20' ]

Hard tab here makes the diff look off, but the file would be aligned.

I think this is good content.  I'm not familiar with the systemd
networking stuff to give an R-b.  But it's only examples, so I think
it should be okay.  I'm not a maintainer, but it would be an Acked-by,
if I were.

Regards,
Jason

Re: [RFC PATCH v2 3/5] tools/hotplug/Linux: Add bridge VLAN support

[Leigh Brown]

Hi Jason,

On 2024-05-15 01:57, Jason Andryuk wrote:
> On Wed, May 8, 2024 at 6:55 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>> 
>> Update add_to_bridge shell function to read the vlan parameter
>> from xenstore and set the bridge VLAN configuration for the VID.
>> 
>> Add additional helper functions to parse the vlan specification,
>> which consists of one or more of the follow:
>> 
>> a) single VLAN (e.g. 10).
>> b) contiguous range of VLANs (e.g. 10-15).
>> c) discontiguous range with base, increment and count
>>    (e.g. 100+10x9 which gives VLAN IDs 100, 110, ... 190).
>> 
>> A single VLAN can be suffixed with "p" to indicate the PVID, or
>> "u" to indicate untagged. A range of VLANs can be suffixed with
>> "u" to indicate untagged.  A complex example would be:
>> 
>>    vlan=1p/10-15/20-25u
>> 
>> This capability only works when using the iproute2 bridge command,
>> so a warning is issued if the vlan parameter is set and the bridge
>> command is not available, as it will be ignored.
>> 
>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
>> ---
>>  tools/hotplug/Linux/xen-network-common.sh | 111 
>> ++++++++++++++++++++++
>>  1 file changed, 111 insertions(+)
>> 
>> diff --git a/tools/hotplug/Linux/xen-network-common.sh 
>> b/tools/hotplug/Linux/xen-network-common.sh
>> index 42fa704e8d..d9fb4f7355 100644
>> --- a/tools/hotplug/Linux/xen-network-common.sh
>> +++ b/tools/hotplug/Linux/xen-network-common.sh
>> @@ -121,10 +121,113 @@ create_bridge () {
>>      fi
>>  }
>> 
>> +_vif_vlan_add() {
>> +    # References vlans, pvid and error variables from the calling 
>> function
>> +    local -i vid=$1
>> +    local flag=${2:-}
>> +
>> +    if (( vid < 1 || vid > 4094 )) ;then
>> +        error="vlan id $vid not between 1 and 4094"
>> +        return
>> +    fi
>> +    if [[ -n "${vlans[$vid]}" ]] ;then
>> +        error="vlan id $vid specified more than once"
>> +        return
> 
> You could do `fatal "vlan id $vid specified more than once"` and just
> terminate the script at this point.  It would simplify your later code
> if you use fatal more.

I will do that.

>> +    fi
>> +    case $flag in
>> +     p) if (( pvid != 0 )) ;then
>> +            error="more than one pvid specified ($vid and $pvid)"
>> +            return
>> +        fi
>> +        pvid=$vid
>> +        vlans[$vid]=p ;;
>> +     u) vlans[$vid]=u ;;
>> +     *) vlans[$vid]=t ;;
>> +    esac
>> +}
>> +
>> +_vif_vlan_parse_term() {
>> +    # References error variable from the calling function
>> +    local vid incr last term=${1:-}
>> +
>> +    if [[ $term =~ ^([0-9]+)([pu])?$ ]] ;then
> 
> I like that you split the different cases into multiple REs.
> 
>> +        _vif_vlan_add ${BASH_REMATCH[1]} ${BASH_REMATCH[2]}
>> +    elif [[ $term =~ ^([0-9]+)-([0-9]+)(u)?$ ]] ;then
>> +        vid=${BASH_REMATCH[1]}
>> +        last=${BASH_REMATCH[2]}
>> +        if (( last >= vid )) ;then
>> +            for (( ; vid<=last; vid++ )) ;do
>> +                _vif_vlan_add $vid ${BASH_REMATCH[3]}
>> +            done
>> +        else
>> +           error="invalid vlan id range: $term"
>> +        fi
>> +    elif [[ $term =~ ^([0-9]+)\+([0-9]+)x([0-9]+)(u)?$ ]] ;then
>> +        vid=${BASH_REMATCH[1]}
>> +        incr=${BASH_REMATCH[2]}
>> +        for (( j=${BASH_REMATCH[3]}; j>0; --j, vid+=incr ))
>> +        do
>> +            _vif_vlan_add $vid ${BASH_REMATCH[4]}
>> +        done
>> +    else
>> +        error="invalid vlan specification: $term"
>> +    fi
>> +}
>> +
>> +_vif_vlan_validate_pvid() {
>> +    # References vlans and pvid variables from the calling function
>> +    if (( pvid == 0 )) ;then
>> +        if (( ${#vlans[@]} == 1 )) ;then
>> +            vlans[${!vlans[*]}]=p
>> +        else
>> +            error="pvid required for multiple vlan ids"
>> +        fi
>> +    fi
>> +}
>> +
>> +_vif_vlan_setup() {
>> +    # References vlans and dev variable from the calling function
>> +    local vid cmd
>> +
>> +    bridge vlan del dev "$dev" vid 1
>> +    for vid in ${!vlans[@]} ;do
>> +        cmd="bridge vlan add dev '$dev' vid $vid"
>> +        case ${vlans[$vid]} in
>> +             p) cmd="$cmd pvid untagged" ;;
>> +             u) cmd="$cmd untagged" ;;
>> +             t) ;;
>> +        esac
>> +        eval "$cmd"
>> +    done
>> +}
>> +
>> +_vif_vlan_membership() {
>> +    # The vlans, pvid, dev and error variables are used by 
>> sub-functions
>> +    local -A vlans=()
>> +    local -a terms=()
>> +    local -i i pvid=0
>> +    local dev=$1 error=""
>> +
>> +    # Split the vlan specification string into its terms
>> +    readarray -d / -t terms <<<$2
>> +    for (( i=0; i<${#terms[@]}; ++i )) ;do
>> +        _vif_vlan_parse_term ${terms[$i]%%[[:space:]]}
>> +        [[ -n "$error" ]] && break
>> +    done
>> +
>> +    [[ -z "$error" ]] && _vif_vlan_validate_pvid
>> +    [[ -z "$error" ]] && _vif_vlan_setup
>> +    [[ -z "$error" ]] && return 0
>> +
>> +    log error "$error"
>> +    return 1
>> +}
>> +
>>  # Usage: add_to_bridge bridge dev
>>  add_to_bridge () {
>>      local bridge=$1
>>      local dev=$2
>> +    local vlan=$(xenstore_read_default "$XENBUS_PATH/vlan" "")
>> 
>>      # Don't add $dev to $bridge if it's already on the bridge.
>>      if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
>> @@ -134,6 +237,14 @@ add_to_bridge () {
>>          else
>>              ip link set ${dev} master ${bridge}
>>          fi
>> +        if [ -n "${vlan}" ] ;then
>> +            if which bridge >&/dev/null; then
>> +                log debug "configuring VLANs for ${dev} on ${bridge}"
>> +                _vif_vlan_membership "${dev}" "${vlan}"
>> +            else
>> +                log warning "bridge command not available, ignoring 
>> vlan config"
> 
> Do you think this should be fatal?  It seems to me that setting up the
> connection but not applying the vlans could be a security issue.

Agreed, I will do that.

> This file before your patch was very close to posix sh.  Afterwards,
> it definitely needs bash.  vif-bridge is /bin/bash, so it is fine.

Great - I would not like to rewrite it in Posix shell!

> Regards,
> Jason

Regards,

Leigh.

Re: [RFC PATCH v2 4/5] docs/man: document VIF vlan keyword

[Leigh Brown]

Hi Jason,

On 2024-05-15 01:57, Jason Andryuk wrote:
> On Wed, May 8, 2024 at 5:39 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>> 
>> Document the new `vlan' keyword in xl-network-configuration(5).
>> 
>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> One nit below
> 
>> ---
>>  docs/man/xl-network-configuration.5.pod.in | 38 
>> ++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>> 
>> diff --git a/docs/man/xl-network-configuration.5.pod.in 
>> b/docs/man/xl-network-configuration.5.pod.in
>> index f3e379bcf8..c35c0922b3 100644
>> --- a/docs/man/xl-network-configuration.5.pod.in
>> +++ b/docs/man/xl-network-configuration.5.pod.in
>> @@ -259,6 +259,44 @@ Specifies the MTU (i.e. the maximum size of an IP 
>> payload, exclusing headers). T
> 
>> +Note, one VLAN ID must be marked as the PVID. In the case of a vlan
>> +specification consisting of a single VLAN ID (e.g. C<vlan=10>), the 
>> B<p> suffix
>> +may be omitted. Specifying more than one untagged VLAN ID is an 
>> advanced
>> +configuration - use with caution.
>> +
>> +For example:
>> +
>> +        'vlan=10' -- meaning a single VLAN that is the PVID.
>> +       'vlan=10p/20' -- VLAN 10 is the PVID and VLAN 20 is tagged.
>> +       'vlan=10p/100+10x4' -- VLANs 10, 100, 110, 120, 130, 140, 150.
> 
> Indent mismatch between 7 and 8 spaces.

FWIW I will sort that too :-)

>> +
>>  =head2 trusted / untrusted
>> 
>>  An advisory setting for the frontend driver on whether the backend 
>> should be
>> --
>> 2.39.2
>> 
>> 

Regards,

Leigh.

Re: [RFC PATCH v2 5/5] tools/examples: Example Linux bridge VLAN config

[Leigh Brown]

Hi Jason,

On 2024-05-15 01:58, Jason Andryuk wrote:
> On Wed, May 8, 2024 at 6:08 PM Leigh Brown <leigh@solinno.co.uk> 
> wrote:>
>> Add a new directory linux-bridge-vlan with examples files showing
>> how to configure systemd-networkd to support a bridge VLAN
>> configuration.
>> 
>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
>> ---
>>  tools/examples/linux-bridge-vlan/README       | 68 
>> +++++++++++++++++++
>>  tools/examples/linux-bridge-vlan/br0.netdev   |  7 ++
>>  tools/examples/linux-bridge-vlan/br0.network  |  8 +++
>>  .../examples/linux-bridge-vlan/enp0s0.network | 16 +++++
>>  4 files changed, 99 insertions(+)
>>  create mode 100644 tools/examples/linux-bridge-vlan/README
>>  create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev
>>  create mode 100644 tools/examples/linux-bridge-vlan/br0.network
>>  create mode 100644 tools/examples/linux-bridge-vlan/enp0s0.network
> 
> I think putting these in docs/misc/linux-bridge-vlan/ might be a
> better location.

No problem, will move.

>> diff --git a/tools/examples/linux-bridge-vlan/README 
>> b/tools/examples/linux-bridge-vlan/README
>> new file mode 100644
>> index 0000000000..83b9fa3fd6
>> --- /dev/null
>> +++ b/tools/examples/linux-bridge-vlan/README
>> @@ -0,0 +1,68 @@
>> +Linux Xen Dom0 single bridge multiple VLAN configuration with systemd
>> +=====================================================================
>> +
>> +Introduction
>> +------------
>> +
>> +This directory contains example files to be placed in 
>> /etc/systemd/network
>> +to enable a single bridge with multiple VLAN support.
>> +
>> +The example is to support the scenario where the Xen host network 
>> interface
>> +is connected to an Ethernet switch configured as a trunk port. Each 
>> domain
>> +VIF can then be configured with one or more VLAN IDs, one of which 
>> will be
>> +the PVID.
>> +
>> +The example files create a bridge device called br0, with a physical 
>> interface
>> +called enp0s0. You will need to update this with your system's device 
>> name.
>> +
>> +Key points of the configuration are:
>> +
>> +1. In br0.netdev, VLANFiltering=on is set. This is required to ensure 
>> the
>> +   VLAN tags are handled correctly.  If it is not set then the 
>> packets
>> +   from the VIF interfaces will not have the correct VLAN tags set.
>> +
>> +2. In br0.network, a system IPv4 address is configured that can be 
>> updated
>> +   according to your local network settings.
>> +
>> +3. In enp0s0.network, Bridge=br0 sets the bridge device to connect 
>> to. There
>> +   is also a [BridgeVLAN] section for each VLAN you want to give 
>> access
>> +   to the switch. Note, if you want to create an internal VLAN 
>> private to
> 
> For
> "for each VLAN you want to give access to the switch"
> do you mean:
> "for each VLAN you want connected with the external network"
> or
> "for each VLAN you want accessible on the external network"
> ?
> The "access to the switch" part I find unclear.
> 

On re-reading it is not as clear as I'd hope - I will adjust.

>> +   the host, do not include its VLAN ID in this file.
> 
> 
>> +Domain configuration
>> +--------------------
>> +
>> +Add the vlan= keyword to the vif definition in the domain. The 
>> simplest
>> +and most common example is a domain that wishes to connect to a 
>> single VLAN:
>> +
>> +vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10' ]
>> +
>> +If you wish to configure a domain to route between two VLANs, you 
>> have two
>> +options. Option 1 is to create multiple interfaces on different 
>> VLANs:
>> +
>> +vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10',
>> +       'max=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=20' ]
> 
> Hard tab here makes the diff look off, but the file would be aligned.

Will fix up so all the indents are spaced, to be consistent.

> I think this is good content.  I'm not familiar with the systemd
> networking stuff to give an R-b.  But it's only examples, so I think
> it should be okay.  I'm not a maintainer, but it would be an Acked-by,
> if I were.
> 
> Regards,
> Jason

Regards,

Leigh.

Re: [RFC PATCH v2 4/5] docs/man: document VIF vlan keyword

[Andrew Cooper]

On 15/05/2024 4:30 pm, Leigh Brown wrote:
> Hi Jason,
>
> On 2024-05-15 01:57, Jason Andryuk wrote:
>> On Wed, May 8, 2024 at 5:39 PM Leigh Brown <leigh@solinno.co.uk> wrote:
>>>
>>> Document the new `vlan' keyword in xl-network-configuration(5).
>>>
>>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
>>
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>>
>> One nit below
>>
>>> ---
>>>  docs/man/xl-network-configuration.5.pod.in | 38 ++++++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>>>
>>> diff --git a/docs/man/xl-network-configuration.5.pod.in
>>> b/docs/man/xl-network-configuration.5.pod.in
>>> index f3e379bcf8..c35c0922b3 100644
>>> --- a/docs/man/xl-network-configuration.5.pod.in
>>> +++ b/docs/man/xl-network-configuration.5.pod.in
>>> @@ -259,6 +259,44 @@ Specifies the MTU (i.e. the maximum size of an
>>> IP payload, exclusing headers). T
>>
>>> +Note, one VLAN ID must be marked as the PVID. In the case of a vlan
>>> +specification consisting of a single VLAN ID (e.g. C<vlan=10>), the
>>> B<p> suffix
>>> +may be omitted. Specifying more than one untagged VLAN ID is an
>>> advanced
>>> +configuration - use with caution.
>>> +
>>> +For example:
>>> +
>>> +        'vlan=10' -- meaning a single VLAN that is the PVID.
>>> +       'vlan=10p/20' -- VLAN 10 is the PVID and VLAN 20 is tagged.
>>> +       'vlan=10p/100+10x4' -- VLANs 10, 100, 110, 120, 130, 140, 150.
>>
>> Indent mismatch between 7 and 8 spaces.
>
> FWIW I will sort that too :-)

This should be merged into patch 2 which introduces the vlan keyword.

As the indentation is the only issue, I'll fix it on commit while merging.

~Andrew