[PATCH] csm: Fix boot priority translation

[David Woodhouse]

For CSM, the highest priority is zero. In SeaBIOS that means "don't", and
the highest priority is 1.

So we end up with the fun outcome that booting from NVMe worked only
when it *wasn't* selected as the primary boot target, because we don't
actually run the nvme_controller_setup() thread for an NVMe controller
if its boot prio is zero.

Signed-off-by: David Woodhouse 
---
 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;

[David Woodhouse]

On Wed, 2019-06-19 at 12:27 +0100, David Woodhouse wrote:
> For CSM, the highest priority is zero. In SeaBIOS that means "don't", and
> the highest priority is 1.
>
> So we end up with the fun outcome that booting from NVMe worked only
> when it *wasn't* selected as the primary boot target, because we don't
> actually run the nvme_controller_setup() thread for an NVMe controller
> if its boot prio is zero.
>
> Signed-off-by: David Woodhouse

Hm, turns out the NVMe hack is something that's only in our tree so for
upstream that second paragraph is a lie and can be dropped.

It's still a correct change to reflect the fact that SeaBIOS doesn't
use zero for the highest priority, and correctly handle
BBS_DO_NOT_BOOT_FROM and BBS_IGNORE_ENTRY values.

[PATCH v2] csm: Fix boot priority translation

[David Woodhouse]

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 
---
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;

[PATCH v2] csm: Fix boot priority translation

[Kevin O'Connor]

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

[PATCH v2] csm: Fix boot priority translation

[David Woodhouse]

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.

[PATCH v2] csm: Fix boot priority translation

[Kevin O'Connor]

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

[PATCH v2] csm: Fix boot priority translation

[Gerd Hoffmann]

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

[PATCH v2] csm: Fix boot priority translation

[David Woodhouse]

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?

[PATCH v2] csm: Fix boot priority translation

[Gerd Hoffmann]

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

[PATCH v2] csm: Fix boot priority translation

[Alexander Graf]

On 20.06.19 14:07, 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.
>
> 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
>
> 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;

Can you please add an inline comment for this to explain why exactly the 
+ 1 is needed? It's non-obvious enough that the git log is too far as 
information source.

Alex

[PATCH v3] csm: Fix boot priority translation

[David Woodhouse]

Explicitly handle the BBS_DO_NOT_BOOT_FROM and BBS_IGNORE_ENTRY values.

Also add one to the other priority values, as find_prio() does for
entries from bootorder. SeaBIOS uses zero for an item explicitly
selected in interactive_bootmenu().

Signed-off-by: David Woodhouse 
---
v2: No code change, just correct the commit message.
v3: Add comment in code as requested by agraf, update commit message more.

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

diff --git a/src/fw/csm.c b/src/fw/csm.c
index 3fcc252..8359bcb 100644
--- a/src/fw/csm.c
+++ b/src/fw/csm.c
@@ -319,6 +319,23 @@ 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:
+        // SeaBIOS default priorities start at 1, with 0 being used for
+        // an item explicitly selected from interactive_bootmenu().
+        // As in find_prio(), add 1 to the value being returned.
+        return csm_prio + 1;
+    }
+}
+
 int csm_bootprio_ata(struct pci_device *pci, int chanid, int slave)
 {
     if (!csm_boot_table)
@@ -327,7 +344,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 +353,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 +367,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;

[PATCH v3] csm: Fix boot priority translation

[Kevin O'Connor]

On Fri, Jun 28, 2019 at 02:57:47PM +0100, David Woodhouse wrote:
> Explicitly handle the BBS_DO_NOT_BOOT_FROM and BBS_IGNORE_ENTRY values.
>
> Also add one to the other priority values, as find_prio() does for
> entries from bootorder. SeaBIOS uses zero for an item explicitly
> selected in interactive_bootmenu().

Thanks, I committed this change.

-Kevin