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

[Anthony Liguori]

Alexander Graf <address@hidden> writes:

> 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?

reg is exposed to the guest via device tree.  My assumption is that the
only reason this reg=0 silliness exists is for early boot code that
hasn't gotten enough working yet to actually read the device tree to
discover the proper reg value for the VTY.

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

Note that if there is, it will still fail today exactly the same way as
it does without this patch.

We can still add work arounds to the reg lookup code to return something
different for reg=0 if a different device type is being searched for.

So even if that's the case, I still think this move is the right way to
handle things.

Regards,

Anthony Liguori

>
>
> 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
>>