[PATCH] tools/libs/light: Fix nic->vlan memory allocation

2024-05-20 Thread Leigh Brown
After the following commit:
3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic")
xl list -l aborts with a double free error if a domain has at least
one vif defined:

  $ sudo xl list -l
  free(): double free detected in tcache 2
  Aborted

Orginally, the vlan field was called vid and was defined as an integer.
It was appropriate to call libxl__xs_read_checked() with gc passed as
the string data was copied to a different variable.  However, the final
version uses a string data type and the call should have been changed
to use NOGC instead of gc to allow that data to live past the gc
controlled lifetime, in line with the other string fields.

This patch makes the change to pass NOGC instead of gc and moves the
new code to be next to the other string fields (fixing a couple of
errant tabs along the way), as recommended by Jason.

Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic")
Signed-off-by: Leigh Brown 

---
 tools/libs/light/libxl_nic.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/libs/light/libxl_nic.c b/tools/libs/light/libxl_nic.c
index d861e3726d..300a96a8b1 100644
--- a/tools/libs/light/libxl_nic.c
+++ b/tools/libs/light/libxl_nic.c
@@ -318,11 +318,6 @@ 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;
@@ -345,6 +340,10 @@ static int libxl__nic_from_xenstore(libxl__gc *gc, const 
char *libxl_path,
 GCSPRINTF("%s/script", libxl_path),
 (const char **)(&nic->script));
 if (rc) goto out;
+rc = libxl__xs_read_checked(NOGC, XBT_NULL,
+GCSPRINTF("%s/vlan", libxl_path),
+(const char **)(&nic->vlan));
+if (rc) goto out;
 rc = libxl__xs_read_checked(NOGC, XBT_NULL,
 GCSPRINTF("%s/forwarddev", libxl_path),
 (const char **)(&nic->coloft_forwarddev));
-- 
2.39.2




Re: xl list -l aborts with double free error with vlan patches

2024-05-20 Thread Leigh Brown

Hi,

On 2024-05-20 15:33, Leigh Brown wrote:

Hello,

When running xl list -l with my VLAN patches applied, a double free 
error is raised. I'm unable to determine why.


broken: 0cc01c603f4287233715a526b056bc20e0e97412 (HEAD) tools/xl: add 
vlan keyword to vif option
okay:   3bc14e4fa4b9832888710759a7dbe5f0d239f33b tools/libs/light: Add 
vlan field to libxl_device_nic
okay:   e27fc7d15eab79e604e8b8728778594accc23cf1 tools/xentop: Fix cpu% 
sort order


Any suggestions appreciated...

Debug run and backtrace:
[snip]


Thanks for Jason for pointing out my error - patch incoming.

Regards,

Leigh.



xl list -l aborts with double free error with vlan patches

2024-05-20 Thread Leigh Brown

Hello,

When running xl list -l with my VLAN patches applied, a double free 
error is raised. I'm unable to determine why.


broken: 0cc01c603f4287233715a526b056bc20e0e97412 (HEAD) tools/xl: add 
vlan keyword to vif option
okay:   3bc14e4fa4b9832888710759a7dbe5f0d239f33b tools/libs/light: Add 
vlan field to libxl_device_nic
okay:   e27fc7d15eab79e604e8b8728778594accc23cf1 tools/xentop: Fix cpu% 
sort order


Any suggestions appreciated...

Debug run and backtrace:

(gdb) run - list -l
Starting program: /usr/sbin/xl - list -l
[Thread debugging using libthread_db enabled]
Using host libthread_db library 
"/lib/x86_64-linux-gnu/libthread_db.so.1".
libxl: debug: libxl_domain.c:2295:libxl_retrieve_domain_configuration: 
ao 0x55592f50: create: how=(nil) callback=(nil) 
poller=0x5559dde0

[Detaching after fork from child process 1044]
libxl: debug: libxl_domain.c:2311:libxl_retrieve_domain_configuration: 
ao 0x55592f50: inprogress: poller=0x5559dde0, flags=i
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
vbd from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
vif from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
vtpm from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
vusb from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
vusb from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
pci from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
vdispl from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: No 
vsnd from xenstore

libxl: debug: libxl_qmp.c:1920:libxl__ev_qmp_dispose:  ev 0x555a0240
libxl: debug: libxl_event.c:2067:libxl__ao_complete: ao 0x55592f50: 
complete, rc=0
libxl: debug: libxl_event.c:2036:libxl__ao__destroy: ao 0x55592f50: 
destroy
libxl: debug: libxl_domain.c:2295:libxl_retrieve_domain_configuration: 
Domain 1:ao 0x5559f830: create: how=(nil) callback=(nil) 
poller=0x5559dde0

[Detaching after fork from child process 1045]
libxl: debug: libxl_domain.c:2311:libxl_retrieve_domain_configuration: 
Domain 1:ao 0x5559f830: inprogress: poller=0x5559dde0, flags=i
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: 
Domain 1:No vtpm from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: 
Domain 1:No vusb from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: 
Domain 1:No vusb from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: 
Domain 1:No pci from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: 
Domain 1:No vdispl from xenstore
libxl: debug: libxl_domain.c:2587:retrieve_domain_configuration_end: 
Domain 1:No vsnd from xenstore
libxl: debug: libxl_qmp.c:1920:libxl__ev_qmp_dispose: Domain 1: ev 
0x555a2820
libxl: debug: libxl_event.c:2067:libxl__ao_complete: ao 0x5559f830: 
complete, rc=0
libxl: debug: libxl_event.c:2036:libxl__ao__destroy: ao 0x5559f830: 
destroy

free(): double free detected in tcache 2

