[PATCH] Install to LVM PVs

[PATCH] Install to LVM PVs

[Gabriel de Perthuis]

Hello,
This patch lets grub install to a reserved area in LVM physical volumes.
These bootloader areas can be created with LVM 2.02.99 and the
--bootloaderareasize argument to pvcreate and vgconvert.
I tested it in QEMU, installing to and booting a disk that contains a PV
and no partition table.


=== modified file 'grub-core/disk/lvm.c'
--- grub-core/disk/lvm.c	2012-06-25 15:52:20 +0000
+++ grub-core/disk/lvm.c	2013-09-25 11:03:21 +0000
@@ -95,6 +95,38 @@
     }
 }
 
+static struct grub_lvm_pv_header *
+grub_lvm_get_pvh(grub_disk_t disk, char buf[static GRUB_LVM_LABEL_SIZE])
+{
+  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
+  unsigned int i;
+
+  /* Search for label. */
+  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
+    {
+      if (grub_disk_read (disk, i, 0, GRUB_LVM_LABEL_SIZE, buf))
+	return NULL;
+
+      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
+			   sizeof (lh->id)))
+	  && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
+			      sizeof (lh->type))))
+	break;
+    }
+
+  /* Return if we didn't find a label. */
+  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
+    {
+#ifdef GRUB_UTIL
+      grub_util_info ("no LVM signature found");
+#endif
+      return NULL;
+    }
+
+  return (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
+}
+
+
 static struct grub_diskfilter_vg * 
 grub_lvm_detect (grub_disk_t disk,
 		 struct grub_diskfilter_pv_id *id,
@@ -102,11 +134,10 @@
 {
   grub_err_t err;
   grub_uint64_t mda_offset, mda_size;
-  char buf[GRUB_LVM_LABEL_SIZE];
   char vg_id[GRUB_LVM_ID_STRLEN+1];
   char pv_id[GRUB_LVM_ID_STRLEN+1];
+  char buf[GRUB_LVM_LABEL_SIZE];
   char *metadatabuf, *p, *q, *vgname;
-  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
   struct grub_lvm_pv_header *pvh;
   struct grub_lvm_disk_locn *dlocn;
   struct grub_lvm_mda_header *mdah;
@@ -115,30 +146,9 @@
   struct grub_diskfilter_vg *vg;
   struct grub_diskfilter_pv *pv;
 
-  /* Search for label. */
-  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
-    {
-      err = grub_disk_read (disk, i, 0, sizeof(buf), buf);
-      if (err)
-	goto fail;
-
-      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
-			   sizeof (lh->id)))
-	  && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
-			      sizeof (lh->type))))
-	break;
-    }
-
-  /* Return if we didn't find a label. */
-  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
-    {
-#ifdef GRUB_UTIL
-      grub_util_info ("no LVM signature found");
-#endif
-      goto fail;
-    }
-
-  pvh = (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
+  pvh = grub_lvm_get_pvh(disk, buf);
+  if (! pvh)
+    goto fail;
 
   for (i = 0, j = 0; i < GRUB_LVM_ID_LEN; i++)
     {
@@ -151,7 +161,7 @@
   dlocn = pvh->disk_areas_xl;
 
   dlocn++;
-  /* Is it possible to have multiple data/metadata areas? I haven't
+  /* Is it possible to have multiple data areas? I haven't
      seen devices that have it. */
   if (dlocn->offset)
     {
@@ -746,6 +756,88 @@
   return NULL;
 }
 
+#ifdef GRUB_UTIL
+int
+grub_util_is_lvm(grub_disk_t disk)
+{
+  struct grub_diskfilter_pv_id id;
+  struct grub_diskfilter_vg *vg;
+  grub_disk_addr_t start_sector;
+  vg = grub_lvm_detect(disk, &id, &start_sector);
+  if (! vg)
+    return 0;
+  /* don't free the vg, it's held by grub_diskfilter_vg_register */
+  grub_free(id.uuid);
+  return 1;
+}
+
+grub_err_t
+grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
+		     unsigned int max_nsectors,
+		     grub_embed_type_t embed_type,
+		     grub_disk_addr_t **sectors)
+{
+  char buf[GRUB_LVM_LABEL_SIZE];
+  struct grub_lvm_pv_header *pvh;
+  struct grub_lvm_pv_header_ext *pvh_ext;
+  struct grub_diskfilter_pv *pv = NULL;
+  struct grub_diskfilter_vg *vg = NULL;
+  struct grub_lvm_disk_locn *dlocn;
+  grub_uint64_t ba_offset, ba_size, ba_start_sector;
+  unsigned int i;
+
+  if (embed_type != GRUB_EMBED_PCBIOS)
+    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+		       "LVM curently supports only PC-BIOS embedding");
+  if (disk->partition)
+    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
+  pv = grub_diskfilter_get_pv_from_disk (disk, &vg);
+  if (! pv)
+    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
+
+  pvh = grub_lvm_get_pvh(disk, buf);
+  if (! pvh)
+    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
+
+  dlocn = pvh->disk_areas_xl;
+
+  /* skip past the data area list */
+  while (dlocn->offset)
+    dlocn++;
+  dlocn++;
+  /* and the metadata area list */
+  while (dlocn->offset)
+    dlocn++;
+  dlocn++;
+
+  pvh_ext = (struct grub_lvm_pv_header_ext*)dlocn;
+  if (! pvh_ext->version_xl)
+    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");
+
+  dlocn = pvh_ext->disk_areas_xl;
+  ba_offset = grub_le_to_cpu64 (dlocn->offset);
+  ba_size = grub_le_to_cpu64 (dlocn->size);
+  if (! (ba_offset && ba_size))
+    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");
+  /* could be worked around with extra arithmetic if this actually happens */
+  if (ba_offset % GRUB_DISK_SECTOR_SIZE)
+    return grub_error (
+      GRUB_ERR_BUG, "LVM bootloader area is improperly aligned");
+  ba_start_sector = ba_offset / GRUB_DISK_SECTOR_SIZE;
+
+  *nsectors = ba_size / GRUB_DISK_SECTOR_SIZE;
+  if (*nsectors > max_nsectors)
+    *nsectors = max_nsectors;
+
+  *sectors = grub_malloc (*nsectors * sizeof (**sectors));
+  if (!*sectors)
+    return grub_errno;
+  for (i = 0; i < *nsectors; i++)
+    (*sectors)[i] = ba_start_sector + i;
+
+  return GRUB_ERR_NONE;
+}
+#endif
 \f
 
 static struct grub_diskfilter grub_lvm_dev = {

=== modified file 'include/grub/diskfilter.h'
--- include/grub/diskfilter.h	2012-06-25 15:36:50 +0000
+++ include/grub/diskfilter.h	2013-09-25 08:59:05 +0000
@@ -75,6 +75,9 @@
 #ifdef GRUB_UTIL
   char **partmaps;
 #endif
+  /* Optional bootloader embedding area */
+  grub_uint64_t ba_offset;
+  grub_uint64_t ba_size;
 };
 
 struct grub_diskfilter_lv {

=== modified file 'include/grub/emu/hostdisk.h'
--- include/grub/emu/hostdisk.h	2013-08-22 14:50:12 +0000
+++ include/grub/emu/hostdisk.h	2013-09-25 07:59:06 +0000
@@ -44,9 +44,16 @@
 char *
 grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
 int
+grub_util_is_lvm (grub_disk_t disk);
+int
 grub_util_is_ldm (grub_disk_t disk);
 #ifdef GRUB_UTIL
 grub_err_t
+grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
+		     unsigned int max_nsectors,
+		     grub_embed_type_t embed_type,
+		     grub_disk_addr_t **sectors);
+grub_err_t
 grub_util_ldm_embed (struct grub_disk *disk, unsigned int *nsectors,
 		     unsigned int max_nsectors,
 		     grub_embed_type_t embed_type,

=== modified file 'include/grub/lvm.h'
--- include/grub/lvm.h	2012-01-29 13:28:01 +0000
+++ include/grub/lvm.h	2013-09-25 08:32:31 +0000
@@ -62,6 +62,13 @@
   struct grub_lvm_disk_locn disk_areas_xl[0];	/* Two lists */
 } __attribute__ ((packed));
 
+struct grub_lvm_pv_header_ext {
+  grub_uint32_t version_xl;
+  grub_uint32_t flags_xl;
+  /* NULL-terminated list of bootloader embedding areas */
+  struct grub_lvm_disk_locn disk_areas_xl[0];
+} __attribute__ ((packed));
+
 #define GRUB_LVM_FMTT_MAGIC "\040\114\126\115\062\040\170\133\065\101\045\162\060\116\052\076"
 #define GRUB_LVM_FMTT_VERSION 1
 #define GRUB_LVM_MDA_HEADER_SIZE 512

=== modified file 'util/grub-setup.c'
--- util/grub-setup.c	2013-04-04 06:55:06 +0000
+++ util/grub-setup.c	2013-09-25 11:31:47 +0000
@@ -387,7 +387,7 @@
       .container = dest_dev->disk->partition,
       .multiple_partmaps = 0
     };
-    int is_ldm;
+    int is_ldm, is_lvm;
     grub_err_t err;
     grub_disk_addr_t *sectors;
     int i;
@@ -411,8 +411,9 @@
       grub_errno = GRUB_ERR_NONE;
 
     is_ldm = grub_util_is_ldm (dest_dev->disk);
+    is_lvm = grub_util_is_lvm (dest_dev->disk);
 
