[SeaBIOS] [PATCH v2] csm: Fix boot priority translation

David Woodhouse posted 1 patch 16 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/330621de51dbf9086e16fd68408c167f09e7aef6.camel@infradead.org
src/fw/csm.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

[SeaBIOS] [PATCH v2] csm: Fix boot priority translation

Posted by David Woodhouse 16 weeks ago
For CSM, the highest priority for a boot entry is zero. SeaBIOS doesn't
use zero, and the highest priority is 1.

Make the results of csm_bootprio_*() conform to that convention. Also
explicitly handle the BBS_DO_NOT_BOOT_FROM and BBS_IGNORE_ENTRY values.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
v2: No code change, just correct the commit message.

 src/fw/csm.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/fw/csm.c b/src/fw/csm.c
index 3fcc252..7663d31 100644
--- a/src/fw/csm.c
+++ b/src/fw/csm.c
@@ -319,6 +319,20 @@ handle_csm(struct bregs *regs)
     csm_return(regs);
 }
 
+static int csm_prio_to_seabios(u16 csm_prio)
+{
+    switch (csm_prio) {
+    case BBS_DO_NOT_BOOT_FROM:
+    case BBS_IGNORE_ENTRY:
+        return -1;
+
+    case BBS_LOWEST_PRIORITY:
+    case BBS_UNPRIORITIZED_ENTRY:
+    default:
+        return csm_prio + 1;
+    }
+}
+
 int csm_bootprio_ata(struct pci_device *pci, int chanid, int slave)
 {
     if (!csm_boot_table)
@@ -327,7 +341,7 @@ int csm_bootprio_ata(struct pci_device *pci, int chanid, int slave)
     int index = 1 + (chanid * 2) + slave;
     dprintf(3, "CSM bootprio for ATA%d,%d (index %d) is %d\n", chanid, slave,
             index, bbs[index].BootPriority);
-    return bbs[index].BootPriority;
+    return csm_prio_to_seabios(bbs[index].BootPriority);
 }
 
 int csm_bootprio_fdc(struct pci_device *pci, int port, int fdid)
@@ -336,7 +350,7 @@ int csm_bootprio_fdc(struct pci_device *pci, int port, int fdid)
         return -1;
     BBS_TABLE *bbs = (void *)csm_boot_table->BbsTable;
     dprintf(3, "CSM bootprio for FDC is %d\n", bbs[0].BootPriority);
-    return bbs[0].BootPriority;
+    return csm_prio_to_seabios(bbs[0].BootPriority);
 }
 
 int csm_bootprio_pci(struct pci_device *pci)
@@ -350,7 +364,7 @@ int csm_bootprio_pci(struct pci_device *pci)
         if (pci->bdf == pci_to_bdf(bbs[i].Bus, bbs[i].Device, bbs[i].Function)) {
             dprintf(3, "CSM bootprio for PCI(%d,%d,%d) is %d\n", bbs[i].Bus,
                     bbs[i].Device, bbs[i].Function, bbs[i].BootPriority);
-            return bbs[i].BootPriority;
+            return csm_prio_to_seabios(bbs[i].BootPriority);
         }
     }
     return -1;

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] csm: Fix boot priority translation

Posted by Kevin O'Connor 16 weeks ago
On Thu, Jun 20, 2019 at 01:07:45PM +0100, David Woodhouse wrote:
> For CSM, the highest priority for a boot entry is zero. SeaBIOS doesn't
> use zero, and the highest priority is 1.

FYI, SeaBIOS does treat zero as the highest priority.  And a negative
priority means "use default priority".

I'm fine with the change, though.

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] csm: Fix boot priority translation

Posted by David Woodhouse 16 weeks ago
On Thu, 2019-06-20 at 09:43 -0400, Kevin O'Connor wrote:
> On Thu, Jun 20, 2019 at 01:07:45PM +0100, David Woodhouse wrote:
> > For CSM, the highest priority for a boot entry is zero. SeaBIOS doesn't
> > use zero, and the highest priority is 1.
> 
> FYI, SeaBIOS does treat zero as the highest priority.  And a negative
> priority means "use default priority".
> 
> I'm fine with the change, though.

I don't think find_prio ever returns zero. 

    dprintf(1, "Searching bootorder for: %s\n", glob);
    int i;
    for (i = 0; i < BootorderCount; i++)
        if (glob_prefix(glob, Bootorder[i]))
            return i+1;
    return -1;


Our downstream patch for not initialising NVMe controllers if we aren't
going to boot from them, makes its decision based on the priority.

Originally it had 'if (bootprio_find_pci_device(pci) == 1)' to start
only the top priority boot device; nowadays it has '> 0' to match all
bootable drives.

Now, I can fix that patch fairly easily to be '>= 0' now that it no
longer wants to match precisely only the first. But in general, it
seems that '1' is the top priority so making the CSM values match that
seems sensible.
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] csm: Fix boot priority translation

