Re: [Qemu-devel] [RFC PATCH] spapr-vty: workaround "reg" property for old kernels

[Alexander Graf <agraf@suse.de>]

On 10/15/2013 10:50 AM, Alexey Kardashevskiy wrote:
> Old kernels (<  3.1) handle hvcX devices different in different parts.
> Sometime the kernel assumes that the hvc device numbers start from zero
> and if there is just one hvc, then it is hvc0.
>
> However kernel's add_preferred_console() uses the very last byte of
> the VTY's "reg" property as an hvc number so it might end up with something
> different than hvc.
>
> The problem appears on SLES11SP3 and RHEL6. If to run QEMU without
> -nodefaults, then the default VTY is created first on a VIO bus and gets
> reg==0x71000000 so it will be hvc0 and everything will be fine.
> If to run QEMU with:
>   -nodefaults \
>   -chardev "socket,id=char1,host=localhost,port=8001,server,telnet,mux=on" \
>   -device "spapr-vty,chardev=char1" \
>   -mon "chardev=char1,mode=readline,id=mon1" \
>
> then the exactly the same config is expected but in this case spapr-vty
> gets reg==0x71000001 and therefore it becomes hvc1 and lots of debug
> output is missing. SLES11SP3 does not even boot as /dev/console is
> redirected to /dev/hvc0 which is dead.
>
> The issue can be solved by manual selection of VTY's "reg" property to
> have last byte equal to zero.
>
> The alternative would be to use separate "reg" property counter for
> automatic "reg" property generation and this is what the patch does.
>
> Signed-off-by: Alexey Kardashevskiy<aik@ozlabs.ru>
> ---
>
> Since libvirt uses "-nodefault" a lot and in this case "spapr-nvram" gets
> created first and gets reg=0x71000000, we cannot just ignore this. Also,
> it does not seem an option to require libvirt users to specify spapr-vty
> "reg" property every time.
>
> Can anyone think of a simpler solutionu? Thanks.
>
>
> ---
>   hw/ppc/spapr_vio.c         | 7 ++++++-
>   include/hw/ppc/spapr_vio.h | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index a6a0a51..2d56950 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -438,7 +438,11 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>           VIOsPAPRBus *bus = DO_UPCAST(VIOsPAPRBus, bus, dev->qdev.parent_bus);
>
>           do {
> -            dev->reg = bus->next_reg++;
> +            if (!object_dynamic_cast(OBJECT(qdev), "spapr-vty")) {
> +                dev->reg = bus->next_reg++;
> +            } else {
> +                dev->reg = bus->next_vty_reg++;
> +            }
>           } while (reg_conflict(dev));
>       }
>
> @@ -501,6 +505,7 @@ VIOsPAPRBus *spapr_vio_bus_init(void)
>       qbus = qbus_create(TYPE_SPAPR_VIO_BUS, dev, "spapr-vio");
>       bus = DO_UPCAST(VIOsPAPRBus, bus, qbus);
>       bus->next_reg = 0x71000000;
> +    bus->next_vty_reg = 0x71000100;

This breaks as soon as you pass in more than 0x100 devices that are 
non-vty into the guest, no?

The reg property really describes the virtual slot a device is in. 
Couldn't we do that allocation explicitly and push it from libvirt, just 
like we do it with the slots for PCI?


Alex


>
>       /* hcall-vio */
>       spapr_register_hypercall(H_VIO_SIGNAL, h_vio_signal);
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index d8b3b03..3a92d9e 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -73,6 +73,7 @@ struct VIOsPAPRDevice {
>   struct VIOsPAPRBus {
>       BusState bus;
>       uint32_t next_reg;
> +    uint32_t next_vty_reg;
>       int (*init)(VIOsPAPRDevice *dev);
>       int (*devnode)(VIOsPAPRDevice *dev, void *fdt, int node_off);
>   };