-    if (fs_probe)
+    if (!is_lvm && fs_probe)
       {
 	if (!fs && !ctx.dest_partmap)
 	  grub_util_error (_("unable to identify a filesystem in %s; safety check can't be performed"),
@@ -459,7 +460,7 @@
 
     free (tmp_img);
     
-    if (! ctx.dest_partmap && ! fs && !is_ldm)
+    if (! ctx.dest_partmap && ! fs && !is_ldm && !is_lvm)
       {
 	grub_util_warn ("%s", _("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
 	goto unable_to_embed;
@@ -492,7 +493,10 @@
       maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
 		>> GRUB_DISK_SECTOR_BITS);
 
-    if (is_ldm)
+    if (is_lvm)
+      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
+				 GRUB_EMBED_PCBIOS, &sectors);
+    else if (is_ldm)
       err = grub_util_ldm_embed (dest_dev->disk, &nsec, maxsec,
 				 GRUB_EMBED_PCBIOS, &sectors);
     else if (ctx.dest_partmap)
@@ -588,7 +592,7 @@
 
   if (dest_dev->disk->dev->id != root_dev->disk->dev->id)
     grub_util_error ("%s", _("embedding is not possible, but this is required for "
-			     "RAID and LVM install"));
+			     "RAID install"));
 
   {
     grub_fs_t fs;

Re: [PATCH] Install to LVM PVs

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 10367 bytes --]

On 25.09.2013 14:39, Gabriel de Perthuis wrote:
> Hello,
> This patch lets grub install to a reserved area in LVM physical volumes.
> These bootloader areas can be created with LVM 2.02.99 and the
> --bootloaderareasize argument to pvcreate and vgconvert.
> I tested it in QEMU, installing to and booting a disk that contains a PV
> and no partition table.
> 
This is not how the use of this area was imagined. There are couple of
subtleties which your patch didn't take in account.
Currently there is joint developpement with LVM guys but it wasn't
published yet.
> 
> === modified file 'grub-core/disk/lvm.c'
> --- grub-core/disk/lvm.c	2012-06-25 15:52:20 +0000
> +++ grub-core/disk/lvm.c	2013-09-25 11:03:21 +0000
> @@ -95,6 +95,38 @@
>      }
>  }
>  
> +static struct grub_lvm_pv_header *
> +grub_lvm_get_pvh(grub_disk_t disk, char buf[static GRUB_LVM_LABEL_SIZE])
> +{
> +  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
> +  unsigned int i;
> +
> +  /* Search for label. */
> +  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
> +    {
> +      if (grub_disk_read (disk, i, 0, GRUB_LVM_LABEL_SIZE, buf))
> +	return NULL;
> +
> +      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
> +			   sizeof (lh->id)))
> +	  && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
> +			      sizeof (lh->type))))
> +	break;
> +    }
> +
> +  /* Return if we didn't find a label. */
> +  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
> +    {
> +#ifdef GRUB_UTIL
> +      grub_util_info ("no LVM signature found");
> +#endif
> +      return NULL;
> +    }
> +
> +  return (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
> +}
> +
> +
>  static struct grub_diskfilter_vg * 
>  grub_lvm_detect (grub_disk_t disk,
>  		 struct grub_diskfilter_pv_id *id,
> @@ -102,11 +134,10 @@
>  {
>    grub_err_t err;
>    grub_uint64_t mda_offset, mda_size;
> -  char buf[GRUB_LVM_LABEL_SIZE];
>    char vg_id[GRUB_LVM_ID_STRLEN+1];
>    char pv_id[GRUB_LVM_ID_STRLEN+1];
> +  char buf[GRUB_LVM_LABEL_SIZE];
>    char *metadatabuf, *p, *q, *vgname;
> -  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
>    struct grub_lvm_pv_header *pvh;
>    struct grub_lvm_disk_locn *dlocn;
>    struct grub_lvm_mda_header *mdah;
> @@ -115,30 +146,9 @@
>    struct grub_diskfilter_vg *vg;
>    struct grub_diskfilter_pv *pv;
>  
> -  /* Search for label. */
> -  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
> -    {
> -      err = grub_disk_read (disk, i, 0, sizeof(buf), buf);
> -      if (err)
> -	goto fail;
> -
> -      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
> -			   sizeof (lh->id)))
> -	  && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
> -			      sizeof (lh->type))))
> -	break;
> -    }
> -
> -  /* Return if we didn't find a label. */
> -  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
> -    {
> -#ifdef GRUB_UTIL
> -      grub_util_info ("no LVM signature found");
> -#endif
> -      goto fail;
> -    }
> -
> -  pvh = (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
> +  pvh = grub_lvm_get_pvh(disk, buf);
> +  if (! pvh)
> +    goto fail;
>  
>    for (i = 0, j = 0; i < GRUB_LVM_ID_LEN; i++)
>      {
> @@ -151,7 +161,7 @@
>    dlocn = pvh->disk_areas_xl;
>  
>    dlocn++;
> -  /* Is it possible to have multiple data/metadata areas? I haven't
> +  /* Is it possible to have multiple data areas? I haven't
>       seen devices that have it. */
>    if (dlocn->offset)
>      {
> @@ -746,6 +756,88 @@
>    return NULL;
>  }
>  
> +#ifdef GRUB_UTIL
> +int
> +grub_util_is_lvm(grub_disk_t disk)
> +{
> +  struct grub_diskfilter_pv_id id;
> +  struct grub_diskfilter_vg *vg;
> +  grub_disk_addr_t start_sector;
> +  vg = grub_lvm_detect(disk, &id, &start_sector);
> +  if (! vg)
> +    return 0;
> +  /* don't free the vg, it's held by grub_diskfilter_vg_register */
> +  grub_free(id.uuid);
> +  return 1;
> +}
> +
> +grub_err_t
> +grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
> +		     unsigned int max_nsectors,
> +		     grub_embed_type_t embed_type,
> +		     grub_disk_addr_t **sectors)
> +{
> +  char buf[GRUB_LVM_LABEL_SIZE];
> +  struct grub_lvm_pv_header *pvh;
> +  struct grub_lvm_pv_header_ext *pvh_ext;
> +  struct grub_diskfilter_pv *pv = NULL;
> +  struct grub_diskfilter_vg *vg = NULL;
> +  struct grub_lvm_disk_locn *dlocn;
> +  grub_uint64_t ba_offset, ba_size, ba_start_sector;
> +  unsigned int i;
> +
> +  if (embed_type != GRUB_EMBED_PCBIOS)
> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> +		       "LVM curently supports only PC-BIOS embedding");
> +  if (disk->partition)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> +  pv = grub_diskfilter_get_pv_from_disk (disk, &vg);
> +  if (! pv)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> +
> +  pvh = grub_lvm_get_pvh(disk, buf);
> +  if (! pvh)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> +
> +  dlocn = pvh->disk_areas_xl;
> +
> +  /* skip past the data area list */
> +  while (dlocn->offset)
> +    dlocn++;
> +  dlocn++;
> +  /* and the metadata area list */
> +  while (dlocn->offset)
> +    dlocn++;
> +  dlocn++;
> +
> +  pvh_ext = (struct grub_lvm_pv_header_ext*)dlocn;
> +  if (! pvh_ext->version_xl)
> +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");
> +
> +  dlocn = pvh_ext->disk_areas_xl;
> +  ba_offset = grub_le_to_cpu64 (dlocn->offset);
> +  ba_size = grub_le_to_cpu64 (dlocn->size);
> +  if (! (ba_offset && ba_size))
> +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");
> +  /* could be worked around with extra arithmetic if this actually happens */
> +  if (ba_offset % GRUB_DISK_SECTOR_SIZE)
> +    return grub_error (
> +      GRUB_ERR_BUG, "LVM bootloader area is improperly aligned");
> +  ba_start_sector = ba_offset / GRUB_DISK_SECTOR_SIZE;
> +
> +  *nsectors = ba_size / GRUB_DISK_SECTOR_SIZE;
> +  if (*nsectors > max_nsectors)
> +    *nsectors = max_nsectors;
> +
> +  *sectors = grub_malloc (*nsectors * sizeof (**sectors));
> +  if (!*sectors)
> +    return grub_errno;
> +  for (i = 0; i < *nsectors; i++)
> +    (*sectors)[i] = ba_start_sector + i;
> +
> +  return GRUB_ERR_NONE;
> +}
> +#endif
>  \f
>  
>  static struct grub_diskfilter grub_lvm_dev = {
> 
> === modified file 'include/grub/diskfilter.h'
> --- include/grub/diskfilter.h	2012-06-25 15:36:50 +0000
> +++ include/grub/diskfilter.h	2013-09-25 08:59:05 +0000
> @@ -75,6 +75,9 @@
>  #ifdef GRUB_UTIL
>    char **partmaps;
>  #endif
> +  /* Optional bootloader embedding area */
> +  grub_uint64_t ba_offset;
> +  grub_uint64_t ba_size;
>  };
>  
>  struct grub_diskfilter_lv {
> 
> === modified file 'include/grub/emu/hostdisk.h'
> --- include/grub/emu/hostdisk.h	2013-08-22 14:50:12 +0000
> +++ include/grub/emu/hostdisk.h	2013-09-25 07:59:06 +0000
> @@ -44,9 +44,16 @@
>  char *
>  grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
>  int
> +grub_util_is_lvm (grub_disk_t disk);
> +int
>  grub_util_is_ldm (grub_disk_t disk);
>  #ifdef GRUB_UTIL
>  grub_err_t
> +grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
> +		     unsigned int max_nsectors,
> +		     grub_embed_type_t embed_type,
> +		     grub_disk_addr_t **sectors);
> +grub_err_t
>  grub_util_ldm_embed (struct grub_disk *disk, unsigned int *nsectors,
>  		     unsigned int max_nsectors,
>  		     grub_embed_type_t embed_type,
> 
> === modified file 'include/grub/lvm.h'
> --- include/grub/lvm.h	2012-01-29 13:28:01 +0000
> +++ include/grub/lvm.h	2013-09-25 08:32:31 +0000
> @@ -62,6 +62,13 @@
>    struct grub_lvm_disk_locn disk_areas_xl[0];	/* Two lists */
>  } __attribute__ ((packed));
>  
> +struct grub_lvm_pv_header_ext {
> +  grub_uint32_t version_xl;
> +  grub_uint32_t flags_xl;
> +  /* NULL-terminated list of bootloader embedding areas */
> +  struct grub_lvm_disk_locn disk_areas_xl[0];
> +} __attribute__ ((packed));
> +
>  #define GRUB_LVM_FMTT_MAGIC "\040\114\126\115\062\040\170\133\065\101\045\162\060\116\052\076"
>  #define GRUB_LVM_FMTT_VERSION 1
>  #define GRUB_LVM_MDA_HEADER_SIZE 512
> 
> === modified file 'util/grub-setup.c'
> --- util/grub-setup.c	2013-04-04 06:55:06 +0000
> +++ util/grub-setup.c	2013-09-25 11:31:47 +0000
> @@ -387,7 +387,7 @@
>        .container = dest_dev->disk->partition,
>        .multiple_partmaps = 0
>      };
> -    int is_ldm;
> +    int is_ldm, is_lvm;
>      grub_err_t err;
>      grub_disk_addr_t *sectors;
>      int i;
> @@ -411,8 +411,9 @@
>        grub_errno = GRUB_ERR_NONE;
>  
>      is_ldm = grub_util_is_ldm (dest_dev->disk);
> +    is_lvm = grub_util_is_lvm (dest_dev->disk);
>  
> -    if (fs_probe)
> +    if (!is_lvm && fs_probe)
>        {
>  	if (!fs && !ctx.dest_partmap)
>  	  grub_util_error (_("unable to identify a filesystem in %s; safety check can't be performed"),
> @@ -459,7 +460,7 @@
>  
>      free (tmp_img);
>      
> -    if (! ctx.dest_partmap && ! fs && !is_ldm)
> +    if (! ctx.dest_partmap && ! fs && !is_ldm && !is_lvm)
>        {
>  	grub_util_warn ("%s", _("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
>  	goto unable_to_embed;
> @@ -492,7 +493,10 @@
>        maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
>  		>> GRUB_DISK_SECTOR_BITS);
>  
> -    if (is_ldm)
> +    if (is_lvm)
> +      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
> +				 GRUB_EMBED_PCBIOS, &sectors);
> +    else if (is_ldm)
>        err = grub_util_ldm_embed (dest_dev->disk, &nsec, maxsec,
>  				 GRUB_EMBED_PCBIOS, &sectors);
>      else if (ctx.dest_partmap)
> @@ -588,7 +592,7 @@
>  
>    if (dest_dev->disk->dev->id != root_dev->disk->dev->id)
>      grub_util_error ("%s", _("embedding is not possible, but this is required for "
> -			     "RAID and LVM install"));
> +			     "RAID install"));
>  
>    {
>      grub_fs_t fs;
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] Install to LVM PVs

[Gabriel de Perthuis]

Le 26/09/2013 10:53, Vladimir 'φ-coder/phcoder' Serbinenko a écrit:
> On 25.09.2013 14:39, Gabriel de Perthuis wrote:
>> Hello,
>> This patch lets grub install to a reserved area in LVM physical volumes.
>> These bootloader areas can be created with LVM 2.02.99 and the
>> --bootloaderareasize argument to pvcreate and vgconvert.
>> I tested it in QEMU, installing to and booting a disk that contains a PV
>> and no partition table.
>>
> This is not how the use of this area was imagined. There are couple of
> subtleties which your patch didn't take in account.
> Currently there is joint developpement with LVM guys but it wasn't
> published yet.

For anyone else who may be interested, apparently patches exist and are
waiting for Peter Rajnoha to finish them.  They haven't been posted or
discussed publicly and I've never seen them.

According to Vladimir:
> the zone will be subdivided to cover more cases and the agreement was
to use "pvs" to get offsets rather than having own code for this

As shipped in 2.02.99, pvs exposes exactly one ba_start/ba_size area.
Other areas will have to use extra extension fields and extra fields in
the pvs output, to be compatible with released versions of LVM.
Parsing pvs output would be easy, and will behave exactly the same as
this patch.

Re: [PATCH] Install to LVM PVs

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On 27.09.2013 12:39, Gabriel de Perthuis wrote:
> Le 26/09/2013 10:53, Vladimir 'φ-coder/phcoder' Serbinenko a écrit:
>> On 25.09.2013 14:39, Gabriel de Perthuis wrote:
>>> Hello,
>>> This patch lets grub install to a reserved area in LVM physical volumes.
>>> These bootloader areas can be created with LVM 2.02.99 and the
>>> --bootloaderareasize argument to pvcreate and vgconvert.
>>> I tested it in QEMU, installing to and booting a disk that contains a PV
>>> and no partition table.
>>>
>> This is not how the use of this area was imagined. There are couple of
>> subtleties which your patch didn't take in account.
>> Currently there is joint developpement with LVM guys but it wasn't
>> published yet.
> 
> For anyone else who may be interested, apparently patches exist and are
> waiting for Peter Rajnoha to finish them.  They haven't been posted or
> discussed publicly and I've never seen them.
> 
> According to Vladimir:
>> the zone will be subdivided to cover more cases and the agreement was
> to use "pvs" to get offsets rather than having own code for this
> 
> As shipped in 2.02.99, pvs exposes exactly one ba_start/ba_size area.
> Other areas will have to use extra extension fields and extra fields in
> the pvs output, to be compatible with released versions of LVM.
No, you didn't understand: this area will have another header, GRUB one
which will subdivide it. LVM gives us area and we take care of subdivision.
> Parsing pvs output would be easy, and will behave exactly the same as
> this patch.
> 

> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] Install to LVM PVs

[Gabriel de Perthuis]

Le 27/09/2013 14:48, Vladimir 'φ-coder/phcoder' Serbinenko a écrit :
> On 27.09.2013 12:39, Gabriel de Perthuis wrote:
>> Le 26/09/2013 10:53, Vladimir 'φ-coder/phcoder' Serbinenko a écrit:
>>> On 25.09.2013 14:39, Gabriel de Perthuis wrote:
>>>> Hello,
>>>> This patch lets grub install to a reserved area in LVM physical volumes.
>>>> These bootloader areas can be created with LVM 2.02.99 and the
>>>> --bootloaderareasize argument to pvcreate and vgconvert.
>>>> I tested it in QEMU, installing to and booting a disk that contains a PV
>>>> and no partition table.
>>>>
>>> This is not how the use of this area was imagined. There are couple of
>>> subtleties which your patch didn't take in account.
>>> Currently there is joint developpement with LVM guys but it wasn't
>>> published yet.
>>
>> For anyone else who may be interested, apparently patches exist and are
>> waiting for Peter Rajnoha to finish them.  They haven't been posted or
>> discussed publicly and I've never seen them.
>>
>> According to Vladimir:
>>> the zone will be subdivided to cover more cases and the agreement was
>> to use "pvs" to get offsets rather than having own code for this
>>
>> As shipped in 2.02.99, pvs exposes exactly one ba_start/ba_size area.
>> Other areas will have to use extra extension fields and extra fields in
>> the pvs output, to be compatible with released versions of LVM.

> No, you didn't understand: this area will have another header, GRUB one
> which will subdivide it. LVM gives us area and we take care of subdivision.

Or you could split the task in two: embed Grub on PVs, like it does on
partition tables, ext2-ext4, Btrfs; then add advanced embedding that
does some mini-partitioning of embedding areas.

