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 10:00:56 -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).
 
 Yeah, the patches I have been testing also have this problem. Couldn't
 we add a supports_partitions(device_t dev) function?
 
 >  
 >  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>
 
 Yes, I have this to pull in MAXPARTITIONS
 
 >   #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;
 
 This is exactly what I am also doing to validate the value for booted_partition,
 except I keep toupper(...) and subtract 'A' instead of 'a', presumably so the code
 works if bootdev=wd0A instead of bootdev=wd0a in boot.cfg. I see the code is
 not consistent about this. I don't know if this is the only place we are
 currently using toupper and 'A'.
 
 >  +
 >  +                               if (xcp.xcp_bootdev[len+1] != '\0')
 >  +                                       continue;
 
 I do not have this in the patch I am testing. This looks like another
 sanity check to prevent accepting a match between bootdev=wd0a1 and
 devname=wd0. I think what we have now would accept such a match, so
 I suppose this is an improvement over what we have.
 
 >   
 >  -               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;
 
 The patch I am pondering avoids this if .. else logic but it looks like this
 is needed the way this patch is organized to make sure we find the bootdev
 when we have is_ifnet instead of is_disk.
 
 >                  }
 >   
 >                  booted_device = dv;