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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 15, 2013 at 3:47 PM, Alexey Kardashevskiy <aik@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>>
>>> 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
>

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]