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 dwmw2@infradead.org --- 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;
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 dwmw2@infradead.org
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.
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;
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
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.
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
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
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?
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
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 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;
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
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 dwmw2@infradead.org --- 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;
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