[Qemu-devel] Re: [PATCH V5 10/29] pci: make pci_bar() aware of header type 1.

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

On Fri, Oct 09, 2009 at 03:28:43PM +0900, Isaku Yamahata wrote:
> make pci_bar() aware of header type 1. When PCI_ROM_SLOT
> it should return PCI_ROM_ADDRESS1 (!= PCI_ROM_ADDRESS)
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Some minor comments below.
Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |   18 ++++++++++++------
>  hw/pci.h |    3 +++
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 755ebad..82c1c3c 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -84,9 +84,15 @@ static const VMStateDescription vmstate_pcibus = {
>      }
>  };
>  
> -static inline int pci_bar(int reg)
> +static int pci_bar(PCIDevice *d, int reg)
>  {
> -    return reg == PCI_ROM_SLOT ? PCI_ROM_ADDRESS : PCI_BASE_ADDRESS_0 + reg * 4;
> +    if (reg == PCI_ROM_SLOT) {
> +        return (d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION)
> +            == PCI_HEADER_TYPE_BRIDGE ?
> +            PCI_ROM_ADDRESS1 : PCI_ROM_ADDRESS;
> +    }
> +
> +    return PCI_BASE_ADDRESS_0 + reg * 4;
>  }

Nit: it would be nice to write it in a way that does not lead to such
wrapping. How about:

static int pci_bar(PCIDevice *d, int reg)
{
    uint8_t type;
    if (reg != PCI_ROM_SLOT)
	    return PCI_BASE_ADDRESS_0 + reg * 4;

    type = d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
    return type == PCI_HEADER_TYPE_BRIDGE ?  PCI_ROM_ADDRESS1 : PCI_ROM_ADDRESS;
}

Neater, isn't it?

>  
>  static void pci_device_reset(PCIDevice *dev)
> @@ -102,7 +108,7 @@ static void pci_device_reset(PCIDevice *dev)
>          if (!dev->io_regions[r].size) {
>              continue;
>          }
> -        pci_set_long(dev->config + pci_bar(r), dev->io_regions[r].type);
> +        pci_set_long(dev->config + pci_bar(dev, r), dev->io_regions[r].type);
>      }
>      pci_update_mappings(dev);
>  }
> @@ -472,7 +478,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      r->map_func = map_func;
>  
>      wmask = ~(size - 1);
> -    addr = pci_bar(region_num);
> +    addr = pci_bar(pci_dev, region_num);
>      if (region_num == PCI_ROM_SLOT) {
>          /* ROM enable bit is writeable */
>          wmask |= PCI_ROM_ADDRESS_ENABLE;
> @@ -494,7 +500,7 @@ static void pci_update_mappings(PCIDevice *d)
>          if (r->size != 0) {
>              if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
>                  if (cmd & PCI_COMMAND_IO) {
> -                    new_addr = pci_get_long(d->config + pci_bar(i));
> +                    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 */
> @@ -507,7 +513,7 @@ static void pci_update_mappings(PCIDevice *d)
>                  }
>              } else {
>                  if (cmd & PCI_COMMAND_MEMORY) {
> -                    new_addr = pci_get_long(d->config + pci_bar(i));
> +                    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;
> diff --git a/hw/pci.h b/hw/pci.h
> index bbce56b..bd63534 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -153,6 +153,9 @@ typedef struct PCIIORegion {
>  
>  #define PCI_COMMAND_RESERVED_MASK_HI (PCI_COMMAND_RESERVED >> 8)
>  
> +/* Header type 1 (PCI-to-PCI bridges) */
> +#define PCI_ROM_ADDRESS1        0x38    /* Same as PCI_ROM_ADDRESS, but for htype 1 */
> +

PCI_COMMAND_RESERVED_MASK_HI is our own macro, PCI_ROM_ADDRESS1 comes
from pci_regs.h, it should come in the same block with macros copied
from there. Also please try to keep macros there in the same order, so
one can diff to easily fine errors.

>  /* Size of the standard PCI config header */
>  #define PCI_CONFIG_HEADER_SIZE 0x40
>  /* Size of the standard PCI config space */
> -- 
> 1.6.0.2