[Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Peter Maydell]

I just got bitten by qdev_get_machine()'s behaviour on the user-only
emulators, where it can return something that isn't NULL and isn't
an instance of TYPE_MACHINE either.

It looks like maybe this can happen in some cases in softmmu too,
judging by the way that qdev_get_hotplug_handler() does an
object_dynamic_cast() check that it really got back a TYPE_MACHINE.

Is this intentional? Does anything rely on qdev_get_machine()
returning something odd like this?

In the code I have which ran into this I can just make it do an
object_dynamic_cast() check like the hotplug_handler code does,
but if the current implementation is intentional we should
probably document that this is what you're supposed to do.

thanks
-- PMM

Re: [Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Igor Mammedov]

On Mon, 4 Sep 2017 17:36:59 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> I just got bitten by qdev_get_machine()'s behaviour on the user-only
> emulators, where it can return something that isn't NULL and isn't
> an instance of TYPE_MACHINE either.
user-only shouldn't get to qdev_get_machine() at all,
issue probably in container_get().
I'd try to fix wrong user if possible and maybe add ifdef build failure
to qdev_get_machine() so it would not build in user mode.
 
> It looks like maybe this can happen in some cases in softmmu too,
> judging by the way that qdev_get_hotplug_handler() does an
> object_dynamic_cast() check that it really got back a TYPE_MACHINE.
As I recall only bus or machine provide hotplug_handler currently,
but it's possible to extend to other objects if we find use-case.

We could do static cast to machine instead dynamic there but
in hotplug case it will abort QEMU if error happens,
hence dynamic check to avoid be more resilient during hotplug.
(well, if qdev_get_machine() returns not machine during startup
we would be screwed anyways, but that should break much earlier)

> Is this intentional? Does anything rely on qdev_get_machine()
> returning something odd like this?
> 
> In the code I have which ran into this I can just make it do an
> object_dynamic_cast() check like the hotplug_handler code does,
> but if the current implementation is intentional we should
> probably document that this is what you're supposed to do.
> 
> thanks
> -- PMM

Re: [Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Peter Maydell]

On 4 September 2017 at 20:38, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 4 Sep 2017 17:36:59 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> I just got bitten by qdev_get_machine()'s behaviour on the user-only
>> emulators, where it can return something that isn't NULL and isn't
>> an instance of TYPE_MACHINE either.
> user-only shouldn't get to qdev_get_machine() at all,
> issue probably in container_get().

I need it in cpu_common_realizefn(), for
http://patchwork.ozlabs.org/patch/797940/

> I'd try to fix wrong user if possible and maybe add ifdef build failure
> to qdev_get_machine() so it would not build in user mode.

Can't ifdef, that source file is built once for all targets.

My fix (which I intend to send to the list today) is to make
it do the object_dynamic_cast() check -- if that doesn't give
a TYPE_MACHINE then we're in user mode and don't need to set
ignore_memory_transaction_failures on the cpu object anyway.

>> It looks like maybe this can happen in some cases in softmmu too,
>> judging by the way that qdev_get_hotplug_handler() does an
>> object_dynamic_cast() check that it really got back a TYPE_MACHINE.
> As I recall only bus or machine provide hotplug_handler currently,
> but it's possible to extend to other objects if we find use-case.
>
> We could do static cast to machine instead dynamic there but
> in hotplug case it will abort QEMU if error happens,
> hence dynamic check to avoid be more resilient during hotplug.
> (well, if qdev_get_machine() returns not machine during startup
> we would be screwed anyways, but that should break much earlier)

If this can't ever happen then we should be aborting; that's
the idea behind the cast macros doing assertions. I'm not
sure hotplug needs to be special here if it doesn't have
a genuine reason to think it might get back something of
the wrong type.

thanks
-- PMM

Re: [Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Igor Mammedov]

On Tue, 5 Sep 2017 10:08:01 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 4 September 2017 at 20:38, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 4 Sep 2017 17:36:59 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >  
> >> I just got bitten by qdev_get_machine()'s behaviour on the user-only
> >> emulators, where it can return something that isn't NULL and isn't
> >> an instance of TYPE_MACHINE either.  
> > user-only shouldn't get to qdev_get_machine() at all,
> > issue probably in container_get().  
> 
> I need it in cpu_common_realizefn(), for
> http://patchwork.ozlabs.org/patch/797940/
Link might be broken (unable to connect to server)

Anyways I'd avoid using machine from cpu_*_realizefn(),
instead of I'd add property to CPU that has needed data
and set it from board code. Should work fine for *-user targets
and maintain clear separation of device impl. and board details. 

> 
> > I'd try to fix wrong user if possible and maybe add ifdef build failure
> > to qdev_get_machine() so it would not build in user mode.  
> 
> Can't ifdef, that source file is built once for all targets.
> 
> My fix (which I intend to send to the list today) is to make
> it do the object_dynamic_cast() check -- if that doesn't give
> a TYPE_MACHINE then we're in user mode and don't need to set
> ignore_memory_transaction_failures on the cpu object anyway.
> 
> >> It looks like maybe this can happen in some cases in softmmu too,
> >> judging by the way that qdev_get_hotplug_handler() does an
> >> object_dynamic_cast() check that it really got back a TYPE_MACHINE.  
> > As I recall only bus or machine provide hotplug_handler currently,
> > but it's possible to extend to other objects if we find use-case.
> >
> > We could do static cast to machine instead dynamic there but
> > in hotplug case it will abort QEMU if error happens,
> > hence dynamic check to avoid be more resilient during hotplug.
> > (well, if qdev_get_machine() returns not machine during startup
> > we would be screwed anyways, but that should break much earlier)  
> 
> If this can't ever happen then we should be aborting; that's
> the idea behind the cast macros doing assertions. I'm not
> sure hotplug needs to be special here if it doesn't have
> a genuine reason to think it might get back something of
> the wrong type.
Yep, we should abort.

