[SeaBIOS] [PATCH] pretty boot menu entry for cdrom drives

Gerd Hoffmann posted 1 patch 11 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/20180921074801.27699-1-kraxel@redhat.com
src/util.h  |  1 +
src/boot.c  |  7 +++++++
src/cdrom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+)

[SeaBIOS] [PATCH] pretty boot menu entry for cdrom drives

Posted by Gerd Hoffmann 11 weeks ago
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 src/util.h  |  1 +
 src/boot.c  |  7 +++++++
 src/cdrom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/src/util.h b/src/util.h
index 7a23b518fc..6dd080f673 100644
--- a/src/util.h
+++ b/src/util.h
@@ -50,6 +50,7 @@ struct disk_op_s;
 int cdemu_process_op(struct disk_op_s *op);
 void cdrom_prepboot(void);
 int cdrom_boot(struct drive_s *drive_g);
+char *cdrom_media_info(struct drive_s *drive_g);
 
 // clock.c
 void clock_setup(void);
diff --git a/src/boot.c b/src/boot.c
index ff705fd47f..80bcc13535 100644
--- a/src/boot.c
+++ b/src/boot.c
@@ -395,6 +395,13 @@ boot_add_hd(struct drive_s *drive, const char *desc, int prio)
 void
 boot_add_cd(struct drive_s *drive, const char *desc, int prio)
 {
+    if (GET_GLOBAL(PlatformRunningOn) & PF_QEMU) {
+        // We want short boot times.  But on physical hardware even
+        // the test unit ready can take several seconds.  So do media
+        // access on qemu only, where we know it will be fast.
+        char *extra = cdrom_media_info(drive);
+        desc = znprintf(MAXDESCSIZE, "%s (%s)", desc, extra);
+    }
     bootentry_add(IPL_TYPE_CDROM, defPrio(prio, DefaultCDPrio)
                   , (u32)drive, desc);
 }
diff --git a/src/cdrom.c b/src/cdrom.c
index 828fb3b842..b4c36622ae 100644
--- a/src/cdrom.c
+++ b/src/cdrom.c
@@ -274,3 +274,54 @@ cdrom_boot(struct drive_s *drive)
 
     return 0;
 }
+
+// go figure some cdrom information, for a pretty boot menu entry.
+char*
+cdrom_media_info(struct drive_s *drive)
+{
+    ASSERT32FLAT();
+
+    struct disk_op_s dop;
+    memset(&dop, 0, sizeof(dop));
+    dop.drive_fl = drive;
+
+    int ret = scsi_is_ready(&dop);
+    if (ret)
+        return "empty";
+
+    // Read the Boot Record Volume Descriptor
+    u8 buffer[CDROM_SECTOR_SIZE];
+    dop.command = CMD_READ;
+    dop.lba = 0x11;
+    dop.count = 1;
+    dop.buf_fl = buffer;
+    ret = process_op(&dop);
+    if (ret)
+        return "read error";
+
+    // Is it bootable?
+    if (buffer[0])
+        return "not bootable";
+    if (strcmp((char*)&buffer[1], "CD001\001EL TORITO SPECIFICATION") != 0)
+        return "not bootable";
+
+    // Read the Primary Volume Descriptor
+    dop.command = CMD_READ;
+    dop.lba = 0x10;
+    dop.count = 1;
+    dop.buf_fl = buffer;
+    ret = process_op(&dop);
+    if (ret)
+        return "read error";
+
+    // Read volume id, trim trailing spaces
+    char *volume = znprintf(30, "%s", buffer + 40);
+    char *h = volume + strlen(volume);
+    while (h > volume) {
+        h--;
+        if (*h != ' ')
+            break;
+        *h = 0;
+    }
+    return volume;
+}
-- 
2.9.3


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] pretty boot menu entry for cdrom drives

