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

[Isaku Yamahata]

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. 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.)

How about adding the following check?
last_addr >= TARGET_PHYS_ADDR_MAX

-- 
yamahata