Re: port-xen/59451: XEN3_DOM0 kernel finds the wrong root device

["Chuck Zmudzinski via gnats" <gnats-admin%NetBSD.org@localhost>]

The following reply was made to PR port-xen/59451; it has been noted by GNATS.

From: Chuck Zmudzinski <frchuckz%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: port-xen/59451: XEN3_DOM0 kernel finds the wrong root device
Date: Mon, 2 Jun 2025 19:41:32 -0400

 On 6/2/2025 8:20 AM, Michael van Elst via gnats wrote:
 > The following reply was made to PR port-xen/59451; it has been noted by GNATS.
 > 
 > From: mlelstv%serpens.de@localhost (Michael van Elst)
 > To: gnats-bugs%netbsd.org@localhost
 > Cc: 
 > Subject: Re: port-xen/59451: XEN3_DOM0 kernel finds the wrong root device
 > Date: Mon, 2 Jun 2025 12:18:41 -0000 (UTC)
 > 
 >  gnats-admin%NetBSD.org@localhost ("Chuck Zmudzinski via gnats") writes:
 >  
 >  > Still, the bootdev=dk12 form needs some fix/kernel patch for this case when
 >  > NetBSD/xen is installed on a wedge with a double digit index, so this PR should
 >  > remain open.
 >  
 >  Yes. I'm currently testing the following patch. The parsing is
 >  still not perfect, e.g. it accepts a 'partition' for devices that
 >  don't support partitions (like dk or dm).
 
 I see your code accepts a 'partition' for dk, but I don't see why for dm.
 We have this:
 
 is_disk = is_valid_disk(dv);
 
 and this:
 
 static int
 is_valid_disk(device_t dv)
 {
 	if (device_class(dv) != DV_DISK)
 		return (0);
 
 	return (device_is_a(dv, "dk") ||
 		device_is_a(dv, "sd") ||
 		device_is_a(dv, "wd") ||
 		device_is_a(dv, "ld") ||
 		device_is_a(dv, "ed") ||
 		device_is_a(dv, "xbd"));
 }
 
 "dk" is there as true for is_disk, but not "dm" and that makes
 sense because my understanding is that NetBSD/xen does not currently support
 booting with an LVM device as root. If we have an LVM device like dm0 and
 set bootdev=dm0 when trying to boot NetBSD/xen PV dom0, what do you think
 would happen with our current code and with your proposed patch? It looks to
 me like you would never get a match, so would the kernel just try dm0 and
 see what happens?
 
 >  
 >  Index: xen_machdep.c
 >  ===================================================================
 >  RCS file: /cvsroot/src/sys/arch/xen/xen/xen_machdep.c,v
 >  retrieving revision 1.29
 >  diff -p -u -r1.29 xen_machdep.c
 >  --- xen_machdep.c       17 Oct 2023 10:24:11 -0000      1.29
 >  +++ xen_machdep.c       2 Jun 2025 11:56:22 -0000
 >  @@ -62,6 +62,7 @@ __KERNEL_RCSID(0, "$NetBSD: xen_machdep.
 >   #include <sys/boot_flag.h>
 >   #include <sys/conf.h>
 >   #include <sys/disk.h>
 >  +#include <sys/disklabel.h>
 >   #include <sys/device.h>
 >   #include <sys/mount.h>
 >   #include <sys/reboot.h>
 >  @@ -546,14 +547,28 @@ xen_bootconf(void)
 >                          break;
 >                  }
 >   
 >  -               if (strncmp(xcp.xcp_bootdev, devname, strlen(devname)))
 >  -                       continue;
 >  +               if (is_disk) {
 >  +                       size_t len;
 >  +                       int part;
 >  +
 >  +                       len = strlen(devname);
 >  +                       if  (strncmp(xcp.xcp_bootdev, devname, len) != 0)
 >  +                               continue;
 >  +
 >  +                       if (xcp.xcp_bootdev[len] != '\0') {
 >  +                               part = xcp.xcp_bootdev[len] - 'a';
 >  +                               if (part < 0 || part >= MAXPARTITIONS)
 >  +                                       continue;
 >  +
 >  +                               if (xcp.xcp_bootdev[len+1] != '\0')
 >  +                                       continue;
 >   
 >  -               if (is_disk && strlen(xcp.xcp_bootdev) > strlen(devname)) {
 >  -                       /* XXX check device_cfdata as in x86_autoconf.c? */
 >  -                       booted_partition = toupper(
 >  -                               xcp.xcp_bootdev[strlen(devname)]) - 'A';
 >  -                       DPRINTF(("%s: booted_partition: %d\n", __func__, booted_partition));
 >  +                               booted_partition = part;
 >  +                               DPRINTF(("%s: booted_partition: %d\n", __func__, booted_partition));
 >  +                       }
 >  +               } else {
 >  +                       if  (strcmp(xcp.xcp_bootdev, devname) != 0)
 >  +                               continue;
 >                  }
 >   
 >                  booted_device = dv;
 >  
 >  
 >  > >  Also, according to a message I received on netbsd-users from Manuel who AFAIK is
 >  > >  a port-xen maintainer, there is no difference in the arch/xen code between
 >  > >  bootdev= and rootdev=. What you say here puts that in doubt, though.
 >  > 
 >  > My testing verified that Manuel is correct: bootdev= and root= behave the same way,
 >  > but actually root= is better because it gives me three more characters for the
 >  > boot command line before it overflows and starts to truncate the end of the command
 >  > line.
 >  
 >  
 >  bootdev= and root= currently work the same way.
 >  
 >  The supplied string is checked against all disk and network interface
 >  device names.
 >  
 >  If there is a match, then the result is a pointer "booted_device"
 >  to the driver and an integer "booted_partition". The kernel will
 >  later try to access the particular driver and read data from the
 >  numbered partition. Obviously the partition is ignored for a
 >  network interface, and it should be 0 for something like a wedge
 >  (that doesn't have partitions).
 >  
 >  If there is no match, then the string is passed as is, similar to
 >  a hardcoded embedded "config root" in the kernel config file. The
 >  result is the string pointer "bootspec" that is evaluated later
 >  by the kernel.
 >  
 >  In either case, the supplied string can have 144 bytes (including
 >  the terminating NUL character), but the parser truncates the
 >  full command line to 255 characters first, not sure why.
 >