> 
> thanks
> -- PMM

Re: [Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Peter Maydell]

On 11 September 2017 at 13:10, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 5 Sep 2017 10:08:01 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 4 September 2017 at 20:38, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Mon, 4 Sep 2017 17:36:59 +0100
>> > Peter Maydell <peter.maydell@linaro.org> wrote:
>> >
>> >> I just got bitten by qdev_get_machine()'s behaviour on the user-only
>> >> emulators, where it can return something that isn't NULL and isn't
>> >> an instance of TYPE_MACHINE either.
>> > user-only shouldn't get to qdev_get_machine() at all,
>> > issue probably in container_get().
>>
>> I need it in cpu_common_realizefn(), for
>> http://patchwork.ozlabs.org/patch/797940/
> Link might be broken (unable to connect to server)

Works for me, but it is in master now anyway, commit
ed860129acd3fcd0b1.

> Anyways I'd avoid using machine from cpu_*_realizefn(),
> instead of I'd add property to CPU that has needed data
> and set it from board code. Should work fine for *-user targets
> and maintain clear separation of device impl. and board details.

It's not possible in all cases to set a CPU property from the
top level board code. In quite a lot of cases the CPU
object is created by an SoC object which is in turn
created by the board code, and there is no plumbing
there to pass arbitrary properties through to the CPU
object...

thanks
-- PMM

Re: [Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Igor Mammedov]

On Mon, 11 Sep 2017 14:33:03 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 11 September 2017 at 13:10, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 5 Sep 2017 10:08:01 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >  
> >> On 4 September 2017 at 20:38, Igor Mammedov <imammedo@redhat.com> wrote:  
> >> > On Mon, 4 Sep 2017 17:36:59 +0100
> >> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >> >  
> >> >> I just got bitten by qdev_get_machine()'s behaviour on the user-only
> >> >> emulators, where it can return something that isn't NULL and isn't
> >> >> an instance of TYPE_MACHINE either.  
> >> > user-only shouldn't get to qdev_get_machine() at all,
> >> > issue probably in container_get().  
> >>
> >> I need it in cpu_common_realizefn(), for
> >> http://patchwork.ozlabs.org/patch/797940/  
> > Link might be broken (unable to connect to server)  
> 
> Works for me, but it is in master now anyway, commit
> ed860129acd3fcd0b1.
> 
> > Anyways I'd avoid using machine from cpu_*_realizefn(),
> > instead of I'd add property to CPU that has needed data
> > and set it from board code. Should work fine for *-user targets
> > and maintain clear separation of device impl. and board details.  
> 
> It's not possible in all cases to set a CPU property from the
> top level board code. In quite a lot of cases the CPU
> object is created by an SoC object which is in turn
> created by the board code, and there is no plumbing
> there to pass arbitrary properties through to the CPU
> object...
there is a cleaner way without cpu accessing machine,
make it property of cpu and use compat machinery that
was invented for fixing up stuff of this kind.

SET_MACHINE_COMPAT(MachineClass,
                   { .driver = "arm-cpu",
                     .property = "foo",
                     .value    = "off",
                   }
                  )

> 
> thanks
> -- PMM

Re: [Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Peter Maydell]

On 12 September 2017 at 08:40, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 11 Sep 2017 14:33:03 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> It's not possible in all cases to set a CPU property from the
>> top level board code. In quite a lot of cases the CPU
>> object is created by an SoC object which is in turn
>> created by the board code, and there is no plumbing
>> there to pass arbitrary properties through to the CPU
>> object...
> there is a cleaner way without cpu accessing machine,
> make it property of cpu and use compat machinery that
> was invented for fixing up stuff of this kind.
>
> SET_MACHINE_COMPAT(MachineClass,
>                    { .driver = "arm-cpu",
>                      .property = "foo",
>                      .value    = "off",
>                    }
>                   )

It looks like we only use that machine-compat stuff on
our versioned boards, which is pretty much the only place
where we don't need to set this particular flag...

thanks
-- PMM

Re: [Qemu-devel] qdev_get_machine() can return something non-NULL but not TYPE_MACHINE

[Igor Mammedov]

On Tue, 12 Sep 2017 10:11:32 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 12 September 2017 at 08:40, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 11 Sep 2017 14:33:03 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
> >> It's not possible in all cases to set a CPU property from the
> >> top level board code. In quite a lot of cases the CPU
> >> object is created by an SoC object which is in turn
> >> created by the board code, and there is no plumbing
> >> there to pass arbitrary properties through to the CPU
> >> object...  
> > there is a cleaner way without cpu accessing machine,
> > make it property of cpu and use compat machinery that
> > was invented for fixing up stuff of this kind.
> >
> > SET_MACHINE_COMPAT(MachineClass,
> >                    { .driver = "arm-cpu",
> >                      .property = "foo",
> >                      .value    = "off",
> >                    }
> >                   )  
> 
> It looks like we only use that machine-compat stuff on
> our versioned boards, which is pretty much the only place
> where we don't need to set this particular flag...
typically, yes it's used for versioned boards to, because
it's where we have to fixup/override defaults to keep compatibility.
But it's does not mean that it's limited to it.

in this case it allows to keep clean separation of device model
and not add an extra member to generic MachineClass which is used
by some old boards.


> thanks
> -- PMM
>