[Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Peter Maydell]

Hi; I'm currently writing '-cpu max' support for ARM. For that I'd
like to be able to do the "probe host kernel for its supported feature
set" in the CPU object's instance-init function, but I'd like to do
it just once and cache the answer. Can I rely on something or other
having the BQL or otherwise ensuring that two threads don't run
the instance_init method in parallel (eg in a hotplug situation),
or do I need to create and use my own mutex to protect the cached
answer data?

thanks
-- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Igor Mammedov]

On Thu, 7 Dec 2017 16:05:50 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> Hi; I'm currently writing '-cpu max' support for ARM. For that I'd
> like to be able to do the "probe host kernel for its supported feature
> set" in the CPU object's instance-init function, but I'd like to do
> it just once and cache the answer. Can I rely on something or other
> having the BQL or otherwise ensuring that two threads don't run
> the instance_init method in parallel (eg in a hotplug situation),
> or do I need to create and use my own mutex to protect the cached
> answer data?
considering cached data shouldn't change during qemu lifetime it
shouldn't be possible for instance_init() to clash at hotplug
time as object_new(cpu) calls serialized within monitor/qmp loop.

But why cpu's instance_init, we could cache host's value at
configure_accelerator() time (could be different depending on accel).

CCing Eduardo as he is the one who worked on -cpu max for x86
(If I recall correctly)


> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Peter Maydell]

On 7 December 2017 at 16:48, Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 7 Dec 2017 16:05:50 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> Hi; I'm currently writing '-cpu max' support for ARM. For that I'd
>> like to be able to do the "probe host kernel for its supported feature
>> set" in the CPU object's instance-init function, but I'd like to do
>> it just once and cache the answer. Can I rely on something or other
>> having the BQL or otherwise ensuring that two threads don't run
>> the instance_init method in parallel (eg in a hotplug situation),
>> or do I need to create and use my own mutex to protect the cached
>> answer data?
> considering cached data shouldn't change during qemu lifetime it
> shouldn't be possible for instance_init() to clash at hotplug
> time as object_new(cpu) calls serialized within monitor/qmp loop.

Right, that was my question -- are we guaranteed that anything
that calls object_new(cpu) does it with a lock?

> But why cpu's instance_init, we could cache host's value at
> configure_accelerator() time (could be different depending on accel).

I don't know if I need the information at configure_accelerator()
time (if you say "-cpu cortex-a57" then we don't need to bother
probing). The first time I know if we want to probe is when a
"-cpu host" CPU is created.

thanks
-- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Eduardo Habkost]

On Thu, Dec 07, 2017 at 04:53:59PM +0000, Peter Maydell wrote:
> On 7 December 2017 at 16:48, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 7 Dec 2017 16:05:50 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> >> Hi; I'm currently writing '-cpu max' support for ARM. For that I'd
> >> like to be able to do the "probe host kernel for its supported feature
> >> set" in the CPU object's instance-init function, but I'd like to do

I don't think instance_init is appropriate for that, as
object_free(object_new(t)) must be always safe to call and free
of side-effects for all types.  Wouldn't it work if you do that
on realize?


> >> it just once and cache the answer. Can I rely on something or other
> >> having the BQL or otherwise ensuring that two threads don't run
> >> the instance_init method in parallel (eg in a hotplug situation),
> >> or do I need to create and use my own mutex to protect the cached
> >> answer data?
> > considering cached data shouldn't change during qemu lifetime it
> > shouldn't be possible for instance_init() to clash at hotplug
> > time as object_new(cpu) calls serialized within monitor/qmp loop.
> 
> Right, that was my question -- are we guaranteed that anything
> that calls object_new(cpu) does it with a lock?

object_new() itself doesn't seem to be thread-safe (see
type_initialize()), so I'm pretty sure there shouldn't be any
object_new() calls in QEMU without the BQL being held.


> 
> > But why cpu's instance_init, we could cache host's value at
> > configure_accelerator() time (could be different depending on accel).
> 
> I don't know if I need the information at configure_accelerator()
> time (if you say "-cpu cortex-a57" then we don't need to bother
> probing). The first time I know if we want to probe is when a
> "-cpu host" CPU is created.
> 
> thanks
> -- PMM

-- 
Eduardo

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Peter Maydell]

On 7 December 2017 at 17:07, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Dec 07, 2017 at 04:53:59PM +0000, Peter Maydell wrote:
>> On 7 December 2017 at 16:48, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Thu, 7 Dec 2017 16:05:50 +0000
>> > Peter Maydell <peter.maydell@linaro.org> wrote:
>> >
>> >> Hi; I'm currently writing '-cpu max' support for ARM. For that I'd
>> >> like to be able to do the "probe host kernel for its supported feature
>> >> set" in the CPU object's instance-init function, but I'd like to do
>
> I don't think instance_init is appropriate for that, as
> object_free(object_new(t)) must be always safe to call and free
> of side-effects for all types.  Wouldn't it work if you do that
> on realize?

I think we need the information before realize, but I'll double
check. In any case the probe is safe to call and free of side-effects.
(If it fails we can stash a flag that we didn't manage to probe, which
realize can check.)

