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<a...@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?
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
