Re: [Qemu-devel] [PATCH 09/12] spapr-vio: move special case handling for reg=0 to vio

[Alexander Graf]

On 19.06.2013, at 22:40, Anthony Liguori wrote:

> The creatively named reg field is a hypervisor assigned global
> identifier for a virtual device.  Despite the fact that no device
> is assigned a reg of 0, guests still use it to refer to early
> console.
> 
> Instead of handling this in the VTY device, handle this in the VIO
> bus since this is ultimately about how devices are decoded.
> 
> This does not produce a change in behavior since reg=0 hcalls to
> non-VTY devices will still fail as gloriously as they did before
> just for a different reason (invalid device instead of invalid reg).

Ben, is it true that reg=0 is guaranteed to be free, regardless of the target 
call? Also, are there no other PAPR calls that could possibly refer to reg=0 
but mean something different from the VTY device?


Alex

> 
> Signed-off-by: Anthony Liguori <address@hidden>
> ---
> hw/char/spapr_vty.c        | 58 ++--------------------------------------------
> hw/ppc/spapr_vio.c         | 46 ++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_vio.h |  2 --
> 3 files changed, 48 insertions(+), 58 deletions(-)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 4bac79e..45fc3ce 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
>     return 0;
> }
> 
> -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
> -{
> -    VIOsPAPRDevice *sdev;
> -
> -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> -    if (!sdev && reg == 0) {
> -        /* Hack for kernel early debug, which always specifies reg==0.
> -         * We search all VIO devices, and grab the vty with the lowest
> -         * reg.  This attempts to mimic existing PowerVM behaviour
> -         * (early debug does work there, despite having no vty with
> -         * reg==0. */
> -        return spapr_vty_get_default(spapr->vio_bus);
> -    }
> -
> -    return sdev;
> -}
> -
> /* Forward declaration */
> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                     target_ulong opcode, target_ulong *args)
> @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, 
> sPAPREnvironment *spapr,
>     VIOsPAPRDevice *sdev;
>     uint8_t buf[16];
> 
> -    sdev = vty_lookup(spapr, reg);
> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>     if (!sdev) {
>         return H_PARAMETER;
>     }
> @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, 
> sPAPREnvironment *spapr,
>     VIOsPAPRDevice *sdev;
>     uint8_t buf[16];
> 
> -    sdev = vty_lookup(spapr, reg);
> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>     if (!sdev) {
>         return H_PARAMETER;
>     }
> @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = {
>     .class_init    = spapr_vty_class_init,
> };
> 
> -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
> -{
> -    VIOsPAPRDevice *sdev, *selected;
> -    BusChild *kid;
> -
> -    /*
> -     * To avoid the console bouncing around we want one VTY to be
> -     * the "default". We haven't really got anything to go on, so
> -     * arbitrarily choose the one with the lowest reg value.
> -     */
> -
> -    selected = NULL;
> -    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> -        DeviceState *iter = kid->child;
> -
> -        /* Only look at VTY devices */
> -        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
> -            continue;
> -        }
> -
> -        sdev = VIO_SPAPR_DEVICE(iter);
> -
> -        /* First VTY we've found, so it is selected for now */
> -        if (!selected) {
> -            selected = sdev;
> -            continue;
> -        }
> -
> -        /* Choose VTY with lowest reg value */
> -        if (sdev->reg < selected->reg) {
> -            selected = sdev;
> -        }
> -    }
> -
> -    return selected;
> -}
> -
> static void spapr_vty_register_types(void)
> {
>     spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 2c5b159..ee99eec 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = {
>     .instance_size = sizeof(VIOsPAPRBus),
> };
> 
> +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
> +{
> +    VIOsPAPRDevice *sdev, *selected;
> +    BusChild *kid;
> +
> +    /*
> +     * To avoid the console bouncing around we want one VTY to be
> +     * the "default". We haven't really got anything to go on, so
> +     * arbitrarily choose the one with the lowest reg value.
> +     */
> +
> +    selected = NULL;
> +    QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> +        DeviceState *iter = kid->child;
> +
> +        /* Only look at VTY devices */
> +        if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) {
> +            continue;
> +        }
> +
> +        sdev = VIO_SPAPR_DEVICE(iter);
> +
> +        /* First VTY we've found, so it is selected for now */
> +        if (!selected) {
> +            selected = sdev;
> +            continue;
> +        }
> +
> +        /* Choose VTY with lowest reg value */
> +        if (sdev->reg < selected->reg) {
> +            selected = sdev;
> +        }
> +    }
> +
> +    return selected;
> +}
> +
> VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
> {
>     BusChild *kid;
>     VIOsPAPRDevice *dev = NULL;
> 
> +    /* Hack for kernel early debug, which always specifies reg==0.
> +     * We search all VIO devices, and grab the vty with the lowest
> +     * reg.  This attempts to mimic existing PowerVM behaviour
> +     * (early debug does work there, despite having no vty with
> +     * reg==0. */
> +    if (reg == 0) {
> +        return spapr_vty_get_default(bus);
> +    }
> +
>     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>         dev = (VIOsPAPRDevice *)kid->child;
>         if (dev->reg == reg) {
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index 817f5ff..f55eb90 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -127,8 +127,6 @@ 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);
> -
> void spapr_vio_quiesce(void);
> 
> #endif /* _HW_SPAPR_VIO_H */
> -- 
> 1.8.0
>