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