The first task is really easy and brings an immediate benefit in making
LVM-only disks bootable.

I don't feel qualified to discuss the second task, because I'm not
seeing the use case.  I don't think it's been brought up.

Re: [PATCH] Install to LVM PVs

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

В Fri, 27 Sep 2013 14:48:33 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 27.09.2013 12:39, Gabriel de Perthuis wrote:
> > Le 26/09/2013 10:53, Vladimir 'φ-coder/phcoder' Serbinenko a écrit:
> >> On 25.09.2013 14:39, Gabriel de Perthuis wrote:
> >>> Hello,
> >>> This patch lets grub install to a reserved area in LVM physical volumes.
> >>> These bootloader areas can be created with LVM 2.02.99 and the
> >>> --bootloaderareasize argument to pvcreate and vgconvert.
> >>> I tested it in QEMU, installing to and booting a disk that contains a PV
> >>> and no partition table.
> >>>
> >> This is not how the use of this area was imagined. There are couple of
> >> subtleties which your patch didn't take in account.
> >> Currently there is joint developpement with LVM guys but it wasn't
> >> published yet.
> > 
> > For anyone else who may be interested, apparently patches exist and are
> > waiting for Peter Rajnoha to finish them.  They haven't been posted or
> > discussed publicly and I've never seen them.
> > 
> > According to Vladimir:
> >> the zone will be subdivided to cover more cases and the agreement was
> > to use "pvs" to get offsets rather than having own code for this
> > 
> > As shipped in 2.02.99, pvs exposes exactly one ba_start/ba_size area.
> > Other areas will have to use extra extension fields and extra fields in
> > the pvs output, to be compatible with released versions of LVM.
> No, you didn't understand: this area will have another header, GRUB one
> which will subdivide it. LVM gives us area and we take care of subdivision.

Is it something specific for LVM? Or useable in all other cases when
grub currently does embedding?

> > Parsing pvs output would be easy, and will behave exactly the same as
> > this patch.
> > 
> 
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> > 
> 
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Install to LVM PVs

[Andrei Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

В Fri, 27 Sep 2013 14:48:33 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 27.09.2013 12:39, Gabriel de Perthuis wrote:
> > Le 26/09/2013 10:53, Vladimir 'φ-coder/phcoder' Serbinenko a écrit:
> >> On 25.09.2013 14:39, Gabriel de Perthuis wrote:
> >>> Hello,
> >>> This patch lets grub install to a reserved area in LVM physical volumes.
> >>> These bootloader areas can be created with LVM 2.02.99 and the
> >>> --bootloaderareasize argument to pvcreate and vgconvert.
> >>> I tested it in QEMU, installing to and booting a disk that contains a PV
> >>> and no partition table.
> >>>
> >> This is not how the use of this area was imagined. There are couple of
> >> subtleties which your patch didn't take in account.
> >> Currently there is joint developpement with LVM guys but it wasn't
> >> published yet.
> > 
> > For anyone else who may be interested, apparently patches exist and are
> > waiting for Peter Rajnoha to finish them.  They haven't been posted or
> > discussed publicly and I've never seen them.
> > 
> > According to Vladimir:
> >> the zone will be subdivided to cover more cases and the agreement was
> > to use "pvs" to get offsets rather than having own code for this
> > 
> > As shipped in 2.02.99, pvs exposes exactly one ba_start/ba_size area.
> > Other areas will have to use extra extension fields and extra fields in
> > the pvs output, to be compatible with released versions of LVM.
> No, you didn't understand: this area will have another header, GRUB one
> which will subdivide it. LVM gives us area and we take care of subdivision.

Did anything happen with it? Do you have any pointers to suggested
design?

> > Parsing pvs output would be easy, and will behave exactly the same as
> > this patch.
> > 
> 
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> > 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] Install to LVM PVs

[Dryden Personalis]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

I updated this guy's patch to work with 2.02 beta 2, at least.

I've just worked on the version that gets downloaded and unpacked on 
Kubuntu / Ubuntu 16.04.

One part of the patch didn't apply because apparently some tabs have 
entered a file, namely grub-core/disk/lvm.c.

Another part didn't apply because code had been refactored from 
grub-setup.c to setup.c.

I've only compiled with these extra packages:

- bison
- flex
- libdevmapper-dev

and automake was also required. I've had to symlink the 1.15 executables 
to 1.14.


I'm not sure why the patch never made it into mainline grub, but back 
then (2013) the argument was that it was doing something to an area that 
was not meant for that.

However LVM contains the --bootloaderareasize flag that seems to be 
meant for this, and if you put it to 2048 sectors (or 1M) it ought to 
work cq. just works.

ie. pvcreate --bootloaderareasize 2048s /dev/sda, followed by, 
grub-install /dev/sda, will just work and result in a bootable system 
from that harddisk.

I don't know if I can attach here, and I don't know what will happen 
with tabs if I inline it.

So I will attach first.

Regards.


ps. I did not test booting from the harddisk to a system on the 
harddisk, but it boots a system on another device just fine, so I see no 
reason why it wouldn't work.

reference: 
https://lists.gnu.org/archive/html/grub-devel/2013-09/msg00113.html

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: grub-install-to-partitionless-lvm-pv_2.02~beta2.patch --]
[-- Type: text/x-diff; name=grub-install-to-partitionless-lvm-pv_2.02~beta2.patch, Size: 8957 bytes --]

=== modified file 'grub-core/disk/lvm.c'
--- grub-core/disk/lvm.c        2012-06-25 15:52:20 +0000
+++ grub-core/disk/lvm.c        2013-09-25 11:03:21 +0000
@@ -95,6 +95,38 @@
     }
 }
 
+static struct grub_lvm_pv_header *
+grub_lvm_get_pvh(grub_disk_t disk, char buf[static GRUB_LVM_LABEL_SIZE])
+{
+  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
+  unsigned int i;
+
+  /* Search for label. */
+  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
+    {
+      if (grub_disk_read (disk, i, 0, GRUB_LVM_LABEL_SIZE, buf))
+        return NULL;
+
+      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
+                          sizeof (lh->id)))
+         && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
+                             sizeof (lh->type))))
+       break;
+    }
+
+  /* Return if we didn't find a label. */
+  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
+    {
+#ifdef GRUB_UTIL
+      grub_util_info ("no LVM signature found");
+#endif
+      return NULL;
+    }
+
+  return (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
+}
+
+
 static struct grub_diskfilter_vg * 
 grub_lvm_detect (grub_disk_t disk,
                 struct grub_diskfilter_pv_id *id,
@@ -102,11 +134,10 @@
 {
   grub_err_t err;
   grub_uint64_t mda_offset, mda_size;
-  char buf[GRUB_LVM_LABEL_SIZE];
   char vg_id[GRUB_LVM_ID_STRLEN+1];
   char pv_id[GRUB_LVM_ID_STRLEN+1];
+  char buf[GRUB_LVM_LABEL_SIZE];
   char *metadatabuf, *p, *q, *vgname;
-  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
   struct grub_lvm_pv_header *pvh;
   struct grub_lvm_disk_locn *dlocn;
   struct grub_lvm_mda_header *mdah;
@@ -115,30 +146,9 @@
   struct grub_diskfilter_vg *vg;
   struct grub_diskfilter_pv *pv;
 
-  /* Search for label. */
-  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
-    {
-      err = grub_disk_read (disk, i, 0, sizeof(buf), buf);
-      if (err)
-	goto fail;
-
-      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
-			   sizeof (lh->id)))
-	  && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
-			      sizeof (lh->type))))
-	break;
-    }
-
-  /* Return if we didn't find a label. */
-  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
-    {
-#ifdef GRUB_UTIL
-      grub_util_info ("no LVM signature found");
-#endif
-      goto fail;
-    }
-
-  pvh = (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
+  pvh = grub_lvm_get_pvh(disk, buf);
+  if (! pvh)
+    goto fail;
 
   for (i = 0, j = 0; i < GRUB_LVM_ID_LEN; i++)
     {
@@ -151,7 +161,7 @@
   dlocn = pvh->disk_areas_xl;
 
   dlocn++;
-  /* Is it possible to have multiple data/metadata areas? I haven't
+  /* Is it possible to have multiple data areas? I haven't
      seen devices that have it. */
   if (dlocn->offset)
     {
@@ -746,6 +756,88 @@
   return NULL;
 }
 
+#ifdef GRUB_UTIL
+int
+grub_util_is_lvm(grub_disk_t disk)
+{
+  struct grub_diskfilter_pv_id id;
+  struct grub_diskfilter_vg *vg;
+  grub_disk_addr_t start_sector;
+  vg = grub_lvm_detect(disk, &id, &start_sector);
+  if (! vg)
+    return 0;
+  /* don't free the vg, it's held by grub_diskfilter_vg_register */
+  grub_free(id.uuid);
+  return 1;
+}
+
+grub_err_t
+grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
+                    unsigned int max_nsectors,
+                    grub_embed_type_t embed_type,
+                    grub_disk_addr_t **sectors)
+{
+  char buf[GRUB_LVM_LABEL_SIZE];
+  struct grub_lvm_pv_header *pvh;
+  struct grub_lvm_pv_header_ext *pvh_ext;
+  struct grub_diskfilter_pv *pv = NULL;
+  struct grub_diskfilter_vg *vg = NULL;
+  struct grub_lvm_disk_locn *dlocn;
+  grub_uint64_t ba_offset, ba_size, ba_start_sector;
+  unsigned int i;
+
+  if (embed_type != GRUB_EMBED_PCBIOS)
+    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+                      "LVM curently supports only PC-BIOS embedding");
+  if (disk->partition)
+    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
+  pv = grub_diskfilter_get_pv_from_disk (disk, &vg);
+  if (! pv)
+    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
+
+  pvh = grub_lvm_get_pvh(disk, buf);
+  if (! pvh)
+    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
+
+  dlocn = pvh->disk_areas_xl;
+
+  /* skip past the data area list */
+  while (dlocn->offset)
+    dlocn++;
+  dlocn++;
+  /* and the metadata area list */
+  while (dlocn->offset)
+    dlocn++;
+  dlocn++;
+
+  pvh_ext = (struct grub_lvm_pv_header_ext*)dlocn;
+  if (! pvh_ext->version_xl)
+    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");
+
+  dlocn = pvh_ext->disk_areas_xl;
+  ba_offset = grub_le_to_cpu64 (dlocn->offset);
+  ba_size = grub_le_to_cpu64 (dlocn->size);
+  if (! (ba_offset && ba_size))
+    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");
+  /* could be worked around with extra arithmetic if this actually happens */
+  if (ba_offset % GRUB_DISK_SECTOR_SIZE)
+    return grub_error (
+      GRUB_ERR_BUG, "LVM bootloader area is improperly aligned");
+  ba_start_sector = ba_offset / GRUB_DISK_SECTOR_SIZE;
+
+  *nsectors = ba_size / GRUB_DISK_SECTOR_SIZE;
+  if (*nsectors > max_nsectors)
+    *nsectors = max_nsectors;
+
+  *sectors = grub_malloc (*nsectors * sizeof (**sectors));
+  if (!*sectors)
+    return grub_errno;
+  for (i = 0; i < *nsectors; i++)
+    (*sectors)[i] = ba_start_sector + i;
+
+  return GRUB_ERR_NONE;
+}
+#endif
 \f
 
 static struct grub_diskfilter grub_lvm_dev = {

=== modified file 'include/grub/diskfilter.h'
--- include/grub/diskfilter.h   2012-06-25 15:36:50 +0000
+++ include/grub/diskfilter.h   2013-09-25 08:59:05 +0000
@@ -75,6 +75,9 @@
 #ifdef GRUB_UTIL
   char **partmaps;
 #endif
+  /* Optional bootloader embedding area */
+  grub_uint64_t ba_offset;
+  grub_uint64_t ba_size;
 };
 
 struct grub_diskfilter_lv {

=== modified file 'include/grub/emu/hostdisk.h'
--- include/grub/emu/hostdisk.h 2013-08-22 14:50:12 +0000
+++ include/grub/emu/hostdisk.h 2013-09-25 07:59:06 +0000
@@ -44,9 +44,16 @@
 char *
 grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
 int
+grub_util_is_lvm (grub_disk_t disk);
+int
 grub_util_is_ldm (grub_disk_t disk);
 #ifdef GRUB_UTIL
 grub_err_t
+grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
+                    unsigned int max_nsectors,
+                    grub_embed_type_t embed_type,
+                    grub_disk_addr_t **sectors);
+grub_err_t
 grub_util_ldm_embed (struct grub_disk *disk, unsigned int *nsectors,
                     unsigned int max_nsectors,
                     grub_embed_type_t embed_type,

=== modified file 'include/grub/lvm.h'
--- include/grub/lvm.h  2012-01-29 13:28:01 +0000
+++ include/grub/lvm.h  2013-09-25 08:32:31 +0000
@@ -62,6 +62,13 @@
   struct grub_lvm_disk_locn disk_areas_xl[0];  /* Two lists */
 } __attribute__ ((packed));
 
+struct grub_lvm_pv_header_ext {
+  grub_uint32_t version_xl;
+  grub_uint32_t flags_xl;
+  /* NULL-terminated list of bootloader embedding areas */
+  struct grub_lvm_disk_locn disk_areas_xl[0];
+} __attribute__ ((packed));
+
 #define GRUB_LVM_FMTT_MAGIC "\040\114\126\115\062\040\170\133\065\101\045\162\060\116\052\076"
 #define GRUB_LVM_FMTT_VERSION 1
 #define GRUB_LVM_MDA_HEADER_SIZE 512

