On Tue, Oct 15, 2013 at 3:47 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

On 10/16/2013 02:03 AM, Alexander Graf wrote:

...

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?

Will we ever have this much? Ah, anyway, this code already checks if the address is taken and fails if it is. And there is still a possibility to assign addresses manually.

...

The reg property really describes the virtual slot a device is in.

We use 0x71000000. I saw xmls from libvirt where VTY's reg is 0x30000000. Whether it is a slot or not, QEMU/SLOF/Kernel does not seem to care about absolute value :)

...

Couldn't we do that allocation explicitly and push it from libvirt, just like we do it with the slots for PCI?

Yes, this is the only solution. We make no promises with respect to argument ordering. libvirt needs to explicitly specify reg values to create a stable device tree (just like it does with PCI). Regards, Anthony Liguori

That is the other possibility, yes. But in this case "-nodefaults" must not create spapr-nvram automatically and if we do that, we'll break existing setups.

...

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); };

-- Alexey