Patchwork [2/7] seabios: shadow: make device finding more generic.

login
register
mail settings
Submitter Isaku Yamahata
Date July 12, 2010, 11:47 a.m.
Message ID <33930ae88cd814000d782f802b962961a9bbad31.1278935094.git.yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/58596/
State New
Headers show

Comments

Isaku Yamahata - July 12, 2010, 11:47 a.m.
pam register offset is north bridge specific.
So determine the offset based on found north bridge.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 src/dev-i440fx.c |    9 +++++++++
 src/dev-i440fx.h |    1 +
 src/post.h       |   13 +++++++++++++
 src/shadow.c     |   44 +++++++++++++++++++++++++++++---------------
 4 files changed, 52 insertions(+), 15 deletions(-)
 create mode 100644 src/post.h
Kevin O'Connor - July 13, 2010, 12:59 a.m.
On Mon, Jul 12, 2010 at 08:47:47PM +0900, Isaku Yamahata wrote:
> pam register offset is north bridge specific.
> So determine the offset based on found north bridge.

Is it really just the offset that is north bridge specific?  I thought
the entire process was very north bridge specific.

If so, I don't think it makes sense to pass back the pam0 register -
instead the north bridge specific code should do the necessary work
(using helper functions if possible).

I have the same concern with part 3 and 4 of this series.

-Kevin
Isaku Yamahata - July 13, 2010, 9:45 a.m.
On Mon, Jul 12, 2010 at 08:59:14PM -0400, Kevin O'Connor wrote:
> On Mon, Jul 12, 2010 at 08:47:47PM +0900, Isaku Yamahata wrote:
> > pam register offset is north bridge specific.
> > So determine the offset based on found north bridge.
> 
> Is it really just the offset that is north bridge specific?  I thought
> the entire process was very north bridge specific.
>
> If so, I don't think it makes sense to pass back the pam0 register -
> instead the north bridge specific code should do the necessary work
> (using helper functions if possible).
> 
> I have the same concern with part 3 and 4 of this series.

I440fx and Q35 (all Intel chipset?) are similar in registers
which seabios programs, so I choice to abstract it at register offset level.
I don't expect that other vendor's chipset support is wanted.

If you want more high level abstract, I'll respin the patch set.
Kevin O'Connor - July 20, 2010, 3:05 a.m.
On Tue, Jul 13, 2010 at 06:45:00PM +0900, Isaku Yamahata wrote:
> On Mon, Jul 12, 2010 at 08:59:14PM -0400, Kevin O'Connor wrote:
> > On Mon, Jul 12, 2010 at 08:47:47PM +0900, Isaku Yamahata wrote:
> > > pam register offset is north bridge specific.
> > > So determine the offset based on found north bridge.
> > 
> > Is it really just the offset that is north bridge specific?  I thought
> > the entire process was very north bridge specific.
> >
> > If so, I don't think it makes sense to pass back the pam0 register -
> > instead the north bridge specific code should do the necessary work
> > (using helper functions if possible).
> > 
> > I have the same concern with part 3 and 4 of this series.
> 
> I440fx and Q35 (all Intel chipset?) are similar in registers
> which seabios programs, so I choice to abstract it at register offset level.
> I don't expect that other vendor's chipset support is wanted.

Although it isn't currently used, the memory locking support is useful
on real machines too.  I'd prefer a solution that would work on both
qemu and real machines.

It's minor for part 2 of the series, but I found part 3/4 to be hard
to follow due to the way the flow of code jumps between machine
specific code in dev-i440fx.c and the smm code in smm.c.

> If you want more high level abstract, I'll respin the patch set.

I've been meaning to look through the full series of changes in your
repo, but have not yet had a chance to do so.  I hope to get to that
in the next few days.  Sorry for the delay.

-Kevin

Patch

diff --git a/src/dev-i440fx.c b/src/dev-i440fx.c
index 864a52c..5961efc 100644
--- a/src/dev-i440fx.c
+++ b/src/dev-i440fx.c
@@ -14,8 +14,17 @@ 
 #include "ioport.h" // outb
 #include "pci.h" // pci_config_writeb
 #include "pci_regs.h" // PCI_INTERRUPT_LINE
+#include "post.h"
 #include "dev-i440fx.h"
 
+#define I440FX_PAM0     0x59
+
+void i440fx_shadow_detected(u16 bdf, void *arg)
+{
+    struct pam_regs *pam_regs = arg;
+    pam_regs->pam0 = I440FX_PAM0;
+}
+
 /* PIIX3/PIIX4 PCI to ISA bridge */
 void piix_isa_bridge_init(u16 bdf, void *arg)
 {
diff --git a/src/dev-i440fx.h b/src/dev-i440fx.h
index ded5740..abcd0d1 100644
--- a/src/dev-i440fx.h
+++ b/src/dev-i440fx.h
@@ -3,6 +3,7 @@ 
 
 #include "types.h" // u16
 
+void i440fx_shadow_detected(u16 bdf, void *arg);
 void piix_isa_bridge_init(u16 bdf, void *arg);
 void piix_ide_init(u16 bdf, void *arg);
 void piix4_pm_init(u16 bdf, void *arg);
diff --git a/src/post.h b/src/post.h
new file mode 100644
index 0000000..18f89fb
--- /dev/null
+++ b/src/post.h
@@ -0,0 +1,13 @@ 
+//
+// Copyright (C) 2010 Isaku Yamahata <yamahata at valinux co jp>
+//
+// This file may be distributed under the terms of the GNU LGPLv3 license.
+#ifndef __POST_H
+#define __POST_H
+
+struct pam_regs
+{
+    u32 pam0;
+};
+
+#endif /* __POST_H */
diff --git a/src/shadow.c b/src/shadow.c
index 978424e..2a360a8 100644
--- a/src/shadow.c
+++ b/src/shadow.c
@@ -9,6 +9,8 @@ 
 #include "pci.h" // pci_config_writeb
 #include "config.h" // CONFIG_*
 #include "pci_ids.h" // PCI_VENDOR_ID_INTEL
+#include "post.h"
+#include "dev-i440fx.h"
 
 // Test if 'addr' is in the range from 'start'..'start+size'
 #define IN_RANGE(addr, start, size) ({   \
@@ -18,35 +20,42 @@ 
             (__addr - __start < __size); \
         })
 
