[5/6] pci: introduce PCIAddress, PCIConfigAddress and helper functions.

Introduce PCIAddress, PCIConfigAddress and helper functions.
They will be used later to clean up pci_data_{read, write}().

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.h      |    7 +++++++
 hw/pci_host.c |   32 ++++++++++++++++++++++++++++++++
 hw/pci_host.h |   16 ++++++++++++++++
 qemu-common.h |    2 ++
 4 files changed, 57 insertions(+), 0 deletions(-)

On Tue, Jan 12, 2010 at 05:52:57PM +0900, Isaku Yamahata wrote:
> Introduce PCIAddress, PCIConfigAddress and helper functions.
> They will be used later to clean up pci_data_{read, write}().
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.h      |    7 +++++++
>  hw/pci_host.c |   32 ++++++++++++++++++++++++++++++++
>  hw/pci_host.h |   16 ++++++++++++++++
>  qemu-common.h |    2 ++
>  4 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index ed048f5..eb87762 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -10,6 +10,13 @@
>  
>  /* PCI bus */
>  
> +struct PCIAddress {
> +    PCIBus *domain;
> +    uint8_t bus;
> +    uint8_t slot;
> +    uint8_t fn;
> +};
> +

This API looks good except probably should be in pci_host.h
and called PCIConfigAddress.

>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>  #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)         ((devfn) & 0x07)
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 307f7d4..fa194e2 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -39,6 +39,38 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>   * bit  0 -  7: offset in configuration space of a given pci device
>   */
>  
> +static void pci_host_decode_config_addr(const PCIHostState *s,
> +                                        uint32_t config_reg,
> +                                        PCIConfigAddress *decoded)
> +{
> +    uint32_t devfn;
> +
> +    decoded->addr.domain = s->bus;
> +    decoded->addr.bus = (config_reg >> 16) & 0xff;
> +    devfn = (config_reg >> 8) & 0xff;
> +    decoded->addr.slot = PCI_SLOT(devfn);
> +    decoded->addr.fn = PCI_FUNC(devfn);
> +    decoded->offset = config_reg & (PCI_CONFIG_SPACE_SIZE - 1);
> +    decoded->addr_mask = 3;
> +}
> +
> +#define PCI_HOST_CFGE   (1u << 31)      /* configuration enable */
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> +                                      uint32_t config_reg,
> +                                      PCIConfigAddress *decoded)
> +{
> +    pci_host_decode_config_addr(s, config_reg, decoded);
> +    decoded->valid = (config_reg & PCI_HOST_CFGE) ? true : false;
> +}
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> +                                       uint32_t config_reg,
> +                                       PCIConfigAddress *decoded)
> +{
> +    pci_host_decode_config_addr(s, config_reg, decoded);
> +    decoded->valid = true;
> +}
> +
>  /* the helper functio to get a PCIDeice* for a given pci address */
>  static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
>  {
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index a006687..ebc95f2 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -30,12 +30,28 @@
>  
>  #include "sysbus.h"
>  
> +/* for config space access */
> +struct PCIConfigAddress {
> +    PCIAddress addr;
> +    uint32_t addr_mask;
> +    uint16_t offset;
> +    bool valid;
> +};
> +

This one seems to add ad-hock stuff that can be
calculated by pci_data_read/write just as well.


>  struct PCIHostState {
>      SysBusDevice busdev;
>      uint32_t config_reg;
>      PCIBus *bus;
>  };
>  
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> +                                      uint32_t config_reg,
> +                                      PCIConfigAddress *decoded);
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> +                                       uint32_t config_reg,
> +                                       PCIConfigAddress *decoded);
> +

So if you rename PCIAddress to PCIConfigAddress and get rid of
the extra fields, you will end up with a single
pci_host_get_config_addr, which is cleaner.

>  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
>  
> diff --git a/qemu-common.h b/qemu-common.h
> index 8630f8c..14e9205 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -207,6 +207,8 @@ typedef struct SMBusDevice SMBusDevice;
>  typedef struct QEMUTimer QEMUTimer;
>  typedef struct PCIHostState PCIHostState;
>  typedef struct PCIExpressHost PCIExpressHost;
> +typedef struct PCIAddress PCIAddress;
> +typedef struct PCIConfigAddress PCIConfigAddress;
>  typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
>  typedef struct SerialState SerialState;
> -- 
> 1.6.5.4