=== modified file 'util/setup.c'
--- util/setup.c	2016-05-08 01:00:00.000000000 +0200
+++ util/setup.c	2016-05-08 02:00:00.000000000 +0200
@@ -390,7 +390,7 @@
       .container = dest_dev->disk->partition,
       .multiple_partmaps = 0
     };
-    int is_ldm;
+    int is_ldm, is_lvm;
     grub_err_t err;
     grub_disk_addr_t *sectors;
     int i;
@@ -423,8 +423,9 @@
       grub_errno = GRUB_ERR_NONE;
 
     is_ldm = grub_util_is_ldm (dest_dev->disk);
+    is_lvm = grub_util_is_lvm (dest_dev->disk);
 
-    if (fs_probe)
+    if (!is_lvm && fs_probe)
       {
 	if (!fs && !ctx.dest_partmap)
 	  grub_util_error (_("unable to identify a filesystem in %s; safety check can't be performed"),
@@ -462,7 +463,7 @@
 
       }
 
-    if (! ctx.dest_partmap && ! fs && !is_ldm)
+    if (! ctx.dest_partmap && ! fs && !is_ldm && !is_lvm)
       {
 	grub_util_warn ("%s", _("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
 	goto unable_to_embed;
@@ -499,7 +500,10 @@
       maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
 		>> GRUB_DISK_SECTOR_BITS);
 
-    if (is_ldm)
+    if (is_lvm)
+      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
+                                GRUB_EMBED_PCBIOS, &sectors);
+    else if (is_ldm)
       err = grub_util_ldm_embed (dest_dev->disk, &nsec, maxsec,
 				 GRUB_EMBED_PCBIOS, &sectors);
     else if (ctx.dest_partmap)
@@ -602,7 +606,7 @@
 
   if (dest_dev->disk->dev->id != root_dev->disk->dev->id)
     grub_util_error ("%s", _("embedding is not possible, but this is required for "
-			     "RAID and LVM install"));
+			     "RAID install"));
 
   {
     grub_fs_t fs;

Re: [PATCH] Install to LVM PVs

[Andrei Borzenkov]

08.05.2016 07:53, Dryden Personalis пишет:
> I updated this guy's patch to work with 2.02 beta 2, at least.
> 
> I've just worked on the version that gets downloaded and unpacked on
> Kubuntu / Ubuntu 16.04.
> 
> One part of the patch didn't apply because apparently some tabs have
> entered a file, namely grub-core/disk/lvm.c.
> 
> Another part didn't apply because code had been refactored from
> grub-setup.c to setup.c.
> 
> I've only compiled with these extra packages:
> 
> - bison
> - flex
> - libdevmapper-dev
> 
> and automake was also required. I've had to symlink the 1.15 executables
> to 1.14.
> 
> 

Did you actually test it by installing GRUB on PV? Does it boot? Did you
test it both with unpartitioned disk and PV in partition?

> I'm not sure why the patch never made it into mainline grub, but back
> then (2013) the argument was that it was doing something to an area that
> was not meant for that.
> 

Neither am I :) I understand that Vladimir had plans to stuff more
information in this area (personally I'd like to use it for environment
block as example) but just guessing. In any case, we really always can
cut off part of this area by artificially reducing size.

> However LVM contains the --bootloaderareasize flag that seems to be
> meant for this, and if you put it to 2048 sectors (or 1M) it ought to
> work cq. just works.
> 
> ie. pvcreate --bootloaderareasize 2048s /dev/sda, followed by,
> grub-install /dev/sda, will just work and result in a bootable system
> from that harddisk.
> 
> I don't know if I can attach here, and I don't know what will happen
> with tabs if I inline it.
> 
> So I will attach first.
> 

That's fine.

> Regards.
> 
> 
> ps. I did not test booting from the harddisk to a system on the
> harddisk, but it boots a system on another device just fine, so I see no
> reason why it wouldn't work.
> 
> reference:
> https://lists.gnu.org/archive/html/grub-devel/2013-09/msg00113.html
> 

> === modified file 'grub-core/disk/lvm.c'
> --- grub-core/disk/lvm.c        2012-06-25 15:52:20 +0000
> +++ grub-core/disk/lvm.c        2013-09-25 11:03:21 +0000
> @@ -95,6 +95,38 @@
>      }
>  }
>  
> +static struct grub_lvm_pv_header *
> +grub_lvm_get_pvh(grub_disk_t disk, char buf[static GRUB_LVM_LABEL_SIZE])
> +{
> +  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
> +  unsigned int i;
> +
> +  /* Search for label. */
> +  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
> +    {
> +      if (grub_disk_read (disk, i, 0, GRUB_LVM_LABEL_SIZE, buf))
> +        return NULL;
> +
> +      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
> +                          sizeof (lh->id)))
> +         && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
> +                             sizeof (lh->type))))
> +       break;
> +    }
> +
> +  /* Return if we didn't find a label. */
> +  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
> +    {
> +#ifdef GRUB_UTIL
> +      grub_util_info ("no LVM signature found");
> +#endif
> +      return NULL;
> +    }
> +
> +  return (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
> +}
> +
> +
>  static struct grub_diskfilter_vg * 
>  grub_lvm_detect (grub_disk_t disk,
>                  struct grub_diskfilter_pv_id *id,
> @@ -102,11 +134,10 @@
>  {
>    grub_err_t err;
>    grub_uint64_t mda_offset, mda_size;
> -  char buf[GRUB_LVM_LABEL_SIZE];
>    char vg_id[GRUB_LVM_ID_STRLEN+1];
>    char pv_id[GRUB_LVM_ID_STRLEN+1];
> +  char buf[GRUB_LVM_LABEL_SIZE];

Why reordering buf definition?

>    char *metadatabuf, *p, *q, *vgname;
> -  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
>    struct grub_lvm_pv_header *pvh;
>    struct grub_lvm_disk_locn *dlocn;
>    struct grub_lvm_mda_header *mdah;
> @@ -115,30 +146,9 @@
>    struct grub_diskfilter_vg *vg;
>    struct grub_diskfilter_pv *pv;
>  
> -  /* Search for label. */
> -  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
> -    {
> -      err = grub_disk_read (disk, i, 0, sizeof(buf), buf);
> -      if (err)
> -	goto fail;
> -
> -      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
> -			   sizeof (lh->id)))
> -	  && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
> -			      sizeof (lh->type))))
> -	break;
> -    }
> -
> -  /* Return if we didn't find a label. */
> -  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
> -    {
> -#ifdef GRUB_UTIL
> -      grub_util_info ("no LVM signature found");
> -#endif
> -      goto fail;
> -    }
> -
> -  pvh = (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
> +  pvh = grub_lvm_get_pvh(disk, buf);
> +  if (! pvh)
> +    goto fail;
>  
>    for (i = 0, j = 0; i < GRUB_LVM_ID_LEN; i++)
>      {
> @@ -151,7 +161,7 @@
>    dlocn = pvh->disk_areas_xl;
>  
>    dlocn++;
> -  /* Is it possible to have multiple data/metadata areas? I haven't
> +  /* Is it possible to have multiple data areas? I haven't

Could you explain why you deleted "metadata"?

>       seen devices that have it. */
>    if (dlocn->offset)
>      {
> @@ -746,6 +756,88 @@
>    return NULL;
>  }
>  
> +#ifdef GRUB_UTIL
> +int
> +grub_util_is_lvm(grub_disk_t disk)
> +{
> +  struct grub_diskfilter_pv_id id;
> +  struct grub_diskfilter_vg *vg;
> +  grub_disk_addr_t start_sector;
> +  vg = grub_lvm_detect(disk, &id, &start_sector);
> +  if (! vg)
> +    return 0;
> +  /* don't free the vg, it's held by grub_diskfilter_vg_register */
> +  grub_free(id.uuid);
> +  return 1;
> +}
> +

This has side effect of adding duplicate VG definitions; this may later
confuse grub. What about just checking array->driver for LVM? Go through
registered arrays, find disk match and check array driver. See
scan_disk_partition_iter () for example.

> +grub_err_t
> +grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
> +                    unsigned int max_nsectors,
> +                    grub_embed_type_t embed_type,
> +                    grub_disk_addr_t **sectors)
> +{
> +  char buf[GRUB_LVM_LABEL_SIZE];
> +  struct grub_lvm_pv_header *pvh;
> +  struct grub_lvm_pv_header_ext *pvh_ext;
> +  struct grub_diskfilter_pv *pv = NULL;
> +  struct grub_diskfilter_vg *vg = NULL;
> +  struct grub_lvm_disk_locn *dlocn;
> +  grub_uint64_t ba_offset, ba_size, ba_start_sector;
> +  unsigned int i;
> +
> +  if (embed_type != GRUB_EMBED_PCBIOS)
> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> +                      "LVM curently supports only PC-BIOS embedding");
> +  if (disk->partition)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");

Why? What's wrong with PV on partition?

> +  pv = grub_diskfilter_get_pv_from_disk (disk, &vg);
> +  if (! pv)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> +
> +  pvh = grub_lvm_get_pvh(disk, buf);
> +  if (! pvh)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> +

Please, more specific error message, so they are actually useful to
identify what happened.

None of those is bug; this should probably be GRUB_ERR_BAD_DEVICE.

> +  dlocn = pvh->disk_areas_xl;
> +
> +  /* skip past the data area list */
> +  while (dlocn->offset)
> +    dlocn++;
> +  dlocn++;
> +  /* and the metadata area list */
> +  while (dlocn->offset)
> +    dlocn++;
> +  dlocn++;
> +
> +  pvh_ext = (struct grub_lvm_pv_header_ext*)dlocn;
> +  if (! pvh_ext->version_xl)
> +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");
> +

Again, this is not a bug.

> +  dlocn = pvh_ext->disk_areas_xl;
> +  ba_offset = grub_le_to_cpu64 (dlocn->offset);
> +  ba_size = grub_le_to_cpu64 (dlocn->size);
> +  if (! (ba_offset && ba_size))
> +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader area");

And this.

> +  /* could be worked around with extra arithmetic if this actually happens */
> +  if (ba_offset % GRUB_DISK_SECTOR_SIZE)
> +    return grub_error (
> +      GRUB_ERR_BUG, "LVM bootloader area is improperly aligned");
> +  ba_start_sector = ba_offset / GRUB_DISK_SECTOR_SIZE;
> +
> +  *nsectors = ba_size / GRUB_DISK_SECTOR_SIZE;
> +  if (*nsectors > max_nsectors)
> +    *nsectors = max_nsectors;
> +
> +  *sectors = grub_malloc (*nsectors * sizeof (**sectors));
> +  if (!*sectors)
> +    return grub_errno;
> +  for (i = 0; i < *nsectors; i++)
> +    (*sectors)[i] = ba_start_sector + i;
> +
> +  return GRUB_ERR_NONE;
> +}
> +#endif
>  
>  
>  static struct grub_diskfilter grub_lvm_dev = {
> 
> === modified file 'include/grub/diskfilter.h'
> --- include/grub/diskfilter.h   2012-06-25 15:36:50 +0000
> +++ include/grub/diskfilter.h   2013-09-25 08:59:05 +0000
> @@ -75,6 +75,9 @@
>  #ifdef GRUB_UTIL
>    char **partmaps;
>  #endif
> +  /* Optional bootloader embedding area */
> +  grub_uint64_t ba_offset;
> +  grub_uint64_t ba_size;
>  };
>  
>  struct grub_diskfilter_lv {
> 
> === modified file 'include/grub/emu/hostdisk.h'
> --- include/grub/emu/hostdisk.h 2013-08-22 14:50:12 +0000
> +++ include/grub/emu/hostdisk.h 2013-09-25 07:59:06 +0000
> @@ -44,9 +44,16 @@
>  char *
>  grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
>  int
> +grub_util_is_lvm (grub_disk_t disk);
> +int
>  grub_util_is_ldm (grub_disk_t disk);
>  #ifdef GRUB_UTIL
>  grub_err_t
> +grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
> +                    unsigned int max_nsectors,
> +                    grub_embed_type_t embed_type,
> +                    grub_disk_addr_t **sectors);
> +grub_err_t
>  grub_util_ldm_embed (struct grub_disk *disk, unsigned int *nsectors,
>                      unsigned int max_nsectors,
>                      grub_embed_type_t embed_type,
> 
> === modified file 'include/grub/lvm.h'
> --- include/grub/lvm.h  2012-01-29 13:28:01 +0000
> +++ include/grub/lvm.h  2013-09-25 08:32:31 +0000
> @@ -62,6 +62,13 @@
>    struct grub_lvm_disk_locn disk_areas_xl[0];  /* Two lists */
>  } __attribute__ ((packed));
>  
> +struct grub_lvm_pv_header_ext {
> +  grub_uint32_t version_xl;
> +  grub_uint32_t flags_xl;
> +  /* NULL-terminated list of bootloader embedding areas */
> +  struct grub_lvm_disk_locn disk_areas_xl[0];
> +} __attribute__ ((packed));
> +
>  #define GRUB_LVM_FMTT_MAGIC "\040\114\126\115\062\040\170\133\065\101\045\162\060\116\052\076"
>  #define GRUB_LVM_FMTT_VERSION 1
>  #define GRUB_LVM_MDA_HEADER_SIZE 512
> 
> === modified file 'util/setup.c'
> --- util/setup.c	2016-05-08 01:00:00.000000000 +0200
> +++ util/setup.c	2016-05-08 02:00:00.000000000 +0200
> @@ -390,7 +390,7 @@
>        .container = dest_dev->disk->partition,
>        .multiple_partmaps = 0
>      };
> -    int is_ldm;
> +    int is_ldm, is_lvm;
>      grub_err_t err;
>      grub_disk_addr_t *sectors;
>      int i;
> @@ -423,8 +423,9 @@
>        grub_errno = GRUB_ERR_NONE;
>  
>      is_ldm = grub_util_is_ldm (dest_dev->disk);
> +    is_lvm = grub_util_is_lvm (dest_dev->disk);
>  
> -    if (fs_probe)
> +    if (!is_lvm && fs_probe)

