Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] pseries: Implement automatic PAPR VIO address allocation

[Andreas Färber]

Am 04.04.2012 07:02, schrieb David Gibson:
> PAPR virtual IO (VIO) devices require a unique, but otherwise arbitrary,
> "address" used as a token to the hypercalls which manipulate them.
> 
> Currently the pseries machine code does an ok job of allocating these
> addresses when the legacy -net nic / -serial and so forth options are used
> but will fail to allocate them properly when using -device.
> 
> Specifically, you can use -device if all addresses are explicitly assigned.
> Without explicit assignment, only one VIO device of each type (network,
> console, SCSI) will be assigned properly, any further ones will attempt
> to take the same address leading to a fatal error.
> 
> This patch fixes the situation by adding a proper address allocator to the
> VIO "bus" code.  This is used both by -device and the legacy options and
> default devices.  Addresses can still be explicitly assigned with -device
> options if desired.
> 
> This patch changes the (guest visible) numbering of VIO devices, but since
> their addresses are discovered using the device tree and already differ
> from the numbering found on existing PowerVM systems, this does not break
> compatibility.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/spapr.c       |    7 ++---
>  hw/spapr_llan.c  |    5 +--
>  hw/spapr_vio.c   |   74 
> ++++++++++++++++++++++++++++++++----------------------
>  hw/spapr_vio.h   |   13 ++++-----
>  hw/spapr_vscsi.c |    5 +--
>  hw/spapr_vty.c   |    5 +--
>  6 files changed, 59 insertions(+), 50 deletions(-)

Reviewed-by: Andreas Färber <address@hidden>

Technically this change looks okay but I'd appreciate a second reviewer
as to what side-effects this change in numbering might have, so I'm
leaving this one to Alex.

Andreas

> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index bfaf260..cca20f9 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -631,8 +631,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>  
>      for (i = 0; i < MAX_SERIAL_PORTS; i++) {
>          if (serial_hds[i]) {
> -            spapr_vty_create(spapr->vio_bus, SPAPR_VTY_BASE_ADDRESS + i,
> -                             serial_hds[i]);
> +            spapr_vty_create(spapr->vio_bus, serial_hds[i]);
>          }
>      }
>  
> @@ -650,14 +649,14 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>          }
>  
>          if (strcmp(nd->model, "ibmveth") == 0) {
> -            spapr_vlan_create(spapr->vio_bus, 0x1000 + i, nd);
> +            spapr_vlan_create(spapr->vio_bus, nd);
>          } else {
>              pci_nic_init_nofail(&nd_table[i], nd->model, NULL);
>          }
>      }
>  
>      for (i = 0; i <= drive_get_max_bus(IF_SCSI); i++) {
> -        spapr_vscsi_create(spapr->vio_bus, 0x2000 + i);
> +        spapr_vscsi_create(spapr->vio_bus);
>      }
>  
>      if (rma_size < (MIN_RMA_SLOF << 20)) {
> diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
> index 32dce17..a0020e9 100644
> --- a/hw/spapr_llan.c
> +++ b/hw/spapr_llan.c
> @@ -195,12 +195,11 @@ static int spapr_vlan_init(VIOsPAPRDevice *sdev)
>      return 0;
>  }
>  
> -void spapr_vlan_create(VIOsPAPRBus *bus, uint32_t reg, NICInfo *nd)
> +void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(&bus->bus, "spapr-vlan");
> -    qdev_prop_set_uint32(dev, "reg", reg);
>  
>      qdev_set_nic_properties(dev, nd);
>  
> @@ -473,7 +472,7 @@ static target_ulong h_multicast_ctrl(CPUPPCState *env, 
> sPAPREnvironment *spapr,
>  }
>  
>  static Property spapr_vlan_properties[] = {
> -    DEFINE_SPAPR_PROPERTIES(VIOsPAPRVLANDevice, sdev, 0x1000, 0x10000000),
> +    DEFINE_SPAPR_PROPERTIES(VIOsPAPRVLANDevice, sdev, 0x10000000),
>      DEFINE_NIC_PROPERTIES(VIOsPAPRVLANDevice, nicconf),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
> index 97d029a..1411f84 100644
> --- a/hw/spapr_vio.c
> +++ b/hw/spapr_vio.c
> @@ -620,41 +620,35 @@ static void rtas_quiesce(sPAPREnvironment *spapr, 
> uint32_t token,
>      rtas_st(rets, 0, 0);
>  }
>  
> -static int spapr_vio_check_reg(VIOsPAPRDevice *sdev)
> +static void spapr_vio_busdev_reset(void *opaque)
>  {
> -    VIOsPAPRDevice *other_sdev;
> -    DeviceState *qdev;
> -    VIOsPAPRBus *sbus;
> +    VIOsPAPRDevice *dev = (VIOsPAPRDevice *)opaque;
> +
> +    if (dev->crq.qsize) {
> +        free_crq(dev);
> +    }
> +}
>  
> -    sbus = DO_UPCAST(VIOsPAPRBus, bus, sdev->qdev.parent_bus);
> +static VIOsPAPRDevice *reg_conflict(VIOsPAPRDevice *dev)
> +{
> +    VIOsPAPRBus *bus = DO_UPCAST(VIOsPAPRBus, bus, dev->qdev.parent_bus);
> +    DeviceState *qdev;
> +    VIOsPAPRDevice *other;
>  
>      /*
> -     * Check two device aren't given clashing addresses by the user (or some
> -     * other mechanism). We have to open code this because we have to check
> -     * for matches with devices other than us.
> +     * Check for a device other than the given one which is already
> +     * using the requested address. We have to open code this because
> +     * the given dev might already be in the list.
>       */
> -    QTAILQ_FOREACH(qdev, &sbus->bus.children, sibling) {
> -        other_sdev = DO_UPCAST(VIOsPAPRDevice, qdev, qdev);
> +    QTAILQ_FOREACH(qdev, &bus->bus.children, sibling) {
> +        other = DO_UPCAST(VIOsPAPRDevice, qdev, qdev);
>  
> -        if (other_sdev != sdev && other_sdev->reg == sdev->reg) {
> -            fprintf(stderr, "vio: %s and %s devices conflict at address 
> %#x\n",
> -                    object_get_typename(OBJECT(sdev)),
> -                    object_get_typename(OBJECT(qdev)),
> -                    sdev->reg);
> -            return -EEXIST;
> +        if (other != dev && other->reg == dev->reg) {
> +            return other;
>          }
>      }
>  
> -    return 0;
> -}
> -
> -static void spapr_vio_busdev_reset(void *opaque)
> -{
> -    VIOsPAPRDevice *dev = (VIOsPAPRDevice *)opaque;
> -
> -    if (dev->crq.qsize) {
> -        free_crq(dev);
> -    }
> +    return NULL;    
>  }
>  
>  static int spapr_vio_busdev_init(DeviceState *qdev)
> @@ -662,11 +656,30 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>      VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
>      VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>      char *id;
> -    int ret;
>  
> -    ret = spapr_vio_check_reg(dev);
> -    if (ret) {
> -        return ret;
> +    if (dev->reg != -1) {
> +        /*
> +         * Explicitly assigned address, just verify that no-one else
> +         * is using it.  other mechanism). We have to open code this
> +         * rather than using spapr_vio_find_by_reg() because sdev
> +         * itself is already in the list.
> +         */
> +        VIOsPAPRDevice *other = reg_conflict(dev);
> +
> +        if (other) {
> +            fprintf(stderr, "vio: %s and %s devices conflict at address 
> %#x\n",
> +                    object_get_typename(OBJECT(qdev)),
> +                    object_get_typename(OBJECT(&other->qdev)),
> +                    dev->reg);
> +            return -1;
> +        }
> +    } else {
> +        /* Need to assign an address */
> +        VIOsPAPRBus *bus = DO_UPCAST(VIOsPAPRBus, bus, dev->qdev.parent_bus);
> +
> +        do {
> +            dev->reg = bus->next_reg++;
> +        } while (reg_conflict(dev));
>      }
>  
>      /* Don't overwrite ids assigned on the command line */
> @@ -728,6 +741,7 @@ VIOsPAPRBus *spapr_vio_bus_init(void)
>  
>      qbus = qbus_create(&spapr_vio_bus_info, dev, "spapr-vio");
>      bus = DO_UPCAST(VIOsPAPRBus, bus, qbus);
> +    bus->next_reg = 0x1000;
>  
>      /* hcall-vio */
>      spapr_register_hypercall(H_VIO_SIGNAL, h_vio_signal);
> diff --git a/hw/spapr_vio.h b/hw/spapr_vio.h
> index fd29c5e..420398c 100644
> --- a/hw/spapr_vio.h
> +++ b/hw/spapr_vio.h
> @@ -32,8 +32,6 @@ enum VIOsPAPR_TCEAccess {
>      SPAPR_TCE_RW = 3,
>  };
>  
> -#define SPAPR_VTY_BASE_ADDRESS     0x30000000
> -
>  #define TYPE_VIO_SPAPR_DEVICE "vio-spapr-device"
>  #define VIO_SPAPR_DEVICE(obj) \
>       OBJECT_CHECK(VIOsPAPRDevice, (obj), TYPE_VIO_SPAPR_DEVICE)
> @@ -82,13 +80,14 @@ struct VIOsPAPRDevice {
>      VIOsPAPR_CRQ crq;
>  };
>  
> -#define DEFINE_SPAPR_PROPERTIES(type, field, default_reg, 
> default_dma_window) \
> -        DEFINE_PROP_UINT32("reg", type, field.reg, default_reg), \
> +#define DEFINE_SPAPR_PROPERTIES(type, field, default_dma_window)       \
> +        DEFINE_PROP_UINT32("reg", type, field.reg, -1),                \
>          DEFINE_PROP_UINT32("dma-window", type, field.rtce_window_size, \
>                             default_dma_window)
>  
>  struct VIOsPAPRBus {
>      BusState bus;
> +    uint32_t next_reg;
>      int (*init)(VIOsPAPRDevice *dev);
>      int (*devnode)(VIOsPAPRDevice *dev, void *fdt, int node_off);
>  };
> @@ -119,9 +118,9 @@ int spapr_vio_send_crq(VIOsPAPRDevice *dev, uint8_t *crq);
>  
>  VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg);
>  void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len);
> -void spapr_vty_create(VIOsPAPRBus *bus, uint32_t reg, CharDriverState 
> *chardev);
> -void spapr_vlan_create(VIOsPAPRBus *bus, uint32_t reg, NICInfo *nd);
> -void spapr_vscsi_create(VIOsPAPRBus *bus, uint32_t reg);
> +void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev);
> +void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd);
> +void spapr_vscsi_create(VIOsPAPRBus *bus);
>  
>  VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus);
>  
> diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
> index 2167017..de2ba68 100644
> --- a/hw/spapr_vscsi.c
> +++ b/hw/spapr_vscsi.c
> @@ -920,12 +920,11 @@ static int spapr_vscsi_init(VIOsPAPRDevice *dev)
>      return 0;
>  }
>  
> -void spapr_vscsi_create(VIOsPAPRBus *bus, uint32_t reg)
> +void spapr_vscsi_create(VIOsPAPRBus *bus)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(&bus->bus, "spapr-vscsi");
> -    qdev_prop_set_uint32(dev, "reg", reg);
>  
>      qdev_init_nofail(dev);
>  }
> @@ -948,7 +947,7 @@ static int spapr_vscsi_devnode(VIOsPAPRDevice *dev, void 
> *fdt, int node_off)
>  }
>  
>  static Property spapr_vscsi_properties[] = {
> -    DEFINE_SPAPR_PROPERTIES(VSCSIState, vdev, 0x2000, 0x10000000),
> +    DEFINE_SPAPR_PROPERTIES(VSCSIState, vdev, 0x10000000),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/spapr_vty.c b/hw/spapr_vty.c
> index a30c040..c9674f3 100644
> --- a/hw/spapr_vty.c
> +++ b/hw/spapr_vty.c
> @@ -123,18 +123,17 @@ static target_ulong h_get_term_char(CPUPPCState *env, 
> sPAPREnvironment *spapr,
>      return H_SUCCESS;
>  }
>  
> -void spapr_vty_create(VIOsPAPRBus *bus, uint32_t reg, CharDriverState 
> *chardev)
> +void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(&bus->bus, "spapr-vty");
> -    qdev_prop_set_uint32(dev, "reg", reg);
>      qdev_prop_set_chr(dev, "chardev", chardev);
>      qdev_init_nofail(dev);
>  }
>  
>  static Property spapr_vty_properties[] = {
> -    DEFINE_SPAPR_PROPERTIES(VIOsPAPRVTYDevice, sdev, SPAPR_VTY_BASE_ADDRESS, 
> 0),
> +    DEFINE_SPAPR_PROPERTIES(VIOsPAPRVTYDevice, sdev, 0),
>      DEFINE_PROP_CHR("chardev", VIOsPAPRVTYDevice, chardev),
>      DEFINE_PROP_END_OF_LIST(),
>  };

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg