From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=47793 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1ObQH6-0000Jk-9U for qemu-devel@nongnu.org; Tue, 20 Jul 2010 23:47:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1ObQH2-0008L5-HX for qemu-devel@nongnu.org; Tue, 20 Jul 2010 23:47:16 -0400 Received: from mail.valinux.co.jp ([210.128.90.3]:40791) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1ObQH1-0008Ks-Rl for qemu-devel@nongnu.org; Tue, 20 Jul 2010 23:47:12 -0400 Date: Wed, 21 Jul 2010 12:49:18 +0900 From: Isaku Yamahata Subject: Re: [Qemu-devel] Re: Unusual physical address when using 64-bit BAR Message-ID: <20100721034918.GA6285@valinux.co.jp> References: <4C2997C5.1020302@redhat.com> <20100630032901.GA19142@valinux.co.jp> <20100713204157.GI31689@valinux.co.jp> <20100714025216.GK31689@valinux.co.jp> <20100720095223.GC24957@valinux.co.jp> <20100721033101.GD11146@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100721033101.GD11146@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Cam Macdonell , seabios@seabios.org, "qemu-devel@nongnu.org Developers" , Avi Kivity Added Cc: seabios@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 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 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