No, we want to probe for FS here to eliminate corner case of both PV and
FS metadata.

>        {
>  	if (!fs && !ctx.dest_partmap)
>  	  grub_util_error (_("unable to identify a filesystem in %s; safety check can't be performed"),
> @@ -462,7 +463,7 @@
>  
>        }
>  
> -    if (! ctx.dest_partmap && ! fs && !is_ldm)
> +    if (! ctx.dest_partmap && ! fs && !is_ldm && !is_lvm)
>        {
>  	grub_util_warn ("%s", _("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
>  	goto unable_to_embed;

And you need to add check for LVM here

    if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm && fs))
      {
        grub_util_warn ("%s", _("Attempting to install GRUB to a disk
with multiple partition labels.  This is not supported yet."));
        goto unable_to_embed;
      }


> @@ -499,7 +500,10 @@
>        maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
>  		>> GRUB_DISK_SECTOR_BITS);
>  
> -    if (is_ldm)
> +    if (is_lvm)
> +      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
> +                                GRUB_EMBED_PCBIOS, &sectors);
> +    else if (is_ldm)
>        err = grub_util_ldm_embed (dest_dev->disk, &nsec, maxsec,
>  				 GRUB_EMBED_PCBIOS, &sectors);
>      else if (ctx.dest_partmap)
> @@ -602,7 +606,7 @@
>  
>    if (dest_dev->disk->dev->id != root_dev->disk->dev->id)
>      grub_util_error ("%s", _("embedding is not possible, but this is required for "
> -			     "RAID and LVM install"));
> +			     "RAID install"));
>  
>    {
>      grub_fs_t fs;

Re: [PATCH] Install to LVM PVs

[Andrei Borzenkov]

08.05.2016 09:05, Andrei Borzenkov пишет:
>>  
>> +#ifdef GRUB_UTIL
>> +int
>> +grub_util_is_lvm(grub_disk_t disk)
>> +{
>> +  struct grub_diskfilter_pv_id id;
>> +  struct grub_diskfilter_vg *vg;
>> +  grub_disk_addr_t start_sector;
>> +  vg = grub_lvm_detect(disk, &id, &start_sector);
>> +  if (! vg)
>> +    return 0;
>> +  /* don't free the vg, it's held by grub_diskfilter_vg_register */
>> +  grub_free(id.uuid);
>> +  return 1;
>> +}
>> +
> 
> This has side effect of adding duplicate VG definitions; this may later
> confuse grub. What about just checking array->driver for LVM? Go through
> registered arrays, find disk match and check array driver. See
> scan_disk_partition_iter () for example.
> 

Which is basically call grub_diskfilter_get_pv_from_disk() and check
vg_out->driver.

Re: [PATCH] Install to LVM PVs

[Andrei Borzenkov]

08.05.2016 07:53, Dryden Personalis пишет:
> +    if (is_lvm)
> +      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
> +                                GRUB_EMBED_PCBIOS, &sectors);

PV label may be in the first sector. In this case label will be
overwritten later by boot image. This needs additional check.

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 08-05-2016 8:05:

> Did you actually test it by installing GRUB on PV? Does it boot? Did 
> you
> test it both with unpartitioned disk and PV in partition?

Hi Andrei,

I must say I have for the most part just copied the patch as it was, and 
just tried to fix it so that the patch would actually apply in full. I 
have no experience yet with Grub code, so I may not be able to answer 
any or all of your questions.

I must say that I did try the former, but not the latter yet. My system 
is not installed though so I can try at any point. What I haven't tried 
yet is booting FROM the PV in the sense of loading a system from it (and 
not just the boot loader) so at this point I have only booted a 
different system from the boot loader on my /dev/sda residing in a 
partitionless PV.

Testing a PV with partitions is trivial, I will do that shortly.

I plan on readily installing a system on that disk after I have my 
scheme sorted out, so then I can test for real the real use case (but 
only in the case of partitionless).


