[Qemu-devel] Re: [PATCH V5 15/29] pci: typedef pcibus_t as uint64_t instead of uint32_t.

[Michael S. Tsirkin]

On Wed, Oct 14, 2009 at 01:35:49PM +0900, Isaku Yamahata wrote:
> On Tue, Oct 13, 2009 at 04:39:15PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Oct 13, 2009 at 10:31:33PM +0900, Isaku Yamahata wrote:
> > > On Sun, Oct 11, 2009 at 12:43:12PM +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 09, 2009 at 03:28:48PM +0900, Isaku Yamahata wrote:
> > > > > This patch is preliminary for 64bit bar.
> > > > > For 64bit bar support, change pcibus_t which represents
> > > > > pci bus addr/size from uint32_t to uint64_t.
> > > > > And also change FMT_pcibus for printf.
> > > > > 
> > > > > In pci_update_mapping() checks 32bit overflow.
> > > > > So the check must be updated too.
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <address@hidden>
> > > > 
> > > > That's all fine, but if you look at users implementing
> > > > map io, they do: cpu_register_physical_memory()
> > > > on the address they are given.  And if target_phys_addr_t is 32 bit,
> > > > this will silently truncate the address.
> > > > 
> > > > So I would like to understand how this will all
> > > > work on 32 bit systems.
> > > 
> > > The case is
> > >   . BAR is memory 64bit and
> > >   . target_phys_addr_t is 32bit and
> > >   . bar is set to >4G.
> > > Hmm, the case isn't checked.
> > > 
> > > It would be checked by
> > > - last_addr <= new_addr
> > > + (target_phys_addr_t)last_addr <= new_addr
> > 
> > That's pretty tricky. Can we just convert everything into
> > 64 bit unconditionally and just do simple range checks?
> > 
> > > 
> > > I'll fix it with comments added. Nice catch.
> > 
> > Is this the right thing to do though?
> > I think 32 bit CPU might address something like 64G
> > memory of highmem support, so a 64 bit value might
> > actually be valid.
> > 
> > Let's step back and understand what the motivation is?
> > Maybe declaring all bars as 32 bit for 32 bit targets is enough?
> 
> It is independent of guest OS for PCI device to have 64 bit BAR.
> It is valid to use PCI card with 64bit bar on 32 bit OS.

Yes, but qemu does not support this yet.

> In that case
> the OS will set the 64 bit bar within addressable region.
> And it is allowed for 32 bit OS to set 64bit BAR to >4GB.
> (which doesn't make sense, though.)

Yes, it does. E.g. with high memory, a 32 bit OS can address
more than 4G RAM.

> 
> How about adding the following check?
> last_addr >= TARGET_PHYS_ADDR_MAX

And then what?

> -- 
> yamahata