On Tue, Jan 12, 2010 at 05:52:57PM +0900, Isaku Yamahata wrote:
> Introduce PCIAddress, PCIConfigAddress and helper functions.
> They will be used later to clean up pci_data_{read, write}().
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.h      |    7 +++++++
>  hw/pci_host.c |   32 ++++++++++++++++++++++++++++++++
>  hw/pci_host.h |   16 ++++++++++++++++
>  qemu-common.h |    2 ++
>  4 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index ed048f5..eb87762 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -10,6 +10,13 @@
>  
>  /* PCI bus */
>  
> +struct PCIAddress {
> +    PCIBus *domain;
> +    uint8_t bus;
> +    uint8_t slot;
> +    uint8_t fn;
> +};
> +
>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>  #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)         ((devfn) & 0x07)
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 307f7d4..fa194e2 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -39,6 +39,38 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>   * bit  0 -  7: offset in configuration space of a given pci device
>   */
>  
> +static void pci_host_decode_config_addr(const PCIHostState *s,
> +                                        uint32_t config_reg,
> +                                        PCIConfigAddress *decoded)
> +{
> +    uint32_t devfn;
> +
> +    decoded->addr.domain = s->bus;

Hang on, this is wrong. Domain is the root complex, if you need the bus
pointer just pass it to pci_data_read directly.

> +    decoded->addr.bus = (config_reg >> 16) & 0xff;
> +    devfn = (config_reg >> 8) & 0xff;
> +    decoded->addr.slot = PCI_SLOT(devfn);
> +    decoded->addr.fn = PCI_FUNC(devfn);
> +    decoded->offset = config_reg & (PCI_CONFIG_SPACE_SIZE - 1);
> +    decoded->addr_mask = 3;
> +}
> +
> +#define PCI_HOST_CFGE   (1u << 31)      /* configuration enable */
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> +                                      uint32_t config_reg,
> +                                      PCIConfigAddress *decoded)
> +{
> +    pci_host_decode_config_addr(s, config_reg, decoded);
> +    decoded->valid = (config_reg & PCI_HOST_CFGE) ? true : false;
> +}
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> +                                       uint32_t config_reg,
> +                                       PCIConfigAddress *decoded)
> +{
> +    pci_host_decode_config_addr(s, config_reg, decoded);
> +    decoded->valid = true;
> +}
> +
>  /* the helper functio to get a PCIDeice* for a given pci address */
>  static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
>  {
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index a006687..ebc95f2 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -30,12 +30,28 @@
>  
>  #include "sysbus.h"
>  
> +/* for config space access */
> +struct PCIConfigAddress {
> +    PCIAddress addr;
> +    uint32_t addr_mask;
> +    uint16_t offset;
> +    bool valid;
> +};
> +
>  struct PCIHostState {
>      SysBusDevice busdev;
>      uint32_t config_reg;
>      PCIBus *bus;
>  };
>  
> +void pci_host_decode_config_addr_cfge(const PCIHostState *s,
> +                                      uint32_t config_reg,
> +                                      PCIConfigAddress *decoded);
> +
> +void pci_host_decode_config_addr_valid(const PCIHostState *s,
> +                                       uint32_t config_reg,
> +                                       PCIConfigAddress *decoded);
> +
>  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
>  
> diff --git a/qemu-common.h b/qemu-common.h
> index 8630f8c..14e9205 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -207,6 +207,8 @@ typedef struct SMBusDevice SMBusDevice;
>  typedef struct QEMUTimer QEMUTimer;
>  typedef struct PCIHostState PCIHostState;
>  typedef struct PCIExpressHost PCIExpressHost;
> +typedef struct PCIAddress PCIAddress;
> +typedef struct PCIConfigAddress PCIConfigAddress;
>  typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
>  typedef struct SerialState SerialState;
> -- 
> 1.6.5.4