Program received signal SIGABRT, Aborted.
__pthread_kill_implementation (threadid=, 
signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44

44  ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (threadid=, 
signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x77d5de8f in __pthread_kill_internal (signo=6, 
threadid=) at ./nptl/pthread_kill.c:78
#2  0x77d0efb2 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/posix/raise.c:26

#3  0x77cf9472 in __GI_abort () at ./stdlib/abort.c:79
#4  0x77d52430 in __libc_message (action=action@entry=do_abort, 
fmt=fmt@entry=0x77e6c459 "%s\n") at 
../sysdeps/posix/libc_fatal.c:155
#5  0x77d677aa in malloc_printerr (str=str@entry=0x77e6f098 
"free(): double free detected in tcache 2") at ./malloc/malloc.c:5660
#6  0x77d69a36 in _int_free (av=0x77ea5c60 , 
p=0x555a0f90, have_lock=have_lock@entry=0) at ./malloc/malloc.c:4469
#7  0x77d6be8f in __GI___libc_free (mem=) at 
./malloc/malloc.c:3385
#8  0x77f160a3 in libxl__free_all (gc=gc@entry=0x5559f870) 
at libxl_internal.c:86
#9  0x77f27c11 in libxl__ao__destroy (ctx=0x55591850, 
ao=ao@entry=0x5559f830) at libxl_event.c:2039
#10 0x77f27c3e in ao__check_destroy (ctx=, 
ao=ao@entry=0x5559f830) at libxl_event.c:2028
#11 0x77f27c76 in ao__manip_leave (ctx=, 
ao=ao@entry=0x5559f830) at libxl_event.c:2021
#12 0x77f2948f in libxl__ao_inprogress 
(ao=ao@entry=0x5559f830, file=file@entry=0x77f80866 
"libxl_domain.c", line=line@entry=2311,
func=func@entry=0x77f814a0 <__func__.3> 
"libxl_retrieve_domain_configuration") at libxl_event.c:2235
#13 0x7fff

[PATCH v4 0/2] Finalise bridge VLAN support

2024-05-17 Thread Leigh Brown
Hello,

The first two patches have been merged so this version has the remaining
two. Only the first of these two patches has changed since v3.

Summary of changes from v3 to v4:
- Drop merged patches.
- _vif_vlan_setup: Add comment that we delete vid 1 due to it being
  automatically added.
- _vif_vlan_setup: Restructure to avoid using eval and simplify.
- _vif_vlan_membership: Add comment that we remove trailing spaces due
  to readarray behaviour and change the code to do it only on the last
  element of the array, prior to entering the loop.
- _vif_vlan_membership: Simplify the loop iterating through terms.

Regards,

Leigh.

---

Leigh Brown (2):
  tools/hotplug/Linux: Add bridge VLAN support
  tools/examples: Example Linux bridge VLAN config

 docs/misc/linux-bridge-vlan/README |  68 +
 docs/misc/linux-bridge-vlan/br0.netdev |   7 ++
 docs/misc/linux-bridge-vlan/br0.network|   8 ++
 docs/misc/linux-bridge-vlan/enp0s0.network |  16 +++
 tools/hotplug/Linux/xen-network-common.sh  | 109 +
 5 files changed, 208 insertions(+)
 create mode 100644 docs/misc/linux-bridge-vlan/README
 create mode 100644 docs/misc/linux-bridge-vlan/br0.netdev
 create mode 100644 docs/misc/linux-bridge-vlan/br0.network
 create mode 100644 docs/misc/linux-bridge-vlan/enp0s0.network

-- 
2.39.2




[PATCH v4 2/2] tools/examples: Example Linux bridge VLAN config

2024-05-17 Thread Leigh Brown
Add a new directory linux-bridge-vlan with example files showing
how to configure systemd-networkd to support a bridge VLAN
configuration.

Signed-off-by: Leigh Brown 
---
 docs/misc/linux-bridge-vlan/README | 68 ++
 docs/misc/linux-bridge-vlan/br0.netdev |  7 +++
 docs/misc/linux-bridge-vlan/br0.network|  8 +++
 docs/misc/linux-bridge-vlan/enp0s0.network | 16 +
 4 files changed, 99 insertions(+)
 create mode 100644 docs/misc/linux-bridge-vlan/README
 create mode 100644 docs/misc/linux-bridge-vlan/br0.netdev
 create mode 100644 docs/misc/linux-bridge-vlan/br0.network
 create mode 100644 docs/misc/linux-bridge-vlan/enp0s0.network

diff --git a/docs/misc/linux-bridge-vlan/README 
b/docs/misc/linux-bridge-vlan/README
new file mode 100644
index 00..9a048bca39
--- /dev/null
+++ b/docs/misc/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 allowed on the external
+   interface. 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//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/docs/misc/linux-bridge-vlan/br0.netdev 
b/docs/misc/linux-bridge-vlan/br0.netdev
new file mode 100644
index 00..ae1fe487c3
--- /dev/null
+++ b/docs/misc/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/docs/misc/linux-bridge-vlan/br0.network 
b/docs/misc/linux-bridge-vlan/br0.network
new file mode 100644
index 00..b56203b66a
--- /dev/null
+++ b/docs/misc/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/docs/misc/linux-bridge-vlan/enp0s0.network 
b/docs/misc/linux-bridge-vlan/enp0s0.network
new file mode 100644
index 00..6ee3154dfc
--- /dev/null
+++ b/docs/misc/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




[PATCH v4 1/2] tools/hotplug/Linux: Add bridge VLAN support

2024-05-17 Thread 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 following:

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 requires the iproute2 bridge command to be
installed.  An error will be generated if the vlan parameter is
set and the bridge command is not available.