Posted by Kevin O'Connor 11 weeks ago
On Fri, Sep 21, 2018 at 09:48:01AM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  src/util.h  |  1 +
>  src/boot.c  |  7 +++++++
>  src/cdrom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
> 
> diff --git a/src/util.h b/src/util.h
> index 7a23b518fc..6dd080f673 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -50,6 +50,7 @@ struct disk_op_s;
>  int cdemu_process_op(struct disk_op_s *op);
>  void cdrom_prepboot(void);
>  int cdrom_boot(struct drive_s *drive_g);
> +char *cdrom_media_info(struct drive_s *drive_g);
>  
>  // clock.c
>  void clock_setup(void);
> diff --git a/src/boot.c b/src/boot.c
> index ff705fd47f..80bcc13535 100644
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -395,6 +395,13 @@ boot_add_hd(struct drive_s *drive, const char *desc, int prio)
>  void
>  boot_add_cd(struct drive_s *drive, const char *desc, int prio)
>  {
> +    if (GET_GLOBAL(PlatformRunningOn) & PF_QEMU) {
> +        // We want short boot times.  But on physical hardware even
> +        // the test unit ready can take several seconds.  So do media
> +        // access on qemu only, where we know it will be fast.
> +        char *extra = cdrom_media_info(drive);
> +        desc = znprintf(MAXDESCSIZE, "%s (%s)", desc, extra);
> +    }
>      bootentry_add(IPL_TYPE_CDROM, defPrio(prio, DefaultCDPrio)
>                    , (u32)drive, desc);
>  }
> diff --git a/src/cdrom.c b/src/cdrom.c
> index 828fb3b842..b4c36622ae 100644
> --- a/src/cdrom.c
> +++ b/src/cdrom.c
> @@ -274,3 +274,54 @@ cdrom_boot(struct drive_s *drive)
>  
>      return 0;
>  }
> +
> +// go figure some cdrom information, for a pretty boot menu entry.
> +char*
> +cdrom_media_info(struct drive_s *drive)
> +{
> +    ASSERT32FLAT();
> +
> +    struct disk_op_s dop;
> +    memset(&dop, 0, sizeof(dop));
> +    dop.drive_fl = drive;
> +
> +    int ret = scsi_is_ready(&dop);
> +    if (ret)
> +        return "empty";
> +
> +    // Read the Boot Record Volume Descriptor
> +    u8 buffer[CDROM_SECTOR_SIZE];
> +    dop.command = CMD_READ;
> +    dop.lba = 0x11;
> +    dop.count = 1;
> +    dop.buf_fl = buffer;
> +    ret = process_op(&dop);
> +    if (ret)
> +        return "read error";
> +
> +    // Is it bootable?
> +    if (buffer[0])
> +        return "not bootable";
> +    if (strcmp((char*)&buffer[1], "CD001\001EL TORITO SPECIFICATION") != 0)
> +        return "not bootable";
> +
> +    // Read the Primary Volume Descriptor
> +    dop.command = CMD_READ;
> +    dop.lba = 0x10;
> +    dop.count = 1;
> +    dop.buf_fl = buffer;
> +    ret = process_op(&dop);
> +    if (ret)
> +        return "read error";
> +
> +    // Read volume id, trim trailing spaces
> +    char *volume = znprintf(30, "%s", buffer + 40);
> +    char *h = volume + strlen(volume);
> +    while (h > volume) {
> +        h--;
> +        if (*h != ' ')
> +            break;
> +        *h = 0;
> +    }
> +    return volume;
> +}

Thanks.  In general, looks fine to me.

Some minor comments:

If there's a read error or non-bootable device, shouldn't it just
revert to the original description?

There's a nullTrailingSpace() function to remove trailing spaces.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] pretty boot menu entry for cdrom drives

Posted by Gerd Hoffmann 11 weeks ago
> Thanks.  In general, looks fine to me.
> 
> Some minor comments:
> 
> If there's a read error or non-bootable device, shouldn't it just
> revert to the original description?

What about "empty" for drives not ready?  Drop that too?

I think we should be consistent here:  Either be verbose and report
"empty" / "read error" / "not-bootable" status in case we figure we
can't boot from the drive for one of the listed reasons.  Or be quiet
and just report the volume label for bootable drives and nothing
otherwise.

I've also toyed with the idea to use a higher default priority for
non-bootable cdrom drives, so with multiple cdroms and no explicit
boot order the bootable would be listed first and used by default.
What do you think about this?

> There's a nullTrailingSpace() function to remove trailing spaces.

Ah, good, I'll use that.

cheers,
  Gerd


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] pretty boot menu entry for cdrom drives

Posted by Kevin O'Connor 10 weeks ago
On Wed, Sep 26, 2018 at 07:03:37AM +0200, Gerd Hoffmann wrote:
> > Thanks.  In general, looks fine to me.
> > 
> > Some minor comments:
> > 
> > If there's a read error or non-bootable device, shouldn't it just
> > revert to the original description?
> 
> What about "empty" for drives not ready?  Drop that too?

Well, if I saw:

4. DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD] (empty)

I don't think I'd know what "empty" meant.  Also, it's a little odd to
have a C function sometimes return a dynamically allocated string and
sometimes return a constant string.  That said, I don't have a strong
opinion.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] pretty boot menu entry for cdrom drives

Posted by Peter Stuge 10 weeks ago
Kevin O'Connor wrote:
> it's a little odd to have a C function sometimes return a dynamically
> allocated string and sometimes return a constant string.

Gerd, please don't do that. Sure, maybe nothing is ever free()d, but
that's still very poor practice. Don't spread it.


//Peter

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios