[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 12, 2009 at 10:12:07PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 12, 2009 at 02:06:22PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 12, 2009 at 02:58:44PM +0900, Isaku Yamahata wrote:
> > > This patch kills nasty goto in pci_update_mappings().
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
> > >  1 files changed, 28 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index cae3d53..2eff7fe 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
> > >                      new_addr = pci_get_long(d->config + pci_bar(d, i));
> > >                  }
> > >                  /* the ROM slot has a specific enable bit */
> > > -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> > > -                    goto no_mem_map;
> > > -                new_addr = new_addr & ~(r->size - 1);
> > > -                last_addr = new_addr + r->size - 1;
> > > -                /* NOTE: we do not support wrapping */
> > > -                /* XXX: as we cannot support really dynamic
> > > -                   mappings, we handle specific values as invalid
> > > -                   mappings. */
> > > -                if (last_addr <= new_addr || new_addr == 0 ||
> > 
> > By the way, any idea why do we need new_addr == 0
> > and last_addr == PCI_BAR_UNMAPPED?
> 
> As for new_addr == 0, since default BAR value is zero, plural BARs can
> overlap with each other. I think qemu can't handle BAR overlap anyway.
> 
> As for last_addr == PCI_BAR_UNMAPPED, it avoid to map
> around 4GB as discussed before.

So it should be ~0x0ull and not PCI_BAR_UNMAPPED,
PCI_BAR_UNMAPPED could be any value e.g. 0x1 would do
as well.

> I observed that guest doesn't boot
> without the check. However I didn't track it down further.
> Now it's 64bit though.

I really think we need to move mapping regions out of devices
and into pci.c. Then we can finally support overlapping BARs
(by being careful not to map common regions).


-- 
MST