Signed-off-by: Leigh Brown 
---
 tools/hotplug/Linux/xen-network-common.sh | 109 ++
 1 file changed, 109 insertions(+)

diff --git a/tools/hotplug/Linux/xen-network-common.sh 
b/tools/hotplug/Linux/xen-network-common.sh
index 42fa704e8d..31d359b83c 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -121,10 +121,111 @@ create_bridge () {
 fi
 }
 
+_vif_vlan_add() {
+# References vlans and pvid variables from the calling function
+local -i vid=$1
+local flag=${2:-}
+
+if (( vid < 1 || vid > 4094 )) ;then
+fatal "vlan id $vid not between 1 and 4094"
+fi
+if [[ -n "${vlans[$vid]}" ]] ;then
+fatal "vlan id $vid specified more than once"
+fi
+case $flag in
+ p) if (( pvid != 0 )) ;then
+fatal "more than one pvid specified ($vid and $pvid)"
+fi
+pvid=$vid
+vlans[$vid]=p ;;
+ u) vlans[$vid]=u ;;
+ *) vlans[$vid]=t ;;
+esac
+}
+
+_vif_vlan_parse_term() {
+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
+fatal "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
+fatal "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
+fatal "pvid required when using multiple vlan ids"
+fi
+fi
+}
+
+_vif_vlan_setup() {
+# References vlans and dev variable from the calling function
+local -i vid
+local -a args
+
+# Remove the default vlan id automatically added to the vif
+bridge vlan del dev $dev vid 1
+
+# Add the required vlans
+for vid in ${!vlans[@]} ;do
+case ${vlans[$vid]} in
+ p) args=(pvid untagged) ;;
+ u) args=(untagged) ;;
+ t) args=() ;;
+esac
+bridge vlan add dev $dev vid $vid ${args[@]}
+done
+}
+
+_vif_vlan_membership() {
+# The vlans, pvid and dev variables are used by sub-functions
+local -A vlans=()
+local -a terms=()
+local -i i pvid=0
+local dev=$1 term
+
+# Split the vlan specification string into its terms, removing the newline
+# that readarray adds to the last element
+readarray -d / -t terms <<<$2
+terms[-1]=${terms[-1]%%[[:space:]]}
+
+for term in ${terms[@]} ;do
+_vif_vlan_parse_term $term
+done
+
+_vif_vlan_validate_pvid
+_vif_vlan_setup
+return 0
+}
+
 # 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 +235,14 @@ add_to_bridge () {
 else
 ip link set ${dev} master ${bridge}
 fi
+if [ -n "${vlan}" ] ;then
+log debug "configuring vlans for ${dev} on ${bridge}"
+if which bridge >&/dev/null; then
+_vif_vlan_membership "${dev}" "${vlan}"
+else
+fatal "vlan configuration failed: bridge command not found"
+fi
+fi
 else
 log debug "$dev already on bridge $bridge"
 fi
-- 
2.39.2




Re: [PATCH v3 3/4] tools/hotplug/Linux: Add bridge VLAN support

2024-05-17 Thread Leigh Brown

Hi Jason,

On 2024-05-17 03:19, Jason Andryuk wrote:
On Thu, May 16, 2024 at 6:56 AM Leigh Brown  
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 following:

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 requires the iproute2 bridge command to be
installed.  An error will be generated if the vlan parameter is
set and the bridge command is not available.

Signed-off-by: Leigh Brown 

---
 tools/hotplug/Linux/xen-network-common.sh | 103 
++

 1 file changed, 103 insertions(+)

diff --git a/tools/hotplug/Linux/xen-network-common.sh 
b/tools/hotplug/Linux/xen-network-common.sh

index 42fa704e8d..fa7615ce0f 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh



+_vif_vlan_setup() {
+# References vlans and dev variable from the calling function
+local vid cmd
+
+bridge vlan del dev "$dev" vid 1


Is vid 1 always added, so you need to remove it before customizing?


Correct. I will add a comment to that effect.


+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"


Sorry if I missed this last time, but `eval` shouldn't be necessary.


local args

case ${vlans[$vid]} in
 p) args="pvid untagged" ;;
 u) args="untagged" ;;
 t) ;;
esac
bridge vlan add dev "$dev" vid "$vid" $args

args unquoted so it expands into maybe two args.  You could also make
args an array and do "${args[@]}"


I will make that change, using an array.


