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: Tue, 3 Jun 2025 11:57:14 -0400

 On 6/3/2025 8:25 AM, Chuck Zmudzinski via gnats wrote:
 > 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: Tue, 3 Jun 2025 08:22:27 -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).
 >  >  
 >  >  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;
 >  >  
 >  
 >  So taking into account everything I have learned, the only thing I would
 >  add to this patch is a check that we do not have a "dk" wedge device
 >  before we try to find a partition and set booted_partition. Probably replacing
 >  the first insertion of the second hunk:
 >  
 >   +               if (is_disk) {
 >  
 >  with
 >  
 >   +               if (is_disk && !device_is_a(dv, "dk")) {
 >  
 >  But I have not tested this yet.
 >  
 >  This patch that attempts to fix the parsing problems here is
 >  probably only suitable for -current. I still plan to propose a
 >  conservative patch more suitable for the stable branches that
 >  will be less likely to introduce unintended consequences for
 >  stable users.
 >  
 
 Here is my proposed conservative patch for the stable branches:
 
 --- xen_machdep.c	2023-10-18 12:53:03.000000000 -0400
 +++ xen_machdep.c	2025-06-03 10:22:46.222889485 -0400
 @@ -62,6 +62,7 @@
  #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>
 @@ -90,6 +91,8 @@
  #define DPRINTF(a)
  #endif
  
 +#define PARTITION_IS_VALID(part) \
 +	((part >= 0) && (part < MAXPARTITIONS))
  
  bool xen_suspend_allow;
  
 @@ -549,10 +552,16 @@
  		if (strncmp(xcp.xcp_bootdev, devname, strlen(devname)))
  			continue;
  
 +		if (device_is_a(dv, "dk"))
 +			continue;
 +
  		if (is_disk && strlen(xcp.xcp_bootdev) > strlen(devname)) {
  			/* XXX check device_cfdata as in x86_autoconf.c? */
 -			booted_partition = toupper(
 +			int part = toupper(
  				xcp.xcp_bootdev[strlen(devname)]) - 'A';
 +			if (!PARTITION_IS_VALID(part))
 +				continue;
 +			booted_partition = part;
  			DPRINTF(("%s: booted_partition: %d\n", __func__, booted_partition));
  		}
  
 
 I tested this on netbsd-10 and it fixes the problem. This file is the same in
 -current, so it should also work for -current.