Posted by Gerd Hoffmann 16 weeks ago
  Hi,

> Our downstream patch for not initialising NVMe controllers if we aren't
> going to boot from them, makes its decision based on the priority.

What is the state of that patch btw?

I remember it being posted a while back, suggestion was to make it
generic (skip init for everything which has a priority higher than
"HALT", not only nvm), but the discussion went silent quickly and IIRC
there never was a v2 of that patch ...

cheers,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] csm: Fix boot priority translation

Posted by David Woodhouse 16 weeks ago
On Fri, 2019-06-21 at 10:40 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Our downstream patch for not initialising NVMe controllers if we aren't
> > going to boot from them, makes its decision based on the priority.
> 
> What is the state of that patch btw?

It's on my "technical debt that I want to kill so we're using a
pristine upstream again" list. I'll take a look after I've fixed the
final quirks and got CSM boots working seamlessly again.

> I remember it being posted a while back, suggestion was to make it
> generic (skip init for everything which has a priority higher than
> "HALT", not only nvm), but the discussion went silent quickly and IIRC
> there never was a v2 of that patch ...

If we do that, then those drives won't be available to INT13h at all.
We'd be saying that just because we don't want to *boot* from them, DOS
can't see them at all.

Would we assign INT13h drive numbers to them and initialise them on
demand? I don't think that can work because we don't even know if an
ATA drive is present, don't know how many NVMe namespaces are present,
so couldn't assign numbers accordingly.

Perhaps the first INT13h access to a drive that doesn't exist (0x81
normally?) would trigger *all* pending drive probes to happen? Can we
even do them that late?

Or do we make it an option — at build time or through fw_cfg or
something else, to just *not* initialise those drives at all because we
know we're not booting DOS today?


_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] csm: Fix boot priority translation

Posted by Gerd Hoffmann 16 weeks ago
On Fri, Jun 21, 2019 at 11:33:08AM +0100, David Woodhouse wrote:
> On Fri, 2019-06-21 at 10:40 +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > Our downstream patch for not initialising NVMe controllers if we aren't
> > > going to boot from them, makes its decision based on the priority.
> > 
> > What is the state of that patch btw?
> 
> It's on my "technical debt that I want to kill so we're using a
> pristine upstream again" list.

Nice.

> > I remember it being posted a while back, suggestion was to make it
> > generic (skip init for everything which has a priority higher than
> > "HALT", not only nvm), but the discussion went silent quickly and IIRC
> > there never was a v2 of that patch ...
> 
> If we do that, then those drives won't be available to INT13h at all.
> We'd be saying that just because we don't want to *boot* from them, DOS
> can't see them at all.
> 
> Would we assign INT13h drive numbers to them and initialise them on
> demand? I don't think that can work because we don't even know if an
> ATA drive is present, don't know how many NVMe namespaces are present,
> so couldn't assign numbers accordingly.
> 
> Perhaps the first INT13h access to a drive that doesn't exist (0x81
> normally?) would trigger *all* pending drive probes to happen? Can we
> even do them that late?
> 
> Or do we make it an option — at build time or through fw_cfg or
> something else, to just *not* initialise those drives at all because we
> know we're not booting DOS today?

It already is configurable.  The "HALT" boot priority entry is only
added to the list by qemu in case the user specifies "-boot strict=on"
on the command line.

With strict=on seabios will only boot from devices which have a boot
priority explicitly assigned.

With strict=off seabios will continue trying the remaining devices in
default priority order in case all devices with an assigned priority
failed.

Extending the strict=on case to also skip the device initialization
looks reasonable to me.  Faster boot time, less memory consumption,
and if you don't want that or need DOS access the second hard disk you
can use strict=off (which is the default btw).

cheers,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH v2] csm: Fix boot priority translation

Posted by Kevin O'Connor 16 weeks ago
On Thu, Jun 20, 2019 at 03:12:33PM +0100, David Woodhouse wrote:
> On Thu, 2019-06-20 at 09:43 -0400, Kevin O'Connor wrote:
> > On Thu, Jun 20, 2019 at 01:07:45PM +0100, David Woodhouse wrote:
> > > For CSM, the highest priority for a boot entry is zero. SeaBIOS doesn't
> > > use zero, and the highest priority is 1.
> > 
> > FYI, SeaBIOS does treat zero as the highest priority.  And a negative
> > priority means "use default priority".
> > 
> > I'm fine with the change, though.
> 
> I don't think find_prio ever returns zero. 
> 
>     dprintf(1, "Searching bootorder for: %s\n", glob);
>     int i;
>     for (i = 0; i < BootorderCount; i++)
>         if (glob_prefix(glob, Bootorder[i]))
>             return i+1;
>     return -1;

True, but interactive_bootmenu() sets the priority to zero if the
device is selected.  (I'm guessing find_prio() returned 1+ just to
avoid confusion with interactive_bootmenu().)

In any case, I'm fine with your change.

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org