+done
+}
+
+_vif_vlan_membership() {
+# The vlans, pvid and dev variables are used by sub-functions
+local -A vlans=()
+local -a terms=()
+local -i i pvid=0
+local dev=$1
+
+# 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:]]}


Because terms is not in double quotes, wouldn't it expand out?  Then
any whitespace would be dropped when calling _vif_vlan_parse_term.
Your %% only drops trailing spaces too.  Maybe you want
"${terms//[[:space:]]/}" to remove all spaces?


That is a hack because readarray adds a newline at the end of the
last element (not sure why).  I will change the code just to fix up
the last element before the loop, and it will be clearer.


Stylistically, you use (( )) more than I would.  I'd do:

local term
for term in "${terms[@]}" ; do
 _vif_vlan_parse_term "$term"

But you can keep it your way if you prefer.


You guessed correctly that like using (( )) but in this case your
suggestion is simpler, so I will make that change.


Regards,
Jason


I am making the changes then will test, after which I will send an
updated version.

Regards,

Leigh.



[PATCH v3 4/4] docs/misc: Example Linux bridge VLAN config

2024-05-16 Thread Leigh Brown
Add a new directory linux-bridge-vlan with example files showing
how to configure systemd-networkd to support a bridge VLAN
configuration.

Signed-off-by: Leigh Brown 

---
 docs/misc/linux-bridge-vlan/README | 68 ++
 docs/misc/linux-bridge-vlan/br0.netdev |  7 +++
 docs/misc/linux-bridge-vlan/br0.network|  8 +++
 docs/misc/linux-bridge-vlan/enp0s0.network | 16 +
 4 files changed, 99 insertions(+)
 create mode 100644 docs/misc/linux-bridge-vlan/README
 create mode 100644 docs/misc/linux-bridge-vlan/br0.netdev
 create mode 100644 docs/misc/linux-bridge-vlan/br0.network
 create mode 100644 docs/misc/linux-bridge-vlan/enp0s0.network

diff --git a/docs/misc/linux-bridge-vlan/README 
b/docs/misc/linux-bridge-vlan/README
new file mode 100644
index 00..9a048bca39
--- /dev/null
+++ b/docs/misc/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 allowed on the external
+   interface. 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//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/docs/misc/linux-bridge-vlan/br0.netdev 
b/docs/misc/linux-bridge-vlan/br0.netdev
new file mode 100644
index 00..ae1fe487c3
--- /dev/null
+++ b/docs/misc/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/docs/misc/linux-bridge-vlan/br0.network 
b/docs/misc/linux-bridge-vlan/br0.network
new file mode 100644
index 00..b56203b66a
--- /dev/null
+++ b/docs/misc/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/docs/misc/linux-bridge-vlan/enp0s0.network 
b/docs/misc/linux-bridge-vlan/enp0s0.network
new file mode 100644
index 00..6ee3154dfc
--- /dev/null
+++ b/docs/misc/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




[PATCH v3 2/4] tools/xl: add vlan keyword to vif option

2024-05-16 Thread 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.

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

Signed-off-by: Leigh Brown 
Reviewed-by: Jason Andryuk 

---
 docs/man/xl-network-configuration.5.pod.in | 38 ++
 tools/xl/xl_parse.c|  2 ++
 2 files changed, 40 insertions(+)

diff --git a/docs/man/xl-network-configuration.5.pod.in 
b/docs/man/xl-network-configuration.5.pod.in
index f3e379bcf8..dfc35e72c6 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 - a single VLAN ID in the range 1 to 4094. This can optionally followed
+by a B to indicate the PVID or by a B to indicate an untagged VLAN. C
+implies B.
+
+=item *
+
+B-B - a range of VLAN IDs from B to B, both between
+1 and 4094 and B being less than or equal to B. This can be
+optionally followed by a B to indicate that the range of VLANs are untagged.
+
+=item *
+
+B+BxB - describing a range of VLAN IDs starting at B
+with B additional entries, each incremented by B. This can be 
+optionally followed by a B 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), the B 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
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




[PATCH v3 0/4] Add bridge VLAN support

2024-05-16 Thread Leigh Brown
Hello All,

I have addressed all the feedback from Jason and merged patch 4 and 2 as
requested by Andrew.

Summary of changes from RFC v2 to v3:

- xen-network-common.sh: use fatal() directly instead of $error variable
- xen-network-common.sh: bridge command not being available is now fatal
- Move examples to docs/misc/linux-bridge-vlan
- README: Make explanation of [BridgeVLAN] section clearer
- Use spaces consistently for indentation
- Merge previous patch 2 and 4 into a single patch 2


Blurb for RFC v2:

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
enp0s01 PVID Egress Untagged
  10
  20
br0   1 PVID Egress Untagged
vif1.01 PVID Egress Untagged
vif2.01 PVID Egress Untagged
vif3.010 PVID Egress Untagged
vif4.010 PVID Egress Untagged
vif5.020 PVID Egress Untagged
vif6.030 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 (4):
  tools/libs/light: Add vlan field to libxl_device_nic
  tools/xl: add vlan keyword to vif option
  Update add_to_bridge shell function to read the vlan parameter from
xenstore and set the bridge VLAN configuration for the VID.
  tools/examples: Example Linux bridge VLAN config

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

-- 
2.39.2




[PATCH v3 3/4] tools/hotplug/Linux: Add bridge VLAN support

2024-05-16 Thread 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 following:

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 requires the iproute2 bridge command to be
installed.  An error will be generated if the vlan parameter is
set and the bridge command is not available.

Signed-off-by: Leigh Brown 

---
 tools/hotplug/Linux/xen-network-common.sh | 103 ++
 1 file changed, 103 insertions(+)

diff --git a/tools/hotplug/Linux/xen-network-common.sh 
b/tools/hotplug/Linux/xen-network-common.sh
index 42fa704e8d..fa7615ce0f 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -121,10 +121,105 @@ create_bridge () {
 fi
 }
 
+_vif_vlan_add() {
+# References vlans and pvid variables from the calling function
+local -i vid=$1
+local flag=${2:-}
+
+if (( vid < 1 || vid > 4094 )) ;then
+fatal "vlan id $vid not between 1 and 4094"
+fi
+if [[ -n "${vlans[$vid]}" ]] ;then
+fatal "vlan id $vid specified more than once"
+fi
+case $flag in
+ p) if (( pvid != 0 )) ;then
+fatal "more than one pvid specified ($vid and $pvid)"
+fi
+pvid=$vid
+vlans[$vid]=p ;;
+ u) vlans[$vid]=u ;;
+ *) vlans[$vid]=t ;;
+esac
+}
+
+_vif_vlan_parse_term() {
+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
+fatal "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
+fatal "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
+fatal "pvid required when using 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 and dev variables are used by sub-functions
+local -A vlans=()
+local -a terms=()
+local -i i pvid=0
+local dev=$1
+
+# 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:]]}
+done
+
+_vif_vlan_validate_pvid
+_vif_vlan_setup
+return 0
+}
+
 # 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 +229,14 @@ add_to_bridge () {
 else
 ip link set ${dev} master ${bridge}
 fi
+if [ -n "${vlan}" ] ;then
+log debug "configuring vlans for ${dev} on ${bridge}"
+if which bridge >&/dev/null; then
+_vif_vlan_membership "${dev}" "${vlan}"
+else
+fatal "vlan configuration failed: bridge command not found"
+fi
+fi
 else
 log debug "$dev already on bridge $bridge"
 fi
-- 
2.39.2




[PATCH v3 1/4] tools/libs/light: Add vlan field to libxl_device_nic

2024-05-16 Thread 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 
Reviewed-by: Jason Andryuk 

---
 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




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

2024-05-15 Thread Leigh Brown

Hi Jason,

On 2024-05-15 01:58, Jason Andryuk wrote:
On Wed, May 8, 2024 at 6:08 PM Leigh Brown  
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 
---
 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 00..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

2024-05-15 Thread Leigh Brown

Hi Jason,

On 2024-05-15 01:57, Jason Andryuk wrote:

On Wed, May 8, 2024 at 5:39 PM Leigh Brown  wrote:


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

Signed-off-by: Leigh Brown 


Reviewed-by: Jason Andryuk 

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), the 
B 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 3/5] tools/hotplug/Linux: Add bridge VLAN support

2024-05-15 Thread Leigh Brown

Hi Jason,

On 2024-05-15 01:57, Jason Andryuk wrote:

On Wed, May 8, 2024 at 6:55 PM Leigh Brown  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 
---
 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&#x

Re: [PATCH for-4.19] tools/xentop: fix cpu% sort order

2024-05-14 Thread Leigh Brown

On 2024-05-14 13:07, Andrew Cooper wrote:

On 14/05/2024 9:13 am, Leigh Brown wrote:

Although using integer comparison to compare doubles kind of
works, it's annoying to see domains slightly out of order when
sorting by cpu%.

Add a compare_dbl() function and update compare_cpu_pct() to
call it.

Signed-off-by: Leigh Brown 
---
 tools/xentop/xentop.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
index 545bd5e96d..99199caec9 100644
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -85,6 +85,7 @@ static void set_delay(const char *value);
 static void set_prompt(const char *new_prompt, void (*func)(const 
char *));

 static int handle_key(int);
 static int compare(unsigned long long, unsigned long long);
+static int compare_dbl(double, double);
 static int compare_domains(xenstat_domain **, xenstat_domain **);
 static unsigned long long tot_net_bytes( xenstat_domain *, int);
 static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long 
*);
@@ -422,6 +423,16 @@ static int compare(unsigned long long i1, 
unsigned long long i2)

return 0;
 }

+/* Compares two double precision numbers, returning -1,0,1 for <,=,> 
*/

+static int compare_dbl(double d1, double d2)
+{
+   if(d1 < d2)
+   return -1;
+   if(d1 > d2)
+   return 1;
+   return 0;
+}
+
 /* Comparison function for use with qsort.  Compares two domains 
using the

  * current sort field. */
 static int compare_domains(xenstat_domain **domain1, xenstat_domain 
**domain2)

@@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)

 static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain 
*domain2)

 {
-   return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
+   return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));


Oh, we were doing an implicit double->unsigned long long conversion. 
Over the range 0.0 to 100.0, that ought to work as expected.  What kind
of out-of-order are you seeing?

Nevertheless, this should comparison should clearly be done using
doubles.  AFACT, get_cpu_pct() shouldn't ever return a NaN, so I think
this simple form is fine.

Oleksii: This is another bugfix to xentop, and should be considered for
4.19 at this point.

~Andrew


Perhaps I overthought it, and this approach might be better:

--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -523,7 +523,8 @@ static double get_cpu_pct(xenstat_domain *domain)

 static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain 
*domain2)

 {
-   return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
+   return -compare(get_cpu_pct(domain1) * 100.0,
+   get_cpu_pct(domain2) * 100.0);
 }

 /* Prints cpu percentage statistic */



Re: [PATCH for-4.19] tools/xentop: fix cpu% sort order

2024-05-14 Thread Leigh Brown

Hello,

On 2024-05-14 13:07, Andrew Cooper wrote:

On 14/05/2024 9:13 am, Leigh Brown wrote:

Although using integer comparison to compare doubles kind of
works, it's annoying to see domains slightly out of order when
sorting by cpu%.

Add a compare_dbl() function and update compare_cpu_pct() to
call it.

Signed-off-by: Leigh Brown 
---
 tools/xentop/xentop.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
index 545bd5e96d..99199caec9 100644
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -85,6 +85,7 @@ static void set_delay(const char *value);
 static void set_prompt(const char *new_prompt, void (*func)(const 
char *));

 static int handle_key(int);
 static int compare(unsigned long long, unsigned long long);
+static int compare_dbl(double, double);
 static int compare_domains(xenstat_domain **, xenstat_domain **);
 static unsigned long long tot_net_bytes( xenstat_domain *, int);
 static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long 
*);
@@ -422,6 +423,16 @@ static int compare(unsigned long long i1, 
unsigned long long i2)

return 0;
 }

+/* Compares two double precision numbers, returning -1,0,1 for <,=,> 
*/

+static int compare_dbl(double d1, double d2)
+{
+   if(d1 < d2)
+   return -1;
+   if(d1 > d2)
+   return 1;
+   return 0;
+}
+
 /* Comparison function for use with qsort.  Compares two domains 
using the

  * current sort field. */
 static int compare_domains(xenstat_domain **domain1, xenstat_domain 
**domain2)

@@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)

 static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain 
*domain2)

 {
-   return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
+   return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));


Oh, we were doing an implicit double->unsigned long long conversion. 
Over the range 0.0 to 100.0, that ought to work as expected.  What kind
of out-of-order are you seeing?


Without patch:

xentop - 13:29:01   Xen 4.18.2
13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0 
shutdown
Mem: 67030640k total, 33097800k used, 33932840k freeCPUs: 24 @ 
3693MHz
  NAME  STATE   CPU(sec) CPU(%) MEM(k) MEM(%)  MAXMEM(k) 
MAXMEM(%)
  icecream --b---   25976.641943686.34195328   
6.3
 xendd --b---   40165.4 5242680.8 525312   
0.8
  Domain-0 -r   10591.710485761.61048576   
1.6
  neon --b---8261.120972163.12098176   
3.1
   blender --b---1210.210486401.61049600   
1.6
 bread --b--- 690.1 5243520.8 525312   
0.8
   bob --b---5020.3   16777284   25.0   16778240  
25.0
cheese --b---2250.510483841.61049600   
1.6
   cassini --b---4890.431457924.73146752   
4.7
  chickpea --b--- 670.1 5243520.8 525312   
0.8
lentil --b--- 670.1 2622080.4 263168   
0.4
   fusilli --b---1590.2 5243520.8 525312   
0.8
 pizza --b---3590.5 5243520.8 525312   
0.8


With patch:

xentop - 13:30:17   Xen 4.18.2
13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0 
shutdown
Mem: 67030640k total, 33097788k used, 33932852k freeCPUs: 24 @ 
3693MHz
  NAME  STATE   CPU(sec) CPU(%) MEM(k) MEM(%)  MAXMEM(k) 
MAXMEM(%)
 xendd --b---   40205.7 5242680.8 525312   
0.8
  icecream --b---   26003.841943686.34195328   
6.3
  Domain-0 -r   10601.510485761.61048576   
1.6
  neon --b---8271.120972163.12098176   
3.1
cheese --b---2250.710483841.61049600   
1.6
 pizza --b---3590.5 5243520.8 525312   
0.8
   cassini --b---4900.431457924.73146752   
4.7
   fusilli --b---1590.2 5243520.8 525312   
0.8
   bob --b---5020.2   16777284   25.0   16778240  
25.0
   blender --b---1210.210486401.61049600   
1.6
 bread --b--- 690.1 5243520.8 525312   
0.8
  chickpea --b--- 670.1 5243520.8 525312   
0.8
lentil --b--- 670.1 2622080.4 263168   
0.4



Nevertheless, this should comparison should clearly be done using
doubles.  AFACT, get_cpu_pct() shouldn't ever return a NaN, so I think
this simple form is fine.

Oleksii: This is another bugfix to xentop, and should be considered for
4.19 at this point.

~Andrew


Regards,

Leigh.



[PATCH] tools/xentop: fix cpu% sort order

2024-05-14 Thread Leigh Brown
Although using integer comparison to compare doubles kind of
works, it's annoying to see domains slightly out of order when
sorting by cpu%.

Add a compare_dbl() function and update compare_cpu_pct() to
call it.

Signed-off-by: Leigh Brown 
---
 tools/xentop/xentop.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
index 545bd5e96d..99199caec9 100644
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -85,6 +85,7 @@ static void set_delay(const char *value);
 static void set_prompt(const char *new_prompt, void (*func)(const char *));
 static int handle_key(int);
 static int compare(unsigned long long, unsigned long long);
+static int compare_dbl(double, double);
 static int compare_domains(xenstat_domain **, xenstat_domain **);
 static unsigned long long tot_net_bytes( xenstat_domain *, int);
 static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long *);
@@ -422,6 +423,16 @@ static int compare(unsigned long long i1, unsigned long 
long i2)
return 0;
 }
 
+/* Compares two double precision numbers, returning -1,0,1 for <,=,> */
+static int compare_dbl(double d1, double d2)
+{
+   if(d1 < d2)
+   return -1;
+   if(d1 > d2)
+   return 1;
+   return 0;
+}
+
 /* Comparison function for use with qsort.  Compares two domains using the
  * current sort field. */
 static int compare_domains(xenstat_domain **domain1, xenstat_domain **domain2)
@@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)
 
 static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain *domain2)
 {
-   return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
+   return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));
 }
 
 /* Prints cpu percentage statistic */
-- 
2.39.2




Re: [PATCH for-4.19] libxl: fix population of the online vCPU bitmap for PVH

2024-05-10 Thread Leigh Brown

Hi Roger,

Thanks for responding and fixing this so quickly.

On 2024-05-10 13:49, Roger Pau Monne wrote:
libxl passes some information to libacpi to create the ACPI table for a 
PVH
guest, and among that information it's a bitmap of which vCPUs are 
online
which can be less than the maximum number of vCPUs assigned to the 
domain.


While the population of the bitmap is done correctly for HVM based on 
the
number of online vCPUs, for PVH the population of the bitmap is done 
based on
the number of maximum vCPUs allowed.  This leads to all local APIC 
entries in
the MADT being set as enabled, which contradicts the data in xenstore 
if vCPUs

is different than maximum vCPUs.

Fix by copying the internal libxl bitmap that's populated based on the 
vCPUs

parameter.

