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 <yamah...@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.
Heh, for that matter, BARs can overlap anyway.
0 just makes it more likely.
> As for last_addr == PCI_BAR_UNMAPPED, it avoid to map
> around 4GB as discussed before. I observed that guest doesn't boot
> without the check. However I didn't track it down further.
> Now it's 64bit though.
Hmm. Would be nice to figure all this out.
> > What would it take to fix these?
> >
> > > - last_addr == PCI_BAR_UNMAPPED ||
> > > -
> > > - /* Now pcibus_t is 64bit.
> > > - * Check if 32 bit BAR wrap around explicitly.
> > > - * Without this, PC ide doesn't work well.
> > > - * TODO: remove this work around.
> > > - */
> > > - (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > > - last_addr >= UINT32_MAX) ||
> > > -
> > > - /*
> > > - * OS is allowed to set BAR beyond its addressable
> > > - * bits. For example, 32 bit OS can set 64bit bar
> > > - * to >4G. Check it.
> > > - */
> > > - last_addr >= TARGET_PHYS_ADDR_MAX) {
> > > + if (i == PCI_ROM_SLOT &&
> > > + !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> > > new_addr = PCI_BAR_UNMAPPED;
> > > + } else {
> > > + 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 ||
> > > + last_addr == PCI_BAR_UNMAPPED ||
> > > +
> > > + /* Now pcibus_t is 64bit.
> > > + * Check if 32 bit BAR wrap around explicitly.
> > > + * Without this, PC ide doesn't work well.
> > > + * TODO: remove this work around.
> > > + */
> > > + (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > > + last_addr >= UINT32_MAX) ||
> > > +
> > > + /*
> > > + * OS is allowed to set BAR beyond its
> > > addressable
> > > + * bits. For example, 32 bit OS can set 64bit bar
> > > + * to >4G. Check it.
> > > + */
> > > + last_addr >= TARGET_PHYS_ADDR_MAX) {
> > > + new_addr = PCI_BAR_UNMAPPED;
> > > + }
> > > }
> > > } else {
> > > - no_mem_map:
> > > new_addr = PCI_BAR_UNMAPPED;
> > > }
> > > }
> >
> > Nesting is too deep in pci_update_mappings already.
> > And this makes it worse. I split out the math into
> > a subfunction, and it looks better IMO:
> >
> > --->
> >
> > From: Michael S. Tsirkin <m...@redhat.com>
> > Subject: pci: split up up pci_update mappings
> >
> > Split bar address math into a separate function.
> > In particular, this gets rid of an ugly forward goto
> > into scope that we have there.
> >
> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
>
> Makes sense. Much more readable.
> Acked-by: Isaku Yamahata <yamah...@valinux.co.jp>
>
>
> >
> > ---
> >
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 380d43c..847d31e 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -720,14 +720,77 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t
> > *addr, pcibus_t *size,
> > }
> > }
> >
> > +static pcibus_t pci_bar_address(PCIDevice *d,
> > + int reg, uint8_t type, pcibus_t size)
> > +{
> > + pcibus_t new_addr, last_addr;
> > + int bar = pci_bar(d, reg);
> > + uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> > +
> > + if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> > + if (!(cmd & PCI_COMMAND_IO)) {
> > + return PCI_BAR_UNMAPPED;
> > + }
> > + new_addr = pci_get_long(d->config + bar) & ~(size - 1);
> > + last_addr = new_addr + size - 1;
> > + /* NOTE: we have only 64K ioports on PC */
> > + if (last_addr <= new_addr || new_addr == 0 || last_addr >
> > UINT16_MAX) {
> > + return PCI_BAR_UNMAPPED;
> > + }
> > + return new_addr;
> > + }
> > +
> > + if (!(cmd & PCI_COMMAND_MEMORY)) {
> > + return PCI_BAR_UNMAPPED;
> > + }
> > + if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > + new_addr = pci_get_quad(d->config + bar);
> > + } else {
> > + new_addr = pci_get_long(d->config + bar);
> > + }
> > + /* the ROM slot has a specific enable bit */
> > + if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> > + return PCI_BAR_UNMAPPED;
> > + }
> > + new_addr &= ~(size - 1);
> > + last_addr = new_addr + 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 ||
> > + last_addr == PCI_BAR_UNMAPPED) {
> > + return PCI_BAR_UNMAPPED;
> > + }
> > +
> > + /* Now pcibus_t is 64bit.
> > + * Check if 32 bit BAR wraps around explicitly.
> > + * Without this, PC ide doesn't work well.
> > + * TODO: remove this work around.
> > + */
> > + if (!(type & PCI_BASE_ADDRESS_MEM_TYPE_64) && last_addr >=
> > UINT32_MAX) {
> > + return PCI_BAR_UNMAPPED;
> > + }
> > +
> > + /*
> > + * OS is allowed to set BAR beyond its addressable
> > + * bits. For example, 32 bit OS can set 64bit bar
> > + * to >4G. Check it. TODO: we might need to support
> > + * it in the future for e.g. PAE.
> > + */
> > + if (last_addr >= TARGET_PHYS_ADDR_MAX) {
> > + return PCI_BAR_UNMAPPED;
> > + }
> > +
> > + return new_addr;
> > +}
> > +
> > static void pci_update_mappings(PCIDevice *d)
> > {
> > PCIIORegion *r;
> > - int cmd, i;
> > - pcibus_t last_addr, new_addr;
> > - pcibus_t filtered_size;
> > + int i;
> > + pcibus_t new_addr, filtered_size;
> >
> > - cmd = pci_get_word(d->config + PCI_COMMAND);
> > for(i = 0; i < PCI_NUM_REGIONS; i++) {
> > r = &d->io_regions[i];
> >
> > @@ -735,59 +798,7 @@ static void pci_update_mappings(PCIDevice *d)
> > if (!r->size)
> > continue;
> >
> > - if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> > - if (cmd & PCI_COMMAND_IO) {
> > - new_addr = pci_get_long(d->config + pci_bar(d, i));
> > - new_addr = new_addr & ~(r->size - 1);
> > - last_addr = new_addr + r->size - 1;
> > - /* NOTE: we have only 64K ioports on PC */
> > - if (last_addr <= new_addr || new_addr == 0 ||
> > - last_addr >= 0x10000) {
> > - new_addr = PCI_BAR_UNMAPPED;
> > - }
> > - } else {
> > - new_addr = PCI_BAR_UNMAPPED;
> > - }
> > - } else {
> > - if (cmd & PCI_COMMAND_MEMORY) {
> > - if (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > - new_addr = pci_get_quad(d->config + pci_bar(d, i));
> > - } else {
> > - 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 ||
> > - last_addr == PCI_BAR_UNMAPPED ||
> > -
> > - /* Now pcibus_t is 64bit.
> > - * Check if 32 bit BAR wrap around explicitly.
> > - * Without this, PC ide doesn't work well.
> > - * TODO: remove this work around.
> > - */
> > - (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > - last_addr >= UINT32_MAX) ||
> > -
> > - /*
> > - * OS is allowed to set BAR beyond its addressable
> > - * bits. For example, 32 bit OS can set 64bit bar
> > - * to >4G. Check it.
> > - */
> > - last_addr >= TARGET_PHYS_ADDR_MAX) {
> > - new_addr = PCI_BAR_UNMAPPED;
> > - }
> > - } else {
> > - no_mem_map:
> > - new_addr = PCI_BAR_UNMAPPED;
> > - }
> > - }
> > + new_addr = pci_bar_address(d, i, r->type, r->size);
> >
> > /* bridge filtering */
> > filtered_size = r->size;
> >
>
> --
> yamahata