+static const struct pci_device_id dram_controller_tbl[] = {
+    PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
+               i440fx_shadow_detected),
+    PCI_DEVICE_END
+};
+
 // On the emulators, the bios at 0xf0000 is also at 0xffff0000
 #define BIOS_SRC_ADDR 0xffff0000
 
 // Enable shadowing and copy bios.
 static void
-__make_bios_writable(u16 bdf)
+__make_bios_writable(u16 bdf, u32 pam0)
 {
     // Make ram from 0xc0000-0xf0000 writable
     int clear = 0;
     int i;
     for (i=0; i<6; i++) {
-        int reg = pci_config_readb(bdf, 0x5a + i);
+      u32 pam = pam0 + 1 + i;
+      int reg = pci_config_readb(bdf, pam);
         if ((reg & 0x11) != 0x11) {
             // Need to copy optionroms to work around qemu implementation
             void *mem = (void*)(BUILD_ROM_START + i * 32*1024);
             memcpy((void*)BUILD_BIOS_TMP_ADDR, mem, 32*1024);
-            pci_config_writeb(bdf, 0x5a + i, 0x33);
+            pci_config_writeb(bdf, pam, 0x33);
             memcpy(mem, (void*)BUILD_BIOS_TMP_ADDR, 32*1024);
             clear = 1;
         } else {
-            pci_config_writeb(bdf, 0x5a + i, 0x33);
+            pci_config_writeb(bdf, pam, 0x33);
         }
     }
     if (clear)
         memset((void*)BUILD_BIOS_TMP_ADDR, 0, 32*1024);
 
     // Make ram from 0xf0000-0x100000 writable
-    int reg = pci_config_readb(bdf, 0x59);
-    pci_config_writeb(bdf, 0x59, 0x30);
+    int reg = pci_config_readb(bdf, pam0);
+    pci_config_writeb(bdf, pam0, 0x30);
     if (reg & 0x10)
         // Ram already present.
         return;
@@ -64,26 +73,29 @@  make_bios_writable(void)
 
     dprintf(3, "enabling shadow ram\n");
 
+    // at this point, staticlly alloacted variable can't written.
+    // so stack should be used.
+    struct pam_regs pam_regs;
     // Locate chip controlling ram shadowing.
-    int bdf = pci_find_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441);
+    int bdf = pci_find_init_device(dram_controller_tbl, &pam_regs);
     if (bdf < 0) {
         dprintf(1, "Unable to unlock ram - bridge not found\n");
         return;
     }
 
-    int reg = pci_config_readb(bdf, 0x59);
+    int reg = pci_config_readb(bdf, pam_regs.pam0);
     if (!(reg & 0x10)) {
         // QEMU doesn't fully implement the piix shadow capabilities -
         // if ram isn't backing the bios segment when shadowing is
         // disabled, the code itself wont be in memory.  So, run the
         // code from the high-memory flash location.
         u32 pos = (u32)__make_bios_writable - BUILD_BIOS_ADDR + BIOS_SRC_ADDR;
-        void (*func)(u16 bdf) = (void*)pos;
-        func(bdf);
+        void (*func)(u16 bdf, u32 pam0) = (void*)pos;
+        func(bdf, pam_regs.pam0);
         return;
     }
     // Ram already present - just enable writes
-    __make_bios_writable(bdf);
+    __make_bios_writable(bdf, pam_regs.pam0);
 }
 
 // Make the BIOS code segment area (0xf0000) read-only.
@@ -95,7 +107,8 @@  make_bios_readonly(void)
 
     dprintf(3, "locking shadow ram\n");
 
-    int bdf = pci_find_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441);
+    struct pam_regs pam_regs;
+    int bdf = pci_find_init_device(dram_controller_tbl, &pam_regs);
     if (bdf < 0) {
         dprintf(1, "Unable to lock ram - bridge not found\n");
         return;
@@ -108,14 +121,15 @@  make_bios_readonly(void)
     int i;
     for (i=0; i<6; i++) {
         u32 mem = BUILD_ROM_START + i * 32*1024;
+        u32 pam = pam_regs.pam0 + 1 + i;
         if (RomEnd <= mem + 16*1024) {
             if (RomEnd > mem)
-                pci_config_writeb(bdf, 0x5a + i, 0x31);
+                pci_config_writeb(bdf, pam, 0x31);
             break;
         }
-        pci_config_writeb(bdf, 0x5a + i, 0x11);
+        pci_config_writeb(bdf, pam, 0x11);
     }
 
     // Write protect 0xf0000-0x100000
-    pci_config_writeb(bdf, 0x59, 0x10);
+    pci_config_writeb(bdf, pam_regs.pam0, 0x10);
 }