Reported-by: Arthur Borsboom 
Link: https://gitlab.com/libvirt/libvirt/-/issues/399
Reported-by: Leigh Brown 
Fixes: 14c0d328da2b ('libxl/acpi: Build ACPI tables for HVMlite 
guests')

Signed-off-by: Roger Pau Monné 
---
Note that the setup of hvm_info_table could be shared between PVH and 
HVM, as
the fields are very limited, see hvm_build_set_params() for the HVM 
side.
However this late in the release it's safer to just adjust the PVH 
path.


Also note the checksum is not provided when hvm_info_table is built for 
PVH.
This is fine so far because such checksum is only consumed by hvmloader 
and not

libacpi itself.

It's a bugfix, so should be considered for 4.19.
---
 tools/libs/light/libxl_x86_acpi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libs/light/libxl_x86_acpi.c 
b/tools/libs/light/libxl_x86_acpi.c

index 620f3c700c3e..5cf261bd6794 100644
--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -89,7 +89,7 @@ static int init_acpi_config(libxl__gc *gc,
 uint32_t domid = dom->guest_domid;
 xc_domaininfo_t info;
 struct hvm_info_table *hvminfo;
-int i, r, rc;
+int r, rc;

 config->dsdt_anycpu = config->dsdt_15cpu = dsdt_pvh;
 config->dsdt_anycpu_len = config->dsdt_15cpu_len = dsdt_pvh_len;
@@ -138,8 +138,8 @@ static int init_acpi_config(libxl__gc *gc,
 hvminfo->nr_vcpus = info.max_vcpu_id + 1;
 }

-for (i = 0; i < hvminfo->nr_vcpus; i++)
-hvminfo->vcpu_online[i / 8] |= 1 << (i & 7);
+memcpy(hvminfo->vcpu_online, b_info->avail_vcpus.map,
+   b_info->avail_vcpus.size);

 config->hvminfo = hvminfo;


Tested-by: Leigh Brown 

Regards,

Leigh.



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

2024-05-09 Thread 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
enp0s01 PVID Egress Untagged
  10
  20
br0   1 PVID Egress Untagged
vif1.01 PVID Egress Untagged
vif2.01 PVID Egress Untagged
vif3.010 PVID Egress Untagged
vif4.010 PVID Egress Untagged
vif5.020 PVID Egress Untagged
vif6.030 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 

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

2024-05-08 Thread Leigh Brown
Document the new `vlan' keyword in xl-network-configuration(5).

Signed-off-by: Leigh Brown 
---
 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 - a single VLAN ID in the range 1 to 4094. This can optionally followed
+by a B to indicate the PVID or by a B to indicate an untagged VLAN. C
+implies B.
+
+=item *
+
+B-B - a range of VLAN IDs from B to B, both between
+1 and 4094 and B being less than or equal to B. This can be
+optionally followed by a B to indicate that the range of VLANs are untagged.
+
+=item *
+
+B+BxB - describing a range of VLAN IDs starting at B
+with B additional entries, each incremented by B. This can be 
+optionally followed by a B 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), the B 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 2/5] tools/xl: add vlan keyword to vif option

2024-05-08 Thread 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 
---
 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 0/5] Add bridge VLAN support

2024-05-08 Thread 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  
enp0s01 PVID Egress Untagged
  10
  20
br0   1 PVID Egress Untagged
vif1.01 PVID Egress Untagged
vif2.01 PVID Egress Untagged
vif3.010 PVID Egress Untagged
vif4.010 PVID Egress Untagged
vif5.020 PVID Egress Untagged
vif6.030 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

2024-05-08 Thread 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 
---
 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 5/5] tools/examples: Example Linux bridge VLAN config

2024-05-08 Thread 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 
---
 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 00..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//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 00..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 00..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 00..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




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

2024-05-08 Thread 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 
---
 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 1/5] tools/libs/light: Add vid field to libxl_device_nic

2024-05-03 Thread Leigh Brown
Add integer member `vid' to libxl_device_nic, to represent the
VLAN ID to assign to the VIF when adding it to the bridge device.

Update libxl_nic.c to read and write the vid 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
then xenstore and perform the required configuration.

Signed-off-by: Leigh Brown 
---
 tools/libs/light/libxl_nic.c | 20 
 tools/libs/light/libxl_types.idl |  1 +
 2 files changed, 21 insertions(+)

diff --git a/tools/libs/light/libxl_nic.c b/tools/libs/light/libxl_nic.c
index d6bf06fc34..e28b9101ee 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->vid) {
+flexarray_append(back, "vid");
+flexarray_append(back, GCSPRINTF("%u", nic->vid));
+}
+
 flexarray_append(back, "bridge");
 flexarray_append(back, libxl__strdup(gc, nic->bridge));
 flexarray_append(back, "handle");
@@ -313,6 +318,21 @@ 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/vid", libxl_path), &tmp);
+if (rc) goto out;
+if (tmp) {
+char *endptr;
+
+nic->vid = strtol(tmp, &endptr, 10);
+if (*endptr != '\0') {
+rc = ERROR_INVAL;
+goto out;
+}
+} else {
+nic->vid = 0;
+}
+
 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..df5185128c 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),
+("vid", integer),
 ("model", string),
 ("mac", libxl_mac),
 ("ip", string),
-- 
2.39.2




[RFC PATCH 0/5] Add bridge VLAN support

2024-05-03 Thread Leigh Brown
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  
enp0s01 PVID Egress Untagged
  10
  20
br0   1 PVID Egress Untagged
vif1.01 PVID Egress Untagged
vif2.010 PVID Egress Untagged
vif3.010 PVID Egress Untagged
vif4.020 PVID Egress Untagged
vif5.020 PVID Egress Untagged
vif6.030 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.


le...@solinno.co.uk (5):
  tools/libs/light: Add vid field to libxl_device_nic
  tools/xl: add vid keyword vif option
  tools/hotplug/Linux: Add bridge VLAN support
  docs/man: document VIF vid keyword
  tools/examples: Examples Linux bridge VLAN config

 docs/man/xl-network-configuration.5.pod.in|  6 +++
 tools/examples/linux-bridge-vlan/README   | 52 +++
 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 |  9 
 tools/libs/light/libxl_nic.c  | 20 +++
 tools/libs/light/libxl_types.idl  |  1 +
 tools/xl/xl_parse.c   |  2 +
 9 files changed, 121 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 4/5] docs/man: document VIF vid keyword

