Hi,
I've discovered a mess:
- bootorder paths for ATA-SFF devices do not use the channel/port-on-controller number in the path, instead they use a volatile and unique-to-each-channel number that has no relation to the actual arrangement of the hardware.
For instance, on a MCP55-based machine (no AHCI, but plenty of ATA-SFF channels/ports:
The master drive on the PATA channel is (and should be, but is only by coincidence) /pci@i0cf8/*@4/drive@0/disk@0 but the first SATA port on /pci@i0cf8/*@5 is currently detected by SeaBIOS as something other than /pci@i0cf8/*@5/drive@0/disk@0 (the drive@ number is something like 2) and the second SATA port is something other than /pci@i0cf8/*@5/drive@1/disk@0 (drive@ number is usually more like 3) The /pci@i0cf8/*@5 controller only has two ports on it.
- bootprio_find_ata_device() is shared between the ahci.c and ata.c drivers. This becomes interesting in a system using SeaBIOS as CSM and/or having certain combinations of ATA-SFF and/or AHCI controllers.
(And why does the SeaBIOS CSM even have drivers in it? I would have thought it would be using the UEFI drivers...but maybe we had to ExitBootServices() to work properly...)
- Thus the chanid argument to csm_bootprio_ata() could be any of two unrelated things:
- the global-based index of a ata.c channel - the controller-based port index of any of the ahci.c controllers in the system
I've attached a patch that attempts to address the issue with the ata.c bootorder paths, but it doesn't really fix the actual problem.
Jonathan Kollasch
On Mon, Jul 06, 2015 at 08:59:58PM -0500, Jonathan A. Kollasch wrote:
Hi,
I've discovered a mess:
bootorder paths for ATA-SFF devices do not use the channel/port-on-controller number in the path, instead they use a volatile and unique-to-each-channel number that has no relation to the actual arrangement of the hardware.
For instance, on a MCP55-based machine (no AHCI, but plenty of ATA-SFF channels/ports:
The master drive on the PATA channel is (and should be, but is only by coincidence) /pci@i0cf8/*@4/drive@0/disk@0 but the first SATA port on /pci@i0cf8/*@5 is currently detected by SeaBIOS as something other than /pci@i0cf8/*@5/drive@0/disk@0 (the drive@ number is something like 2) and the second SATA port is something other than /pci@i0cf8/*@5/drive@1/disk@0 (drive@ number is usually more like 3) The /pci@i0cf8/*@5 controller only has two ports on it.
Thanks. Can you post a full debug log (log level 8) from a boot with incorrect bootorder designations?
-Kevin
On Mon, Jul 06, 2015 at 10:17:18PM -0400, Kevin O'Connor wrote:
On Mon, Jul 06, 2015 at 08:59:58PM -0500, Jonathan A. Kollasch wrote:
Hi,
I've discovered a mess:
bootorder paths for ATA-SFF devices do not use the channel/port-on-controller number in the path, instead they use a volatile and unique-to-each-channel number that has no relation to the actual arrangement of the hardware.
For instance, on a MCP55-based machine (no AHCI, but plenty of ATA-SFF channels/ports:
The master drive on the PATA channel is (and should be, but is only by coincidence) /pci@i0cf8/*@4/drive@0/disk@0 but the first SATA port on /pci@i0cf8/*@5 is currently detected by SeaBIOS as something other than /pci@i0cf8/*@5/drive@0/disk@0 (the drive@ number is something like 2) and the second SATA port is something other than /pci@i0cf8/*@5/drive@1/disk@0 (drive@ number is usually more like 3) The /pci@i0cf8/*@5 controller only has two ports on it.
Thanks. Can you post a full debug log (log level 8) from a boot with incorrect bootorder designations?
-Kevin
Attached.
On Mon, Jul 06, 2015 at 08:59:58PM -0500, Jonathan A. Kollasch wrote:
Hi,
I've discovered a mess:
bootorder paths for ATA-SFF devices do not use the channel/port-on-controller number in the path, instead they use a volatile and unique-to-each-channel number that has no relation to the actual arrangement of the hardware.
For instance, on a MCP55-based machine (no AHCI, but plenty of ATA-SFF channels/ports:
The master drive on the PATA channel is (and should be, but is only by coincidence) /pci@i0cf8/*@4/drive@0/disk@0 but the first SATA port on /pci@i0cf8/*@5 is currently detected by SeaBIOS as something other than /pci@i0cf8/*@5/drive@0/disk@0 (the drive@ number is something like 2) and the second SATA port is something other than /pci@i0cf8/*@5/drive@1/disk@0 (drive@ number is usually more like 3) The /pci@i0cf8/*@5 controller only has two ports on it.
I agree the current code is incorrect. However, I don't think it requires changing the CSM or AHCI code. I think the ATA code should just track which controller number it is relative to the given PCI id and pass that to bootprio_find_ata_device() instead of chanid.
- bootprio_find_ata_device() is shared between the ahci.c and ata.c drivers. This becomes interesting in a system using SeaBIOS as CSM and/or having certain combinations of ATA-SFF and/or AHCI controllers.
I think the CSM code is just not capable of handling a system with more than one ATA controller or with drives of a non-ATA type. I vaguely recall David saying something like that when he committed the code. (I think he was going to try and chase it down with the CSM spec owners.) So, I would just pass in the updated "chanid" to CSM without worrying about the different behavior on secondary ATA controllers.
(And why does the SeaBIOS CSM even have drivers in it? I would have thought it would be using the UEFI drivers...but maybe we had to ExitBootServices() to work properly...)
I'm not sure, but I don't think it's valid to call back into UEFI during the system runtime - only during the init phase. (And, SeaBIOS needs drivers through the lifetime of the system for old DOS apps.)
Thus the chanid argument to csm_bootprio_ata() could be any of two unrelated things:
- the global-based index of a ata.c channel
- the controller-based port index of any of the ahci.c controllers in the system
I've attached a patch that attempts to address the issue with the ata.c bootorder paths, but it doesn't really fix the actual problem.
Thanks. Are you willing to respin a simpler version of the patch?
-Kevin
On Tue, Jul 07, 2015 at 05:06:03PM -0400, Kevin O'Connor wrote:
Thanks. Are you willing to respin a simpler version of the patch?
Attached.
On Wed, Jul 08, 2015 at 10:21:57PM -0500, Jonathan A. Kollasch wrote:
On Tue, Jul 07, 2015 at 05:06:03PM -0400, Kevin O'Connor wrote:
Thanks. Are you willing to respin a simpler version of the patch?
Attached.
I don't think the user visible drive descriptions or the debug strings should change. How about the patch below instead?
-Kevin
From 6a668b35e1691de8ebccf9c8b627c58f333cea7a Mon Sep 17 00:00:00 2001
From: Kevin O'Connor kevin@koconnor.net Date: Tue, 14 Jul 2015 15:12:25 -0400 Subject: [PATCH] ata: Make sure "chanid" is relative to PCI device for bootorder file
When specifying drives in the bootorder file, the "drive@x" parameter should be relative to the given PCI device and not relative to the total number of ATA controllers in the machine. This patch separates the tracking of "chanid" (channel number relative to a given PCI devices) from the "ataid" (channel number relative to the total number of ATA channels).
Reported-by: Jonathan A. Kollasch jakllsch@kollasch.net Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/block.c | 6 ++---- src/hw/ata.c | 25 +++++++++++++------------ src/hw/ata.h | 1 + 3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/block.c b/src/block.c index 5e08663..a1054ac 100644 --- a/src/block.c +++ b/src/block.c @@ -74,10 +74,8 @@ get_translation(struct drive_s *drive) u8 type = drive->type; if (CONFIG_QEMU && type == DTYPE_ATA) { // Emulators pass in the translation info via nvram. - u8 ataid = drive->cntl_id; - u8 channel = ataid / 2; - u8 translation = rtc_read(CMOS_BIOS_DISKTRANSFLAG + channel/2); - translation >>= 2 * (ataid % 4); + u8 translation = rtc_read(CMOS_BIOS_DISKTRANSFLAG + drive->cntl_id/4); + translation >>= 2 * (drive->cntl_id % 4); translation &= 0x03; return translation; } diff --git a/src/hw/ata.c b/src/hw/ata.c index 3efc6ab..fbbbbc1 100644 --- a/src/hw/ata.c +++ b/src/hw/ata.c @@ -718,7 +718,7 @@ init_atadrive(struct atadrive_s *dummy, u16 *buffer) memset(adrive, 0, sizeof(*adrive)); adrive->chan_gf = dummy->chan_gf; adrive->slave = dummy->slave; - adrive->drive.cntl_id = adrive->chan_gf->chanid * 2 + dummy->slave; + adrive->drive.cntl_id = adrive->chan_gf->ataid * 2 + dummy->slave; adrive->drive.removable = (buffer[0] & 0x80) ? 1 : 0; return adrive; } @@ -743,7 +743,7 @@ init_drive_atapi(struct atadrive_s *dummy, u16 *buffer) char model[MAXMODEL+1]; char *desc = znprintf(MAXDESCSIZE , "DVD/CD [ata%d-%d: %s ATAPI-%d %s]" - , adrive->chan_gf->chanid, adrive->slave + , adrive->chan_gf->ataid, adrive->slave , ata_extract_model(model, MAXMODEL, buffer) , ata_extract_version(buffer) , (iscd ? "DVD/CD" : "Device")); @@ -795,7 +795,7 @@ init_drive_ata(struct atadrive_s *dummy, u16 *buffer) char model[MAXMODEL+1]; char *desc = znprintf(MAXDESCSIZE , "ata%d-%d: %s ATA-%d Hard-Disk (%u %ciBytes)" - , adrive->chan_gf->chanid, adrive->slave + , adrive->chan_gf->ataid, adrive->slave , ata_extract_model(model, MAXMODEL, buffer) , ata_extract_version(buffer) , (u32)adjsize, adjprefix); @@ -869,7 +869,7 @@ ata_detect(void *data) u8 sc = inb(iobase1+ATA_CB_SC); u8 sn = inb(iobase1+ATA_CB_SN); dprintf(6, "ata_detect ata%d-%d: sc=%x sn=%x dh=%x\n" - , chan_gf->chanid, slave, sc, sn, dh); + , chan_gf->ataid, slave, sc, sn, dh); if (sc != 0x55 || sn != 0xaa || dh != newdh) continue;
@@ -916,16 +916,17 @@ ata_detect(void *data)
// Initialize an ata controller and detect its drives. static void -init_controller(struct pci_device *pci, int irq +init_controller(struct pci_device *pci, int chanid, int irq , u32 port1, u32 port2, u32 master) { - static int chanid = 0; + static int ataid = 0; struct ata_channel_s *chan_gf = malloc_fseg(sizeof(*chan_gf)); if (!chan_gf) { warn_noalloc(); return; } - chan_gf->chanid = chanid++; + chan_gf->ataid = ataid++; + chan_gf->chanid = chanid; chan_gf->irq = irq; chan_gf->pci_bdf = pci ? pci->bdf : -1; chan_gf->pci_tmp = pci; @@ -933,7 +934,7 @@ init_controller(struct pci_device *pci, int irq chan_gf->iobase2 = port2; chan_gf->iomaster = master; dprintf(1, "ATA controller %d at %x/%x/%x (irq %d dev %x)\n" - , chanid, port1, port2, master, irq, chan_gf->pci_bdf); + , ataid, port1, port2, master, irq, chan_gf->pci_bdf); run_thread(ata_detect, chan_gf); }
@@ -969,7 +970,7 @@ init_pciata(struct pci_device *pci, u8 prog_if) port2 = PORT_ATA1_CTRL_BASE; irq = IRQ_ATA1; } - init_controller(pci, irq, port1, port2, master); + init_controller(pci, 0, irq, port1, port2, master);
if (prog_if & 4) { port1 = (pci_config_readl(bdf, PCI_BASE_ADDRESS_2) @@ -982,7 +983,7 @@ init_pciata(struct pci_device *pci, u8 prog_if) port2 = PORT_ATA2_CTRL_BASE; irq = IRQ_ATA2; } - init_controller(pci, irq, port1, port2, master ? master + 8 : 0); + init_controller(pci, 1, irq, port1, port2, master ? master + 8 : 0); }
static void @@ -1014,9 +1015,9 @@ ata_scan(void) if (CONFIG_QEMU && hlist_empty(&PCIDevices)) { // No PCI devices found - probably a QEMU "-M isapc" machine. // Try using ISA ports for ATA controllers. - init_controller(NULL, IRQ_ATA1 + init_controller(NULL, 0, IRQ_ATA1 , PORT_ATA1_CMD_BASE, PORT_ATA1_CTRL_BASE, 0); - init_controller(NULL, IRQ_ATA2 + init_controller(NULL, 1, IRQ_ATA2 , PORT_ATA2_CMD_BASE, PORT_ATA2_CTRL_BASE, 0); return; } diff --git a/src/hw/ata.h b/src/hw/ata.h index 65bfcb7..cd14e59 100644 --- a/src/hw/ata.h +++ b/src/hw/ata.h @@ -11,6 +11,7 @@ struct ata_channel_s { u16 iomaster; u8 irq; u8 chanid; + u8 ataid; int pci_bdf; struct pci_device *pci_tmp; };
On Tue, Jul 14, 2015 at 03:18:25PM -0400, Kevin O'Connor wrote:
On Wed, Jul 08, 2015 at 10:21:57PM -0500, Jonathan A. Kollasch wrote:
On Tue, Jul 07, 2015 at 05:06:03PM -0400, Kevin O'Connor wrote:
Thanks. Are you willing to respin a simpler version of the patch?
Attached.
I don't think the user visible drive descriptions or the debug strings should change. How about the patch below instead?
-Kevin
Looks fine. But if you're going to add ataid to ata_channel_s you can probably also avoid any changes to CSM-mode behavior too. I had shied away from adding a member to ata_channel_s because it was already nicely packed (a multiple of 4 bytes).
Also changing the bootorder names is a user-visible change, one we should change, but it's a change none the less, so maybe changing the descriptions isn't such a bad thing. But maybe changing the descriptions should be in a different changeset if we do decide to do it.
Jonathan Kollasch
On Thu, Jul 23, 2015 at 08:41:46AM -0500, Jonathan A. Kollasch wrote:
On Tue, Jul 14, 2015 at 03:18:25PM -0400, Kevin O'Connor wrote:
On Wed, Jul 08, 2015 at 10:21:57PM -0500, Jonathan A. Kollasch wrote:
On Tue, Jul 07, 2015 at 05:06:03PM -0400, Kevin O'Connor wrote:
Thanks. Are you willing to respin a simpler version of the patch?
Attached.
I don't think the user visible drive descriptions or the debug strings should change. How about the patch below instead?
-Kevin
Looks fine. But if you're going to add ataid to ata_channel_s you can probably also avoid any changes to CSM-mode behavior too. I had shied away from adding a member to ata_channel_s because it was already nicely packed (a multiple of 4 bytes).
Also changing the bootorder names is a user-visible change, one we should change, but it's a change none the less, so maybe changing the descriptions isn't such a bad thing. But maybe changing the descriptions should be in a different changeset if we do decide to do it.
Thanks. I committed the patch.
I don't think using the PCI bus/device/function id in the user visible string makes sense as only hardcore developers would know what that means. (It would be confusing to a general user to see ATA@00b2 or any similar non-linear identifier.)
-Kevin
-
Jonathan A. Kollasch -
Kevin O'Connor