On Sun, Feb 21, 2010 at 04:18:38PM -0700, Brandon Bennett wrote:
> > On Sat, Feb 20, 2010 at 9:05 PM, Kevin O'Connor <kevin(a)koconnor.net> wrote:
> >> Should a kernel fail during boot, I'd suspect it doesn't like one of
> >> the apm/pcibios callbacks, or it doesn't like one of the
> >> smbios/mptable/acpi tables. You could try compiling the SeaBIOS code
> >> (see http://seabios.org/Download ) and increasing the debugging by
> >> modifying src/config.h. Specifically, you could increase
> >> CONFIG_DEBUG_LEVEL, and set DEBUG_HDL_pcibios32 and DEBUG_HDL_apm to
> >> 1. Also, you could try disabling some of the features to see if that
> >> prevents the fault (eg, disabling CONFIG_ACPI / CONFIG_SMBIOS /
> >> CONFIG_MPTABLE).
> >
>
> I have narrowed it down to SMBIOS. If I disable CONFIG_SMBIOS the
> image boots up fine.
Gleb, have you seen this thread?
Some of the recent changes to smbios that look like possible culprits
are:
Make SMBIOS table pass MS SVVP test
Use MaxCountCPUs during building of per cpu tables.
Add malloc_high/fseg() and rework bios table creation to use them.
There were other changes, but the comments indicate they were only
ports of changes already in bochs. I suppose it's also possible the
lack of smbios is turning off some other feature in the guest (eg,
acpi) that's the real culprit.
-Kevin
Added Cc: seabios(a)seabios.org
On Wed, Jul 21, 2010 at 06:31:01AM +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 20, 2010 at 06:52:23PM +0900, Isaku Yamahata wrote:
> > On Wed, Jul 14, 2010 at 09:10:28AM -0600, Cam Macdonell wrote:
> > > On Tue, Jul 13, 2010 at 8:52 PM, Isaku Yamahata <yamahata(a)valinux.co.jp> wrote:
> > > > On Tue, Jul 13, 2010 at 04:48:19PM -0600, Cam Macdonell wrote:
> > > >> On Tue, Jul 13, 2010 at 2:41 PM, Isaku Yamahata <yamahata(a)valinux.co.jp> wrote:
> > > >> > On Tue, Jul 13, 2010 at 02:05:51PM -0600, Cam Macdonell wrote:
> > > >> >> >> > Seabios completely ignore the 64-bitness of the BAR. ?Looks like it also
> > > >> >> >> > thinks the second half of the BAR is an I/O region instead of memory (hence
> > > >> >> >> > the c200, that's part of the pci portio region.
> > > >> >> >
> > > >> >> > I've sent the patches to address it. But they haven't been merged yet.
> > > >> >> > seabios doesn't map BARs beyond 4GB.
> > > >> >> > If bar is mapped beyond 4GB, guest BIOS does it.
> > > >> >>
> > > >> >> Have those patches been merged yet?
> > > >> >
> > > >> > They have been merged into seabios upstream now.
> > > >> > qemu seabios fork hasn't pulled for a while, though.
> > > >> >
> > > >> >
> > > >> >> > To see how seabios works, it would help to increase CONFIG_DEBUG_LEVEL
> > > >> >> > in config.h of seabios
> > > >> >>
> > > >> >> Where does the output from seabios end up? ?Inside dmesg?
> > > >> >
> > > >> > It outputs them to the serial console which qemu emulates.
> > > >> > seabios is out of kernel control, so dmesg doesn't show it.
> > > >> >
> > > >> >
> > > >> >> >> pci_read_config: (val) 0x0 <- 0x1c (addr)
> > > >> >> >> pci_write_config: (val) 0x0 -> 0x1c (addr)
> > > >> >> >> pci_read_config: (val) 0xffffffff <- 0x1c (addr)
> > > >> >> >> pci_write_config: (val) 0x0 -> 0x1c (addr)
> > > >> >> >> pci_read_config: (val) 0x0 <- 0x1c (addr)
> > > >> >> >> pci_write_config: (val) 0x0 -> 0x1c (addr)
> > > >> >> >
> > > >> >> > seabios BAR3. Not sure how it is mapped from this
> > > >> >> > message.
> > > >> >>
> > > >> >> Isn't the BAR3 from the fact that a 64-bit BAR would use both BAR2 and
> > > >> >> BAR3 to store all 64-bits?
> > > >> >
> > > >> > Yes. Seabios misbehaves. 64bit bar is(was) a missing feature.
> > > >> > --
> > > >> > yamahata
> > > >> >
> > > >> >
> > > >>
> > > >> With the latest seabios git passed via -bios, I no longer see the
> > > >> 48-bit address, but instead a 32-bit address and then
> > > >> ffffffff00000000. ?This guest has 1gb of RAM so the address isn't be
> > > >> mapped beyond 4g.
> > > >
> > > > Can I see the debug log like before?
> > > > (hopefully seabios with CONFIG_DEBUG_LEVEL enabled.)
> > >
> > > Here's the dump from SeaBIOS in the region related to the PCI devices.
> > > The SeaBIOS output is identical whether the BAR is 32-bit or 64-bit.
> > >
> > > PCI: bus=0 devfn=0x10: vendor_id=0x1013 device_id=0x00b8
> > > region 0: 0xf0000000
> > > region 1: 0xf2000000
> > > region 6: 0xf2010000
> > > PCI: bus=0 devfn=0x18: vendor_id=0x1af4 device_id=0x1000
> > > region 0: 0x0000c020
> > > region 1: 0xf2020000
> > > region 6: 0xf2030000
> > > PCI: bus=0 devfn=0x20: vendor_id=0x1af4 device_id=0x1110
> > > region 0: 0xf2040000
> > > region 1: 0xf2041000
> > > region 2: 0x00000000
> >
> > Is this region (region 2 of devfn=0x20: vendor_id=0x1af4 device_id=0x1110)
> > the BAR in quistion?
> > The value 0 seems odd. Probably BAR address calculation overflowed.
> > Currently seabios doesn't check overflow. I attached the patch.
> >
> >
> > > > Do you know who sets the BAR to ffffffff00000000?
> > >
> > > Here are the config reads/writes related to the 0x18/1c, the 'IVSHMEM'
> > > lines are from the map function passed to pci_register_bar(). It
> > > looks like SeaBIOS sets the address to 0 and then the potentially
> > > useful e0000000 address gets mangled into ffffffff000000.
> >
> > There is something wrong with the debug message of write case, I suppose.
> > All written value are 0, but the resulted effect doesn't seems so.
> >
> > >
> > > IVSHMEM: guest pci addr = 0, guest h/w addr = 1090912256, size = 536870912
> > >
> > > ...snip...
> > >
> > > pci_read_config: (val) 0x4 <- 0x18 (addr)
> > > pci_write_config: (val) 0x0 -> 0x18 (addr)
> > > IVSHMEM: guest pci addr = e0000000, guest h/w addr = 1090912256, size = 20000000
> >
> > If 0 is written to 0x18, the bar address should be 0, but it says e0000000.
> >
> > > pci_read_config: (val) 0xe0000004 <- 0x18 (addr)
> >
> > The read value isn't 0. and so on...
> >
> > > pci_write_config: (val) 0x0 -> 0x18 (addr)
> > > pci_read_config: (val) 0x0 <- 0x1c (addr)
> > > pci_write_config: (val) 0x0 -> 0x1c (addr)
> > > IVSHMEM: guest pci addr = ffffffff00000000, guest h/w addr =
> > > 1090912256, size = 20000000
> > > pci_read_config: (val) 0xffffffff <- 0x1c (addr)
> > > pci_write_config: (val) 0x0 -> 0x1c (addr)
> > >
> > > and with the 64-bit guest I get this error as well (recall the guest
> > > fails to boot on 64-bit)
> > >
> > > BUG: kvm_dirty_pages_log_change: invalid parameters
> > > 00000000f0000000-00000000f0ffffff
> >
> >
> > diff --git a/src/pciinit.c b/src/pciinit.c
> > index b110531..6eca2ce 100644
> > --- a/src/pciinit.c
> > +++ b/src/pciinit.c
> > @@ -90,7 +90,8 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
> > /* If pci_bios_prefmem_addr == 0, keep old behaviour */
> > pci_bios_prefmem_addr != 0) {
> > paddr = &pci_bios_prefmem_addr;
> > - if (ALIGN(*paddr, size) + size >= BUILD_PCIPREFMEM_END) {
> > + if (ALIGN(*paddr, size) + size < *paddr ||
> > + ALIGN(*paddr, size) + size >= BUILD_PCIPREFMEM_END) {
> > dprintf(1,
> > "prefmem region of (bdf 0x%x bar %d) can't be mapped. "
> > "decrease BUILD_PCIMEM_SIZE and recompile. size %x\n",
> > @@ -99,7 +100,8 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
> > }
> > } else {
> > paddr = &pci_bios_mem_addr;
> > - if (ALIGN(*paddr, size) + size >= BUILD_PCIMEM_END) {
> > + if (ALIGN(*paddr, size) + size < *paddr ||
> > + ALIGN(*paddr, size) + size >= BUILD_PCIMEM_END) {
> > dprintf(1,
> > "mem region of (bdf 0x%x bar %d) can't be mapped. "
> > "increase BUILD_PCIMEM_SIZE and recompile. size %x\n",
>
> Looking at the source, all of the values like pci_bios_prefmem_addr seem to be
> 32 bit. Since in the spec prefetcheable memory is up to 64 bit,
> can't the math overflow, here and elsewhere?
> Maybe we should switch to 64 bit values all over ...
Make sense. I'll create a patch to convert them into u64.
>
> > @@ -116,12 +118,8 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
> >
> > int is_64bit = !(val & PCI_BASE_ADDRESS_SPACE_IO) &&
> > (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64;
> > - if (is_64bit) {
> > - if (size > 0) {
> > - pci_config_writel(bdf, ofs + 4, 0);
> > - } else {
> > - pci_config_writel(bdf, ofs + 4, ~0);
> > - }
> > + if (is_64bit && size > 0) {
> > + pci_config_writel(bdf, ofs + 4, 0);
> > }
> > return is_64bit;
> > }
>
>
> Was there any reason we wrote all-ones there on size 0?
> BAR sizing?
No reason. It's just left over from debugging.
So I'd like to remove it.
--
yamahata
This series further enhances the bootsplash code. The last patch
enables it by default (for coreboot users).
Kevin O'Connor (5):
Avoid using BSS variables in jpeg.c.
Breakup jpeg_decode into parsing and displaying phases.
Rename "decdata" to "jpeg" in bootsplash - to be consistent with
jpeg.c.
Autodetect video mode based on bootsplash jpeg dimensions.
Default bootsplash on (for coreboot users).
src/bootsplash.c | 117 +++++++++++++-------
src/config.h | 10 +--
src/jpeg.c | 307 +++++++++++++++++++++++++++++-------------------------
src/jpeg.h | 37 +------
4 files changed, 248 insertions(+), 223 deletions(-)
--
1.7.2
On Sat, Jul 10, 2010 at 12:41:18AM +0800, Liu, Jinsong wrote:
> Kevin O'Connor wrote:
> > On Thu, Jul 08, 2010 at 09:19:13PM +0800, Liu, Jinsong wrote:
> >> Avi Kivity wrote:
> >>> Very nice. I thought about doing this but abandoned it as
> >>> unmaintainable. Using external functions and the ID variable,
> >>> however, reduces the mess to tolerable proportions, and gains us a
> >>> lot of flexibility. We can now have any combinations of sockets and
> >>> installed cpus.
> >>
> >> Agree, only 1 concern
> >> will it bring debugable/ scalable issue by hardcode aml code?
> >
> > I've updated the patch (see below). This version documents how one
> > can build a new version of the Processor() ssdt snippet.
> >
> > I've tested this under linux - there were a few bugs in the previous
> > patch. I also had to replace the dynamically created CPUS array with
> > a dynamically created NTFY method - which is a bit more complicated.
>
> Yeah, thanks Kevin.
> After you done patch and draft test, our QA may do nightly test.
Hi Jinsong,
Have you had any feedback from tests?
Thanks,
-Kevin
On Mon, Jul 26, 2010 at 02:02:46PM +0900, Isaku Yamahata wrote:
> When allocating bar, overflow can occur.
> So add overflow check and don't allocate bar if overflowed.
> Overflow check is ugly, but necessary.
> Another suggested way is to change related variables u64 from u32
> thus overflow can't occur because the related value are all u32 addressable.
> Anyway even with u64, it is necessary to the resulted value > max_u32.
I wonder if we could create malloc zones for this and then use
pmm_alloc. The pmm_alloc function already handles overflow and
alignments.
One difficulty, though, is that pmm_alloc doesn't guarentee linear
allocations, and it looks like pci_bios_init_device_bridge assumes
this.
-Kevin
This patch series fixes some bugs in the bootsplash code, and adds
some cleanups. It is in preparation for turning bootsplash on by
default.
Kevin O'Connor (6):
Bootsplash fixes and cleanups.
Check that malloc succeeds in bootsplash code.
Don't do "double buffering" in bootsplash code.
Add call16_int10 helper to bootsplash.c.
Be sure to disable bootsplash on all BIOS boot cases.
Cleanup bootsplash vesa signature detection.
src/biosvar.h | 3 +-
src/boot.c | 8 +-
src/bootsplash.c | 210 ++++++++++++++++++++++--------------------------------
src/post.c | 8 ++-
src/util.h | 5 +-
5 files changed, 101 insertions(+), 133 deletions(-)
