Re: [Qemu-devel] [PATCH 09/13] pci: Introduce and apply PCIDeviceAddress

[Michael S. Tsirkin]

On Mon, Jun 04, 2012 at 10:52:17AM +0200, Jan Kiszka wrote:
> This type encapsulates everything from domain to function. Use it first
> to simplify the pci_parse_devaddr interface.
> 
> Signed-off-by: Jan Kiszka <address@hidden>

OK.
Except this really is a legacy interface.
So please call it PCILegacyDeviceAddress
or something like that.

> ---
>  hw/pci-hotplug.c |   29 ++++++++++++++---------------
>  hw/pci.c         |   31 ++++++++++++++-----------------
>  hw/pci.h         |    6 ++++--
>  qemu-common.h    |    7 +++++++
>  4 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index e1654dc..aff4d85 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -34,14 +34,14 @@
>  #include "blockdev.h"
>  #include "error.h"
>  
> -static int read_pci_devaddr(Monitor *mon, const char *addr, unsigned int 
> *domp,
> -                            unsigned int *busp, unsigned *slotp)
> +static int read_pci_devaddr(Monitor *mon, const char *addrstr,
> +                            PCIDeviceAddress *addr)
>  {
>      /* strip legacy tag */
> -    if (!strncmp(addr, "pci_addr=", 9)) {
> -        addr += 9;
> +    if (!strncmp(addrstr, "pci_addr=", 9)) {
> +        addrstr += 9;
>      }
> -    if (pci_parse_devaddr(addr, domp, busp, slotp, NULL)) {
> +    if (pci_parse_devaddr(addrstr, addr, 0)) {
>          monitor_printf(mon, "Invalid pci address\n");
>          return -1;
>      }
> @@ -122,18 +122,17 @@ static int scsi_hot_add(Monitor *mon, DeviceState 
> *adapter,
>  int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
>                        DriveInfo *dinfo, int type)
>  {
> -    unsigned int dom, pci_bus;
> -    unsigned slot;
> +    PCIDeviceAddress addr;
>      PCIDevice *dev;
>      const char *pci_addr = qdict_get_str(qdict, "pci_addr");
>  
>      switch (type) {
>      case IF_SCSI:
> -        if (read_pci_devaddr(mon, pci_addr, &dom, &pci_bus, &slot)) {
> +        if (read_pci_devaddr(mon, pci_addr, &addr)) {
>              goto err;
>          }
> -        dev = pci_find_device(pci_find_root_bus(dom), pci_bus,
> -                              PCI_DEVFN(slot, 0));
> +        dev = pci_find_device(pci_find_root_bus(addr.domain), addr.bus,
> +                              PCI_DEVFN(addr.slot, 0));
>          if (!dev) {
>              monitor_printf(mon, "no pci device with address %s\n", pci_addr);
>              goto err;
> @@ -270,18 +269,18 @@ void pci_device_hot_add(Monitor *mon, const QDict 
> *qdict)
>  
>  static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
>  {
> +    PCIDeviceAddress addr;
>      PCIDevice *d;
> -    unsigned int dom, bus;
> -    unsigned slot;
>      Error *local_err = NULL;
>  
> -    if (read_pci_devaddr(mon, pci_addr, &dom, &bus, &slot)) {
> +    if (read_pci_devaddr(mon, pci_addr, &addr)) {
>          return -1;
>      }
>  
> -    d = pci_find_device(pci_find_root_bus(dom), bus, PCI_DEVFN(slot, 0));
> +    d = pci_find_device(pci_find_root_bus(addr.domain), addr.bus,
> +                        PCI_DEVFN(addr.slot, 0));
>      if (!d) {
> -        monitor_printf(mon, "slot %d empty\n", slot);
> +        monitor_printf(mon, "slot %d empty\n", addr.slot);
>          return -1;
>      }
>  
> diff --git a/hw/pci.c b/hw/pci.c
> index 6471a68..4d700a9 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -509,20 +509,20 @@ static void pci_set_default_subsystem_id(PCIDevice 
> *pci_dev)
>  }
>  
>  /*
> - * Parse [[<domain>:]<bus>:]<slot>, return -1 on error if funcp == NULL
> + * Parse [[<domain>:]<bus>:]<slot>, return -1 on error if 
> !PCI_DEVADDR_WITH_FUNC
>   *       [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
>   */
> -int pci_parse_devaddr(const char *addr, unsigned int *domp, unsigned int 
> *busp,
> -                      unsigned int *slotp, unsigned int *funcp)
> +int pci_parse_devaddr(const char *addrstr, PCIDeviceAddress *addr,
> +                      unsigned int flags)
>  {
>      const char *p;
>      char *e;
>      unsigned long val;
>      unsigned long dom = 0, bus = 0;
> -    unsigned int slot = 0;
> +    unsigned int slot;
>      unsigned int func = 0;
>  
> -    p = addr;
> +    p = addrstr;
>      val = strtoul(p, &e, 16);
>      if (e == p)
>       return -1;
> @@ -544,7 +544,7 @@ int pci_parse_devaddr(const char *addr, unsigned int 
> *domp, unsigned int *busp,
>  
>      slot = val;
>  
> -    if (funcp != NULL) {
> +    if (flags & PCI_DEVADDR_WITH_FUNC) {
>          if (*e != '.')
>              return -1;
>  
> @@ -556,37 +556,34 @@ int pci_parse_devaddr(const char *addr, unsigned int 
> *domp, unsigned int *busp,
>          func = val;
>      }
>  
> -    /* if funcp == NULL func is 0 */
>      if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
>       return -1;
>  
>      if (*e)
>       return -1;
>  
> -    *domp = dom;
> -    *busp = bus;
> -    *slotp = slot;
> -    if (funcp != NULL)
> -        *funcp = func;
> +    addr->domain = dom;
> +    addr->bus = bus;
> +    addr->slot = slot;
> +    addr->function = func;
>      return 0;
>  }
>  
>  PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr)
>  {
> -    unsigned int dom, bus;
> -    unsigned slot;
> +    PCIDeviceAddress addr;
>  
>      if (!devaddr) {
>          *devfnp = -1;
>          return pci_find_bus_nr(pci_find_root_bus(0), 0);
>      }
>  
> -    if (pci_parse_devaddr(devaddr, &dom, &bus, &slot, NULL) < 0) {
> +    if (pci_parse_devaddr(devaddr, &addr, 0) < 0) {
>          return NULL;
>      }
>  
> -    *devfnp = PCI_DEVFN(slot, 0);
> -    return pci_find_bus_nr(pci_find_root_bus(dom), bus);
> +    *devfnp = PCI_DEVFN(addr.slot, 0);
> +    return pci_find_bus_nr(pci_find_root_bus(addr.domain), addr.bus);
>  }
>  
>  static void pci_init_cmask(PCIDevice *dev)
> diff --git a/hw/pci.h b/hw/pci.h
> index 552a586..6c48ffa 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -340,8 +340,10 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, 
> uint8_t devfn);
>  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>  PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr);
>  
> -int pci_parse_devaddr(const char *addr, unsigned int *domp, unsigned int 
> *busp,
> -                      unsigned int *slotp, unsigned int *funcp);
> +#define PCI_DEVADDR_WITH_FUNC           2
> +
> +int pci_parse_devaddr(const char *addrstr, PCIDeviceAddress *addr,
> +                      unsigned int flags);
>  
>  void pci_device_deassert_intx(PCIDevice *dev);
>  
> diff --git a/qemu-common.h b/qemu-common.h
> index 91e0562..32a35bc 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -274,6 +274,13 @@ typedef enum LostTickPolicy {
>      LOST_TICK_MAX
>  } LostTickPolicy;
>  
> +typedef struct PCIDeviceAddress {
> +    unsigned int domain;
> +    unsigned int bus;
> +    unsigned int slot;
> +    unsigned int function;
> +} PCIDeviceAddress;
> +
>  void tcg_exec_init(unsigned long tb_size);
>  bool tcg_enabled(void);
>  
> -- 
> 1.7.3.4