2024-05-03 Thread Leigh Brown
Document the new `vid' keyword in xl-network-configuration(5).

Signed-off-by: Leigh Brown 
---
 docs/man/xl-network-configuration.5.pod.in | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/docs/man/xl-network-configuration.5.pod.in 
b/docs/man/xl-network-configuration.5.pod.in
index f3e379bcf8..fe2615ae30 100644
--- a/docs/man/xl-network-configuration.5.pod.in
+++ b/docs/man/xl-network-configuration.5.pod.in
@@ -259,6 +259,12 @@ 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 vid
+
+Specifies the VLAN ID. If this is set to a non-zero value, it will be specified
+when attaching the VIF to a bridge.  This can be used on operating systems that
+support bridge VLANs (e.g. Linux using iproute2).
+
 =head2 trusted / untrusted
 
 An advisory setting for the frontend driver on whether the backend should be
-- 
2.39.2




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

2024-05-03 Thread Leigh Brown
Update add_to_bridge shell function to read the vid parameter
from xenstore and set the bridge LAN for the VID to the given
value. This only works when using the iproute2 bridge command,
so a warning is issued if using the legacy brctl command and a
vid is set.

Signed-off-by: Leigh Brown 
---
 tools/hotplug/Linux/xen-network-common.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tools/hotplug/Linux/xen-network-common.sh 
b/tools/hotplug/Linux/xen-network-common.sh
index 42fa704e8d..19a8b3c7e5 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -125,14 +125,23 @@ create_bridge () {
 add_to_bridge () {
 local bridge=$1
 local dev=$2
+local vid=$(xenstore_read_default "$XENBUS_PATH/vid" "")
 
 # 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}
+if [ -n "${vid}" ] ;then
+log warning "bridge command not available, ignoring vid"
+fi
 else
 ip link set ${dev} master ${bridge}
+if [ -n "${vid}" ] ;then
+log debug "Assigning $dev to vid $vid"
+bridge vlan del dev ${dev} vid 1
+bridge vlan add dev ${dev} vid ${vid} pvid untagged
+fi
 fi
 else
 log debug "$dev already on bridge $bridge"
-- 
2.39.2




[RFC PATCH 2/5] tools/xl: add vid keyword vif option

2024-05-03 Thread Leigh Brown
Update parse_nic_config() to support a new `vid' keyword. This
keyword specifies the numeric VLAN ID to assign to the VIF when
attaching it to the bridge port, on operating systems that support
the capability (e.g. Linux).

Signed-off-by: Leigh Brown 
---
 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..5d5dd4ec04 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("vid", token, oparg)) {
+nic->vid = parse_ulong(oparg);
 } else if (!strcmp("trusted", token)) {
 libxl_defbool_set(&nic->trusted, true);
 } else if (!strcmp("untrusted", token)) {
-- 
2.39.2




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

2024-05-03 Thread Leigh Brown
Add a new directory linux-bridge-vlan showing how to configure
systemd-networkd to support a bridge VLAN configuration.

Signed-off-by: Leigh Brown 
---
 tools/examples/linux-bridge-vlan/README   | 52 +++
 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, 83 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 00..b287710e0f
--- /dev/null
+++ b/tools/examples/linux-bridge-vlan/README
@@ -0,0 +1,52 @@
+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 the VLAN id (vid) of the required VLAN.
+
+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.  I
+   observed them with the pvid in the switch MAC address table.
+
+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 and
+   there is 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 that VLAN id in this file.
+
+
+Domain configuration
+
+
+Add the vid= keyword to the vif definition in the domain. For example:
+
+vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vid=10' ]
+
+
+Hints and tips
+--
+
+1. To check if vlan_filtering is enabled, run:
+   # cat /sys/devices/virtual/net//bridge/vlan_filtering
+
+2. To check the bridge port VLAN assignments, run:
+   # bridge vlan
+
+3. To check the vid setting in the xenstore, run:
+   # xenstore-ls -f | grep 'vid ='
+
diff --git a/tools/examples/linux-bridge-vlan/br0.netdev 
b/tools/examples/linux-bridge-vlan/br0.netdev
new file mode 100644
index 00..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 00..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 00..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: [PATCH v2 2/6] tools/misc: rework xenwatchdogd signal handling

2024-04-11 Thread Leigh Brown

Hi Andrew,

On 2024-04-11 13:12, Andrew Cooper wrote:

On 29/03/2024 11:10 am, le...@solinno.co.uk wrote:

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 2f7c822d61..35a0df655a 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -9,9 +9,11 @@
 #include 
 #include 
 #include 
+#include 

 xc_interface *h;
-int id = 0;
+static bool safeexit = false;
+static bool done = false;


It's a common bug, but these need to be volatile.  Right now, ...


I'm an idiot (I'm sure I've mentioned this a couple of times :-) )


@@ -90,10 +90,14 @@ int main(int argc, char **argv)
 if (id <= 0)
 err(EXIT_FAILURE, "xc_watchdog setup");

-for (;;) {
+while (!done) {
 sleep(s);


... the only reason this isn't an infinite loop is because the compiler
can't prove that sleep() doesn't modify the variable.  This goes wrong
in subtle and fun ways when using LTO.

I'll fix on commit.

For the record, I've taken 1-3 which are ready.  You'll need to rework 
4

and later per Anthony's feedback.

~Andrew


Thanks, I will get those done this evening, hopefully.

Regards,

Leigh.