> Neither am I :) I understand that Vladimir had plans to stuff more
> information in this area (personally I'd like to use it for environment
> block as example) but just guessing. In any case, we really always can
> cut off part of this area by artificially reducing size.
>> === modified file 'grub-core/disk/lvm.c'
>> --- grub-core/disk/lvm.c        2012-06-25 15:52:20 +0000
>> +++ grub-core/disk/lvm.c        2013-09-25 11:03:21 +0000
>> @@ -102,11 +134,10 @@
>>  {
>>    grub_err_t err;
>>    grub_uint64_t mda_offset, mda_size;
>> -  char buf[GRUB_LVM_LABEL_SIZE];
>>    char vg_id[GRUB_LVM_ID_STRLEN+1];
>>    char pv_id[GRUB_LVM_ID_STRLEN+1];
>> +  char buf[GRUB_LVM_LABEL_SIZE];
> 
> Why reordering buf definition?

That was just in the original patch. You can see the dates. I put some 
artificial times on the latest hunk, from yesterday. I didn't do 
anything really to the hunks from 2012/2013.

So I don't know the rationale, we'd have to ask the original author, it 
probably makes no sense from my POV, but it does seem nicer lol.

>> -  /* Is it possible to have multiple data/metadata areas? I haven't
>> +  /* Is it possible to have multiple data areas? I haven't
> 
> Could you explain why you deleted "metadata"?

I don't know but I think the guy wanted to simplify by only having one 
block? Or something of the kind?

> 
>>       seen devices that have it. */
>>    if (dlocn->offset)
>>      {
>> @@ -746,6 +756,88 @@
>>    return NULL;
>>  }
>> 
>> +#ifdef GRUB_UTIL
>> +int
>> +grub_util_is_lvm(grub_disk_t disk)
>> +{
>> +  struct grub_diskfilter_pv_id id;
>> +  struct grub_diskfilter_vg *vg;
>> +  grub_disk_addr_t start_sector;
>> +  vg = grub_lvm_detect(disk, &id, &start_sector);
>> +  if (! vg)
>> +    return 0;
>> +  /* don't free the vg, it's held by grub_diskfilter_vg_register */
>> +  grub_free(id.uuid);
>> +  return 1;
>> +}
>> +
> 
> This has side effect of adding duplicate VG definitions; this may later
> confuse grub. What about just checking array->driver for LVM? Go 
> through
> registered arrays, find disk match and check array driver. See
> scan_disk_partition_iter () for example.

Alright this is beyond me at this point, I'd prefer if you gave feedback 
on the other items first.

But do you mean not to use grub_lvm_detect?

Are you saying that "array->driver" would already be loaded before we do 
grub_lvm_detect? (Or rather, before grub_util_is_lvm)? You are meaning 
to imply to just use a different identification measure right, because 
apparently grub_lvm_detect has already been done in that case.


>> +grub_err_t
>> +grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
>> +                    unsigned int max_nsectors,
>> +                    grub_embed_type_t embed_type,
>> +                    grub_disk_addr_t **sectors)
>> +{
>> +  char buf[GRUB_LVM_LABEL_SIZE];
>> +  struct grub_lvm_pv_header *pvh;
>> +  struct grub_lvm_pv_header_ext *pvh_ext;
>> +  struct grub_diskfilter_pv *pv = NULL;
>> +  struct grub_diskfilter_vg *vg = NULL;
>> +  struct grub_lvm_disk_locn *dlocn;
>> +  grub_uint64_t ba_offset, ba_size, ba_start_sector;
>> +  unsigned int i;
>> +
>> +  if (embed_type != GRUB_EMBED_PCBIOS)
>> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>> +                      "LVM curently supports only PC-BIOS 
>> embedding");
>> +  if (disk->partition)
>> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> 
> Why? What's wrong with PV on partition?

Just for my recollection:

* The function "grub_util_info" now calls "grub_util_is_lvm"
* grub_util_is_lvm only scans the first 4 sectors of a disk, so it will 
not find anything on a partitioned /dev/sda
* The above function grub_util_lvm_embed is only called when it does 
return true.

* If you don't have a VG in the PV, the error never gets triggered.

I don't know, the guy designed the feature for partitionless. Does it 
make sense to use if you don't have partitions? If it is all equivalent 
to Grub, I don't see why it shouldn't be possible.

It means you'd want to embed grub in a partition that has a PV. Don't 
see why it shouldn't be possible.

I can remove it but I will have to find out how to boot from an actual 
partition to test it :p.

Maybe easiest is to clear the MBR and set bootable flag? Don't know if 
that works for grub. If default msdos label has such code....

>> +  pv = grub_diskfilter_get_pv_from_disk (disk, &vg);
>> +  if (! pv)
>> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
>> +
>> +  pvh = grub_lvm_get_pvh(disk, buf);
>> +  if (! pvh)
>> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
>> +
> 
> Please, more specific error message, so they are actually useful to
> identify what happened.

Yes, can do. If I understand get_pvh, but that shouldn't be hard.

> None of those is bug; this should probably be GRUB_ERR_BAD_DEVICE.

Aye, it even reports as a warning while being fatal. Can do.

>> +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a 
>> bootloader area");
>> +
> 
> Again, this is not a bug.

Roger. Will find some other error if I can.

>> -    if (fs_probe)
>> +    if (!is_lvm && fs_probe)
> 
> No, we want to probe for FS here to eliminate corner case of both PV 
> and
> FS metadata.

You mean you want to change it to:

...something that gives a different error as in:

if (fs_probe) {
   if (is_lvm && fs ) {
     grub_util_error (_("a filesystem signature is present on %s, but 
embedding in an LVM PV was requested"), dest_dev->disk->name);
   } else if (!is_lvm && !fs && !ctx.dest_partmap) {
     grub_util_error (_("unable to identify a filesystem in %s; safety 
check can't be performed"), dest_dev->disk->name);
   }
   <therest>
}

That is just my coding style, I understand yours.

>> -    if (! ctx.dest_partmap && ! fs && !is_ldm)
>> +    if (! ctx.dest_partmap && ! fs && !is_ldm && !is_lvm)
>>        {
>>  	grub_util_warn ("%s", _("Attempting to install GRUB to a 
>> partitionless disk or to a partition.  This is a BAD idea."));
>>  	goto unable_to_embed;
> 
> And you need to add check for LVM here
> 
>     if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm && 
> fs))

I take it you mean:

if (!is_lvm && (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || 
(is_ldm && fs)))

?

Or do you mean:

if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm && fs) 
|| is_lvm))

?

I take it the former: with is_lvm there should be no warning.

I will work on the changes and post a new patch (not git format, but 
shouldn't matter).

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 08-05-2016 11:23:
> 08.05.2016 07:53, Dryden Personalis пишет:
>> +    if (is_lvm)
>> +      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
>> +                                GRUB_EMBED_PCBIOS, &sectors);
> 
> PV label may be in the first sector. In this case label will be
> overwritten later by boot image. This needs additional check.

Is that only true for the first sector? I saw it checks the first 4. So 
I don't know what it does by default?

pvcreate will not work on disks that have a partition, but they are okay 
with having a label but no partitions.

the man page suggests wiping the first sector, but it says nothing about 
where it will install itself.

You are suggesting to check whether PV header is in 1-3, and not proceed 
if it is in 0.

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 08-05-2016 10:47:
> 08.05.2016 09:05, Andrei Borzenkov пишет:
>>> 
>>> +#ifdef GRUB_UTIL
>>> +int
>>> +grub_util_is_lvm(grub_disk_t disk)
>>> +{
>>> +  struct grub_diskfilter_pv_id id;
>>> +  struct grub_diskfilter_vg *vg;
>>> +  grub_disk_addr_t start_sector;
>>> +  vg = grub_lvm_detect(disk, &id, &start_sector);
>>> +  if (! vg)
>>> +    return 0;
>>> +  /* don't free the vg, it's held by grub_diskfilter_vg_register */
>>> +  grub_free(id.uuid);
>>> +  return 1;
>>> +}
>>> +
>> 
>> This has side effect of adding duplicate VG definitions; this may 
>> later
>> confuse grub. What about just checking array->driver for LVM? Go 
>> through
>> registered arrays, find disk match and check array driver. See
>> scan_disk_partition_iter () for example.
>> 
> 
> Which is basically call grub_diskfilter_get_pv_from_disk() and check
> vg_out->driver.

Roger.

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 08-05-2016 10:47:
> 08.05.2016 09:05, Andrei Borzenkov пишет:
>>> 
>>> +#ifdef GRUB_UTIL
>>> +int
>>> +grub_util_is_lvm(grub_disk_t disk)
>>> +{
>>> +  struct grub_diskfilter_pv_id id;
>>> +  struct grub_diskfilter_vg *vg;
>>> +  grub_disk_addr_t start_sector;
>>> +  vg = grub_lvm_detect(disk, &id, &start_sector);
>>> +  if (! vg)
>>> +    return 0;
>>> +  /* don't free the vg, it's held by grub_diskfilter_vg_register */
>>> +  grub_free(id.uuid);
>>> +  return 1;
>>> +}
>>> +
>> 
>> This has side effect of adding duplicate VG definitions; this may 
>> later
>> confuse grub. What about just checking array->driver for LVM? Go 
>> through
>> registered arrays, find disk match and check array driver. See
>> scan_disk_partition_iter () for example.
>> 
> 
> Which is basically call grub_diskfilter_get_pv_from_disk() and check
> vg_out->driver.

This method also has the downside that is_lvm will fail without any 
indication as to why, when no volume group has been created. lvm_detect 
will return null because it can only return a VG. And, you cannot error 
out on that if a non-existing LVM should be okay.

So this call to lvm_detect is pretty annoying.

If it had more info, I could at least give an error in setup.c.

I am not sure whether installation is *possible* without a VG but it 
would be odd if it couldn't?

Anyway I would like a better error message on this because I just wasted 
2 hours trying to get GRUB_UTIL to work and failing. That means as per 
your method I can probably distinguish between PV present and VG present 
(or not present).

Then the only way I know how to report back is to give more values to 
is_lvm, but do we need a VG? Why error out on no VG? I will test.

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Dryden Personalis schreef op 08-05-2016 19:16:

> I just wasted 2 hours trying to get GRUB_UTIL to work and failing.

I am just not always that brilliant.

Why on earth is that flag in there? If you enable it, stuff won't 
compile anymore. And I merely needed to turn on verbose output for 
grub-install :p. Not that it would have helped much, but at least 
troubleshooting would have been faster.

Omg. I blame the heat. Trying to get a system working that already works 
just fine.

:( :P.

Re: [PATCH] Install to LVM PVs

[Xen]

Andrei Borzenkov schreef op 08-05-2016 11:23:

> PV label may be in the first sector. In this case label will be
> overwritten later by boot image. This needs additional check.

Pardon my last mail.

ON OLD PV SIGNATURES
--------------------

If you have an old PV that had a boatloaderarea, then wiping and or 
recreating the PV doesn't actually clear or reset or reformat that thing 
so as to fail.

It is not necessary to have a VG in order to install grub to a PV.

However, if the VG code is not called (both grub_lvm_detect and 
grub_diskfilter_get_pv_from_disk) then the code will not know how big 
the bootloaderarea is going to be (or whether you have one) -- you still 
don't need a VG, you just need to call that function the way this patch 
is.

Having or not having a VG in the PV makes no difference as to the 
results of any operation.

My code currently pretends that having a PV header is enough, and it 
works out just fine. I have not exhaustively tested what happens if you 
create a VG after (successfully booting) but I doubt it is going to give 
issues.

So the most important thing for this checking code currently is that the 
PV is actually created on a clean disk, ie a simple:

dd if=/dev/zero of=/dev/sda bs=1M count=20

Will ensure correct behaviour.

So it is possible to create a PV without a bootloaderarea on top of an 
older PV that did have one, and the code will see the data structures of 
the old one, but the real PV and its VGs will be based on the new one 
(you can see the difference in that you lose an extent if you create or 
remove an area of 4M).

So the code is not entirely correct in that sense (And I do not 
understand it yet at all) but in most cases it functions correctly.


LOCATION OF SANITY CHECKING
---------------------------

Presently what I am doing is to let the code in setup.c only check the 
basics, if a PV header is present, fine, let's get on with it. The 
embedding code (in lvm.c) then checks whether everything is alright.

The annoying part is that if (and only if) you have placed your PV 
header in the first sector (the way we needed to prevent cq. check) it 
can mean that some info message is going to be output multiple times. 
Each time the checking function is called, it will see the header 
sitting in the first sector, and output some message.

Apart from that the error resolution is pretty outstanding currently.

So I propose to:

- not need any VG
- is_lvm will return true if only a header is present
- sanity checking is not done much in setup.c unless it is a call to 
lvm.c


If you really have zero sector PV header, currently the "info" message 
will be output at least twice, and possibly trice:

- in lvm_detect
- in lvm_embed
- in any checking function called from setup.c

I have put a parameter to get_lvh that supresses the verbosity if zero.

You will see.

At this point the whole VG initialisation system for embedding is only 
apparently needed to check the actual bootloaderarea.


CURRENT RESULTS:
----------------

First off, booting works fine from a non-partition setup. I assume there 
is no difference installing to a disk with a partition table and no LVM.

Personally I cannot get Grub to install on /dev/sda1:

error: cannot find a GRUB drive for /dev/sdal.  Check your device.map.


Presently, this is the output if you try to install on a first-sector PV 
without VG:

info: Scanning for lvm devices on disk hostdisk//dev/sda.
info: couldn't find ID
info: LVM signature in first sector.
warning: installing inside LVM PV, but its header is sitting in the 
first sector; cannot install there.
error: embedding is not possible, but this is required for LVM and RAID 
install.

Upon creating a volume group, LVM will actually restore the PV header to 
the 2nd sector, if you've botched it.

A zero sector (first sector) PV is just not reality.

They are not recognized.


Installing on a PV without bootloaderarea will result in:

info: Scanning for lvm devices on disk hostdisk//dev/sda.
info: Found array linux.
info: Inserting hostdisk//dev/sda (+0,976773168) into linux (lvm)
info: drive = 1.
info: the size of hostdisk//dev/sda is 976773168.
info: Scanning for DISKFILTER devices on disk hostdisk//dev/sda.
warning: you have chosen to install inside an LVM PV, but it doesn't 
have a bootloader area (check pvcreate --bootloaderareasize).
error: embedding is not possible, but this is required for LVM and RAID 
install.

When a VG is present.

I don't think it actually checks for the SIZE of the bootloaderarea but 
that could easily be added I think.

When you do the same without a VG:

info: Scanning for lvm devices on disk hostdisk//dev/sda.
info: couldn't find ID
warning: you have chosen to install inside an LVM PV, but it doesn't 
have a bootloader area (check pvcreate --bootloaderareasize).
error: embedding is not possible, but this is required for LVM and RAID 
install.

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 08-05-2016 11:23:

> PV label may be in the first sector. In this case label will be
> overwritten later by boot image. This needs additional check.

Pardon my last mail.

ON OLD PV SIGNATURES
--------------------

If you have an old PV that had a boatloaderarea, then wiping and or 
recreating the PV doesn't actually clear or reset or reformat that thing 
so as to fail.

It is not necessary to have a VG in order to install grub to a PV.

However, if the VG code is not called (both grub_lvm_detect and 
grub_diskfilter_get_pv_from_disk) then the code will not know how big 
the bootloaderarea is going to be (or whether you have one) -- you still 
don't need a VG, you just need to call that function the way this patch 
is.

Having or not having a VG in the PV makes no difference as to the 
results of any operation.

My code currently pretends that having a PV header is enough, and it 
works out just fine. I have not exhaustively tested what happens if you 
create a VG after (successfully booting) but I doubt it is going to give 
issues.

So the most important thing for this checking code currently is that the 
PV is actually created on a clean disk, ie a simple:

dd if=/dev/zero of=/dev/sda bs=1M count=20

Will ensure correct behaviour.

So it is possible to create a PV without a bootloaderarea on top of an 
older PV that did have one, and the code will see the data structures of 
the old one, but the real PV and its VGs will be based on the new one 
(you can see the difference in that you lose an extent if you create or 
remove an area of 4M).

So the code is not entirely correct in that sense (And I do not 
understand it yet at all) but in most cases it functions correctly.


LOCATION OF SANITY CHECKING
---------------------------

Presently what I am doing is to let the code in setup.c only check the 
basics, if a PV header is present, fine, let's get on with it. The 
embedding code (in lvm.c) then checks whether everything is alright.

The annoying part is that if (and only if) you have placed your PV 
header in the first sector (the way we needed to prevent cq. check) it 
can mean that some info message is going to be output multiple times. 
Each time the checking function is called, it will see the header 
sitting in the first sector, and output some message.

Apart from that the error resolution is pretty outstanding currently.

So I propose to:

- not need any VG
- is_lvm will return true if only a header is present
- sanity checking is not done much in setup.c unless it is a call to 
lvm.c


If you really have zero sector PV header, currently the "info" message 
will be output at least twice, and possibly trice:

- in lvm_detect
- in lvm_embed
- in any checking function called from setup.c

I have put a parameter to get_lvh that supresses the verbosity if zero.

You will see.

At this point the whole VG initialisation system for embedding is only 
apparently needed to check the actual bootloaderarea.


CURRENT RESULTS:
----------------

First off, booting works fine from a non-partition setup. I assume there 
is no difference installing to a disk with a partition table and no LVM.

Personally I cannot get Grub to install on /dev/sda1:

error: cannot find a GRUB drive for /dev/sdal.  Check your device.map.


Presently, this is the output if you try to install on a first-sector PV 
without VG:

info: Scanning for lvm devices on disk hostdisk//dev/sda.
info: couldn't find ID
info: LVM signature in first sector.
warning: installing inside LVM PV, but its header is sitting in the 
first sector; cannot install there.
error: embedding is not possible, but this is required for LVM and RAID 
install.

Upon creating a volume group, LVM will actually restore the PV header to 
the 2nd sector, if you've botched it.

A zero sector (first sector) PV is just not reality.

They are not recognized.


Installing on a PV without bootloaderarea will result in:

info: Scanning for lvm devices on disk hostdisk//dev/sda.
info: Found array linux.
info: Inserting hostdisk//dev/sda (+0,976773168) into linux (lvm)
info: drive = 1.
info: the size of hostdisk//dev/sda is 976773168.
info: Scanning for DISKFILTER devices on disk hostdisk//dev/sda.
warning: you have chosen to install inside an LVM PV, but it doesn't 
have a bootloader area (check pvcreate --bootloaderareasize).
error: embedding is not possible, but this is required for LVM and RAID 
install.

When a VG is present.

I don't think it actually checks for the SIZE of the bootloaderarea but 
that could easily be added I think.

When you do the same without a VG:

info: Scanning for lvm devices on disk hostdisk//dev/sda.
info: couldn't find ID
warning: you have chosen to install inside an LVM PV, but it doesn't 
have a bootloader area (check pvcreate --bootloaderareasize).
error: embedding is not possible, but this is required for LVM and RAID 
install.

Re: [PATCH] Install to LVM PVs

[Andrei Borzenkov]

08.05.2016 16:01, Dryden Personalis пишет:
> 
>>> -    if (fs_probe)
>>> +    if (!is_lvm && fs_probe)
>>
>> No, we want to probe for FS here to eliminate corner case of both PV and
>> FS metadata.
> 
> You mean you want to change it to:
> 

No, I do not want to change it at all. It should remain as is and always
probe for FS so that test below actually works
...
>> And you need to add check for LVM here
>>
>>     if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm
>> && fs))
> 
> I take it you mean:
> 
> if (!is_lvm && (ctx.multiple_partmaps || (ctx.dest_partmap && fs) ||
> (is_ldm && fs)))
>

I mean

if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || ((is_ldm ||
is_lvm) && fs))

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 09-05-2016 8:07:

> No, I do not want to change it at all. It should remain as is and 
> always
> probe for FS so that test below actually works

But its output makes no sense, there cannot be a filesystem in a PV. It 
could check for existence of VG AND LV AND filesystem but does it do 
that right now?

Installing Grub is generally a non-destructive operation, apart from 
overwriting the boot sector.

Anyway my foot is hurting and I can't sit behind the computer, I will 
just show the code at some point so to discuss that alright.

>> I take it you mean:
>> 
>> if (!is_lvm && (ctx.multiple_partmaps || (ctx.dest_partmap && fs) ||
>> (is_ldm && fs)))
>> 
> 
> I mean
> 
> if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || ((is_ldm ||
> is_lvm) && fs))

Yes, I realized. Sorry.

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Currently:

- still searches in first sector for label, and reports error if found 
there
- get_pvh split into regular and one with 2 extra parameters for 
first_sector detection
- some documentation
- VG is not required to install
- does not even check for VG, but function is retained
- is lvm returns true iff header is found
- currently still skips filesystem check for lvm.

- untested installing on partitions
- note: LVM does not support first sector PVs
- we could also skip checking the first sector (my beautiful code)
- in that case split get_pvh is (currently) not needed, but you would 
also not warn people about the bootsector. The header would just not be 
found.

So in principle the solution becomes really simple (not requiring VG).

You can however corrupt the thing by first installing a 
bootloaderareasize PV, then a regular one on top of that, and the code 
will mistakenly believe there is room.

But this is all I can do without diving into the mechanics currently.

This is the function I thought Andrei wanted:

int grub_util_has_lvm_vg (grub_disk_t disk)
{
   struct grub_diskfilter_pv *pv = NULL;
   struct grub_diskfilter_vg *vg = NULL;

   pv = grub_diskfilter_get_pv_from_disk(disk, &vg);
   return pv && vg && vg == vg->driver->detect(disk, &pv->id, 
&pv->start_sector)
     ? 1 : 0;
}

If you are okay, I will attach the patch, but I also did some cleanup in 
lvm.c particularly (removing extraneous \n for instance messing up info 
messages) but I need to separate that with Git.

If you still feel VG should be required, let me know.

But also whether filesystem check (without -s) should be required when 
none can ever be found (at least not in our case here).

I have reverted to that of the patch (or similar):