>> Right, that was my question -- are we guaranteed that anything
>> that calls object_new(cpu) does it with a lock?
>
> object_new() itself doesn't seem to be thread-safe (see
> type_initialize()), so I'm pretty sure there shouldn't be any
> object_new() calls in QEMU without the BQL being held.

Cool.

thanks
-- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Peter Maydell]

On 7 December 2017 at 17:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 December 2017 at 17:07, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> On Thu, Dec 07, 2017 at 04:53:59PM +0000, Peter Maydell wrote:
>>> On 7 December 2017 at 16:48, Igor Mammedov <imammedo@redhat.com> wrote:
>>> > On Thu, 7 Dec 2017 16:05:50 +0000
>>> > Peter Maydell <peter.maydell@linaro.org> wrote:
>>> >
>>> >> Hi; I'm currently writing '-cpu max' support for ARM. For that I'd
>>> >> like to be able to do the "probe host kernel for its supported feature
>>> >> set" in the CPU object's instance-init function, but I'd like to do
>>
>> I don't think instance_init is appropriate for that, as
>> object_free(object_new(t)) must be always safe to call and free
>> of side-effects for all types.  Wouldn't it work if you do that
>> on realize?
>
> I think we need the information before realize, but I'll double
> check.

We do need the information before realize, because the probe
is what tells us what feature bits we need to set, and the
ARM instance_post_init hook needs to look at those to determine
eg which other feature bits to set and which QOM properties to
expose as a result, and all that has to happen at init time,
not realize time.

thanks
-- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Igor Mammedov]

On Thu, 7 Dec 2017 17:26:53 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 7 December 2017 at 17:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 7 December 2017 at 17:07, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> >> On Thu, Dec 07, 2017 at 04:53:59PM +0000, Peter Maydell wrote:  
> >>> On 7 December 2017 at 16:48, Igor Mammedov <imammedo@redhat.com> wrote:  
> >>> > On Thu, 7 Dec 2017 16:05:50 +0000
> >>> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> >  
> >>> >> Hi; I'm currently writing '-cpu max' support for ARM. For that I'd
> >>> >> like to be able to do the "probe host kernel for its supported feature
> >>> >> set" in the CPU object's instance-init function, but I'd like to do  
> >>
> >> I don't think instance_init is appropriate for that, as
> >> object_free(object_new(t)) must be always safe to call and free
> >> of side-effects for all types.  Wouldn't it work if you do that
> >> on realize?  
> >
> > I think we need the information before realize, but I'll double
> > check.  
> 
> We do need the information before realize, because the probe
> is what tells us what feature bits we need to set, and the
> ARM instance_post_init hook needs to look at those to determine
> eg which other feature bits to set and which QOM properties to
> expose as a result, and all that has to happen at init time,
> not realize time.
maybe it could be modeled after  kvm_ppc_register_host_cpu_type(),
i.e. create type with necessary feature bits set at cpu's class
init time (sort of combo of what ppc and x86 do).

i.e. one caches host's feature bits at cpu_class_init time and
then loads applies them to object instance at instance init time,
like in x86_cpu_initfn()->x86_cpu_load_def().

TBH:
 I do not recall why we have x86 max/host cpu types do feature
 loading at realize time instead of at class init like the rest
 of static cpu types.


> thanks
> -- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Peter Maydell]

On 8 December 2017 at 13:16, Igor Mammedov <imammedo@redhat.com> wrote:
> TBH:
>  I do not recall why we have x86 max/host cpu types do feature
>  loading at realize time instead of at class init like the rest
>  of static cpu types.

class init is too early, IIRC -- it's before KVM has been set up at all.

thanks
-- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Igor Mammedov]

On Fri, 8 Dec 2017 13:19:27 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 8 December 2017 at 13:16, Igor Mammedov <imammedo@redhat.com> wrote:
> > TBH:
> >  I do not recall why we have x86 max/host cpu types do feature
> >  loading at realize time instead of at class init like the rest
> >  of static cpu types.  
> 
> class init is too early, IIRC -- it's before KVM has been set up at all.

that shouldn't be an issue as kvm_ppc_register_host_cpu_type() demonstrates
(i.e. an additional class init at kvm/tcg init time),

so it might be some compat issue or just legacy approach why it
havn't been rewritten to class_init for x86 the way PPC does.
But Eduardo probably knows better if there is anything left that
prevents using class init there.

> 
> thanks
> -- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Peter Maydell]

On 8 December 2017 at 14:50, Igor Mammedov <imammedo@redhat.com> wrote:
> On Fri, 8 Dec 2017 13:19:27 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 8 December 2017 at 13:16, Igor Mammedov <imammedo@redhat.com> wrote:
>> > TBH:
>> >  I do not recall why we have x86 max/host cpu types do feature
>> >  loading at realize time instead of at class init like the rest
>> >  of static cpu types.
>>
>> class init is too early, IIRC -- it's before KVM has been set up at all.
>
> that shouldn't be an issue as kvm_ppc_register_host_cpu_type() demonstrates
> (i.e. an additional class init at kvm/tcg init time),
>
> so it might be some compat issue or just legacy approach why it
> havn't been rewritten to class_init for x86 the way PPC does.
> But Eduardo probably knows better if there is anything left that
> prevents using class init there.

