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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2013-06-19 at 23:28 +0200, Alexander Graf wrote:
> 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?

There is no guarantee other than what qemu does... I didn't write that
code so I don't know :-) But I don't see why it can't be kept free.

> Also, are there no other PAPR calls that could possibly refer to reg=0 but
> mean something different from the VTY device?

Not that come to mind.

Ben.

> 
> Alex
> 
> > 
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > ---
> > 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
> >