if (fs_probe)
   {
     if (!is_lvm && !fs && !ctx.dest_partmap)
       grub_util_error (_("unable to identify a filesystem in %s; safety 
check can't be performed"), dest_dev->disk->name);

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Dryden Personalis schreef op 09-05-2016 18:10:

> I have reverted to that of the patch (or similar):
> 
> if (fs_probe)
>   {
>     if (!is_lvm && !fs && !ctx.dest_partmap)
>       grub_util_error (_("unable to identify a filesystem in %s;
> safety check can't be performed"), dest_dev->disk->name);

In particular I feel !is_lvm and !ctx.dest_partmap are equivalent. Why, 
they both indicate partitions, instead of being a partition (with a 
filesystem).

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

[-- Attachment #1: Type: text/plain, Size: 145 bytes --]

Attached my current working patch.

Also attached some housekeeping.

Not sure if it applies to current git version.

Take from it what you like.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: grub-install-to-partitionless-lvm-pv_2.02~beta2_v3.patch --]
[-- Type: text/x-diff; name=grub-install-to-partitionless-lvm-pv_2.02~beta2_v3.patch, Size: 12369 bytes --]

diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
index 22c7b64..37e715a 100644
--- a/grub-core/disk/lvm.c
+++ b/grub-core/disk/lvm.c
@@ -95,6 +95,78 @@ grub_lvm_check_flag (char *p, const char *str, const char *flag)
     }
 }
 
+/**
+ * Attribute function obtaining PV header from disk. Currently scans the
+ * first 4 sectors and returns anything found in either of those sectors.
+ *
+ * Sets *first_sector to 1 if header was in first sector, 0 otherwise.
+ * May print first_sector info message if verbose was set.
+ *
+ * Returns header if found, NULL otherwise. Also returns header if first_sector.
+ */
+static struct grub_lvm_pv_header *
+grub_lvm_get_pvh_at(grub_disk_t disk, char buf[static GRUB_LVM_LABEL_SIZE], 
+#ifdef GRUB_UTIL
+    int *first_sector, int verbose
+#else
+    int *first_sector
+#endif
+  )
+
+{
+  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
+  unsigned int i;
+
+  /* Search for label. */
+  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
+    {
+      if (grub_disk_read (disk, i, 0, GRUB_LVM_LABEL_SIZE, buf))
+        return NULL;
+
+      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
+                          sizeof (lh->id)))
+         && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
+                             sizeof (lh->type))))
+       break;
+    }
+
+#ifdef GRUB_UTIL
+  if (first_sector) *first_sector = 0;
+#endif
+
+  /* Return if we didn't find a label. */
+  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
+    {
+#ifdef GRUB_UTIL
+      grub_util_info ("no LVM signature found");
+#endif
+      return NULL;
+    }
+  else if (i == 0)
+    {
+#ifdef GRUB_UTIL
+      if (verbose) grub_util_info ("LVM signature in first sector");
+#endif
+      if (first_sector) *first_sector = 1;
+    }
+
+  return (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
+}
+
+static inline struct grub_lvm_pv_header *grub_lvm_get_pvh(grub_disk_t disk, char buf[static GRUB_LVM_LABEL_SIZE]) 
+{
+#ifdef GRUB_UTIL
+  return grub_lvm_get_pvh_at(disk, buf, NULL, 0);
+#else
+  return grub_lvm_get_pvh_at(disk, buf, NULL);
+#endif
+}
+
+/**
+ * Preexisting function that detects the LVM PV header from disk and proceeds
+ * to fill in VG datastructures. Called from diskfilter as part of the
+ * "driver->detect" functionality.
+ */
 static struct grub_diskfilter_vg * 
 grub_lvm_detect (grub_disk_t disk,
 		 struct grub_diskfilter_pv_id *id,
@@ -106,7 +178,6 @@ grub_lvm_detect (grub_disk_t disk,
   char vg_id[GRUB_LVM_ID_STRLEN+1];
   char pv_id[GRUB_LVM_ID_STRLEN+1];
   char *metadatabuf, *p, *q, *vgname;
-  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
   struct grub_lvm_pv_header *pvh;
   struct grub_lvm_disk_locn *dlocn;
   struct grub_lvm_mda_header *mdah;
@@ -116,30 +187,9 @@ grub_lvm_detect (grub_disk_t disk,
   struct grub_diskfilter_vg *vg;
   struct grub_diskfilter_pv *pv;
 
-  /* Search for label. */
-  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
-    {
-      err = grub_disk_read (disk, i, 0, sizeof(buf), buf);
-      if (err)
-	goto fail;
-
-      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
-			   sizeof (lh->id)))
-	  && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
-			      sizeof (lh->type))))
-	break;
-    }
-
-  /* Return if we didn't find a label. */
-  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
-    {
-#ifdef GRUB_UTIL
-      grub_util_info ("no LVM signature found");
-#endif
-      goto fail;
-    }
-
-  pvh = (struct grub_lvm_pv_header *) (buf + grub_le_to_cpu32(lh->offset_xl));
+  pvh = grub_lvm_get_pvh(disk, buf);
+  if (!pvh)
+    goto fail;
 
   for (i = 0, j = 0; i < GRUB_LVM_ID_LEN; i++)
     {
@@ -770,6 +820,112 @@ grub_lvm_detect (grub_disk_t disk,
   return NULL;
 }
 
+#ifdef GRUB_UTIL
+/**
+ * Requests physical volume from disk and checks whether relations hold up?
+ *
+ * This function only returns true iff a volume group was found
+ */
+int grub_util_has_lvm_vg (grub_disk_t disk)
+{
+  struct grub_diskfilter_pv *pv = NULL;
+  struct grub_diskfilter_vg *vg = NULL;
+
+  pv = grub_diskfilter_get_pv_from_disk(disk, &vg);
+  return pv && vg && vg == vg->driver->detect(disk, &pv->id, &pv->start_sector)
+         ? 1 : 0;
+}
+
+/**
+ * Does nothing other than scanning the first 4 sectors of the device for
+ * a LVM PV header
+ */
+int grub_util_is_lvm (grub_disk_t disk)
+{
+  char buf[GRUB_LVM_LABEL_SIZE];
+  return grub_lvm_get_pvh(disk, buf) ? 1 : 0;
+}
+
+/**
+ * This function embeds the bootloader inside an LVM PV bootloaderarea
+ * specified by --bootloaderareasize on the pvcreate command line.
+ *
+ * It is called from setup.c and returns an error message to the caller,
+ * that gets treated as a warning, if it fails.
+*/
+
+grub_err_t
+grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
+                    unsigned int max_nsectors,
+                    grub_embed_type_t embed_type,
+                    grub_disk_addr_t **sectors)
+{
+  char buf[GRUB_LVM_LABEL_SIZE];
+  struct grub_lvm_pv_header *pvh;
+  struct grub_lvm_pv_header_ext *pvh_ext;
+  struct grub_diskfilter_vg *vg = NULL;
+  struct grub_lvm_disk_locn *dlocn;
+  grub_uint64_t ba_offset, ba_size, ba_start_sector;
+  unsigned int i;
+  int first_sector;
+
+  if (embed_type != GRUB_EMBED_PCBIOS)
+    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+                      "LVM curently supports only PC-BIOS embedding");
+#ifdef GRUB_UTIL
+  pvh = grub_lvm_get_pvh_at(disk, buf, &first_sector, 1);
+#else
+  pvh = grub_lvm_get_pvh_at(disk, buf, &first_sector);
+#endif
+  /* if pvh is NULL, we probably have a bug */
+  if (!pvh)
+    return grub_error (GRUB_ERR_BUG, "trying to install on non-existing PV header, but should already have been prevented by initial screening");
+
+  if (first_sector)
+    return grub_error (GRUB_ERR_BAD_DEVICE, "PV detected inside boot sector, but we need the boot sector to install in");
+
+  /* for some reason the bootloaderarea check will fail if we don't do this here */
+  grub_diskfilter_get_pv_from_disk(disk, &vg);
+
+  dlocn = pvh->disk_areas_xl;
+
+  /* skip past the data area list */
+  while (dlocn->offset)
+    dlocn++;
+  dlocn++;
+  /* and the metadata area list */
+  while (dlocn->offset)
+    dlocn++;
+  dlocn++;
+
+  pvh_ext = (struct grub_lvm_pv_header_ext*)dlocn;
+  if (!pvh_ext->version_xl)
+    return grub_error (GRUB_ERR_BAD_DEVICE, "trying to install on LVM PV but it does not have a bootloader area to embed in. Check pvcreate --bootloaderareasize");
+
+  dlocn = pvh_ext->disk_areas_xl;
+  ba_offset = grub_le_to_cpu64 (dlocn->offset);
+  ba_size = grub_le_to_cpu64 (dlocn->size);
+  if (!(ba_offset && ba_size))
+    return grub_error (GRUB_ERR_BAD_DEVICE, "trying to install on LVM PV but it does not have a bootloader area to embed in. Check pvcreate --bootloaderareasize");
+  /* could be worked around with extra arithmetic if this actually happens */
+  if (ba_offset % GRUB_DISK_SECTOR_SIZE)
+    return grub_error (
+      GRUB_ERR_BAD_DEVICE, "LVM bootloader area is not aligned on sector boundaries (%d)", GRUB_DISK_SECTOR_SIZE);
+  ba_start_sector = ba_offset / GRUB_DISK_SECTOR_SIZE;
+
+  *nsectors = ba_size / GRUB_DISK_SECTOR_SIZE;
+  if (*nsectors > max_nsectors)
+    *nsectors = max_nsectors;
+
+  *sectors = grub_malloc (*nsectors * sizeof (**sectors));
+  if (!*sectors)
+    return grub_errno;
+  for (i = 0; i < *nsectors; i++)
+    (*sectors)[i] = ba_start_sector + i;
+
+  return GRUB_ERR_NONE;
+}
+#endif
 \f
 
 static struct grub_diskfilter grub_lvm_dev = {
diff --git a/include/grub/diskfilter.h b/include/grub/diskfilter.h
index 1aedcd3..ca95dea 100644
--- a/include/grub/diskfilter.h
+++ b/include/grub/diskfilter.h
@@ -75,6 +75,9 @@ struct grub_diskfilter_pv {
 #ifdef GRUB_UTIL
   char **partmaps;
 #endif
+  /* Optional bootloader embedding area */
+  grub_uint64_t ba_offset;
+  grub_uint64_t ba_size;
 };
 
 struct grub_diskfilter_lv {
diff --git a/include/grub/emu/hostdisk.h b/include/grub/emu/hostdisk.h
index e233d37..4e4ddea 100644
--- a/include/grub/emu/hostdisk.h
+++ b/include/grub/emu/hostdisk.h
@@ -44,6 +44,14 @@ char *grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
 int grub_util_is_ldm (grub_disk_t disk);
 
 #ifdef GRUB_UTIL
+int grub_util_is_lvm (grub_disk_t disk);
+int grub_util_has_lvm_vg (grub_disk_t disk);
+
+grub_err_t
+grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
+                    unsigned int max_nsectors,
+                    grub_embed_type_t embed_type,
+                    grub_disk_addr_t **sectors);
 grub_err_t
 grub_util_ldm_embed (struct grub_disk *disk, unsigned int *nsectors,
 		     unsigned int max_nsectors,
diff --git a/include/grub/lvm.h b/include/grub/lvm.h
index 30a609c..dd19552 100644
--- a/include/grub/lvm.h
+++ b/include/grub/lvm.h
@@ -62,6 +62,13 @@ struct grub_lvm_pv_header {
   struct grub_lvm_disk_locn disk_areas_xl[0];	/* Two lists */
 } GRUB_PACKED;
 
+struct grub_lvm_pv_header_ext {
+  grub_uint32_t version_xl;
+  grub_uint32_t flags_xl;
+  /* NULL-terminated list of bootloader embedding areas */
+  struct grub_lvm_disk_locn disk_areas_xl[0];
+} __attribute__ ((packed));
+
 #define GRUB_LVM_FMTT_MAGIC "\040\114\126\115\062\040\170\133\065\101\045\162\060\116\052\076"
 #define GRUB_LVM_FMTT_VERSION 1
 #define GRUB_LVM_MDA_HEADER_SIZE 512
diff --git a/util/setup.c b/util/setup.c
index 7c60083..34c7f80 100644
--- a/util/setup.c
+++ b/util/setup.c
@@ -390,7 +390,7 @@ SETUP (const char *dir,
       .container = dest_dev->disk->partition,
       .multiple_partmaps = 0
     };
-    int is_ldm;
+    int is_ldm, is_lvm;
     grub_err_t err;
     grub_disk_addr_t *sectors;
     int i;
@@ -423,15 +423,17 @@ SETUP (const char *dir,
       grub_errno = GRUB_ERR_NONE;
 
     is_ldm = grub_util_is_ldm (dest_dev->disk);
+    is_lvm = grub_util_is_lvm (dest_dev->disk);
 
     if (fs_probe)
       {
-	if (!fs && !ctx.dest_partmap)
-	  grub_util_error (_("unable to identify a filesystem in %s; safety check can't be performed"),
-			   dest_dev->disk->name);
-	if (fs && !fs->reserved_first_sector)
-	  /* TRANSLATORS: Filesystem may reserve the space just GRUB isn't sure about it.  */
-	  grub_util_error (_("%s appears to contain a %s filesystem which isn't known to "
+        /* Doubt there can be any FS signature on LVM? */
+        if (!is_lvm && !fs && !ctx.dest_partmap)
+          grub_util_error (_("unable to identify a filesystem in %s; safety check can't be performed"), dest_dev->disk->name);
+
+        if (fs && !fs->reserved_first_sector)
+          /* TRANSLATORS: Filesystem may reserve the space just GRUB isn't sure about it. */
+          grub_util_error (_("%s appears to contain a %s filesystem which isn't known to "
 			     "reserve space for DOS-style boot.  Installing GRUB there could "
 			     "result in FILESYSTEM DESTRUCTION if valuable data is overwritten "
 			     "by grub-setup (--skip-fs-probe disables this "
@@ -462,12 +464,12 @@ SETUP (const char *dir,
 
       }
 
-    if (! ctx.dest_partmap && ! fs && !is_ldm)
+    if (! ctx.dest_partmap && ! fs && !is_ldm && !is_lvm)
       {
 	grub_util_warn ("%s", _("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
 	goto unable_to_embed;
       }
-    if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm && fs))
+    if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || ((is_ldm || is_lvm) && fs))
       {
 	grub_util_warn ("%s", _("Attempting to install GRUB to a disk with multiple partition labels.  This is not supported yet."));
 	goto unable_to_embed;
@@ -499,7 +501,10 @@ SETUP (const char *dir,
       maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
 		>> GRUB_DISK_SECTOR_BITS);
 
-    if (is_ldm)
+    if (is_lvm)
+      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
+                                GRUB_EMBED_PCBIOS, &sectors);
+    else if (is_ldm)
       err = grub_util_ldm_embed (dest_dev->disk, &nsec, maxsec,
 				 GRUB_EMBED_PCBIOS, &sectors);
     else if (ctx.dest_partmap)
@@ -602,7 +607,7 @@ unable_to_embed:
 
   if (dest_dev->disk->dev->id != root_dev->disk->dev->id)
     grub_util_error ("%s", _("embedding is not possible, but this is required for "
-			     "RAID and LVM install"));
+			     "LVM and RAID install"));
 
   {
     grub_fs_t fs;

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: housekeeping.patch --]
[-- Type: text/x-diff; name=housekeeping.patch, Size: 6968 bytes --]

diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
index 483c17e..22c7b64 100644
--- a/grub-core/disk/lvm.c
+++ b/grub-core/disk/lvm.c
@@ -160,7 +160,7 @@ grub_lvm_detect (grub_disk_t disk,
 		  "we don't support multiple LVM data areas");
 
 #ifdef GRUB_UTIL
-      grub_util_info ("we don't support multiple LVM data areas\n");
+      grub_util_info ("we don't support multiple LVM data areas");
 #endif
       goto fail;
     }
@@ -189,7 +189,7 @@ grub_lvm_detect (grub_disk_t disk,
       grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		  "unknown LVM metadata header");
 #ifdef GRUB_UTIL
-      grub_util_info ("unknown LVM metadata header\n");
+      grub_util_info ("unknown LVM metadata header");
 #endif
       goto fail2;
     }
@@ -213,7 +213,7 @@ grub_lvm_detect (grub_disk_t disk,
   if (q == metadatabuf + mda_size)
     {
 #ifdef GRUB_UTIL
-      grub_util_info ("error parsing metadata\n");
+      grub_util_info ("error parsing metadata");
 #endif
       goto fail2;
     }
@@ -230,7 +230,7 @@ grub_lvm_detect (grub_disk_t disk,
   if (p == NULL)
     {
 #ifdef GRUB_UTIL
-      grub_util_info ("couldn't find ID\n");
+      grub_util_info ("couldn't find ID");
 #endif
       goto fail3;
     }
@@ -258,7 +258,7 @@ grub_lvm_detect (grub_disk_t disk,
       if (p == NULL)
 	{
 #ifdef GRUB_UTIL
-	  grub_util_info ("unknown extent size\n");
+	  grub_util_info ("unknown extent size");
 #endif
 	  goto fail4;
 	}
@@ -306,7 +306,7 @@ grub_lvm_detect (grub_disk_t disk,
 	      if (p == NULL)
 		{
 #ifdef GRUB_UTIL
-		  grub_util_info ("unknown pe_start\n");
+		  grub_util_info ("unknown pe_start");
 #endif
 		  goto pvs_fail;
 		}
@@ -315,7 +315,7 @@ grub_lvm_detect (grub_disk_t disk,
 	      if (p == NULL)
 		{
 #ifdef GRUB_UTIL
-		  grub_util_info ("error parsing pe_start\n");
+		  grub_util_info ("error parsing pe_start");
 #endif
 		  goto pvs_fail;
 		}
@@ -402,7 +402,7 @@ grub_lvm_detect (grub_disk_t disk,
 		if (p == NULL)
 		  {
 #ifdef GRUB_UTIL
-		    grub_util_info ("couldn't find ID\n");
+		    grub_util_info ("couldn't find ID");
 #endif
 		    goto lvs_fail;
 		  }
@@ -422,7 +422,7 @@ grub_lvm_detect (grub_disk_t disk,
 	      if (p == NULL)
 		{
 #ifdef GRUB_UTIL
-		  grub_util_info ("unknown segment_count\n");
+		  grub_util_info ("unknown segment_count");
 #endif
 		  goto lvs_fail;
 		}
@@ -436,7 +436,7 @@ grub_lvm_detect (grub_disk_t disk,
 		  if (p == NULL)
 		    {
 #ifdef GRUB_UTIL
-		      grub_util_info ("unknown segment\n");
+		      grub_util_info ("unknown segment");
 #endif
 		      goto lvs_segment_fail;
 		    }
@@ -445,7 +445,7 @@ grub_lvm_detect (grub_disk_t disk,
 		  if (p == NULL)
 		    {
 #ifdef GRUB_UTIL
-		      grub_util_info ("unknown start_extent\n");
+		      grub_util_info ("unknown start_extent");
 #endif
 		      goto lvs_segment_fail;
 		    }
@@ -453,7 +453,7 @@ grub_lvm_detect (grub_disk_t disk,
 		  if (p == NULL)
 		    {
 #ifdef GRUB_UTIL
-		      grub_util_info ("unknown extent_count\n");
+		      grub_util_info ("unknown extent_count");
 #endif
 		      goto lvs_segment_fail;
 		    }
@@ -475,7 +475,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      if (p == NULL)
 			{
 #ifdef GRUB_UTIL
-			  grub_util_info ("unknown stripe_count\n");
+			  grub_util_info ("unknown stripe_count");
 #endif
 			  goto lvs_segment_fail;
 			}
@@ -491,7 +491,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      if (p == NULL)
 			{
 #ifdef GRUB_UTIL
-			  grub_util_info ("unknown stripes\n");
+			  grub_util_info ("unknown stripes");
 #endif
 			  goto lvs_segment_fail2;
 			}
@@ -533,7 +533,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      if (p == NULL)
 			{
 #ifdef GRUB_UTIL
-			  grub_util_info ("unknown mirror_count\n");
+			  grub_util_info ("unknown mirror_count");
 #endif
 			  goto lvs_segment_fail;
 			}
@@ -545,7 +545,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      if (p == NULL)
 			{
 #ifdef GRUB_UTIL
-			  grub_util_info ("unknown mirrors\n");
+			  grub_util_info ("unknown mirrors");
 #endif
 			  goto lvs_segment_fail2;
 			}
@@ -603,7 +603,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      if (p == NULL)
 			{
 #ifdef GRUB_UTIL
-			  grub_util_info ("unknown device_count\n");
+			  grub_util_info ("unknown device_count");
 #endif
 			  goto lvs_segment_fail;
 			}
@@ -612,7 +612,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      if (p == NULL)
 			{
 #ifdef GRUB_UTIL
-			  grub_util_info ("unknown stripe_size\n");
+			  grub_util_info ("unknown stripe_size");
 #endif
 			  goto lvs_segment_fail;
 			}
@@ -625,7 +625,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      if (p == NULL)
 			{
 #ifdef GRUB_UTIL
-			  grub_util_info ("unknown mirrors\n");
+			  grub_util_info ("unknown mirrors");
 #endif
 			  goto lvs_segment_fail2;
 			}
@@ -672,7 +672,7 @@ grub_lvm_detect (grub_disk_t disk,
 		      p2 = grub_strchr (p, '"');
 		      if (p2)
 			*p2 = 0;
-		      grub_util_info ("unknown LVM type %s\n", p);
+		      grub_util_info ("unknown LVM type %s", p);
 		      if (p2)
 			*p2 ='"';
 #endif
diff --git a/include/grub/emu/hostdisk.h b/include/grub/emu/hostdisk.h
index e006f0b..e233d37 100644
--- a/include/grub/emu/hostdisk.h
+++ b/include/grub/emu/hostdisk.h
@@ -26,8 +26,8 @@
 #include <grub/emu/hostfile.h>
 
 grub_util_fd_t
-grub_util_fd_open_device (const grub_disk_t disk, grub_disk_addr_t sector, int flags,
-			  grub_disk_addr_t *max);
+grub_util_fd_open_device (const grub_disk_t disk, grub_disk_addr_t sector,
+                         int flags, grub_disk_addr_t *max);
 
 void grub_util_biosdisk_init (const char *dev_map);
 void grub_util_biosdisk_fini (void);
@@ -35,17 +35,14 @@ char *grub_util_biosdisk_get_grub_dev (const char *os_dev);
 const char *grub_util_biosdisk_get_osdev (grub_disk_t disk);
 int grub_util_biosdisk_is_present (const char *name);
 int grub_util_biosdisk_is_floppy (grub_disk_t disk);
-const char *
-grub_util_biosdisk_get_compatibility_hint (grub_disk_t disk);
+const char *grub_util_biosdisk_get_compatibility_hint (grub_disk_t disk);
 grub_err_t grub_util_biosdisk_flush (struct grub_disk *disk);
 grub_err_t
 grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat);
-const char *
-grub_util_cryptodisk_get_uuid (grub_disk_t disk);
-char *
-grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
-int
-grub_util_is_ldm (grub_disk_t disk);
+const char *grub_util_cryptodisk_get_uuid (grub_disk_t disk);
+char *grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
+int grub_util_is_ldm (grub_disk_t disk);
+
 #ifdef GRUB_UTIL
 grub_err_t
 grub_util_ldm_embed (struct grub_disk *disk, unsigned int *nsectors,
diff --git a/util/setup.c b/util/setup.c
index 3945eff..7c60083 100644
--- a/util/setup.c
+++ b/util/setup.c
@@ -1,4 +1,4 @@
-/* grub-setup.c - make GRUB usable */
+/* setup.c - make GRUB usable */
 /*
  *  GRUB  --  GRand Unified Bootloader
  *  Copyright (C) 1999,2000,2001,2002,2003,2004,2005,2006,2007,2008,2009,2010,2011  Free Software Foundation, Inc.

Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 09-05-2016 8:07:


I'm sorry, this email had gone missing from my system. I just found it 
back in the trash, after thinking I had already searched for it.

I'm not sure why no one responds anymore, of course Andrei was the only 
one to respond in the first place. I hope nothing is the matter with 
Andrei himself, I mean it could be personal circumstances.

I have basically implemented all suggestions, with just two questions 
that I would like to have answered:

- is it okay to drop the requirement of having a VG configured in the PV
- is it okay to skip the FS-presence test in a LVM that cannot have it 
in any case?

If the group or the ones in charge of this software would be willing to 
shed their light on these issues, I could at least make something that 
you would agree with, if it would be intended for real submission.

So from my perspective the feature works, I tend to think these are the 
only choices to make, and perhaps additional safety checks are required 
to deal with the size of the bootloaderarea, which I can look into if 
needed.

Presently I do not have the knowledge about the PV structure yet.

I did not write the original patch, after all.

I could easily test or demonstrate what would happen if the 
bootloaderareasize were really small; whether grub would overwrite 
essential structures if I did.

I think safety is the biggest requirement here?

As demonstrated, booting itself is not an issue except that I cannot 
test installing on partitions (at all) currently. I do not know why, but 
Grub on my system will not install on any partition whatsoever, not the 
new version, or the standard ubuntu version.

I have still not installed my system again on that harddisk, still 
working off of a USB stick. ;-). So I am actually waiting with 
installing until people have indicated whether additional development or 
testing is required?

I would hope to work with you on this.

(Not just grub, I'm also worried about LVM thin safety, but that is 
another issue).

I'm sorry if I seem not to understand how things work around here, or if 
I actually do not (understand). I have tried to reach Andrei off-list, 
but no response. But I've waited for a week, so I need to go and start 
doing something else if it is not going to happen anyway.

So please.

And regards, and thanks for cooperating (thus far) and of having been of 
help, in any case, regardless. Regards.



> 08.05.2016 16:01, Dryden Personalis пишет:
>> 
>>>> -    if (fs_probe)
>>>> +    if (!is_lvm && fs_probe)
>>> 
>>> No, we want to probe for FS here to eliminate corner case of both PV 
>>> and
>>> FS metadata.
>> 
>> You mean you want to change it to:
>> 
> 
> No, I do not want to change it at all. It should remain as is and 
> always
> probe for FS so that test below actually works
> ...
>>> And you need to add check for LVM here
>>> 
>>>     if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm
>>> && fs))
>> 
>> I take it you mean:
>> 
>> if (!is_lvm && (ctx.multiple_partmaps || (ctx.dest_partmap && fs) ||
>> (is_ldm && fs)))
>> 
> 
> I mean
> 
> if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || ((is_ldm ||
> is_lvm) && fs))