[V6,13/32] pci_host: consolidate pci config address access.

consolidate pci_config address access into pci_host.c

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/apb_pci.c     |   43 +---------------------
 hw/grackle_pci.c |   45 +---------------------
 hw/pci_host.c    |  108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci_host.h    |    3 +
 hw/piix_pci.c    |   15 +-------
 hw/ppce500_pci.c |   34 +----------------
 hw/prep_pci.c    |   15 +-------
 hw/unin_pci.c    |   81 ++--------------------------------------
 8 files changed, 121 insertions(+), 223 deletions(-)

On Fri, Oct 30, 2009 at 09:21:07PM +0900, Isaku Yamahata wrote:
> consolidate pci_config address access into pci_host.c
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/apb_pci.c     |   43 +---------------------
>  hw/grackle_pci.c |   45 +---------------------
>  hw/pci_host.c    |  108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci_host.h    |    3 +
>  hw/piix_pci.c    |   15 +-------
>  hw/ppce500_pci.c |   34 +----------------
>  hw/prep_pci.c    |   15 +-------
>  hw/unin_pci.c    |   81 ++--------------------------------------
>  8 files changed, 121 insertions(+), 223 deletions(-)
> 
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 560617a..3999879 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -54,46 +54,6 @@ typedef struct APBState {
>      PCIHostState host_state;
>  } APBState;
>  
> -static void pci_apb_config_writel (void *opaque, target_phys_addr_t addr,
> -                                         uint32_t val)
> -{
> -    APBState *s = opaque;
> -
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    APB_DPRINTF("config_writel addr " TARGET_FMT_plx " val %x\n", addr,
> -                val);
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_apb_config_readl (void *opaque,
> -                                            target_phys_addr_t addr)
> -{
> -    APBState *s = opaque;
> -    uint32_t val;
> -
> -    val = s->host_state.config_reg;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    APB_DPRINTF("config_readl addr " TARGET_FMT_plx " val %x\n", addr,
> -                val);
> -    return val;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_apb_config_write[] = {
> -    &pci_apb_config_writel,
> -    &pci_apb_config_writel,
> -    &pci_apb_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_apb_config_read[] = {
> -    &pci_apb_config_readl,
> -    &pci_apb_config_readl,
> -    &pci_apb_config_readl,
> -};
> -
>  static void apb_config_writel (void *opaque, target_phys_addr_t addr,
>                                 uint32_t val)
>  {
> @@ -275,8 +235,7 @@ static int pci_pbm_init_device(SysBusDevice *dev)
>                                            pci_apb_iowrite, s);
>      sysbus_init_mmio(dev, 0x10000ULL, pci_ioport);
>      /* mem_config  */
> -    pci_mem_config = cpu_register_io_memory(pci_apb_config_read,
> -                                            pci_apb_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x10ULL, pci_mem_config);
>      /* mem_data */
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 8407cd2..58dcd11 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -43,45 +43,6 @@ typedef struct GrackleState {
>      PCIHostState host_state;
>  } GrackleState;
>  
> -static void pci_grackle_config_writel (void *opaque, target_phys_addr_t addr,
> -                                       uint32_t val)
> -{
> -    GrackleState *s = opaque;
> -
> -    GRACKLE_DPRINTF("config_writel addr " TARGET_FMT_plx " val %x\n", addr,
> -                    val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_grackle_config_readl (void *opaque, target_phys_addr_t addr)
> -{
> -    GrackleState *s = opaque;
> -    uint32_t val;
> -
> -    val = s->host_state.config_reg;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    GRACKLE_DPRINTF("config_readl addr " TARGET_FMT_plx " val %x\n", addr,
> -                    val);
> -    return val;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_grackle_config_write[] = {
> -    &pci_grackle_config_writel,
> -    &pci_grackle_config_writel,
> -    &pci_grackle_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_grackle_config_read[] = {
> -    &pci_grackle_config_readl,
> -    &pci_grackle_config_readl,
> -    &pci_grackle_config_readl,
> -};
> -
>  /* Don't know if this matches real hardware, but it agrees with OHW.  */
>  static int pci_grackle_map_irq(PCIDevice *pci_dev, int irq_num)
>  {
> @@ -147,8 +108,7 @@ static int pci_grackle_init_device(SysBusDevice *dev)
>  
>      s = FROM_SYSBUS(GrackleState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
> -                                            pci_grackle_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> @@ -167,8 +127,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>  
>      s = FROM_SYSBUS(GrackleState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
> -                                            pci_grackle_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 45da1e7..6009e37 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>  #define PCI_DPRINTF(fmt, ...)
>  #endif
>  
> +static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> +                                   uint32_t val)
> +{
> +    PCIHostState *s = opaque;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif

I know you just copied it, but isn't this just
	val = le32_to_cpu(val);

?

> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    s->config_reg = val;
> +}
> +
> +static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    PCIHostState *s = opaque;
> +    uint32_t val = s->config_reg;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif

same here.

> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    return val;
> +}
> +
> +static CPUWriteMemoryFunc * const pci_host_config_write[] = {
> +    &pci_host_config_writel,
> +    &pci_host_config_writel,
> +    &pci_host_config_writel,
> +};
> +
> +static CPUReadMemoryFunc * const pci_host_config_read[] = {
> +    &pci_host_config_readl,
> +    &pci_host_config_readl,
> +    &pci_host_config_readl,
> +};
> +
> +int pci_host_config_register_io_memory(PCIHostState *s)
> +{
> +    return cpu_register_io_memory(pci_host_config_read,
> +                                  pci_host_config_write, s);
> +}
> +
> +static void pci_host_config_writel_noswap(void *opaque,
> +                                          target_phys_addr_t addr,
> +                                          uint32_t val)
> +{
> +    PCIHostState *s = opaque;
> +
> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    s->config_reg = val;
> +}
> +
> +static uint32_t pci_host_config_readl_noswap(void *opaque,
> +                                             target_phys_addr_t addr)
> +{
> +    PCIHostState *s = opaque;
> +    uint32_t val = s->config_reg;
> +
> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    return val;
> +}
> +
> +static CPUWriteMemoryFunc * const pci_host_config_write_noswap[] = {
> +    &pci_host_config_writel_noswap,
> +    &pci_host_config_writel_noswap,
> +    &pci_host_config_writel_noswap,
> +};
> +
> +static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
> +    &pci_host_config_readl_noswap,
> +    &pci_host_config_readl_noswap,
> +    &pci_host_config_readl_noswap,
> +};
> +
> +int pci_host_config_register_io_memory_noswap(PCIHostState *s)

This name is clearly too long,
as a result when you use it you run over the 80 character
limit. Let's not fix it, let's make name shorter.
io_memory -> memory?

> +{
> +    return cpu_register_io_memory(pci_host_config_read_noswap,
> +                                  pci_host_config_write_noswap, s);
> +}

Do you happen to know whether no swap is really needed?  I suspect some
of these places really only happen to work because everyone uses intel,
so they simply would not work on ppc host.

> +
> +static void pci_host_config_writel_ioport(void *opaque,
> +                                          uint32_t addr, uint32_t val)
> +{
> +    PCIHostState *s = opaque;
> +
> +    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, val);
> +    s->config_reg = val;
> +}
> +
> +static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
> +{
> +    PCIHostState *s = opaque;
> +    uint32_t val = s->config_reg;
> +
> +    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr, val);
> +    return val;
> +}
> +
> +void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s)
> +{
> +    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport, s);
> +    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
> +}
> +
>  #define PCI_ADDR_T      target_phys_addr_t
>  #define PCI_HOST_SUFFIX _mmio
>  
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index 92a35f9..e5e877f 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -37,9 +37,12 @@ typedef struct {
>  } PCIHostState;
>  
>  /* for mmio */
> +int pci_host_config_register_io_memory(PCIHostState *s);
> +int pci_host_config_register_io_memory_noswap(PCIHostState *s);
>  int pci_host_data_register_io_memory(PCIHostState *s);
>  
>  /* for ioio */
> +void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s);
>  void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s);
>  
>  #endif /* PCI_HOST_H */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 866348d..bf0005e 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -44,18 +44,6 @@ struct PCII440FXState {
>      PIIX3State *piix3;
>  };
>  
> -static void i440fx_addr_writel(void* opaque, uint32_t addr, uint32_t val)
> -{
> -    I440FXState *s = opaque;
> -    s->config_reg = val;
> -}
> -
> -static uint32_t i440fx_addr_readl(void* opaque, uint32_t addr)
> -{
> -    I440FXState *s = opaque;
> -    return s->config_reg;
> -}
> -
>  static void piix3_set_irq(void *opaque, int irq_num, int level);
>  
>  /* return the global irq number corresponding to a given device irq
> @@ -192,8 +180,7 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
>  {
>      I440FXState *s = FROM_SYSBUS(I440FXState, dev);
>  
> -    register_ioport_write(0xcf8, 4, 4, i440fx_addr_writel, s);
> -    register_ioport_read(0xcf8, 4, 4, i440fx_addr_readl, s);
> +    pci_host_config_register_ioport(0xcf8, s);
>  
>      pci_host_data_register_ioport(0xcfc, s);
>      return 0;
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 7c8cdad..223de3a 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -84,37 +84,6 @@ struct PPCE500PCIState {
>  
>  typedef struct PPCE500PCIState PPCE500PCIState;
>  
> -static uint32_t pcie500_cfgaddr_readl(void *opaque, target_phys_addr_t addr)
> -{
> -    PPCE500PCIState *pci = opaque;
> -
> -    pci_debug("%s: (addr:" TARGET_FMT_plx ") -> value:%x\n", __func__, addr,
> -              pci->pci_state.config_reg);
> -    return pci->pci_state.config_reg;
> -}
> -
> -static CPUReadMemoryFunc * const pcie500_cfgaddr_read[] = {
> -    &pcie500_cfgaddr_readl,
> -    &pcie500_cfgaddr_readl,
> -    &pcie500_cfgaddr_readl,
> -};
> -
> -static void pcie500_cfgaddr_writel(void *opaque, target_phys_addr_t addr,
> -                                  uint32_t value)
> -{
> -    PPCE500PCIState *controller = opaque;
> -
> -    pci_debug("%s: value:%x -> (addr:" TARGET_FMT_plx ")\n", __func__, value,
> -              addr);
> -    controller->pci_state.config_reg = value & ~0x3;
> -}
> -
> -static CPUWriteMemoryFunc * const pcie500_cfgaddr_write[] = {
> -    &pcie500_cfgaddr_writel,
> -    &pcie500_cfgaddr_writel,
> -    &pcie500_cfgaddr_writel,
> -};
> -
>  static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
>  {
>      PPCE500PCIState *pci = opaque;
> @@ -324,8 +293,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>      controller->pci_dev = d;
>  
>      /* CFGADDR */
> -    index = cpu_register_io_memory(pcie500_cfgaddr_read,
> -                                   pcie500_cfgaddr_write, controller);
> +    index = pci_host_config_register_io_memory_noswap(&controller->pci_state);
>      if (index < 0)
>          goto free;
>      cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 5a5b3da..a338f81 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -28,18 +28,6 @@
>  
>  typedef PCIHostState PREPPCIState;
>  
> -static void pci_prep_addr_writel(void* opaque, uint32_t addr, uint32_t val)
> -{
> -    PREPPCIState *s = opaque;
> -    s->config_reg = val;
> -}
> -
> -static uint32_t pci_prep_addr_readl(void* opaque, uint32_t addr)
> -{
> -    PREPPCIState *s = opaque;
> -    return s->config_reg;
> -}
> -
>  static inline uint32_t PPC_PCIIO_config(target_phys_addr_t addr)
>  {
>      int i;
> @@ -139,8 +127,7 @@ PCIBus *pci_prep_init(qemu_irq *pic)
>      s->bus = pci_register_bus(NULL, "pci",
>                                prep_set_irq, prep_map_irq, pic, 0, 4);
>  
> -    register_ioport_write(0xcf8, 4, 4, pci_prep_addr_writel, s);
> -    register_ioport_read(0xcf8, 4, 4, pci_prep_addr_readl, s);
> +    pci_host_config_register_ioport(0xcf8, s);
>  
>      pci_host_data_register_ioport(0xcfc, s);
>  
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 6b8f98b..a9a62fd 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -41,74 +41,6 @@ typedef struct UNINState {
>      PCIHostState host_state;
>  } UNINState;
>  
> -static void pci_unin_main_config_writel (void *opaque, target_phys_addr_t addr,
> -                                         uint32_t val)
> -{
> -    UNINState *s = opaque;
> -
> -    UNIN_DPRINTF("config_writel addr " TARGET_FMT_plx " val %x\n", addr, val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_unin_main_config_readl (void *opaque,
> -                                            target_phys_addr_t addr)
> -{
> -    UNINState *s = opaque;
> -    uint32_t val;
> -
> -    val = s->host_state.config_reg;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    UNIN_DPRINTF("config_readl addr " TARGET_FMT_plx " val %x\n", addr, val);
> -
> -    return val;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_unin_main_config_write[] = {
> -    &pci_unin_main_config_writel,
> -    &pci_unin_main_config_writel,
> -    &pci_unin_main_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_unin_main_config_read[] = {
> -    &pci_unin_main_config_readl,
> -    &pci_unin_main_config_readl,
> -    &pci_unin_main_config_readl,
> -};
> -
> -static void pci_unin_config_writel (void *opaque, target_phys_addr_t addr,
> -                                    uint32_t val)
> -{
> -    UNINState *s = opaque;
> -
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_unin_config_readl (void *opaque,
> -                                       target_phys_addr_t addr)
> -{
> -    UNINState *s = opaque;
> -
> -    return s->host_state.config_reg;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_unin_config_write[] = {
> -    &pci_unin_config_writel,
> -    &pci_unin_config_writel,
> -    &pci_unin_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_unin_config_read[] = {
> -    &pci_unin_config_readl,
> -    &pci_unin_config_readl,
> -    &pci_unin_config_readl,
> -};
> -
>  /* Don't know if this matches real hardware, but it agrees with OHW.  */
>  static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
>  {
> @@ -152,10 +84,8 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>      /* Uninorth main bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_unin_main_config_read,
> -                                            pci_unin_main_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> -
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>  
> @@ -174,8 +104,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>      s = FROM_SYSBUS(UNINState, dev);
>  
>      // XXX: s = &pci_bridge[2];
> -    pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> -                                            pci_unin_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> @@ -190,8 +119,7 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
>      /* Uninorth AGP bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> -                                            pci_unin_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> @@ -206,8 +134,7 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
>      /* Uninorth internal bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> -                                            pci_unin_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> -- 
> 1.6.0.2

On Tue, Nov 03, 2009 at 03:45:12PM +0200, Michael S. Tsirkin wrote:
> > --- a/hw/pci_host.c
> > +++ b/hw/pci_host.c
> > @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> >  #define PCI_DPRINTF(fmt, ...)
> >  #endif
> >  
> > +static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> > +                                   uint32_t val)
> > +{
> > +    PCIHostState *s = opaque;
> > +
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    val = bswap32(val);
> > +#endif
> 
> I know you just copied it, but isn't this just
> 	val = le32_to_cpu(val);
> 
> ?

Makes sense.


> > +static void pci_host_config_writel_noswap(void *opaque,
> > +                                          target_phys_addr_t addr,
> > +                                          uint32_t val)
> > +{
> > +    PCIHostState *s = opaque;
> > +
> > +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> > +                __func__, addr, val);
> > +    s->config_reg = val;
> > +}
> > +
> > +static uint32_t pci_host_config_readl_noswap(void *opaque,
> > +                                             target_phys_addr_t addr)
> > +{
> > +    PCIHostState *s = opaque;
> > +    uint32_t val = s->config_reg;
> > +
> > +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> > +                __func__, addr, val);
> > +    return val;
> > +}
> > +
> > +static CPUWriteMemoryFunc * const pci_host_config_write_noswap[] = {
> > +    &pci_host_config_writel_noswap,
> > +    &pci_host_config_writel_noswap,
> > +    &pci_host_config_writel_noswap,
> > +};
> > +
> > +static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
> > +    &pci_host_config_readl_noswap,
> > +    &pci_host_config_readl_noswap,
> > +    &pci_host_config_readl_noswap,
> > +};
> > +
> > +int pci_host_config_register_io_memory_noswap(PCIHostState *s)
> 
> This name is clearly too long,
> as a result when you use it you run over the 80 character
> limit. Let's not fix it, let's make name shorter.
> io_memory -> memory?
> 
> > +{
> > +    return cpu_register_io_memory(pci_host_config_read_noswap,
> > +                                  pci_host_config_write_noswap, s);
> > +}
> 
> Do you happen to know whether no swap is really needed?  I suspect some
> of these places really only happen to work because everyone uses intel,
> so they simply would not work on ppc host.

I just followed the original code not to break it.
I dug into the commit logs a bit.
Liu, Aurelian and Alexander, could you give me a comment on
whether those functions needs byte swap (le32_to_cpu()) or not.
  - ppcie500_cfgaddr_{write, read}l() in ppce500_pci.c
  - pci_unin_config_{write, read}l() in unin_pci.c

ppce500_pci.c case:
The related commit is 74c62ba88902575be9ac3badf95d773470884b1c.
The comment on the top in the file says that it is derived from ppc4xx_pci.c.
ppc4xx_pci.c has byte swap, on the other hand ppce500_pci.c doesn't.
So I'm inclined to suspect that the byte swap was dropped accidently.
However I don't know its pci host bridge spec.


unin_pci.c case:
I don't know its spec neither.
The following changeset seems to remove the byte swap deliberately.
Alexander, could you please give us comment on it?

commit 783a20dcb51f7197c56f77c8012fa4abe8a23391
Author: blueswir1 <blueswir1@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Sat Mar 7 20:53:18 2009 +0000

    Activate uninorth AGP bridge
    
    Linux tries to poke the AGP bridge port and is pretty sad when it can't,
    so let's activate the old code again and throw out the bit modifications,
    as we don't really do anything with the values anyways.
    
    Signed-off-by: Alexander Graf <alex@csgraf.de>
    
    
    git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@6750 c046a42c-6fe2-441c-8c8c-71466251a162

On 04.11.2009, at 07:14, Isaku Yamahata wrote:

> On Tue, Nov 03, 2009 at 03:45:12PM +0200, Michael S. Tsirkin wrote:
>>> --- a/hw/pci_host.c
>>> +++ b/hw/pci_host.c
>>> @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ##  
>>> __VA_ARGS__); } while (0)
>>> #define PCI_DPRINTF(fmt, ...)
>>> #endif
>>>
>>> +static void pci_host_config_writel(void *opaque,  
>>> target_phys_addr_t addr,
>>> +                                   uint32_t val)
>>> +{
>>> +    PCIHostState *s = opaque;
>>> +
>>> +#ifdef TARGET_WORDS_BIGENDIAN
>>> +    val = bswap32(val);
>>> +#endif
>>
>> I know you just copied it, but isn't this just
>> 	val = le32_to_cpu(val);
>>
>> ?
>
> Makes sense.
>
>
>>> +static void pci_host_config_writel_noswap(void *opaque,
>>> +                                          target_phys_addr_t addr,
>>> +                                          uint32_t val)
>>> +{
>>> +    PCIHostState *s = opaque;
>>> +
>>> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
>>> +                __func__, addr, val);
>>> +    s->config_reg = val;
>>> +}
>>> +
>>> +static uint32_t pci_host_config_readl_noswap(void *opaque,
>>> +                                             target_phys_addr_t  
>>> addr)
>>> +{
>>> +    PCIHostState *s = opaque;
>>> +    uint32_t val = s->config_reg;
>>> +
>>> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
>>> +                __func__, addr, val);
>>> +    return val;
>>> +}
>>> +
>>> +static CPUWriteMemoryFunc * const pci_host_config_write_noswap[]  
>>> = {
>>> +    &pci_host_config_writel_noswap,
>>> +    &pci_host_config_writel_noswap,
>>> +    &pci_host_config_writel_noswap,
>>> +};
>>> +
>>> +static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
>>> +    &pci_host_config_readl_noswap,
>>> +    &pci_host_config_readl_noswap,
>>> +    &pci_host_config_readl_noswap,
>>> +};
>>> +
>>> +int pci_host_config_register_io_memory_noswap(PCIHostState *s)
>>
>> This name is clearly too long,
>> as a result when you use it you run over the 80 character
>> limit. Let's not fix it, let's make name shorter.
>> io_memory -> memory?
>>
>>> +{
>>> +    return cpu_register_io_memory(pci_host_config_read_noswap,
>>> +                                  pci_host_config_write_noswap, s);
>>> +}
>>
>> Do you happen to know whether no swap is really needed?  I suspect  
>> some
>> of these places really only happen to work because everyone uses  
>> intel,
>> so they simply would not work on ppc host.
>
> I just followed the original code not to break it.
> I dug into the commit logs a bit.
> Liu, Aurelian and Alexander, could you give me a comment on
> whether those functions needs byte swap (le32_to_cpu()) or not.
>  - ppcie500_cfgaddr_{write, read}l() in ppce500_pci.c
>  - pci_unin_config_{write, read}l() in unin_pci.c
>
> ppce500_pci.c case:
> The related commit is 74c62ba88902575be9ac3badf95d773470884b1c.
> The comment on the top in the file says that it is derived from  
> ppc4xx_pci.c.
> ppc4xx_pci.c has byte swap, on the other hand ppce500_pci.c doesn't.
> So I'm inclined to suspect that the byte swap was dropped accidently.
> However I don't know its pci host bridge spec.
>
>
> unin_pci.c case:
> I don't know its spec neither.
> The following changeset seems to remove the byte swap deliberately.
> Alexander, could you please give us comment on it?

Phew - I frankly don't remember. Something broke because some bits  
were strangely mangled and the way it's not it worked more than it did  
before :-). The whole Uninorth thing is still utterly broken, so  
please don't take anything from there as example.

I do know that Hollis was working a lot about LE/BE issues on PPC in  
Qemu, so he's probably the best person to CC and ask here.


Alex

On Wed, Nov 04, 2009 at 03:14:26PM +0900, Isaku Yamahata wrote:
> On Tue, Nov 03, 2009 at 03:45:12PM +0200, Michael S. Tsirkin wrote:
> > > --- a/hw/pci_host.c
> > > +++ b/hw/pci_host.c
> > > @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> > >  #define PCI_DPRINTF(fmt, ...)
> > >  #endif
> > >  
> > > +static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> > > +                                   uint32_t val)
> > > +{
> > > +    PCIHostState *s = opaque;
> > > +
> > > +#ifdef TARGET_WORDS_BIGENDIAN
> > > +    val = bswap32(val);
> > > +#endif
> > 
> > I know you just copied it, but isn't this just
> > 	val = le32_to_cpu(val);
> > 
> > ?
> 
> Makes sense.
 
The original code is actually wrong, but le32_to_cpu(val), will break on
big endian hosts.

The fact is that QEMU doesn't emulate byteswap on buses. Hopefully on all
big endian machines we emulate, the PCI bus is always connected backward,
so we can simply do the byteswap depending on TARGET_WORDS_BIGENDIAN.

On Wed, Nov 04, 2009 at 04:17:46PM +0100, Aurelien Jarno wrote:
> On Wed, Nov 04, 2009 at 03:14:26PM +0900, Isaku Yamahata wrote:
> > On Tue, Nov 03, 2009 at 03:45:12PM +0200, Michael S. Tsirkin wrote:
> > > > --- a/hw/pci_host.c
> > > > +++ b/hw/pci_host.c
> > > > @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> > > >  #define PCI_DPRINTF(fmt, ...)
> > > >  #endif
> > > >  
> > > > +static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> > > > +                                   uint32_t val)
> > > > +{
> > > > +    PCIHostState *s = opaque;
> > > > +
> > > > +#ifdef TARGET_WORDS_BIGENDIAN
> > > > +    val = bswap32(val);
> > > > +#endif
> > > 
> > > I know you just copied it, but isn't this just
> > > 	val = le32_to_cpu(val);
> > > 
> > > ?
> > 
> > Makes sense.
>  
> The original code is actually wrong, but le32_to_cpu(val), will break on
> big endian hosts.
> 
> The fact is that QEMU doesn't emulate byteswap on buses. Hopefully on all
> big endian machines we emulate, the PCI bus is always connected backward,
> so we can simply do the byteswap depending on TARGET_WORDS_BIGENDIAN.
> 
> -- 
> Aurelien Jarno	                        GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net

Are you speaking about bit endian hosts with little endian guests?
Ugh ... my head hurts. bswap32 is evil because there's no way to
figure out what is converted to what. big to little? guest to host?

On Wed, Nov 04, 2009 at 05:37:13PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 04, 2009 at 04:17:46PM +0100, Aurelien Jarno wrote:
> > On Wed, Nov 04, 2009 at 03:14:26PM +0900, Isaku Yamahata wrote:
> > > On Tue, Nov 03, 2009 at 03:45:12PM +0200, Michael S. Tsirkin wrote:
> > > > > --- a/hw/pci_host.c
> > > > > +++ b/hw/pci_host.c
> > > > > @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> > > > >  #define PCI_DPRINTF(fmt, ...)
> > > > >  #endif
> > > > >  
> > > > > +static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> > > > > +                                   uint32_t val)
> > > > > +{
> > > > > +    PCIHostState *s = opaque;
> > > > > +
> > > > > +#ifdef TARGET_WORDS_BIGENDIAN
> > > > > +    val = bswap32(val);
> > > > > +#endif
> > > > 
> > > > I know you just copied it, but isn't this just
> > > > 	val = le32_to_cpu(val);
> > > > 
> > > > ?
> > > 
> > > Makes sense.
> >  
> > The original code is actually wrong, but le32_to_cpu(val), will break on
> > big endian hosts.
> > 
> > The fact is that QEMU doesn't emulate byteswap on buses. Hopefully on all
> > big endian machines we emulate, the PCI bus is always connected backward,
> > so we can simply do the byteswap depending on TARGET_WORDS_BIGENDIAN.
> > 
> > -- 
> > Aurelien Jarno	                        GPG: 1024D/F1BCDB73
> > aurelien@aurel32.net                 http://www.aurel32.net
> 
> Are you speaking about bit endian hosts with little endian guests?
> Ugh ... my head hurts. bswap32 is evil because there's no way to
> figure out what is converted to what. big to little? guest to host?
> 

This is not related to the host and guest endianess, but just of the
way the PCI bus is connected to the CPU on all the big endian guest we 
emulate.

On theses machine, a byteswap is needed when transferring a word between
the PCI bus and the CPU.