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

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&lt;aik(a)ozlabs.ru&gt; >> --- >> >> 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?

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