Hmm. In any case, feel free to review
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg01225.html
and suggest changes if you think there's a better way of structuring
things.

thanks
-- PMM

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Eduardo Habkost]

On Fri, Dec 08, 2017 at 03:50:50PM +0100, Igor Mammedov wrote:
> On Fri, 8 Dec 2017 13:19:27 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On 8 December 2017 at 13:16, Igor Mammedov <imammedo@redhat.com> wrote:
> > > TBH:
> > >  I do not recall why we have x86 max/host cpu types do feature
> > >  loading at realize time instead of at class init like the rest
> > >  of static cpu types.  
> > 
> > class init is too early, IIRC -- it's before KVM has been set up at all.
> 
> that shouldn't be an issue as kvm_ppc_register_host_cpu_type() demonstrates
> (i.e. an additional class init at kvm/tcg init time),

It is possible, but IMO it's not a good idea.  We should be able
to enumerate all CPU types before the accelerator has been
initialized, so query-cpu-definitions and "-cpu help" will always
work.


> 
> so it might be some compat issue or just legacy approach why it
> havn't been rewritten to class_init for x86 the way PPC does.
> But Eduardo probably knows better if there is anything left that
> prevents using class init there.

It's the opposite: x86 "host" CPU model used to work the same way
as PPC, but we changed it so all classes are registered at
type_init()-time.

-- 
Eduardo

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Igor Mammedov]

On Fri, 8 Dec 2017 14:14:22 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Dec 08, 2017 at 03:50:50PM +0100, Igor Mammedov wrote:
> > On Fri, 8 Dec 2017 13:19:27 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >   
> > > On 8 December 2017 at 13:16, Igor Mammedov <imammedo@redhat.com> wrote:  
> > > > TBH:
> > > >  I do not recall why we have x86 max/host cpu types do feature
> > > >  loading at realize time instead of at class init like the rest
> > > >  of static cpu types.    
> > > 
> > > class init is too early, IIRC -- it's before KVM has been set up at all.  
> > 
> > that shouldn't be an issue as kvm_ppc_register_host_cpu_type() demonstrates
> > (i.e. an additional class init at kvm/tcg init time),  
> 
> It is possible, but IMO it's not a good idea.  We should be able
> to enumerate all CPU types before the accelerator has been
> initialized, so query-cpu-definitions and "-cpu help" will always
> work.
> 
> 
> > 
> > so it might be some compat issue or just legacy approach why it
> > havn't been rewritten to class_init for x86 the way PPC does.
> > But Eduardo probably knows better if there is anything left that
> > prevents using class init there.  
> 
> It's the opposite: x86 "host" CPU model used to work the same way
> as PPC, but we changed it so all classes are registered at
> type_init()-time.
Is it for libvirt convenience, so that it would be able to cache
all supported cpus regardless of whether they would actually work
or not?

Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?

[Eduardo Habkost]

On Fri, Dec 08, 2017 at 06:32:03PM +0100, Igor Mammedov wrote:
> On Fri, 8 Dec 2017 14:14:22 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Dec 08, 2017 at 03:50:50PM +0100, Igor Mammedov wrote:
> > > On Fri, 8 Dec 2017 13:19:27 +0000
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > >   
> > > > On 8 December 2017 at 13:16, Igor Mammedov <imammedo@redhat.com> wrote:  
> > > > > TBH:
> > > > >  I do not recall why we have x86 max/host cpu types do feature
> > > > >  loading at realize time instead of at class init like the rest
> > > > >  of static cpu types.    
> > > > 
> > > > class init is too early, IIRC -- it's before KVM has been set up at all.  
> > > 
> > > that shouldn't be an issue as kvm_ppc_register_host_cpu_type() demonstrates
> > > (i.e. an additional class init at kvm/tcg init time),  
> > 
> > It is possible, but IMO it's not a good idea.  We should be able
> > to enumerate all CPU types before the accelerator has been
> > initialized, so query-cpu-definitions and "-cpu help" will always
> > work.
> > 
> > 
> > > 
> > > so it might be some compat issue or just legacy approach why it
> > > havn't been rewritten to class_init for x86 the way PPC does.
> > > But Eduardo probably knows better if there is anything left that
> > > prevents using class init there.  
> > 
> > It's the opposite: x86 "host" CPU model used to work the same way
> > as PPC, but we changed it so all classes are registered at
> > type_init()-time.
> Is it for libvirt convenience, so that it would be able to cache
> all supported cpus regardless of whether they would actually work
> or not?

I'd say it's for our own convenience, so that things won't break
if we make QMP available before machine/accel initialization in
the future, or if we reorder the code that handles "-cpu ?" in
main().

Moving code around is simpler if we know that all QOM types are
registered at the same point, so we can treat them as static data
after module_call_init(MODULE_INIT_QOM) runs.

-- 
Eduardo