Re: [PATCH] docs: Add a QEMU Code of Conduct and Conflict Resolution Policy document

[Andreas Färber]

Hi Paolo,

On 13.04.21 09:42, Paolo Bonzini wrote:
> On 07/04/21 17:42, Kevin Wolf wrote:
>>> +* Publishing other's private information, such as physical or
>>> electronic
>>> +addresses, without explicit permission
>>
>> Yes, it's pretty clear that I'm not publishing new information about
>> people when I'm keeping them in Cc: when replying to a thread, or even
>> when they posted in another thread on the list recently. It becomes much
>> less clear for adding people who aren't usually part of the QEMU
>> community.
> 
> If you took the email from, say, the Libvirt or kernel mailing lists,
> that would not be considered private.  If somebody has two email
> addresses and you deliberately Cc him on an address that he's only using
> for communications within his family, that would be a problem.

I have to admit I had originally stumbled over this bullet point myself,
reading it as private=personal. So maybe it might help avoid ambiguities
for non-native readers to formulate it as "non-public" instead?

Like, if someone posts to a public mailing list with their private as
opposed to business address in the footer. Then I would consider it
public. I did intentionally use a private email for topics such as PReP.

Or consider the case you get a bug report not copied to the public
mailing lists from someone you don't know. Then I would still expect to
be allowed to attribute a commit via Reported-by/CC to that person, as
it seems in his/her interest to get the bug fixed and be notified,
unless explicitly requested otherwise.

Mistakes can always happen, but I feel it needs to be the responsibility
of the sender, not of the receiver, to ensure that only data is shared
that the project members may use for valid development purposes.

Not sure how to extend that bullet point to make its purpose/scope clearer.

Cheers,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 01/17] MAINTAINERS: new maintainers for QOM

[Andreas Färber]

Am 19.06.19 um 22:10 schrieb Markus Armbruster:
> From: Paolo Bonzini 
> 
> QOM is not a particularly active subsystem now: 51 commits in two years.
> But, we need active maintainers to review and merge patches, and Git
> shows the following top committers taking on QOM:
> 
> Markus Armbruster 
> Eduardo Habkost 
> Paolo Bonzini 
> Marc-André Lureau 
> Eric Blake 
> 
> I volunteer myself, and also volunteer Eduardo and Daniel as reviewers
> since they understand the code well.
> 
> Cc: Andreas Färber 
> Cc: Daniel P. Berrange 
> Cc: Eduardo Habkost 
> Suggested-by: Markus Armbruster 
> Signed-off-by: Paolo Bonzini 
> Message-Id: <20190607113737.13473-1-pbonz...@redhat.com>
> Signed-off-by: Markus Armbruster 

I'm pretty sure I gave an Acked-by that's missing above?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] qgraph

[Andreas Färber]

Am 11.06.19 um 12:31 schrieb Paolo Bonzini:
> On 11/06/19 10:56, Markus Armbruster wrote:
>> Yes, this is how introspection (both QMP and QOM) is commonly used.
>> Just keep in mind one difference: QMP is static, QOM is dynamic.
>>
>> QMP being static means it's defined at compile time.  So is the value of
>> query-qmp-schema.  Same QEMU build, same value.  This permits caching.
>>
>> QOM being dynamic means to introspect an object's properties, you have
>> to create it.  Worse, an object's properties may (in theory) change at
>> any time.  *Properties*, not just property *values*.  In practice, I'd
>> expect properties to change only at realize time.
> 
> Right, and we should move more towards class-based properties so that
> the dynamic nature of QOM is only used for the bare minimum needed (e.g.
> memory regions).

I believe it was Paolo who once reminded me that all child<> properties
are dynamic. And link<> properties for bus devices are also dynamic. I
don't see a good way around that.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] qgraph

[Andreas Färber]

Am 10.06.19 um 15:52 schrieb Paolo Bonzini:
> On 10/06/19 15:28, Andreas Färber wrote:
>> So if we want a new QMP operation, the most sense would probably make
>> where-can-I-attach-type(foo) returning a list of QOM paths, showing only
>> the first free slot per bus. That would allow a more efficient lookup
>> implementation inside QEMU than needing to check each slot[n] property
>> via qom-get after discovering it with qom-list.
> 
> Note that what Natalia is seeking is an introspection mechanism to be
> used _before_ creating a virtual machine though.

QMP implied creating a virtual machine though.

This then goes back to the long-discussed topic of not doing recursive
realized=true when starting halted with -s but deferring that to the
cont operation. I doubt that's been implemented in the meantime?

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] qgraph

[Andreas Färber]

Am 10.06.19 um 14:03 schrieb Paolo Bonzini:
> On 10/06/19 13:57, Andreas Färber wrote:
>> Your question doesn't make sense grammatically or conceptually. As Paolo
>> explained below, QOM is a pure object model, with object types/classes
>> and properties. Buses are just object instances attached as properties
>> and don't necessarily even need their own type of bus object (e.g, CPU).
>> An answer you don't like doesn't change by asking it to other people...
>> The information is all there, you just need to interpret it correctly.
>>
>> There is no technical discussion (no concrete proposal of yours) to
>> comment on here, and kindly refer to last week's change of maintainers.
>>
>> You would be better off just explaining what you really want to achieve.
> 
> Well, that was explained upthread---finding out what device can be
> plugged where.
> 
> Let's see what is in QOM right now:
> 
> $ qemu-kvm -qmp unix:foo.sock,server,nowait -device virtio-scsi-pci,id=vs
> $ ./qmp/qom-list -s ~/foo.sock /machine/peripheral/vs|less
> 
> There is a "virtio-bus" here, and iside it there is a scsi-bus.
> 
> $ ./qmp/qom-list -s ~/foo.sock /machine/peripheral/vs/virtio-bus/child[0]/
> vs.0/
> 
> I guess you could add to virtio-scsi-pci a class property for the bus,
> and then make "vs.0" an alias to that class property.  This would allow
> you find buses by enumerating the class properties.

That kind of goes against the spirit of QOM though and ignores simpler
mechanisms of hot-plugging, as mentioned.

The theoretical generic way would be to discover that any random object
beneath /machine has a null property of a link<> type that inherits from
device and is compatible with the device type to be plugged.

In practice we decided against Anthony's idea of adding tons of empty
slot properties for e.g. PCI buses (the number space is too large). We
did add QOM support for wildcard "foo[*]" array properties though. A
null slot[42] property of a certain link<> type then means a device can
be plugged there, and the property's setter then needs to take care of
hot-plugging and un-plugging.
This was unfortunately obscured by the legacy qdev model where we
magically add new child[0] properties (pointing to
/machine/peripheral(-anon) sub-nodes) to its bus parent during device
instantiation IIRC.

So if we want a new QMP operation, the most sense would probably make
where-can-I-attach-type(foo) returning a list of QOM paths, showing only
the first free slot per bus. That would allow a more efficient lookup
implementation inside QEMU than needing to check each slot[n] property
via qom-get after discovering it with qom-list.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] qgraph

[Andreas Färber]

Hi Natalia,

Am 10.06.19 um 11:53 schrieb Natalia Fursova:
> Hi there!
> Read please our correspondence and answer on last question (Will it ok for
> QOM conception?)
> Also forwarding to QOM and QMP maintainers. Can you make comments on this
> discussion?

Your question doesn't make sense grammatically or conceptually. As Paolo
explained below, QOM is a pure object model, with object types/classes
and properties. Buses are just object instances attached as properties
and don't necessarily even need their own type of bus object (e.g, CPU).
An answer you don't like doesn't change by asking it to other people...
The information is all there, you just need to interpret it correctly.

There is no technical discussion (no concrete proposal of yours) to
comment on here, and kindly refer to last week's change of maintainers.

You would be better off just explaining what you really want to achieve.

Regards,
Andreas

> -Original Message-
> From: Natalia Fursova [mailto:natalia.furs...@ispras.ru] 
> Sent: Wednesday, June 05, 2019 5:23 PM
> To: 'Paolo Bonzini'; 'qemu-devel@nongnu.org'; 'Паша'
> Subject: RE: [Qemu-devel] qgraph
> 
> I see.
> We need this opportunity and want to implement it. Will it ok for QOM
> conception?
> 
> 
> 
> Best regards,
> Natalia
> 
> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com] 
> Sent: Wednesday, June 05, 2019 4:07 PM
> To: Natalia Fursova; qemu-devel@nongnu.org; 'Паша'
> Subject: Re: [Qemu-devel] qgraph
> 
> On 05/06/19 14:34, Natalia Fursova wrote:
>>
>> Thank you for your answer. I would like to clarify something about the qmp
>> commands.
>> For example, consider SCSI controller "lsi53c895a". For getting
> information
>> we use two commands: "device-list-properties" and "qom-list-properties".
>> Output consists of many properties, but there is no information about
>> provided buses by this device. Is there a qmp command which provides this
>> information?
> 
> Unfortunately there is no information in QMP about buses that are
> provided.   qom-list-types gives the buses that are requested.
> 
> Paolo
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] MAINTAINERS: new maintainers for QOM

[Andreas Färber]

Am 07.06.19 um 13:37 schrieb Paolo Bonzini:
> QOM is not a particularly active subsystem now: 51 commits in two years.
> But, we need active maintainers to review and merge patches, and Git
> shows the following top committers taking on QOM:
> 
> Markus Armbruster 
> Eduardo Habkost 
> Paolo Bonzini 
> Marc-André Lureau 
> Eric Blake 
> 
> I volunteer myself, and also volunteer Eduardo and Daniel as reviewers
> since they understand the code well.
> 
> Cc: Andreas Färber 
> Cc: Daniel P. Berrange 
> Cc: Eduardo Habkost 
> Suggested-by: Markus Armbruster 
> Signed-off-by: Paolo Bonzini 
> ---
>  MAINTAINERS | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a96829ea83..c5862db154 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2031,9 +2031,10 @@ F: docs/interop/qemu-ga-ref.texi
>  T: git https://github.com/mdroth/qemu.git qga
>  
>  QOM
> -M: Andreas Färber 
> +M: Paolo Bonzini 
> +R: Daniel P. Berrange 
> +R: Eduardo Habkost 

Thanks for volunteering,

Acked-by: Andreas Färber 

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] What's our ABI promise for externally visible QOM?

[Andreas Färber]

Am 30.11.18 um 07:59 schrieb Markus Armbruster:
> QOM types and the QOM graph are externally visible:
> 
> * qom-list-types returns QOM type names and parents.
> 
>   Fine print: the result is limited to concrete types by default.
>   Aside: that's the only way to figure out whether a type is abstract.
>   Interface design fail.  The result can optionally be limited to
>   sub-types of a certain type.
> 
> * qom-list-properties returns a named type's static properties.
> 
> * qom-list returns an object's properties.  This lets clients traverse
>   the QOM graph.
> 
> * qom-get returns a property's value.
> 
> * qom-set changes a property's value.
> 
> * -object and object-add create a QOM object of a certain type with
>   certain property values.  The type must be a sub-type of
>   "user-creatable".
> 
> * Types are identified by name.
> 
> * Objects and properties are identified by QOM path.  An absolute QOM
>   paths is the path within the composition tree starting at the root.
>   Partial paths are a convenience you don't want to understand.
> 
> What promises do we make regarding the stability  / backward
> compatibility of these externally visible entities?

Mainly we agreed on not changing the type of an existing property for
backwards compatibility (e.g., don't switch between int and string,
change the property name then). More difficult will be structs in that
regard - does adding a field count as changing the type? Adding and
removing properties should be okay, as it can be discovered or results
in a detectable error. Don't recall we made any other promises, but I
might've forgotten something in this ad-hoc list.

Regards,
Andreas

> 
> The QMP command documentation is silent on it.  A user could reasonably
> assume that the general QMP stability promise extends to all of QOM.
> Does it?
> 
> Was that the intent when we created qom-list, qom-set, qom-get?
> Andreas, do you remember?
> 
> Is it practical given the current state of affairs?
> 
> Is it a good idea?
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 2/3] qtest: new functions for pulsed IRQs

[Andreas Färber]

Am 31.10.18 um 09:31 schrieb Michael Walle:
> Am 2018-07-18 12:48, schrieb Michael Walle:
>> It is only possible to retrieve the current state of an interrupt
>> line. But
>> there are devices which just pulses the interrupt line. Introduce a latch
>> which is set by qtest and which can be cleared by the test case.
>>
>> Signed-off-by: Michael Walle 
>> Cc: Paolo Bonzini 
>> Cc: Andreas Färber 
> 
> Hi,
> 
> unfortunately, there was never any feedback. Are you ok with this patch
> in general? There is still one patch for an additional test case in my
> queue, which depends on this.

No objection from my side, but Paolo may be a better reviewer for irq.

Regards,
Andreas

> 
> -michael
> 
>> ---
>>  tests/libqtest.c | 19 +++
>>  tests/libqtest.h | 40 
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index 098af6aec4..85e1f6f92a 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -40,6 +40,7 @@ struct QTestState
>>  int fd;
>>  int qmp_fd;
>>  bool irq_level[MAX_IRQ];
>> +    bool irq_latch[MAX_IRQ];
>>  GString *rx;
>>  pid_t qemu_pid;  /* our child QEMU process */
>>  bool big_endian;
>> @@ -233,6 +234,7 @@ QTestState *qtest_init_without_qmp_handshake(bool
>> use_oob,
>>  s->rx = g_string_new("");
>>  for (i = 0; i < MAX_IRQ; i++) {
>>  s->irq_level[i] = false;
>> +    s->irq_latch[i] = false;
>>  }
>>
>>  if (getenv("QTEST_STOP")) {
>> @@ -386,6 +388,7 @@ redo:
>>
>>  if (strcmp(words[1], "raise") == 0) {
>>  s->irq_level[irq] = true;
>> +    s->irq_latch[irq] = true;
>>  } else {
>>  s->irq_level[irq] = false;
>>  }
>> @@ -678,6 +681,22 @@ bool qtest_get_irq(QTestState *s, int num)
>>  return s->irq_level[num];
>>  }
>>
>> +bool qtest_get_irq_latched(QTestState *s, int num)
>> +{
>> +    g_assert_cmpint(num, <, MAX_IRQ);
>> +
>> +    /* dummy operation in order to make sure irq is up to date */
>> +    qtest_inb(s, 0);
>> +
>> +    return s->irq_latch[num];
>> +}
>> +
>> +void qtest_clear_irq_latch(QTestState *s, int num)
>> +{
>> +    g_assert_cmpint(num, <, MAX_IRQ);
>> +    s->irq_latch[num] = false;
>> +}
>> +
>>  static int64_t qtest_clock_rsp(QTestState *s)
>>  {
>>  gchar **words;
>> diff --git a/tests/libqtest.h b/tests/libqtest.h
>> index ac52872cbe..721dd50863 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -192,6 +192,24 @@ char *qtest_hmpv(QTestState *s, const char *fmt,
>> va_list ap);
>>  bool qtest_get_irq(QTestState *s, int num);
>>
>>  /**
>> + * qtest_get_irq_latched:
>> + * @s: #QTestState instance to operate on.
>> + * @num: Interrupt to observe.
>> + *
>> + * Returns: The latched state of the @num interrupt.
>> + */
>> +bool qtest_get_irq_latched(QTestState *s, int num);
>> +
>> +/**
>> + * qtest_clear_irq_latch:
>> + * @s: #QTestState instance to operate on.
>> + * @num: Interrupt to operate on.
>> + *
>> + * Clears the latch of the @num interrupt.
>> + */
>> +void qtest_clear_irq_latch(QTestState *s, int num);
>> +
>> +/**
>>   * qtest_irq_intercept_in:
>>   * @s: #QTestState instance to operate on.
>>   * @string: QOM path of a device.
>> @@ -638,6 +656,28 @@ static inline bool get_irq(int num)
>>  }
>>
>>  /**
>> + * get_irq_latched:
>> + * @num: Interrupt to observe.
>> + *
>> + * Returns: The latched level of the @num interrupt.
>> + */
>> +static inline bool get_irq_latched(int num)
>> +{
>> +    return qtest_get_irq_latched(global_qtest, num);
>> +}
>> +
>> +/**
>> + * clear_irq_latch:
>> + * @num: Interrupt to operate on.
>> + *
>> + * Clears the latch of the @num interrupt.
>> + */
>> +static inline void clear_irq_latch(int num)
>> +{
>> +    return qtest_clear_irq_latch(global_qtest, num);
>> +}
>> +
>> +/**
>>   * irq_intercept_in:
>>   * @string: QOM path of a device.
>>   *


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] hw/qdev-core: Fix description of instance_init

[Andreas Färber]

Am 10.09.18 um 10:09 schrieb Thomas Huth:
> The part of the documentation of DeviceClass that talks about instance_init
> is partly wrong: instance_init() functions must not abort or exit, since
> the function is also called during introspection of the device already.
> So if a device calls exit() during its instance_init() function, QEMU
> terminates unexpectedly if somebody tries to just have a look at the
> interfaces from the device with "device_add xyz,help" or with the
> "device-list-properties" QOM command. This should never happen.
> 
> Signed-off-by: Thomas Huth 

Reviewed-by: Andreas Färber 

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Andreas Färber]

Am 13.07.2018 um 23:16 schrieb Eduardo Habkost:
> I wonder if we should deprecate object_initialize() and support
> only object_initialize_child() later.  Initializing an object
> contained inside another one without making it a child of the
> parent object is a recipe for trouble.

The root container object needs to be initialized, too.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2] Show values and description when using "qom-list"

[Andreas Färber]

Am 08.06.2018 um 11:41 schrieb Dr. David Alan Gilbert:
> * Andreas Färber (afaer...@suse.de) wrote:
>> Am 01.06.2018 um 17:39 schrieb Ricardo Perez Blanco:
>>> For debugging purposes it is very useful to:
>>>  - See the description of the field. This information is already filled
>>>in but not shown in "qom-list" command.
>>
>> No objection on this part.
>>
>>>  - Display value of the field.
>>
>> That is by definition the qom-get operation, not qom-list. Just like the
>> ls command does not show file contents, there's cat etc. for that. For
>> debugging purposes we had a qom-tree (?) command that would combine
>> both.
> 
> I'm not too bothered about distinguishing between the two commands;
> but it would be nice - one reason I'm not too bothered is because we've
> failed to get a qom-get in multiple years of trying.
> 
> 
>>   There might be unmerged patches on qemu-devel related to display
>> of certain data types.
> 
> Which ones?

My original qom-info series needed StringOutputVisitor changes for enums
(test case: rtc) that did not get accepted immediately and thus some
part of HMP qom-info/qom-get got stuck due to risking assertions for
qom-info / otherwise; QMP was not affected IIRC.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2] Show values and description when using "qom-list"

[Andreas Färber]

Am 01.06.2018 um 17:39 schrieb Ricardo Perez Blanco:
> For debugging purposes it is very useful to:
>  - See the description of the field. This information is already filled
>in but not shown in "qom-list" command.

No objection on this part.

>  - Display value of the field.

That is by definition the qom-get operation, not qom-list. Just like the
ls command does not show file contents, there's cat etc. for that. For
debugging purposes we had a qom-tree (?) command that would combine
both. There might be unmerged patches on qemu-devel related to display
of certain data types.

Regards,
Andreas

> 
> Signed-off-by: Ricardo Perez Blanco 
> ---
>  hmp.c  | 13 +++--
>  qapi/misc.json |  6 --
>  qmp.c  |  7 +++
>  qom/object.c   |  8 +++-
>  4 files changed, 25 insertions(+), 9 deletions(-)
[snip]

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 35/38] qom: Add MMU_DEBUG_LOAD

[Andreas Färber]

Hi,

Since you're specifically CC'ing me, some nits:

Am 29.12.2017 um 07:31 schrieb Richard Henderson:
> This lets us tell bottom levels of virtual memory translation
> routines that the access is from within QEMU itself and bypass
> certain tests.

This sentence could use a tweak for clarity: tell bottom levels of
virtual memory translation routines ... to bypass? or that the access
... bypasses? or that this lets us ... bypass?

> 
> Cc: Andreas Färber <afaer...@suse.de>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
>  include/qom/cpu.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index c2fa151228..d5361ffd0e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -64,7 +64,8 @@ typedef uint64_t vaddr;
>  typedef enum MMUAccessType {
>  MMU_DATA_LOAD  = 0,
>  MMU_DATA_STORE = 1,
> -MMU_INST_FETCH = 2
> +MMU_INST_FETCH = 2,
> +MMU_DEBUG_LOAD = 3

Given that you had to touch the previous line for your comma, it would
be advisable to add one on the new line to avoid that next time.

>  } MMUAccessType;
>  
>  typedef struct CPUWatchpoint CPUWatchpoint;

Either way please take the patch through a suitable tree.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 20/50] qom: Introduce CPUClass.tcg_initialize

[Andreas Färber]

Am 18.10.2017 um 01:53 schrieb Emilio G. Cota:
> On Mon, Oct 16, 2017 at 10:25:39 -0700, Richard Henderson wrote:
>> Move target cpu tcg initialization to common code,
>> called from cpu_exec_realizefn.
>>
>> Cc: Andreas Färber <afaer...@suse.de>
>> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> 
> Much cleaner!
> 
> Reviewed-by: Emilio G. Cota <c...@braap.org>

Looks like a good approach,

Acked-by: Andreas Färber <afaer...@suse.de>

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Andreas Färber]

Hi,

Am 25.09.2017 um 10:14 schrieb Peter Xu:
> On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:
>> Am 25.09.2017 um 08:37 schrieb Peter Xu:
>>> We have object_get_objects_root() to keep user created objects, however
>>> no place for objects that will be used internally.  Create such a
>>> container for internal objects.
>>>
>>> CC: Andreas Färber <afaer...@suse.de>
>>> CC: Markus Armbruster <arm...@redhat.com>
>>> CC: Paolo Bonzini <pbonz...@redhat.com>
>>> Suggested-by: Daniel P. Berrange <berra...@redhat.com>
>>> Signed-off-by: Peter Xu <pet...@redhat.com>
>>> ---
>>>  include/qom/object.h | 10 ++
>>>  qom/object.c |  5 +
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>> index f3e5cff..f567052 100644
>>> --- a/include/qom/object.h
>>> +++ b/include/qom/object.h
>>> @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
>>>  Object *object_get_objects_root(void);
>>>  
>>>  /**
>>> + * object_get_internal_root:
>>> + *
>>> + * Get the container object that holds internally used object
>>> + * instances. This is the object at path "/internal-objects"
>>> + *
>>> + * Returns: the internal object container
>>> + */
>>> +Object *object_get_internal_root(void);
>>> +
>>> +/**
>>>   * object_get_canonical_path_component:
>>>   *
>>>   * Returns: The final component in the object's canonical path.  The 
>>> canonical
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 3e18537..857cee7 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
>>>  return container_get(object_get_root(), "/objects");
>>>  }
>>>  
>>> +Object *object_get_internal_root(void)
>>> +{
>>> +return container_get(object_get_root(), "/internal-objects");
>>
>> Whatever you expose in the QOM tree is no longer internal. Other name?
> 
> Hi, Andreas,
> 
> If you mean "info qom-tree" here, can we still treat it as internal?
> Since after all it's not exposed in QMP (while IMHO QMP is the
> official protocol for QEMU clients).  And IIUC some HMP commands do
> dump some internal structs for debugging purpose (like: "info
> ramblock").
> 
> Or, do you have any suggestion?
> 
> (I did think about something like "hidden-objects", but I believe they
>  are not hidden as well if we think they are not internal...)

I'm on travels and only seeing this 1/4 without context - according to
Manos apparently something about IOThreads.

The reason that certain container groups such as "/machine/peripheral"
exist was more of a legacy reason for migration from qdev, so yes I am
critical of "/hidden-objects" as well. If the objects are not related to
an existing device, you could just place them somewhere outside
"/machine", e.g. directly in the root node or in a container specific to
that use case (/io? /threads?), rather than a new generic bucket of
whatever name. Any objects not under /machine should be close to what
you consider internal.

Note that "info qom-tree" is not the only way to query the objects,
there's some Python QMP scripts as well.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Andreas Färber]

Am 25.09.2017 um 08:37 schrieb Peter Xu:
> We have object_get_objects_root() to keep user created objects, however
> no place for objects that will be used internally.  Create such a
> container for internal objects.
> 
> CC: Andreas Färber <afaer...@suse.de>
> CC: Markus Armbruster <arm...@redhat.com>
> CC: Paolo Bonzini <pbonz...@redhat.com>
> Suggested-by: Daniel P. Berrange <berra...@redhat.com>
> Signed-off-by: Peter Xu <pet...@redhat.com>
> ---
>  include/qom/object.h | 10 ++
>  qom/object.c |  5 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3e5cff..f567052 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
>  Object *object_get_objects_root(void);
>  
>  /**
> + * object_get_internal_root:
> + *
> + * Get the container object that holds internally used object
> + * instances. This is the object at path "/internal-objects"
> + *
> + * Returns: the internal object container
> + */
> +Object *object_get_internal_root(void);
> +
> +/**
>   * object_get_canonical_path_component:
>   *
>   * Returns: The final component in the object's canonical path.  The 
> canonical
> diff --git a/qom/object.c b/qom/object.c
> index 3e18537..857cee7 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
>  return container_get(object_get_root(), "/objects");
>  }
>  
> +Object *object_get_internal_root(void)
> +{
> +return container_get(object_get_root(), "/internal-objects");

Whatever you expose in the QOM tree is no longer internal. Other name?

Regards,
Andreas

> +}
> +
>  static void object_get_child_property(Object *obj, Visitor *v,
>const char *name, void *opaque,
>Error **errp)
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 14/28] m68k: replace cpu_m68k_init() with cpu_generic_init()

[Andreas Färber]

Am 17.07.2017 um 12:41 schrieb Igor Mammedov:
> On Sat, 15 Jul 2017 08:08:58 -1000
> Richard Henderson  wrote:
> 
>> On 07/14/2017 03:52 AM, Igor Mammedov wrote:
>>> @@ -230,6 +230,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error 
>>> **errp)
>>>   M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
>>>   Error *local_err = NULL;
>>>   
>>> +register_m68k_insns(>env);
>>> +  
>>
>> I think it would make more sense to do this during m68k_tcg_init.
>>
> it seems that m68k_cpu_initfn accesses 'env' via some global,
> while cpu_mk68k_init() used to access concrete pointer of just created cpu,
> 
> how about moving register_m68k_insns() to m68k_cpu_initfn(), instead?
> it should be equivalent to what cpu_mk68k_init() used to do.

As a general note, realize should be re-entrant. Can't tell from the
above diff whether that is the case here.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/7] qom: Make link property API public

[Andreas Färber]

Am 28.06.2017 um 15:32 schrieb Paolo Bonzini:
> On 28/06/2017 14:48, Fam Zheng wrote:
>> The get/set pair and the struct will be reused by qdev link prop, make
>> them public.
>>
>> Signed-off-by: Fam Zheng 
> 
> Maybe it's better to make it a separate header file.  No other objections.

So you think it should be added?

The alternative would be moving info qom-tree forward and finally
deprecating info qtree. There was a patch of mine to make it list
properties with -v or so, but there were StringOutputVisitor
dependencies where it got stuck in review.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Andreas Färber]

Am 10.02.2017 um 19:18 schrieb Andrew Jones:
> On Fri, Feb 10, 2017 at 05:16:59PM +0100, Igor Mammedov wrote:
>> On Fri, 10 Feb 2017 17:31:33 +0200
>> "Michael S. Tsirkin"  wrote:
>>
>>> On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:
 On 02/05/17 10:11, b...@skyportsystems.com wrote:  
> From: Ben Warren 
>> [...]
>>

 - or else, add another boolean property to vmgenid, one that parallels
 "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
 realize() when this property is false.  
>>>
>>> That's probably the easiest way. x-fw-cfg-dma-enabled.
>>> Won't delay merging because of this, can be done with
>>> patch on top.
>> (not related to this series)
>>
>> I've thought that there still were no consensus on x-foo prefix,
>> not to mention that x- might be legitimate prefix for some properties.
>>
>> Maybe we should add a flag to property like INTERNAL_PROPERTY
>> and then set it explicitly on for internal stuff.
>>
>> That way we could cleanly exclude internal properties from
>>  -device foo,help and make sure that user won't set them from CLI.
>> I'd even volunteer to add this API to Object
> 
> Yes, please. I know of a property or two where it would be nice to
> have that flag.

Apart from documentation, what effect would such a flag have?

With QOM I don't really see "internal" as being a thing: Besides -device
and the likes, we expose qom-set operation and -global option, and I
don't think it makes sense to restrict the latter two. For -device,
"realized" is a property I would classify as non-user maybe.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/3] cleanup: remove not used header

[Andreas Färber]

Am 03.01.2017 um 16:25 schrieb Eric Blake:
> On 01/02/2017 09:44 AM, Igor Mammedov wrote:
> 
> perhaps s/not used/unused/ in the subject

... and qom: instead of cleanup: if this gets re-spun.

Andreas

> 
> Reviewed-by: Eric Blake 
> 
>> Signed-off-by: Igor Mammedov 
>> ---
>>  qom/object_interfaces.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index ded4d84..4b880d0 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -3,7 +3,6 @@
>>  #include "qom/object_interfaces.h"
>>  #include "qemu/module.h"
>>  #include "qapi-visit.h"
>> -#include "qapi/qobject-output-visitor.h"
>>  #include "qapi/opts-visitor.h"
>>  
>>  void user_creatable_complete(Object *obj, Error **errp)
>>
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH for-2.9] qom: Make all interface types abstract

[Andreas Färber]

Am 09.12.2016 um 19:06 schrieb Eduardo Habkost:
> "qom-list-types abstract=false" currently returns all interface
> types, as if they were not abstract. Fix this by making sure all
> interface types are abstract.
> 
> All interface types have instance_size == 0, so we can use
> it to set abstract=true on
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  qom/object.c   |  4 +++
>  tests/device-introspect-test.c | 61 
> +++---
>  2 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 7a05e35..3870c1b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -272,6 +272,10 @@ static void type_initialize(TypeImpl *ti)
>  
>  ti->class_size = type_class_get_size(ti);
>  ti->instance_size = type_object_get_size(ti);
> +/* Any type with zero instance_size is implicitly abstract.
> + * This means interface types are all abstract.
> + */
> +ti->abstract |= ti->instance_size == 0;

IIRC this is a bool field, so I would prefer to avoid |= by using an
old-fashioned if statement.

The ->abstract = false for interfaces itself makes sense to me, but I'd
appreciate confirmation from Paolo or some other interface expert.

>  
>  ti->class = g_malloc0(ti->class_size);
>  
> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index 37debc1..d1b0ea6 100644
> --- a/tests/device-introspect-test.c
> +++ b/tests/device-introspect-test.c
> @@ -20,18 +20,24 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/qdict.h"
>  #include "libqtest.h"
>  
>  const char common_args[] = "-nodefaults -machine none";
>  
> -static QList *device_type_list(bool abstract)
> +static QList *qom_list_types(const char *implements, bool abstract)
>  {
>  QDict *resp;
>  QList *ret;
> +QDict *args = qdict_new();
>  
> +qdict_put(args, "abstract", qbool_from_bool(abstract));
> +if (implements) {
> +qdict_put(args, "implements", qstring_from_str(implements));
> +}
>  resp = qmp("{'execute': 'qom-list-types',"
> -   " 'arguments': {'implements': 'device', 'abstract': %i}}",
> -   abstract);
> +   " 'arguments': %p }", args);
>  g_assert(qdict_haskey(resp, "return"));
>  ret = qdict_get_qlist(resp, "return");
>  QINCREF(ret);
> @@ -39,6 +45,11 @@ static QList *device_type_list(bool abstract)
>  return ret;
>  }
>  
> +static QList *device_type_list(bool abstract)
> +{
> +return qom_list_types("device", abstract);
> +}
> +
>  static void test_one_device(const char *type)
>  {
>  QDict *resp;
> @@ -110,6 +121,48 @@ static void test_device_intro_concrete(void)
>  qtest_end();
>  }
>  
> +static void test_abstract_interfaces(void)
> +{
> +QList *all_types;
> +QList *obj_types;
> +QListEntry *ae;
> +
> +qtest_start(common_args);
> +/* qom-list-types implements=interface would return any type
> + * that implements _any_ interface (not just interface types),
> + * so use a trick to find the interface type names:
> + * - list all object types
> + * - list all types, and look for items that are not
> + *   on the first list
> + */
> +all_types = qom_list_types(NULL, false);
> +obj_types = qom_list_types("object", false);
> +
> +QLIST_FOREACH_ENTRY(all_types, ae) {
> +QDict *at = qobject_to_qdict(qlist_entry_obj(ae));
> +const char *aname = qdict_get_str(at, "name");
> +QListEntry *oe;
> +const char *found = NULL;
> +
> +QLIST_FOREACH_ENTRY(obj_types, oe) {
> +QDict *ot = qobject_to_qdict(qlist_entry_obj(oe));
> +const char *oname = qdict_get_str(ot, "name");
> +if (!strcmp(aname, oname)) {
> +found = oname;
> +break;
> +}
> +}
> +
> +/* Using g_assert_cmpstr() will give more useful failure
> + * messages than g_assert(found) */
> +g_assert_cmpstr(aname, ==, found);
> +}
> +
> +QDECREF(all_types);
> +QDECREF(obj_types);
> +qtest_end();
> +}
> +
>  int main(int argc, char **argv)
>  {
>  g_test_init(, , NULL);
> @@ -119,5 +172,7 @@ int main(int argc, char **argv)
>  qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
>  qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
>  
> +qtest_add_func("qom/introspect/abstract-interfaces", 
> test_abstract_interfaces);

I see why you combine the two for reuse, but this path looks odd, it's
supposed to indicate which testsuite the test comes from.
"device/introspect/abstract-qom-interfaces" maybe?

> +
>  return g_test_run();
>  }

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v4 1/8] tests: check-qom-proplist: Remove duplicate "bv" property

[Andreas Färber]

Am 04.11.2016 um 17:07 schrieb Eduardo Habkost:
> On Fri, Nov 04, 2016 at 04:56:29PM +0100, Andreas Färber wrote:
>> Am 29.10.2016 um 03:48 schrieb Eduardo Habkost:
>>> The object_property_add_bool() call in dummy_init() is always
>>> failing because there is an existing "bv" class property. We need
>>> to remove either the "bv" class property or the "bv" instance
>>> property.
>>>
>>> Remove the class property so both object properties and class
>>> properties are covered by the test code.
>>>
>>> Reviewed-by: Igor Mammedov <imamm...@redhat.com>
>>> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
>>> ---
>>> Changes series v2 -> v3:
>>> * Patch imported from "tests: A few check-qom-proplist fixes"
>>>   series
>>> * Reworded commit message for clarity
>>
>> So I understand this patch was reordered from a previous separate
>> series. Surely Peter did not merge any pulls that cause test failures,
>> so the first paragraph in the commit message needs rewording?
> 
> The object_property_add_bool() call is always failing on current
> master. It doesn't mean the test case is failing (because the
> errors reported by object_property_add_bool() are being silently
> ignored).
> 
> Would it be clearer if I rewrite it as "The object_property_add_bool()
> call in dummy_init() is always failing (but the error is being
> silently ignored) because [...]"?

Thanks, I can make that tweak myself then.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v4 1/8] tests: check-qom-proplist: Remove duplicate "bv" property

[Andreas Färber]

Am 29.10.2016 um 03:48 schrieb Eduardo Habkost:
> The object_property_add_bool() call in dummy_init() is always
> failing because there is an existing "bv" class property. We need
> to remove either the "bv" class property or the "bv" instance
> property.
> 
> Remove the class property so both object properties and class
> properties are covered by the test code.
> 
> Reviewed-by: Igor Mammedov 
> Signed-off-by: Eduardo Habkost 
> ---
> Changes series v2 -> v3:
> * Patch imported from "tests: A few check-qom-proplist fixes"
>   series
> * Reworded commit message for clarity

So I understand this patch was reordered from a previous separate
series. Surely Peter did not merge any pulls that cause test failures,
so the first paragraph in the commit message needs rewording?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 0/6] qdev class properties + abstract class support on device-list-properties

[Andreas Färber]

Am 17.10.2016 um 23:04 schrieb Eduardo Habkost:
> On Tue, Oct 11, 2016 at 05:41:13PM -0300, Eduardo Habkost wrote:
>> Eduardo Habkost (6):
>>   qdev: qdev_class_set_props() function

s/qdev_/device_/?

Regards,
Andreas

>>   qdev: Extract property-default code to qdev_property_set_to_default()
>>   qdev: Register static properties as class properties
>>   qom: object_class_property_iter_init() function
>>   qmp: Support abstract classes on device-list-properties
>>   qdev: Warning about using object_class_property_add() in new code

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 0/2] tests: A few check-qom-proplist fixes

[Andreas Färber]

Am 17.10.2016 um 23:02 schrieb Eduardo Habkost:
> Ping?
> 
> Markus, do you want to merge this through your tree?
> 
> On Tue, Oct 11, 2016 at 09:37:45AM -0300, Eduardo Habkost wrote:
>> A few fixes on check-qom-proplist that will ensure we test both
>> class properties and object properties, and catch errors when
>> registering properties in test code.
>>
>> Eduardo Habkost (2):
>>   tests: check-qom-proplist: Remove "bv" class property from class

This part I didn't quite understand from looking at the patch, but...

>>   tests: check-qom-proplist: Use _abort to catch errors

For this one:

Reviewed-by: Andreas Färber <afaer...@suse.de>

Regards,
Andreas

>>  tests/check-qom-proplist.c | 10 +++---
>>  1 file changed, 3 insertions(+), 7 deletions(-)

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC 0/4] QOM class properties - do we need them?

[Andreas Färber]

Am 29.09.2016 um 12:21 schrieb Daniel P. Berrange:
> On Thu, Sep 29, 2016 at 12:12:32PM +0200, Andreas Färber wrote:
>> Am 29.09.2016 um 10:14 schrieb Daniel P. Berrange:
>>> Practically all instances properties should become class properties
>>> as its going to save wasting memory once most are converted.
>>
>> Not all, but most. child<> properties were the reason to have properties
>> on the instance.
> 
> That's why I said "Practically all", instead of just "all" :-)

To me as non-native speaker "practically" is the opposite of
"theoretically". :)

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC 0/4] QOM class properties - do we need them?

[Andreas Färber]

Am 29.09.2016 um 02:16 schrieb David Gibson:
> is there really any value to supporting the "class"
> properties in addition to the "instance" properties?

Yes, it makes enumerating available properties easier by not requiring
to instantiate a new instance for printing, e.g., ",help" information.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC 0/4] QOM class properties - do we need them?

[Andreas Färber]

Am 29.09.2016 um 10:14 schrieb Daniel P. Berrange:
> On Thu, Sep 29, 2016 at 10:16:41AM +1000, David Gibson wrote:
>> QOM has the concept of both "object class" properties and "object
>> instance" properties.
>>
>> The accessor functions installed for the rarely-used class properties
>> still take an Object *, so the *value* of such properties is still
>> per-instance; it's just the *existence* (and type) of the property
>> that is per-class.
> 
> Yes, of course. This is the whole point of class properties. It avoids
> allocating the same ObjectProperty struct against every object instance
> which wastes massive amounts of memory in scenarios where there are lots
> of instances created.

+1

>> Of course, that's also true in practice for the great majority of
>> "instance" properties, because they're created identically and
>> unconditionally for every instance from the per-class instance_init
>> hook.
>>
>> This also means that the (unused) object_class_property_add_*_ptr()
>> functions don't make a lot of sense, since they require a fixed
>> pointer which means the value of such a property would only be
>> per-class.
>>
>> Given that, is there really any value to supporting the "class"
>> properties in addition to the "instance" properties?  This series is
>> an RFC which removes all support for class properties, changing the
>> few existing users to instance properties instead.
>>
>> Alternatively, if we *don't* want to remove class properties, should
>> we instead be trying to convert the many, many "instance" properties
>> whose existence is actually per-class to be class properties?
> 
> Practically all instances properties should become class properties
> as its going to save wasting memory once most are converted.

Not all, but most. child<> properties were the reason to have properties
on the instance.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 00/18] target-riscv: Add full-system emulation support for the RISC-V Instruction Set Architecture (RV64G, RV32G)

[Andreas Färber]

Am 26.09.2016 um 18:24 schrieb Paolo Bonzini:
> On 26/09/2016 18:20, Andreas Färber wrote:
>> Am 26.09.2016 um 18:17 schrieb Richard Henderson:
>>> On 09/26/2016 05:20 AM, Paolo Bonzini wrote:
>>>> On 26/09/2016 12:56, Sagar Karandikar wrote:
>>>>> -cpu-qom.h merged into cpu.h
>>>>
>>>> Please follow the model of other targets.  RISCVCPUClass and the
>>>> RISCVCPU typedef should be in cpu-qom.h.

After discussion with Paolo, the key word here is typedef. It doesn't
make sense to have the struct RISCVCPU in cpu-qom.h (the old approach)
but it makes perfect sense to have struct RISCVCPUClass and typedef
RISCVCPU in a separate cpu-qom.h.

Sorry for misunderstanding,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 00/18] target-riscv: Add full-system emulation support for the RISC-V Instruction Set Architecture (RV64G, RV32G)

[Andreas Färber]

Am 26.09.2016 um 18:17 schrieb Richard Henderson:
> On 09/26/2016 05:20 AM, Paolo Bonzini wrote:
>> On 26/09/2016 12:56, Sagar Karandikar wrote:
>>> -cpu-qom.h merged into cpu.h
>>
>> Please follow the model of other targets.  RISCVCPUClass and the
>> RISCVCPU typedef should be in cpu-qom.h.
> 
> I thought we had this discussion before, and cpu-qom.h is sort of a
> legacy header, and that new targets shouldn't use it.

Yes, a concept that didn't quite work out, sadly. Just cpu.h is fine.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] steps towards deprecation of old boards and devices

[Andreas Färber]

Am 20.09.2016 um 10:08 schrieb Markus Armbruster:
> Peter Maydell  writes:
> 
>> If we're going to aim for deprecating and eventually removing
>> some of our unmaintained device and board models, it seems to
>> me that a good first step would be to come up with a definition
>> of what our baseline "needs to be at least this good" level is.
>> I'm guessing that ought to include at least "devices are QOM"
>> and "uses vmstate rather than save/load functions".
> 
> Sounds like a start.  We can always refine.
> 
> Qdevified devices that aren't fully QOMified are reasonably easy to
> find: search for init() and exit() methods.
> 
> Non-qdevified devices are harder to find.  Anything that maps memory or
> wires up interrupts might be a device.  If it's done outside QOM
> realize(), chances are it's either wrong or legacy crap.
> 
> In my opinion, legacy crap is much more tolerable when it doesn't have
> any configuration knobs.  See also below.
> 
>> So
>> (a) are there any other things we want to include?
> 
> A few ideas:
> 
> * Anything configurable needs to be configurable with non-legacy means:
>   -machine, -device.
> 
>   Counter-example: a board has serial devices that can only be
>   configured with -serial.  Hmm, almost certainly covered by "devices
>   are QOM" already, but it may still be a useful approach to finding
>   problematic stuff that is actually relevant.
> 
> * A smoke test exists: can boot at least into firmware with generally
>   available bits.  Ideally, the bits are in tree, and the smoke test is
>   run by "make check".  Perhaps too ambitious for the first round, but I
>   think it makes sense.
> 
> * A maintainer exists (d'oh): the machine initialization function is
>   covered by MAINTAINERS.
> 
>> (b) does anybody feel like writing up a helpful wiki page
>> on how to update old devices, that we can point prospective
>> maintainers at?
>>
>> (In particular I would appreciate the documentation on how to
>> write a state-of-the-art QOMified device as I don't really have
>> a good idea myself...)
> 
> I guess the first step is identifying good examples, and examples of
> stuff that needs work.
> 
> Paolo, Andreas, can you point us to some reasonably QOMified devices?

I see Paolo already replied, so just a few more comments.

(Reminds me that I still have some ColdFire QOM conversions from a KVM
Forum session...)

If you want to replace some of the legacy command line options, we need
to finish the work of defining named paths from /machine. Only then (and
when giving all user devices IDs for /machine/peripheral) can you
realistically use qom-set operations for tweaking things in a new way.

Another aspect is that most properties can't be changed any more after
the device is realized. So I would need to finish the deferred
(recursive) realization patchset, for which ordering concerns remained -
we wanted to generate a list of to-be-realized devices and sort them
before starting the realization.

Don't assume PC behavior where -device serial-pci magically replaces the
default device with your customized one, the serial device may be hidden
beneath a Super I/O chipset like on PReP or inside a SoC device.

Similarly we used to have ARM machines where the new -netdev stuff
couldn't be used because the device doesn't get user-added.

One idea once was to extend the by-ID-reference semantics to allow a QOM
path as transparent means of conversion. I don't think we ever
implemented that?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] 答复： Re: [PATCH v2] object: Add 'help' option for all available backends and properties

[Andreas Färber]

Hi Lin and Markus,

Am 19.09.2016 um 13:58 schrieb Markus Armbruster:
> This is about QOM use.  Cc: Andreas in case he has smart ideas.
> Andreas, you may want to skip ahead to "EnumProperty".
> 
> "Lin Ma"  writes:
> 
> Markus Armbruster  2016/9/12 星期一 下午 11:42 >>>
>>> Lin Ma  writes:
>>>
 '-object help' prints available user creatable backends.
 '-object $typename,help' prints relevant properties.

 Signed-off-by: Lin Ma 
 ---
  include/qom/object_interfaces.h |   2 +
  qemu-options.hx |   7 ++-
  qom/object_interfaces.c | 112 
 
  vl.c   |   5 ++
  4 files changed, 125 insertions(+), 1 deletion(-)

> [...]
 diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
 index bf59846..4ee8643 100644
 --- a/qom/object_interfaces.c
 +++ b/qom/object_interfaces.c
 @@ -5,6 +5,7 @@
  #include "qapi-visit.h"
  #include "qapi/qmp-output-visitor.h"
  #include "qapi/opts-visitor.h"
 +#include "qemu/help_option.h"
  
  void user_creatable_complete(Object *obj, Error **errp)
  {
 @@ -212,6 +213,117 @@ void user_creatable_del(const char *id, Error **errp)
  object_unparent(obj);
  }
  
 +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
 +{
 +Visitor *v;
 +char *type = NULL;
 +Error *local_err = NULL;
 +
>>>
>>> Why this blank line?
>>>
>> I'll remove it.
>>
 +int i;
 +char *values = NULL;
 +Object *obj;
 +ObjectPropertyInfoList *props = NULL;
 +ObjectProperty *prop;
 +ObjectPropertyIterator iter;
 +ObjectPropertyInfoList *start;
 +
 +struct EnumProperty {
 +  const char * const *strings;
 +  int (*get)(Object *, Error **);
 +  void (*set)(Object *, int, Error **);
 +} *enumprop;
>>>
>>> Copied from object.c.  Big no-no.  Whatever you do with this probably
>>> belongs into object.c instead.
>>>
>> Yes, this way is ugly.
>> What I want to do is parsing the enum <-> string mappings through the 
>> EnumProperty
>> to make the output more reasonable.
>> It should be parsed in object.c, But I can't assume the size of enum str 
>> list, then can't
>> pre-alloc it in user_creatable_help_func. So far I havn't figured out a good 
>> way to return
>> a string array that including the enum str list to user_creatable_help_func 
>> for printing.
>> May I have your suggestions?
> 
> See below.
> 
 +
 +   
>>  v = opts_visitor_new(opts);
 +visit_start_struct(v, NULL, NULL, 0, _err);
 +if (local_err) {
 +  goto out;
 +}
 +
 +visit_type_str(v, "qom-type", , _err);
 +if (local_err) {
 +  goto out_visit;
 +}
 +
 +if (type && is_help_option(type)) {
>>>
>>> I think the Options visitor is overkill.  Much simpler:
>>>
>>>type = qemu_opt_get(opts, "qom-type");
>>>if (type && is_help_option(type)) {
>>>
>> Yes, it is much simpler, I'll use this instead of visitor code.
>>
 +  printf("Available object backend types:\n");
 +  GSList *list = object_class_get_list(TYPE_USER_CREATABLE, false);
 +  while (list) {
 +  const char *name;
 +  name = object_class_get_name(OBJECT_CLASS(list->data));
 +  if (strcmp(name, TYPE_USER_CREATABLE)) {
>>>
>>> Why do you need to skip TYPE_USER_CREATABLE?
>>>
>>> Hmm...
>>>
>>>$ qemu-system-x86_64 -object user-creatable,id=foo
>>>**
>>>ERROR:/work/armbru/qemu/qom/object.c:361:object_initialize_with_type: 
>>> assertion failed (type->instance_size >= sizeof(Object)): (0 >= 40)
>>>Aborted (core dumped)
>>>
>>> Should this type be abstract?
>>>
>> Yes, The object type user-creatable is abstract, But obviously it missed the 
>> abstract property.
>> I'll add it in next patch(patch 1/2), Then I dont need to skip it at here 
>> anymore.
> 
> Yes, please.
> 
 +  printf("%s\n", name);
 +  }
 +  list = list->next;
 +  }
>>>
>>> Recommend to keep the loop control in one place:
>>>
>>>for (list = object_class_get_list(TYPE_USER_CREATABLE, 
>>> false);
>>> list;
>>> list = list->next) {
>>>...
>>>}
>>>
>>> If you hate multi-line for (...), you can do
>>>
>>>GSList *head = object_class_get_list(TYPE_USER_CREATABLE, 
>>> false);
>>>
>>>for (list = head; list; list->next) {
>>>...
>>>}
>>>
>> Will do it.
>>
 +  g_slist_free(list);
 +  goto out_visit;
 +}
 +
 +if (!type || !qemu_opt_has_help_opt(opts)) {

Re: [Qemu-devel] [PULL v3 6/6] MAINTAINERS: add virtio-* tests

[Andreas Färber]

Am 15.09.2016 um 22:38 schrieb Michael S. Tsirkin:
> From: Greg Kurz 
> 
> Except virtio-9p, all virtio-* tests are orphan. This patch tries to fix
> it, according to the following logic:
> 
> - when the related subsystem has its own section in MAINTAINERS, the test
>   is added there
> - otherwise it is added to the "parent" section (aka. SCSI, Network devices,
>   virtio)
> 
> Signed-off-by: Greg Kurz 
> Reviewed-by: Amit Shah 
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: Michael S. Tsirkin 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  MAINTAINERS | 7 +++
>  1 file changed, 7 insertions(+)

Makes perfect sense, thanks for taking care.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command

[Andreas Färber]

Am 06.09.2016 um 12:18 schrieb Dr. David Alan Gilbert (git):
> From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> 
> This started off as Andreas Färber's implementation from
> March 2015, but after feedback from Paolo morphed into
> using the json output which handles structs reasonably.
> 
> Use with qom-list to find the members of an object.
> 
> (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
> 65536
> (qemu) qom-get /machine smm
> "auto"
> (qemu) qom-get /machine rtc-time
> {
> "tm_year": 116,
> "tm_sec": 0,
> "tm_hour": 9,
> "tm_min": 46,
> "tm_mon": 8,
> "tm_mday": 6
> }
> (qemu) qom-get /machine frob
> Property '.frob' not found
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>

Acked-by: Andreas Färber <afaer...@suse.de>

Thanks for working on this,

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] 答复： Re: [PATCH] object: Add 'help' option to print all available object backend types

[Andreas Färber]

Am 18.08.2016 um 11:57 schrieb Lin Ma:
 Markus Armbruster  2016/8/17 星期三 下午 2:48 >>>
>>Lin Ma  writes:
>>
>>> Signed-off-by: Lin Ma 
>>> ---
>>>  qemu-options.hx |  5 -
>>>  qom/object_interfaces.c | 16 
>>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index a71aaf8..c5f4a12 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -3752,7 +3752,8 @@ DEF("object", HAS_ARG, QEMU_OPTION_object,
>>>  "create a new object of type TYPENAME setting
> properties\n"
>>>  "in the order they are specified.  Note that the
> 'id'\n"
>>>  "property must be set.  These objects are placed
> in the\n"
>>> -"'/objects' path.\n",
>>> +"'/objects' path.\n"
>>> +"Use '-object help' to print available backend
> types.\n",
>>>  QEMU_ARCH_ALL)
>>>  STEXI
>>>  @item -object @var{typename}[,@var{prop1}=@var{value1},...]
>>> @@ -3762,6 +3763,8 @@ in the order they are specified.  Note that the
> 'id'
>>>  property must be set.  These objects are placed in the
>>>  '/objects' path.
>>> 
>>> +Use '-object help' to print available backend types.
>>> +
>>>  @table @option
>>> 
>>>  @item -object
> memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
>>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>>> index bf59846..8f820a4 100644
>>> --- a/qom/object_interfaces.c
>>> +++ b/qom/object_interfaces.c
>>> @@ -58,6 +58,22 @@ Object *user_creatable_add(const QDict *qdict,
>>>  goto out_visit;
>>>  }
>>> 
>>> +if (!strcmp(type, "help")) {
>>
>>Please use is_help_option().
> ok, will do it.

Thanks, otherwise looks good to me. However, I am not really the
maintainer of object_interfaces.c, please check the git log and also CC
Stefan/Paolo or whomever it was for review.

> btw, Should I add the behaviour like -device argument T,help
> to show additional help for type T ?

Not necessarily in this patch, could be done as follow-up.

Cheers,
Andreas

>>> +printf("Available object backend types:\n");
>>> +GSList *list = object_class_get_list(TYPE_USER_CREATABLE,
> false);
>>> +while (list) {
>>> +const char *name;
>>> +name = object_class_get_name(OBJECT_CLASS(list->data));
>>> +/* Ignore user-creatable. */
>>> +if (strcmp(name, TYPE_USER_CREATABLE)) {
>>> +printf("%s\n", name);
>>> +}
>>> +list = list->next;
>>> +}
>>> +g_slist_free(list);
>>> +exit(0);
>>> +}
>>> +
>>>  qdict_del(pdict, "id");
>>>  visit_type_str(v, "id", , _err);
>>>  if (local_err) {

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] QOM: best way for parents to pass information to children? (was Re: [PATCH RFC 07/16] qom/cpu: make nr-cores, nr-threads real properties)

[Andreas Färber]

Am 15.07.2016 um 18:10 schrieb Eduardo Habkost:
> On Fri, Jul 15, 2016 at 11:11:38AM +0200, Igor Mammedov wrote:
>> On Fri, 15 Jul 2016 08:35:30 +0200
>> Andrew Jones  wrote:
>>> On Thu, Jul 14, 2016 at 05:07:43PM -0300, Eduardo Habkost wrote:

 First of all, sorry for the horrible delay in replying to this
 thread.

 On Wed, Jun 15, 2016 at 10:56:20AM +1000, David Gibson wrote:  
> On Tue, Jun 14, 2016 at 08:19:49AM +0200, Andrew Jones wrote:  
>> On Tue, Jun 14, 2016 at 12:12:16PM +1000, David Gibson wrote:  
>>> On Sun, Jun 12, 2016 at 03:48:10PM +0200, Andrew Jones wrote:  
> [...]
>> +static Property cpu_common_properties[] = {
>> +DEFINE_PROP_INT32("nr-cores", CPUState, nr_cores, 1),
>> +DEFINE_PROP_INT32("nr-threads", CPUState, nr_threads, 1),
>> +DEFINE_PROP_END_OF_LIST()
>> +};  
>
> Are you aware of the current CPU hotplug discussion that is going on? 
>  

 I'm aware of it going on, but haven't been following it.
   
> I'm not very involved there, but I think some of these reworks also 
> move
> "nr_threads" into the CPU state already, e.g. see:  

 nr_threads (and nr_cores) are already state in CPUState. This patch 
 just
 exposes that state via properties.
   
>
> https://github.com/dgibson/qemu/commit/9d07719784ecbeebea71
>
> ... so you might want to check these patches first to see whether you
> can base your rework on them?  

 Every cpu, and thus every machine, uses CPUState for its cpus. I'm
 not sure every machine will want to use that new abstract core class
 though. If they did, then we could indeed use nr_threads from there
 instead (and remove it from CPUState), but we'd still need nr_cores
 from the abstract cpu package class (CPUState).  
>>>
>>> Hmm.  Since the CPUState object represents just a single thread, it
>>> seems weird to me that it would have nr_threads and nr_cores
>>> information.  

 Agreed it is weird, and I think we should try to move it away
 from CPUState, not make it part of the TYPE_CPU interface.
 nr_threads belongs to the actual container of the Thread objects,
 and nr_cores in the actual container of the Core objects.

 The problem is how to implement that in a non-intrusive way that
 would require changing the object hierarchy of all architectures.

   
>>>
>>> Exposing those as properties makes that much worse, because it's now
>>> ABI, rather than internal detail we can clean up at some future time.  
>>
>> CPUState is supposed to be "State of one CPU core or thread", which
>> justifies having nr_threads state, as it may be describing a core.  
>
> Um.. does it ever actually represent a (multithread) core in practice?
> It would need to have duplicated register state for every thread were
> that the case.  

 AFAIK, CPUState is still always thread state. Or has this changed
 in some architectures, already?
   
>   
>> I guess there's no justification for having nr_cores in there though.
>> I agree adding the Core class is a good idea, assuming it will get used
>> by all machines, and CPUState then gets changed to a Thread class. The
>> question then, though, is do we also create a Socket class that contains
>> nr_cores?  
>
> That was roughly our intention with the way the cross platform hotplug
> stuff is evolving.  But the intention was that the Socket objects
> would only need to be constructed for machine types where it makes
> sense.  So for example on the paravirt pseries platform, we'll only
> have Core objects, because the socket distinction isn't really
> meaningful.
>   
>> And how will a Thread method get that information when it
>> needs to emulate, e.g. CPUID, that requires it? It's a bit messy, so
>> I'm open to all suggestions on it.  
>
> So, if the Thread needs this information, I'm not opposed to it having
> it internally (presumably populated earlier from the Core object).
> But I am opposed to it being a locked in part of the interface by
> having it as an exposed property.  

 I agree we don't want to make this part of the external
 interface. In this case, if we don't add the properties, how
 exactly is the Machine or Core code supposed to pass that
 information to the Thread object?

 Maybe the intermediate steps could be:

 * Make the Thread code that uses CPUState::nr_{cores,threads} and
   smp_{cores,threads} get that info from MachineState instead.  
>>>
>>> I have some patches already headed down this road.
>>>
 * On the architectures where we already have a reasonable
   

Re: [Qemu-devel] [PATCH] qom: add option -dt-printf wich writes DeviceClass hierarchy in file

[Andreas Färber]

Am 13.07.2016 um 17:30 schrieb goremykin:
> Andreas Färber писал 2016-07-12 18:41:
>> Am 12.07.2016 um 17:33 schrieb goremy...@ispras.ru:
>>> From: goremykin <oleg.goremi...@yandex.ru>
>>>
>>> Option -dt-printf 'file' writes Device Tree (DiveceClass hierarchy) to
>>> 'file' using Json format. This helps developers visualize the hierarchy
>>> of DeviceClass and its properties.
>>>
>>> Signed-off-by: Oleg Goremykin <goremy...@ispras.ru>
>>> ---
>>>  include/qom/qom-dt.h |  22 
>>>  qemu-options.hx  |  16 ++
>>>  qom/Makefile.objs|   1 +
>>>  qom/qom-dt.c | 142
>>> +++
>>>  vl.c |   4 ++
>>>  5 files changed, 185 insertions(+)
>>>  create mode 100644 include/qom/qom-dt.h
>>>  create mode 100644 qom/qom-dt.c
>>
>> Hm, apart from some typos I don't quite like the file placement and
>> implementation. Can't you just implement this as a QMP/HMP command and
>> leave the qom/ dirs untouched? Or maybe even extract that info from an
>> external Python script like qom-list etc. do?
> 
> We are developing automatic boards and device templates creation tools.
> It would be better if I get DeviceClass hierarchy without QEMU running,
> but it's unachievable due to implementation of QEMU device registration.
> External Python script is bad because it requires connection with QEMU.
> This script should establish the connection and follow the QMP protocol.
> In my opinion, it's quite complicated way and brings a lot of overhead.
> Unlike QMP/HMP command dt-printf option terminates before creation of
> board. It requires only start QEMU with this option.

You can already get a list of the available devices from the command
line today.

The hierarchy is internal though and not considered ABI. What is the use
case for exposing it? I would rather not want to freeze those
implementation details as stable.

> Where better to place the code? Maybe /util?

Probably not. For HMP there is hmp.c for example, or qdev-monitor.c.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] qom: add option -dt-printf wich writes DeviceClass hierarchy in file

[Andreas Färber]

Am 12.07.2016 um 18:15 schrieb Peter Maydell:
> On 12 July 2016 at 16:33,   wrote:
>> From: goremykin 
>>
>> Option -dt-printf 'file' writes Device Tree (DiveceClass hierarchy) to
>> 'file' using Json format. This helps developers visualize the hierarchy
>> of DeviceClass and its properties.
> 
> Is there a particular problem with the existing
> "-machine dumpdtb=" support that you're trying
> to address? Once you've dumped the dtb blob you can use
> dtc to convert it to whatever human readable format you
> like that dtc supports.

>From what I understood this is not about dtb but about QOM.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] qom: add option -dt-printf wich writes DeviceClass hierarchy in file

[Andreas Färber]

Am 12.07.2016 um 17:33 schrieb goremy...@ispras.ru:
> From: goremykin 
> 
> Option -dt-printf 'file' writes Device Tree (DiveceClass hierarchy) to
> 'file' using Json format. This helps developers visualize the hierarchy
> of DeviceClass and its properties.
> 
> Signed-off-by: Oleg Goremykin 
> ---
>  include/qom/qom-dt.h |  22 
>  qemu-options.hx  |  16 ++
>  qom/Makefile.objs|   1 +
>  qom/qom-dt.c | 142 
> +++
>  vl.c |   4 ++
>  5 files changed, 185 insertions(+)
>  create mode 100644 include/qom/qom-dt.h
>  create mode 100644 qom/qom-dt.c

Hm, apart from some typos I don't quite like the file placement and
implementation. Can't you just implement this as a QMP/HMP command and
leave the qom/ dirs untouched? Or maybe even extract that info from an
external Python script like qom-list etc. do?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/1] OpenBIOS: Switch over to official OpenBIOS git repo

[Andreas Färber]

Am 07.07.2016 um 23:22 schrieb Jeff Cody:
> This update should preserve git history, and switches over to the official
> openbios git repo, rather than pulling from the svn mirror.  All prior history
> from the svn repository should still be preserved (i.e., commit hashes are the
> same for historical commits).
> 
> The origin/master tag now follows the github hashes, so that means
> users will need to update their local tracking branch:
> 
> e.g.: git reset --hard origin/master

Does that actually update the tracked branch? I would've assumed you
additionally need git branch --set-upstream-to=origin/master.

> 
> If you have local commits or branches to preserver, a rebase may be more

"preserve"

> appropriate:
> 
> e.g. git rebase origin/master
> 
> N.B.: The server-side git changes have already happened, independent of this
>   patch.
> 
> Proposed-by: Mark Cave-Ayland 
> Signed-off-by: Jeff Cody 
> ---
>  roms/openbios | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/roms/openbios b/roms/openbios
> index 422b916..36785d7 16
> --- a/roms/openbios
> +++ b/roms/openbios
> @@ -1 +1 @@
> -Subproject commit 422b916649aa0db8c5edadccb22387b3e807e3b2
> +Subproject commit 36785d7b59726beca320a0b99cfa0a903782c6f3

Otherwise looks good, thanks.

Peter, will you be picking this up?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[Qemu-devel] [PULL 0/1] QOM devices patch queue 2016-07-06

[Andreas Färber]

Hello Peter,

This is my QOM (devices) patch queue. Please pull.

Thanks,
Andreas

Cc: Peter Maydell 

The following changes since commit 791b7d2340cfafcac9af7864343cf23504d57804:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging 
(2016-07-05 16:48:24 +0100)

are available in the git repository at:

  git://github.com/afaerber/qemu-cpu.git tags/qom-devices-for-peter

for you to fetch changes up to ada03a0e8423ef8950e30d216f56a9661a4070e2:

  qom: Fix comment typo (2016-07-06 09:19:35 +0200)


QOM infrastructure fixes and device conversions

* Documentation fix


Changlong Xie (1):
  qom: Fix comment typo

 include/qom/object.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

[Qemu-devel] [PULL 1/1] qom: Fix comment typo

[Andreas Färber]

From: Changlong Xie <xiecl.f...@cn.fujitsu.com>

It's qom_unref, not qdef_unref.

Signed-off-by: Changlong Xie <xiecl.f...@cn.fujitsu.com>
Reviewed-by: Laurent Vivier <lviv...@redhat.com>
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 include/qom/object.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 2f8ac47..5ecc2d1 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -901,7 +901,7 @@ GSList *object_class_get_list(const char *implements_type,
 void object_ref(Object *obj);
 
 /**
- * qdef_unref:
+ * object_unref:
  * @obj: the object
  *
  * Decrease the reference count of a object.  A object cannot be freed as long
-- 
2.6.6

Re: [Qemu-devel] [PATCH] qom/object: fix comment typo

[Andreas Färber]

Am 06.07.2016 um 07:52 schrieb Changlong Xie:
> Would any maintainer pick this one?

Please in the future use just "qom:" in the subject. How to find out?
git log --oneline -- include/qom/object.h

Also please mention the typo in the commit message. Fixing.

Thanks,
Andreas

> 
> On 06/14/2016 03:27 PM, Changlong Xie wrote:
>> Signed-off-by: Changlong Xie 
>> ---
>>   include/qom/object.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 99de539..925c279 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -901,7 +901,7 @@ GSList *object_class_get_list(const char
>> *implements_type,
>>   void object_ref(Object *obj);
>>
>>   /**
>> - * qdef_unref:
>> + * object_unref:
>>* @obj: the object
>>*
>>* Decrease the reference count of a object.  A object cannot be
>> freed as long
>>
> 
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] MAINTAINERS: add Marcel to PCI

[Andreas Färber]

Am 13.06.2016 um 22:07 schrieb Michael S. Tsirkin:
> Marcel is reviewing PCI patches anyway, things will
> be easier if people remember to Cc him.
> 
> Cc: Marcel Apfelbaum 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index df990a8..fe2279e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -780,6 +780,7 @@ F: hw/ipack/
>  
>  PCI
>  M: Michael S. Tsirkin 
> +M: Marcel Apfelbaum 

If he is "just" reviewing you should probably use R: instead.

But either way no objection.

Cheers,
Andreas

>  S: Supported
>  F: include/hw/pci/*
>  F: hw/misc/pci-testdev.c

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[Qemu-devel] [PULL 0/1] QOM devices patch queue 2016-05-25

[Andreas Färber]

Hello Peter,

This is my QOM (devices) patch queue. Please pull.

I've needed to build-fix it twice by now, so if I fixed the #includes wrongly
please pick it up as patch and tweak it or apply a cleanup on top.

Thanks,
Andreas

P.S. I don't seem to have a MAINTAINERS patch to go with it yet, but part of
the discussion prompting the patch was about creating more fine-grained sections
reflecting maintenance reality, so we might want to consider adding a new one?
The intended follow-up was refactoring tree-walking and device reset for s390x.

Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Eduardo Habkost <ehabk...@redhat.com>
Cc: David Hildenbrand <d...@linux.vnet.ibm.com>

The following changes since commit 287db79df8af8e31f18e262feb5e05103a09e4d4:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into 
staging (2016-05-24 13:06:33 +0100)

are available in the git repository at:

  git://github.com/afaerber/qemu-cpu.git tags/qom-devices-for-peter

for you to fetch changes up to 8cce06115b6f69e4596e9f81455140759cca8c9d:

  qdev: Start disentangling bus from device (2016-05-25 23:24:35 +0200)


QOM infrastructure fixes and device conversions

* Start splitting up qdev.c

------------
Andreas Färber (1):
  qdev: Start disentangling bus from device

 hw/core/Makefile.objs |   1 +
 hw/core/bus.c | 251 ++
 hw/core/qdev.c| 222 
 3 files changed, 252 insertions(+), 222 deletions(-)
 create mode 100644 hw/core/bus.c

[Qemu-devel] [PULL 1/1] qdev: Start disentangling bus from device

[Andreas Färber]

Move bus type and related APIs to a separate file bus.c.
This is a first step in breaking up qdev.c into more manageable chunks.

Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
[AF: Rebased onto osdep.h]
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 hw/core/Makefile.objs |   1 +
 hw/core/bus.c | 251 ++
 hw/core/qdev.c| 222 
 3 files changed, 252 insertions(+), 222 deletions(-)
 create mode 100644 hw/core/bus.c

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 70951d4..82a9ef8 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,5 +1,6 @@
 # core qdev-related obj files, also used by *-user:
 common-obj-y += qdev.o qdev-properties.o
+common-obj-y += bus.o
 common-obj-y += fw-path-provider.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
diff --git a/hw/core/bus.c b/hw/core/bus.c
new file mode 100644
index 000..3e3f8ac
--- /dev/null
+++ b/hw/core/bus.c
@@ -0,0 +1,251 @@
+/*
+ *  Dynamic device configuration and creation -- buses.
+ *
+ *  Copyright (c) 2009 CodeSourcery
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/qdev.h"
+#include "qapi/error.h"
+
+static void qbus_set_hotplug_handler_internal(BusState *bus, Object *handler,
+  Error **errp)
+{
+
+object_property_set_link(OBJECT(bus), OBJECT(handler),
+ QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
+}
+
+void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, Error 
**errp)
+{
+qbus_set_hotplug_handler_internal(bus, OBJECT(handler), errp);
+}
+
+void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
+{
+qbus_set_hotplug_handler_internal(bus, OBJECT(bus), errp);
+}
+
+int qbus_walk_children(BusState *bus,
+   qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+   qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+   void *opaque)
+{
+BusChild *kid;
+int err;
+
+if (pre_busfn) {
+err = pre_busfn(bus, opaque);
+if (err) {
+return err;
+}
+}
+
+QTAILQ_FOREACH(kid, >children, sibling) {
+err = qdev_walk_children(kid->child,
+ pre_devfn, pre_busfn,
+ post_devfn, post_busfn, opaque);
+if (err < 0) {
+return err;
+}
+}
+
+if (post_busfn) {
+err = post_busfn(bus, opaque);
+if (err) {
+return err;
+}
+}
+
+return 0;
+}
+
+static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
+{
+const char *typename = object_get_typename(OBJECT(bus));
+BusClass *bc;
+char *buf;
+int i, len, bus_id;
+
+bus->parent = parent;
+
+if (name) {
+bus->name = g_strdup(name);
+} else if (bus->parent && bus->parent->id) {
+/* parent device has id -> use it plus parent-bus-id for bus name */
+bus_id = bus->parent->num_child_bus;
+
+len = strlen(bus->parent->id) + 16;
+buf = g_malloc(len);
+snprintf(buf, len, "%s.%d", bus->parent->id, bus_id);
+bus->name = buf;
+} else {
+/* no id -> use lowercase bus type plus global bus-id for bus name */
+bc = BUS_GET_CLASS(bus);
+bus_id = bc->automatic_ids++;
+
+len = strlen(typename) + 16;
+buf = g_malloc(len);
+len = snprintf(buf, len, "%s.%d", typename, bus_id);
+for (i = 0; i < len; i++) {
+buf[i] = qemu_tolower(buf[i]);
+}
+bus->name = buf;
+}
+
+if (bus->parent) {
+QLIST_INSERT_HEAD(>parent->child_bus, bus, sibling);
+bus->parent->num_child_bus++;
+object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), 
NULL);
+object_unref(OBJECT(bus));
+} else if (bus != sysbus_get_default()) {
+/* TODO: once all bus devices are qdevified,
+   only reset handler for main_system_bus should be registered

[Qemu-devel] [PULL 1/4] MAINTAINERS: Drop Andreas as Cocoa maintainer

[Andreas Färber]

From: Andreas Färber <andreas.faer...@web.de>

Peter has taken over Cocoa maintainership.

Signed-off-by: Andreas Färber <andreas.faer...@web.de>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 81e7fac..87ad14f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1105,7 +1105,6 @@ F: ui/
 F: include/ui/
 
 Cocoa graphics
-M: Andreas Färber <andreas.faer...@web.de>
 M: Peter Maydell <peter.mayd...@linaro.org>
 S: Odd Fixes
 F: ui/cocoa.m
-- 
2.6.6

[Qemu-devel] [PULL 4/4] MAINTAINERS: Drop Andreas as CPU maintainer

[Andreas Färber]

Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 77bef88..3700d51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1046,7 +1046,7 @@ S: Supported
 F: scripts/coverity-model.c
 
 CPU
-M: Andreas Färber <afaer...@suse.de>
+L: qemu-devel@nongnu.org
 S: Supported
 F: qom/cpu.c
 F: include/qom/cpu.h
-- 
2.6.6

[Qemu-devel] [PULL 2/4] MAINTAINERS: Drop Andreas as PReP maintainer

[Andreas Färber]

From: Andreas Färber <andreas.faer...@web.de>

Signed-off-by: Andreas Färber <andreas.faer...@web.de>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 87ad14f..988ce3a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -597,7 +597,7 @@ F: hw/pci-host/grackle.c
 F: hw/misc/macio/
 
 PReP
-M: Andreas Färber <andreas.faer...@web.de>
+L: qemu-devel@nongnu.org
 L: qemu-...@nongnu.org
 S: Odd Fixes
 F: hw/ppc/prep.c
-- 
2.6.6

[Qemu-devel] [PULL 0/4] Cut down on Andreas' maintainer responsibilities

[Andreas Färber]

Hello Peter et al.,

In a desperate attempt to restore sanity to my inbox and my personal well-being
I am pulling the plug on most of my maintainer responsibilities:

1) I am no longer able to test nor interested in Cocoa patches. Peter has
taken this over and dropped ppc64 support but did not yet drop me.

2) I have not actively worked on PReP for a long time. My last patches
to refactor Super I/O for Markus got stuck (floppy, isa-serial, etc.)
and probably no longer apply due to their large code movements.
Someone picking up that series will be appreciated.

3) We never fully figured out the workflow for maintaining stable branches
post-mdroth, so those activities have ceased and newer releases are no longer
being documented in MAINTAINERS anyway.

4) I have been unable and unwilling to spend as much time on the QOM CPUs
as some people are expecting of me, nor am I using KVM much myself these days
for testing, and I don't want to block hot-plug work.

That leaves the core QOM code, which was never exclusively me, so I'm hoping
we can distribute that well enough over the various trees.

I may contribute to some of those areas, but please stop CC'ing me on huge 
series,
including areas that I was never maintainer for, or putting time pressure on me.
If you have concrete questions for me, please ask me directly or I am likely
to just ignore patches. But for now after weeks of illness I am taking a break 
and
will be unavailable as of tomorrow for the next two weeks.

I also recall having uint64 visitor fixes from ~2.5 somewhere; someone picking
them up would be appreciated, I think it was only minor requests for a respin.

Regards,
Andreas

Cc: Peter Maydell <peter.mayd...@linaro.org>

Cc: Thomas Huth <th...@redhat.com>
Cc: Eduardo Habkost <ehabk...@redhat.com>
Cc: Aleksandar Markovic <aleksandar.marko...@imgtec.com>

Cc: Markus Armbruster <arm...@redhat.com>

Cc: Alexander Graf <ag...@suse.de>
Cc: Bruce Rogers <brog...@suse.com>

The following changes since commit 287db79df8af8e31f18e262feb5e05103a09e4d4:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into 
staging (2016-05-24 13:06:33 +0100)

are available in the git repository at:

  git://github.com/afaerber/qemu-cpu.git tags/maintainers-for-peter

for you to fetch changes up to 12b0e69cd71f3f71bacb51ed7dc71393ec384da6:

  MAINTAINERS: Drop Andreas as CPU maintainer (2016-05-25 17:44:15 +0200)


Andreas stepping down from most maintainer positions

------------
Andreas Färber (4):
  MAINTAINERS: Drop Andreas as Cocoa maintainer
  MAINTAINERS: Drop Andreas as PReP maintainer
  MAINTAINERS: Drop Andreas as 0.15 maintainer
  MAINTAINERS: Drop Andreas as CPU maintainer

 MAINTAINERS | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

[Qemu-devel] [PULL 3/4] MAINTAINERS: Drop Andreas as 0.15 maintainer

[Andreas Färber]

Downgrade to orphan status, like all other remaining stable entries.

Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 MAINTAINERS | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 988ce3a..77bef88 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1399,9 +1399,8 @@ S: Orphan
 
 Stable 0.15
 L: qemu-sta...@nongnu.org
-M: Andreas Färber <afaer...@suse.de>
 T: git git://git.qemu-project.org/qemu-stable-0.15.git
-S: Supported
+S: Orphan
 
 Stable 0.14
 L: qemu-sta...@nongnu.org
-- 
2.6.6

Re: [Qemu-devel] [for-2.7 PATCH v3 00/15] Core based CPU hotplug for PowerPC sPAPR

[Andreas Färber]

Am 25.05.2016 um 08:54 schrieb Thomas Huth:
> On 12.05.2016 05:48, Bharata B Rao wrote:
>> Hi,
>>
>> This is v3 of "Core based CPU hotplug for PowerPC sPAPR". The hotplug
>> semantics looks like this:
>>
>> (qemu) device_add POWER8E-spapr-cpu-core,id=core2,core=16[,threads=4]
>> (qemu) device_add POWER8E_v2.1-spapr-cpu-core,id=core2,core=16[,threads=4]
> 
> *ping*
> 
> Andreas, could you please, please review this series? At least the
> generic patches (patch 1-6 and patch 13-15)?

No. (And I still don't see any qtest additions in the diffstat.)

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 06/15] target-i386: Set constant model_id for qemu63/qemu32/athlon

[Andreas Färber]

qemu64

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[Qemu-devel] [PULL 1/1] MAINTAINERS: Drop target-i386 from CPU subsystem

[Andreas Färber]

X86CPU QOM type is in good hands and actively maintained these days, so
drop it from the generic QOM CPU subsystem.

Some refactorings and design questions will still intersect, but review
and discussions of individual series can still take place while opting out
of general X86CPU patch review.

Acked-by: Eduardo Habkost <ehabk...@redhat.com>
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1c30dfa..28e2b17 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1050,7 +1050,6 @@ M: Andreas Färber <afaer...@suse.de>
 S: Supported
 F: qom/cpu.c
 F: include/qom/cpu.h
-F: target-i386/cpu.c
 
 ICC Bus
 M: Igor Mammedov <imamm...@redhat.com>
-- 
2.6.6

[Qemu-devel] [PULL 0/1] QOM CPUState patch queue 2016-04-18

[Andreas Färber]

Hello Peter,

This is my QOM CPU patch queue. Please pull.

Regards,
Andreas

Cc: Peter Maydell <peter.mayd...@linaro.org>

The following changes since commit 6a6fa68ae2884cc1834110e549faa9cd86050ce6:

  Merge remote-tracking branch 'remotes/mcayland/tags/qemu-openbios-signed' 
into staging (2016-04-18 11:55:10 +0100)

are available in the git repository at:

  git://github.com/afaerber/qemu-cpu.git tags/qom-cpu-for-peter

for you to fetch changes up to 2e4cad283372c8e7e2db1fd4b0c6f2cb0b9122fb:

  MAINTAINERS: Drop target-i386 from CPU subsystem (2016-04-18 18:14:52 +0200)


QOM CPUState and X86CPU

* MAINTAINERS cleanup

----
Andreas Färber (1):
  MAINTAINERS: Drop target-i386 from CPU subsystem

 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

Re: [Qemu-devel] [PATCH] qdev_try_create(): Assert that devices we put onto the system bus are SysBusDevices

[Andreas Färber]

Am 16.03.2016 um 15:11 schrieb Peter Maydell:
> If qdev_try_create() is passed NULL for the bus, it will automatically
> put the newly created device onto the default system bus. However
> if the device is not actually a SysBusDevice then this will result
> in later crashes (for instance when running the monitor "info qtree"
> command) because code reasonably assumes that all devices on the system
> bus are system bus devices.
> 
> Generally the mistake is that the calling code should create the
> object with object_new(TYPE_FOO) rather than qdev_create(NULL, TYPE_FOO);
> see commit 6749695eaaf346c1 for an example of fixing this bug.
> 
> Assert in qdev_try_create() if the device isn't suitable to put on
> the system bus, so that this mistake results in failure earlier
> and more reliably.
> 
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> This needs to go in after http://patchwork.ozlabs.org/patch/597716/
> as otherwise the bug fixed by that patch will become a 'make check'
> failure.

Looks strange, but okay,

Reviewed-by: Andreas Färber <afaer...@suse.de>

Through whose queue?

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC PATCH v0 0/6] Core based CPU hotplug for PowerPC sPAPR

[Andreas Färber]

Am 01.03.2016 um 11:00 schrieb Bharata B Rao:
> On Thu, Feb 25, 2016 at 09:52:36PM +0530, Bharata B Rao wrote:
>> Hi,
>>
>> This is an attempt to implement CPU hotplug for PowerPC sPAPR based on
>> the approach suggested by Andreas. While I say that, I should also explicitly
>> add that I have tried to follow Andreas' suggestions to the best of my
>> understanding and hence there could be bits which are still not
>> as per expectations.
>>
>> I have tried to model this similarly to what Andreas did for x86 an year 
>> back at
>> https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04858.html
> 
> Andreas - Do you have any comments on this implementation ? Is it worth
> pursuing further in your view ?

As indicated to some of you, I was absent last week - that means I did
not yet read the flood of emails I got from David, Igor and others.

Maybe you can give a summary of open questions and problems on the call?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 1/7] s390x/cpu: Cleanup init in preparation for hotplug

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> Ensure a valid cpu_model is set upfront by setting the
> default value directly into the MachineState when none is
> specified.  This is needed to ensure hotplugged CPUs share
> the same cpu_model.
> 
> Signed-off-by: Matthew Rosato 
> Reviewed-by: David Hildenbrand 
> ---
>  hw/s390x/s390-virtio-ccw.c | 2 +-
>  hw/s390x/s390-virtio.c | 8 
>  hw/s390x/s390-virtio.h | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 89f5d0d..b05ed8b 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -136,7 +136,7 @@ static void ccw_init(MachineState *machine)
>  virtio_ccw_register_hcalls();
>  
>  /* init CPUs */
> -s390_init_cpus(machine->cpu_model);
> +s390_init_cpus(machine);
>  
>  if (kvm_enabled()) {
>  kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index c320878..b576811 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -95,12 +95,12 @@ void s390_init_ipl_dev(const char *kernel_filename,
>  qdev_init_nofail(dev);
>  }
>  
> -void s390_init_cpus(const char *cpu_model)
> +void s390_init_cpus(MachineState *machine)
>  {
>  int i;
>  
> -if (cpu_model == NULL) {
> -cpu_model = "host";
> +if (machine->cpu_model == NULL) {
> +machine->cpu_model = "host";

When/why is cpu_model == NULL? Could you simply set it as a default in
your machine's instance_init?

Regards,
Andreas

>  }
>  
>  ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> @@ -109,7 +109,7 @@ void s390_init_cpus(const char *cpu_model)
>  S390CPU *cpu;
>  CPUState *cs;
>  
> -cpu = cpu_s390x_init(cpu_model);
> +cpu = cpu_s390x_init(machine->cpu_model);
>  cs = CPU(cpu);
>  
>  ipi_states[i] = cpu;
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index eebce8e..ffd014c 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -19,7 +19,7 @@
>  typedef int (*s390_virtio_fn)(const uint64_t *args);
>  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
>  
> -void s390_init_cpus(const char *cpu_model);
> +void s390_init_cpus(MachineState *machine);
>  void s390_init_ipl_dev(const char *kernel_filename,
> const char *kernel_cmdline,
> const char *initrd_filename,
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 3/7] s390x/cpu: Move some CPU initialization into realize

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> In preparation for hotplug, defer some CPU initialization
> until the device is actually being realized.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>
> ---
>  target-s390x/cpu.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Looks reasonable on a brief sight,

Reviewed-by: Andreas Färber <afaer...@suse.de>

What is env->cpu_num used for? In particular, have you thought about
linux-user creating multiple CPUs and possibly destroying them again?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 6/7] cpu: Add a last_cpu macro

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> Add last_cpu to grab last CPU in the queue.  Rename one existing
> use of last_cpu as a variable name.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/intc/openpic.c | 12 ++--
>  include/qom/cpu.h |  1 +
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 903888c..0dd908e 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -217,7 +217,7 @@ typedef struct IRQSource {
>  uint32_t ivpr;  /* IRQ vector/priority register */
>  uint32_t idr;   /* IRQ destination register */
>  uint32_t destmask; /* bitmap of CPU destinations */
> -int last_cpu;
> +int cpu_last;

If we do need to rename this, what about last_cpu_index? cpu_last reads
really ugly.

>  int output; /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
>  int pending;/* TRUE if IRQ is pending */
>  IRQType type;
> @@ -476,9 +476,9 @@ static void openpic_update_irq(OpenPICState *opp, int 
> n_IRQ)
>  return;
>  }
>  
> -if (src->destmask == (1 << src->last_cpu)) {
> +if (src->destmask == (1 << src->cpu_last)) {
>  /* Only one CPU is allowed to receive this IRQ */
> -IRQ_local_pipe(opp, src->last_cpu, n_IRQ, active, was_active);
> +IRQ_local_pipe(opp, src->cpu_last, n_IRQ, active, was_active);
>  } else if (!(src->ivpr & IVPR_MODE_MASK)) {
>  /* Directed delivery mode */
>  for (i = 0; i < opp->nb_cpus; i++) {
> @@ -488,13 +488,13 @@ static void openpic_update_irq(OpenPICState *opp, int 
> n_IRQ)
>  }
>  } else {
>  /* Distributed delivery mode */
> -for (i = src->last_cpu + 1; i != src->last_cpu; i++) {
> +for (i = src->cpu_last + 1; i != src->cpu_last; i++) {
>  if (i == opp->nb_cpus) {
>  i = 0;
>  }
>  if (src->destmask & (1 << i)) {
>  IRQ_local_pipe(opp, i, n_IRQ, active, was_active);
> -src->last_cpu = i;
> +src->cpu_last = i;
>  break;
>  }
>  }
> @@ -1444,7 +1444,7 @@ static const VMStateDescription 
> vmstate_openpic_irqsource = {
>  VMSTATE_UINT32(ivpr, IRQSource),
>  VMSTATE_UINT32(idr, IRQSource),
>  VMSTATE_UINT32(destmask, IRQSource),
> -VMSTATE_INT32(last_cpu, IRQSource),
> +VMSTATE_INT32(cpu_last, IRQSource),

This name change shows up in the VMState description, e.g. in the JSON.
Migration will still work, but we should avoid it for debugging
cross-version migration.

>  VMSTATE_INT32(pending, IRQSource),
>  VMSTATE_END_OF_LIST()
>  }
[actual change snipped]

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs

[Andreas Färber]

Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
> Implement cpu hotplug routine and add the machine hook.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/s390x/s390-virtio-ccw.c | 33 +
>  target-s390x/cpu.c |  7 +++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3090e76..ea007e8 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -22,6 +22,7 @@
>  #include "s390-pci-bus.h"
>  #include "hw/s390x/storage-keys.h"
>  #include "hw/compat.h"
> +#include "qom/cpu.h"
>  
>  #define TYPE_S390_CCW_MACHINE   "s390-ccw-machine"
>  
> @@ -185,6 +186,37 @@ static HotplugHandler 
> *s390_get_hotplug_handler(MachineState *machine,
>  return NULL;
>  }
>  
> +static void s390_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +MachineState *machine = MACHINE(qdev_get_machine());
> +
> +if (id < 0) {
> +error_setg(errp, "Invalid CPU id: %" PRIi64, id);
> +return;
> +}
> +
> +if (cpu_exists(id)) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", it already exists", id);
> +return;
> +}
> +
> +if (id >= max_cpus) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", max allowed: %d", id, max_cpus - 1);
> +return;
> +}
> +
> +if (id != (last_cpu->cpu_index + 1)) {

This assumption about the order of the CPU list looks dangerous to me.
Aren't there global int fields used by x86 already for the number of
CPUs that you could use instead? That will allow you to drop the ugly
preceding renaming patch. Please avoid messing with the CPU list
directly from target code.

> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", The next available id is %d", id,
> +   (last_cpu->cpu_index + 1));
> +return;
> +}
> +
> +cpu_s390x_init(machine->cpu_model);

No proper error handling - that's why blindly reusing these old helpers
is a bad idea.

Regards,
Andreas

> +}
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>  MachineClass *mc = MACHINE_CLASS(oc);
> @@ -193,6 +225,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void 
> *data)
>  
>  mc->init = ccw_init;
>  mc->reset = s390_machine_reset;
> +mc->hot_add_cpu = s390_hot_add_cpu;
>  mc->block_default_type = IF_VIRTIO;
>  mc->no_cdrom = 1;
>  mc->no_floppy = 1;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 8dfd063..1a4977e 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -32,6 +32,7 @@
>  #include "trace.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "hw/s390x/sclp.h"
>  #endif
>  
>  #define CR0_RESET   0xE0UL
> @@ -211,6 +212,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  #endif
>  
>  scc->parent_realize(dev, errp);
> +
> +#if !defined(CONFIG_USER_ONLY)
> +if (dev->hotplugged) {
> +raise_irq_cpu_hotplug();
> +}
> +#endif
>  }
>  
>  static void s390_cpu_initfn(Object *obj)
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC PATCH v0 4/8] spapr: Introduce CPU core device

[Andreas Färber]

Am 22.02.2016 um 08:47 schrieb David Gibson:
> On Mon, Feb 22, 2016 at 07:44:40AM +0100, Andreas Färber wrote:
>> Am 22.02.2016 um 06:01 schrieb Bharata B Rao:
>>> sPAPR CPU core device is a container of CPU thread devices. CPU hotplug is
>>> performed in the granularity of CPU core device by setting the "realized"
>>> property of this device to "true". When hotplugged, CPU core creates CPU
>>> thread devices.
>>>
>>> TODO: Right now allows for only homogeneous configurations as we depend
>>> on global smp_threads and machine->cpu_model.
>>>
>>> Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com>
>>> ---
>>>  hw/ppc/Makefile.objs   |  1 +
>>>  hw/ppc/spapr_cpu_package.c | 50 
>>> ++
>>>  include/hw/ppc/spapr_cpu_package.h | 26 
>>>  3 files changed, 77 insertions(+)
>>>  create mode 100644 hw/ppc/spapr_cpu_package.c
>>>  create mode 100644 include/hw/ppc/spapr_cpu_package.h
>>>
>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>> index c1ffc77..3000982 100644
>>> --- a/hw/ppc/Makefile.objs
>>> +++ b/hw/ppc/Makefile.objs
>>> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>>>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>>>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>>> +obj-$(CONFIG_PSERIES) += spapr_cpu_package.o
>>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>>  obj-y += spapr_pci_vfio.o
>>>  endif
>>> diff --git a/hw/ppc/spapr_cpu_package.c b/hw/ppc/spapr_cpu_package.c
>>> new file mode 100644
>>> index 000..3120a16
>>> --- /dev/null
>>> +++ b/hw/ppc/spapr_cpu_package.c
>>> @@ -0,0 +1,50 @@
>>> +/*
>>> + * sPAPR CPU package device, acts as container of CPU thread devices.
>>> + *
>>> + * Copyright (C) 2016 Bharata B Rao <bhar...@linux.vnet.ibm.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +#include "hw/cpu/package.h"
>>> +#include "hw/ppc/spapr_cpu_package.h"
>>> +#include "hw/boards.h"
>>> +#include 
>>> +#include "qemu/error-report.h"
>>> +
>>> +static void spapr_cpu_package_instance_init(Object *obj)
>>> +{
>>> +int i;
>>> +CPUState *cpu;
>>> +MachineState *machine = MACHINE(qdev_get_machine());
>>> +sPAPRCPUPackage *package = SPAPR_CPU_PACKAGE(obj);
>>> +
>>> +/* Create as many CPU threads as specified in the topology */
>>> +for (i = 0; i < smp_threads; i++) {
>>> +cpu = cpu_generic_init(machine->cpu_type, machine->cpu_model);
>>
>> No, no, no. This is horribly violating QOM design.
> 
> Ok.. why?  There does not, to me, seem to be any remotely easily
> discoverable means of finding out what QOM design principles are.
> 
> It also would have been nice if you weighed in on my RFC this code is
> based on.

It would've been nice had you joined our KVM call prompted by _your_
suggestions (which again you could've discussed at KVM Forum where I had
a session specifically for discussing this topic), but you didn't.
I did discuss several aspects on the call and in the recorded session
and will not write it by email again. Feel free to put it on the call
agenda again.

I told Bharata that you can't have a base type that suddenly mutates its
topology, therefore we discussed the possibility of having a QOM
_interface_, not a base type as apparently done here. We deliberately do
not have multi-inheritence in QOM; there's not just ppc in the world.

My time for reading list mails is limited. Make it easy for me to
understand what the difference is between your package and my core and
why instead of reusing my posted cpu-core type you need to propose your
own cpu-package type. In one point you are right, we keep going in
circles and sometimes backwards rather than forward here.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC PATCH v0 4/8] spapr: Introduce CPU core device

[Andreas Färber]

Am 22.02.2016 um 09:05 schrieb Bharata B Rao:
> On Mon, Feb 22, 2016 at 07:44:40AM +0100, Andreas Färber wrote:
>> Am 22.02.2016 um 06:01 schrieb Bharata B Rao:
>>> sPAPR CPU core device is a container of CPU thread devices. CPU hotplug is
>>> performed in the granularity of CPU core device by setting the "realized"
>>> property of this device to "true". When hotplugged, CPU core creates CPU
>>> thread devices.
>>>
>>> TODO: Right now allows for only homogeneous configurations as we depend
>>> on global smp_threads and machine->cpu_model.
>>>
>>> Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com>
>>> ---
>>>  hw/ppc/Makefile.objs   |  1 +
>>>  hw/ppc/spapr_cpu_package.c | 50 
>>> ++
>>>  include/hw/ppc/spapr_cpu_package.h | 26 
>>>  3 files changed, 77 insertions(+)
>>>  create mode 100644 hw/ppc/spapr_cpu_package.c
>>>  create mode 100644 include/hw/ppc/spapr_cpu_package.h
>>>
>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>> index c1ffc77..3000982 100644
>>> --- a/hw/ppc/Makefile.objs
>>> +++ b/hw/ppc/Makefile.objs
>>> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>>>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>>>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>>> +obj-$(CONFIG_PSERIES) += spapr_cpu_package.o
>>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>>  obj-y += spapr_pci_vfio.o
>>>  endif
>>> diff --git a/hw/ppc/spapr_cpu_package.c b/hw/ppc/spapr_cpu_package.c
>>> new file mode 100644
>>> index 000..3120a16
>>> --- /dev/null
>>> +++ b/hw/ppc/spapr_cpu_package.c
>>> @@ -0,0 +1,50 @@
>>> +/*
>>> + * sPAPR CPU package device, acts as container of CPU thread devices.
>>> + *
>>> + * Copyright (C) 2016 Bharata B Rao <bhar...@linux.vnet.ibm.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +#include "hw/cpu/package.h"
>>> +#include "hw/ppc/spapr_cpu_package.h"
>>> +#include "hw/boards.h"
>>> +#include 
>>> +#include "qemu/error-report.h"
>>> +
>>> +static void spapr_cpu_package_instance_init(Object *obj)
>>> +{
>>> +int i;
>>> +CPUState *cpu;
>>> +MachineState *machine = MACHINE(qdev_get_machine());
>>> +sPAPRCPUPackage *package = SPAPR_CPU_PACKAGE(obj);
>>> +
>>> +/* Create as many CPU threads as specified in the topology */
>>> +for (i = 0; i < smp_threads; i++) {
>>> +cpu = cpu_generic_init(machine->cpu_type, machine->cpu_model);
>>
>> No, no, no. This is horribly violating QOM design.
>>
>> Please compare the x86 RFC.
> 
> Are you referring to the in-place initialization of child objects like
> that you did in socket based x86 hotplug RFC last year ?

Yes, in-place object_initialize() is one of two options. The other is
using object_initialize() with g_try_malloc0() from property setters,
like Alexey did for xics I think.

But mainly the idea is to not bend your QOM types to reuse the legacy
glue but to use true QOM concepts in the design of those new objects.

If there is some big hurdle I'm missing, please say so openly.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC PATCH v0 0/8] cpu-package hotplug

[Andreas Färber]

Hi Bharata,

Am 22.02.2016 um 06:01 schrieb Bharata B Rao:
> This is an attempt to implement David Gibson's RFC that was posted at
> https://lists.gnu.org/archive/html/qemu-ppc/2016-02/msg0.html
> I am not sure if I have followed all the aspects of the RFC fully, but we
> can make changes going forward.

I am not familiar with David's RFC beyond what was portrayed on the KVM
call - this is not what we discussed on the call and I don't like it.

Further, your commits are pretty cryptic to me. Please improve your
commit messages.

For example, you add a cpu_type field and you assign it the value
TYPE_POWERPC_CPU. That's not the user-chosen CPU type then, it's a base
CPU type that cannot be instantiated. Either name it cpu_base_type or
fill it in with proper values in one patch - that patch on its own does
not create value and does not explain your claim:
"Storing CPU typename in MachineState lets us to create CPU threads
for all architectures in uniform manner from arch-neutral code."
I'm pretty sure that CPU threads cannot be created from that type, as it
would run into an assertion.

Next, you make a functionally correct refactoring of cpu_generic_init(),
but I don't understand why you duplicate that code. cpu_foo_init() still
expects things to be realized, so instead of realizing once in a central
place you do it in nine different places. Had you touched all helper
functions we might be able to move that to three places, once for
softmmu, once or twice for linux-user and once for bsd-user. But I
rather get the feeling that you misunderstand those legacy helper
functions, they're for -cpu handling and not to my knowledge used for
cpu-add at all. You should not be using them and then won't need to
touch them in this way. By using them in your supposedly QOM code you
are hiding an object_new() call inside deep layers of helper functions
instead of using QOM native functions such as object_initialize(),
object_new() and object_property_set*().

Is "CPU package" some IBM sPAPR term? It is new to me and does not match
-smp precedence, so I really don't think we should be forcing that term
on all architectures for no good reason.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC PATCH v0 4/8] spapr: Introduce CPU core device

[Andreas Färber]

Am 22.02.2016 um 06:01 schrieb Bharata B Rao:
> sPAPR CPU core device is a container of CPU thread devices. CPU hotplug is
> performed in the granularity of CPU core device by setting the "realized"
> property of this device to "true". When hotplugged, CPU core creates CPU
> thread devices.
> 
> TODO: Right now allows for only homogeneous configurations as we depend
> on global smp_threads and machine->cpu_model.
> 
> Signed-off-by: Bharata B Rao 
> ---
>  hw/ppc/Makefile.objs   |  1 +
>  hw/ppc/spapr_cpu_package.c | 50 
> ++
>  include/hw/ppc/spapr_cpu_package.h | 26 
>  3 files changed, 77 insertions(+)
>  create mode 100644 hw/ppc/spapr_cpu_package.c
>  create mode 100644 include/hw/ppc/spapr_cpu_package.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..3000982 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_package.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/spapr_cpu_package.c b/hw/ppc/spapr_cpu_package.c
> new file mode 100644
> index 000..3120a16
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_package.c
> @@ -0,0 +1,50 @@
> +/*
> + * sPAPR CPU package device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2016 Bharata B Rao 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/cpu/package.h"
> +#include "hw/ppc/spapr_cpu_package.h"
> +#include "hw/boards.h"
> +#include 
> +#include "qemu/error-report.h"
> +
> +static void spapr_cpu_package_instance_init(Object *obj)
> +{
> +int i;
> +CPUState *cpu;
> +MachineState *machine = MACHINE(qdev_get_machine());
> +sPAPRCPUPackage *package = SPAPR_CPU_PACKAGE(obj);
> +
> +/* Create as many CPU threads as specified in the topology */
> +for (i = 0; i < smp_threads; i++) {
> +cpu = cpu_generic_init(machine->cpu_type, machine->cpu_model);

No, no, no. This is horribly violating QOM design.

Please compare the x86 RFC.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Andreas Färber]

Am 18.02.2016 um 10:56 schrieb Markus Armbruster:
> Alistair Francis  writes:
> 
>> If the device being added when running qdev_device_add() has
>> a reset function, register it so that it can be called.
>>
>> Signed-off-by: Alistair Francis 
>> ---
>>
>>  qdev-monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81e3ff3..0a99d01 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>  
>>  if (bus) {
>>  qdev_set_parent_bus(dev, bus);
>> +} else if (dc->reset) {
>> +qemu_register_reset((void (*)(void *))dc->reset, dev);
>>  }
>>  
>>  id = qemu_opts_id(opts);
> 
> This looks wrong to me.
> 
> You stuff all the device reset methods into the global reset_handlers
> list, where they get called in some semi-random order.  This breaks when
> there are reset order dependencies between devices, e.g. between a
> device and the bus it plugs into.
> 
> Propagating the reset signal to all the devices is a qdev problem.
> Copying Andreas for further insight.

We had a similar discussion for s390x, and I had started a big reset
refactoring, but we agreed to go for a stop-gap solution for 2.5. The
recently posted hw/core/bus.c refactoring originated from that branch.

The overall idea was that for buses reset propagates along buses (so
yes, NACK to this patch), but where no bus exists it shall propagate to
QOM children, too.

So Alistair, if you have a device that needs a reset while not sitting
on a bus, please register the register hook in your device's realize
hook for now.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QMP: add query-hotpluggable-cpus

[Andreas Färber]

Am 16.02.2016 um 13:35 schrieb Markus Armbruster:
> Igor Mammedov  writes:
> 
>> On Mon, 15 Feb 2016 20:43:41 +0100
>> Markus Armbruster  wrote:
>>
>>> Igor Mammedov  writes:
>>>
 it will allow mgmt to query present and possible to hotplug CPUs
 it is required from a target platform that wish to support
 command to set board specific MachineClass.possible_cpus() hook,
 which will return a list of possible CPUs with options
 that would be needed for hotplugging possible CPUs.

 For RFC there are:
'arch_id': 'int' - mandatory unique CPU number,
   for x86 it's APIC ID for ARM it's MPIDR
'type': 'str' - CPU object type for usage with device_add

 and a set of optional fields that would allows mgmt tools
 to know at what granularity and where a new CPU could be
 hotplugged;
 [node],[socket],[core],[thread]
 Hopefully that should cover needs for CPU hotplug porposes for
 magor targets and we can extend structure in future adding
 more fields if it will be needed.

 also for present CPUs there is a 'cpu_link' field which
 would allow mgmt inspect whatever object/abstraction
 the target platform considers as CPU object.

 For RFC purposes implements only for x86 target so far.  
>>>
>>> Adding ad hoc queries as we go won't scale.  Could this be solved by a
>>> generic introspection interface?
>> Do you mean generic QOM introspection?
> 
> Possibly, but I don't want to prematurely limit the conversation to QOM
> introspection.
> 
>> Using QOM we could have '/cpus' container and create QOM links
>> for exiting (populated links) and possible (empty links) CPUs.
>> However in that case link's name will need have a special format
>> that will convey an information necessary for mgmt to hotplug
>> a CPU object, at least:
>>   - where: [node],[socket],[core],[thread] options
>>   - optionally what CPU object to use with device_add command
> 
> Encoding information in names feels wrong.
> 
>> Another approach to do QOM introspection would be to model hierarchy 
>> of objects like node/socket/core..., That's what Andreas
>> worked on. Only it still suffers the same issue as above
>> wrt introspection and hotplug, One can pre-create empty
>> [nodes][sockets[cores]] containers at startup but then
>> leaf nodes that could be hotplugged would be a links anyway
>> and then again we need to give them special formatted names
>> (not well documented at that mgmt could make sense of).
>> That hierarchy would need to become stable ABI once
>> mgmt will start using it and QOM tree is quite unstable
>> now for that. For some targets it involves creating dummy
>> containers like node/socket/core for x86 where just modeling
>> a thread is sufficient.
> 
> I acknowledge your concern regarding QOM tree stability.  We have QOM
> introspection commands since 1.2.  They make the QOM tree part of the
> external interface, but we've never spelled out which parts of it (if
> any) are ABI.  Until we do, parts become de facto ABI by being used in
> anger.  As a result, we don't know something's ABI until it breaks.
> 
> Andreas, do you have an opinion on proper use of QOM by external
> software?

This is absolutely untrue, there have been ABI rules in place and I held
a presentation covering them in 2012...

Andreas

> 
>> The similar but a bit more abstract approach was suggested
>> by David https://lists.gnu.org/archive/html/qemu-ppc/2016-02/msg0.html
> 
> Cc'ing him.  If I understand the high-level idea correctly, David
> proposes to have an abstract type cpu-package with generic properties.
> Its concrete subtypes are composed of whatever components make up the
> hot-pluggable unit.
> 
> Management software can then use the generic properties to deal with hot
> plug without having to know about the concrete subtypes, at least to
> some useful degree.
> 
> Similarly, the generic properties suffice for implementing generic
> high-level interfaces like -smp.
> 
> David, is that a fair summary?
> 
> Naturally, we need a way to introspect available subtypes of cpu-package
> to answer questions like what concrete types can actually be plugged
> into this board.
> 
> This could be an instance of the generic QOM introspection question
> "what can plug into this socket"?  Unfortunately, I don't know enough
> QOM to put that into more concrete terms.  Andreas, Paolo, can you help
> out?
> 
>> Benefit of dedicated CPU hotplug focused QMP command is that
>> it can be quite abstract to suite most targets and not depend
>> on how a target models CPUs internally and still provide
>> information needed for hotplugging a CPU object.
>> That way we can split efforts on how we model/refactor CPUs
>> internally and how mgmt would work with them using
>> -device/device_add.
> 
> CPUs might be special enough to warrant special commands.  Nevertheless,
> non-special solutions 

Re: [Qemu-devel] [PATCH 1/1] hyperv: cpu hotplug fix with HyperV enabled

[Andreas Färber]

Am 11.02.2016 um 21:19 schrieb Denis V. Lunev:
> From: "Alexey V. Kostyushko" <alek...@virtuozzo.com>
> 
> With Hyper-V enabled CPU hotplug stops working. The CPU appears in device
> manager on Windows but does not appear in peformance monitor and control
> panel.
> 
> The root of the problem is the following. Windows checks
> HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE bit in CPUID. The presence of
> this bit is enough to cure the situation.
> 
> Add option 'hv-cpuhotplug' to control this behavior.
> 
> Signed-off-by: Alexey V. Kostyushko <alek...@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <d...@openvz.org>
> CC: Paolo Bonzini <pbonz...@redhat.com>
> CC: Richard Henderson <r...@twiddle.net>
> CC: Eduardo Habkost <ehabk...@redhat.com>
> CC: "Andreas Färber" <afaer...@suse.de>
> ---
>  target-i386/cpu-qom.h | 1 +
>  target-i386/cpu.c | 1 +
>  target-i386/kvm.c | 6 +-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 5f9d960..4aec616 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -96,6 +96,7 @@ typedef struct X86CPU {
>  bool hyperv_runtime;
>  bool hyperv_synic;
>  bool hyperv_stimer;
> +bool hyperv_cpuhotplug;
>  bool check_cpuid;
>  bool enforce_cpuid;
>  bool expose_kvm;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b255644..32c38ae 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3172,6 +3172,7 @@ static Property x86_cpu_properties[] = {
>  DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false),
>  DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false),
>  DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false),
> +DEFINE_PROP_BOOL("hv-cpuhotplug", X86CPU, hyperv_cpuhotplug, false),

Is "cpuhotplug" some fixed HyperV name? Otherwise we generally use a
dashes convention for QOM properties, i.e. "hv-cpu-hotplug".

Regards,
Andreas

>  DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
>  DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>  DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
[snip]

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PATCH] qdev: Start disentangling bus from device

[Andreas Färber]

Move bus type and related APIs to a separate file bus.c.
This is a first step in breaking up qdev.c into more manageable chunks.

Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 Here's a first step in breaking up qdev.c, originally prepared as part of my
 QOM device reset refactoring. Amazingly it still applied.
 
 hw/core/Makefile.objs |   1 +
 hw/core/bus.c | 248 ++
 hw/core/qdev.c| 222 
 3 files changed, 249 insertions(+), 222 deletions(-)
 create mode 100644 hw/core/bus.c

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index abb3560..b4f88e3 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,5 +1,6 @@
 # core qdev-related obj files, also used by *-user:
 common-obj-y += qdev.o qdev-properties.o
+common-obj-y += bus.o
 common-obj-y += fw-path-provider.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
diff --git a/hw/core/bus.c b/hw/core/bus.c
new file mode 100644
index 000..7294f91
--- /dev/null
+++ b/hw/core/bus.c
@@ -0,0 +1,248 @@
+/*
+ *  Dynamic device configuration and creation -- buses.
+ *
+ *  Copyright (c) 2009 CodeSourcery
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/qdev.h"
+
+static void qbus_set_hotplug_handler_internal(BusState *bus, Object *handler,
+  Error **errp)
+{
+
+object_property_set_link(OBJECT(bus), OBJECT(handler),
+ QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
+}
+
+void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, Error 
**errp)
+{
+qbus_set_hotplug_handler_internal(bus, OBJECT(handler), errp);
+}
+
+void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
+{
+qbus_set_hotplug_handler_internal(bus, OBJECT(bus), errp);
+}
+
+int qbus_walk_children(BusState *bus,
+   qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
+   qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
+   void *opaque)
+{
+BusChild *kid;
+int err;
+
+if (pre_busfn) {
+err = pre_busfn(bus, opaque);
+if (err) {
+return err;
+}
+}
+
+QTAILQ_FOREACH(kid, >children, sibling) {
+err = qdev_walk_children(kid->child,
+ pre_devfn, pre_busfn,
+ post_devfn, post_busfn, opaque);
+if (err < 0) {
+return err;
+}
+}
+
+if (post_busfn) {
+err = post_busfn(bus, opaque);
+if (err) {
+return err;
+}
+}
+
+return 0;
+}
+
+static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
+{
+const char *typename = object_get_typename(OBJECT(bus));
+BusClass *bc;
+char *buf;
+int i, len, bus_id;
+
+bus->parent = parent;
+
+if (name) {
+bus->name = g_strdup(name);
+} else if (bus->parent && bus->parent->id) {
+/* parent device has id -> use it plus parent-bus-id for bus name */
+bus_id = bus->parent->num_child_bus;
+
+len = strlen(bus->parent->id) + 16;
+buf = g_malloc(len);
+snprintf(buf, len, "%s.%d", bus->parent->id, bus_id);
+bus->name = buf;
+} else {
+/* no id -> use lowercase bus type plus global bus-id for bus name */
+bc = BUS_GET_CLASS(bus);
+bus_id = bc->automatic_ids++;
+
+len = strlen(typename) + 16;
+buf = g_malloc(len);
+len = snprintf(buf, len, "%s.%d", typename, bus_id);
+for (i = 0; i < len; i++) {
+buf[i] = qemu_tolower(buf[i]);
+}
+bus->name = buf;
+}
+
+if (bus->parent) {
+QLIST_INSERT_HEAD(>parent->child_bus, bus, sibling);
+bus->parent->num_child_bus++;
+object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus), 
NULL);
+object_unref(OBJECT(bus));
+} else if (bus != sysbus_get_default()) {
+/* TODO: once all bus devices are qdevified,
+   only reset handler for main_system_bus should be registered here. */
+qemu_register_reset(qbus_reset_all_fn

Re: [Qemu-devel] [PATCH 1/1] hyperv: cpu hotplug fix with HyperV enabled

[Andreas Färber]

Am 12.02.2016 um 12:08 schrieb Denis V. Lunev:
> On 02/12/2016 02:00 PM, Andreas Färber wrote:
>> Am 11.02.2016 um 21:19 schrieb Denis V. Lunev:
>>> From: "Alexey V. Kostyushko" <alek...@virtuozzo.com>
>>>
>>> With Hyper-V enabled CPU hotplug stops working. The CPU appears in
>>> device
>>> manager on Windows but does not appear in peformance monitor and control
>>> panel.
>>>
>>> The root of the problem is the following. Windows checks
>>> HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE bit in CPUID. The presence of
>>> this bit is enough to cure the situation.
>>>
>>> Add option 'hv-cpuhotplug' to control this behavior.
>>>
>>> Signed-off-by: Alexey V. Kostyushko <alek...@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <d...@openvz.org>
>>> CC: Paolo Bonzini <pbonz...@redhat.com>
>>> CC: Richard Henderson <r...@twiddle.net>
>>> CC: Eduardo Habkost <ehabk...@redhat.com>
>>> CC: "Andreas Färber" <afaer...@suse.de>
>>> ---
>>>   target-i386/cpu-qom.h | 1 +
>>>   target-i386/cpu.c | 1 +
>>>   target-i386/kvm.c | 6 +-
>>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
>>> index 5f9d960..4aec616 100644
>>> --- a/target-i386/cpu-qom.h
>>> +++ b/target-i386/cpu-qom.h
>>> @@ -96,6 +96,7 @@ typedef struct X86CPU {
>>>   bool hyperv_runtime;
>>>   bool hyperv_synic;
>>>   bool hyperv_stimer;
>>> +bool hyperv_cpuhotplug;
>>>   bool check_cpuid;
>>>   bool enforce_cpuid;
>>>   bool expose_kvm;
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index b255644..32c38ae 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -3172,6 +3172,7 @@ static Property x86_cpu_properties[] = {
>>>   DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false),
>>>   DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false),
>>>   DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false),
>>> +DEFINE_PROP_BOOL("hv-cpuhotplug", X86CPU, hyperv_cpuhotplug,
>>> false),
>> Is "cpuhotplug" some fixed HyperV name? Otherwise we generally use a
>> dashes convention for QOM properties, i.e. "hv-cpu-hotplug".
>>
>> Regards,
>> Andreas
> This name is for libvirt. We can take one one. This is not a problem.
> 
> Roman Kagan has proposed verbally a bit different approach.
> He suggests not to introduce the option but
> check here that HyperV is enabled (any single option is enough
> to face the problem) and CPU hotplug is enabled and automatically
> enable this bit.
> 
> Paolo, Andreas, is there any opinion on this?

That implicit proposal sounds even more appealing to me, yes.

You could still additionally do a dynamic property though, in case you
want to inspect or toggle it at runtime (no clue when Windows reads it).

Andreas

> As far as I can see for the time being we lend such decisions to
> libvirt. But this bit does not require any processing.
> 
> Den


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] qdev & hw/core owner? (was Re: [PATCH v19 7/9] machine: add properties to compat_props incrementaly)

[Andreas Färber]

Am 12.02.2016 um 10:17 schrieb Marcel Apfelbaum:
> On 02/11/2016 09:41 PM, Eduardo Habkost wrote:
>> On Fri, Feb 05, 2016 at 09:51:07AM +0200, Marcel Apfelbaum wrote:
>>> On 02/05/2016 09:49 AM, Markus Armbruster wrote:
>>>> "Michael S. Tsirkin" <m...@redhat.com> writes:
>>>>
>>>>> On Thu, Feb 04, 2016 at 12:55:22PM +0100, Paolo Bonzini wrote:
>>>>>>
>>>>>>
>>>>>> On 04/02/2016 12:41, Andreas Färber wrote:
>>>>>>> You're talking about machine, right? Some time ago I had proposed
>>>>>>> Marcel
>>>>>>> who initially worked on it, but I'm fine with anyone taking it.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> For some (but not all) core qdev parts related to the (stalled) QOM
>>>>>>> migration I've been taking care of via qom-next. Last time this
>>>>>>> came up
>>>>>>> you didn't want anyone to be M: for qdev, so maybe we can use R:
>>>>>>> so that
>>>>>>> at least people automatically get CC'ed and we avoid this recurring
>>>>>>> discussion?
>>>>>>
>>>>>> I might have changed my mind on that.  You definitely should be M:
>>>>>> for qdev.
>>>>>>
>>>>>> Paolo
>>>>>
>>>>> If Andreas wants to, that's also fine. Several maintainers are
>>>>> better than one.
>>>>
>>>> *If* the maintainers are all willing and able to work together.
>>>>
>>>
>>> No problem here from my point of view :)
>>
>> No problem to me, too. :)
>>
>> I am going to be away from work for 15 days starting on Tuesday
>> Feb 16th. So if Marcel wants to start queueing patches already,
>> please be my guest. I will be able to help on that after I'm
>> back.
>>
> 
> Hi,
> 
> If there are only a few patches on the mailing list, they can wait.
> If the number will grow I'll send a pull request.
> 
> So the MAINTAINER file should look like this, right?
> 
> Regarding qdev, Andreas, I also think you are the most qualified
> to take it, will you?
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2d6ee17..a86491a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1200,6 +1200,13 @@ F: docs/*qmp-*
>  F: scripts/qmp/
>  T: git git://repo.or.cz/qemu/armbru.git qapi-next
> 
> +Machine
> +M: Eduardo Habkost <ehabk...@redhat.com>
> +M: Marcel Apfelbaum <mar...@redhat.com>
> +S: Supported
> +F: hw/core/machine.c
> +F: include/hw/boards.h
> +

Fine with me, ack.

For qdev.c itself I prefer not to create a misleading "QDev" section but
rather just proposed a first step to split up qdev.c not just into
common vs. system-only code but also in better maintainable subareas.
That's targeted at having a section like "Core device API" covering a
to-be-created device.c with myself plus some backup as maintainer, then
Igor/mst/whomever for "Device hotplug interface" or the like.
qdev-system.c we could consider to split up so that the block/net/char
specific parts can be assigned clear maintainers - haven't investigated
that part yet. In the meantime we could simply create multiple sections
covering different aspects of qdev* files.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC] qmp, target-i386: device_list_properties for TYPE_CPU

[Andreas Färber]

Hi,

Am 12.02.2016 um 10:13 schrieb Valentin Rakush:
> This is RFC because there is another implementation option: it is
> possible to implement this functionality in the object_finalize for
> all available targets. All targets change will require more testing.
> Please let me know if all targets should be changed at once.

I thought I had already seen some Fujitsu/IBM patch to fix this...
(pointers appreciated) It should be fixed on the level where the problem
is introduced - i.e. if qom/cpu.c calls the cpu_exec_init() in
instance_init it needs to call the corresponding exit function in
instance_finalize; dito for target-i386/cpu.c or wherever. Or we try to
move it from instance_init to realize, avoiding it getting called in the
first place.

> 
> This patch changes qmp_device_list_properties and only target-i386
> to allow TYPE_CPU class properties to be quered with QMP interface and
> with -device core2duo-x86_64-cpu,help command line.
> 
> Signed-off-by: Valentin Rakush <valentin.rak...@gmail.com>
> Cc: Luiz Capitulino <lcapitul...@redhat.com>
> Cc: Eric Blake <ebl...@redhat.com>
> Cc: Markus Armbruster <arm...@redhat.com>
> Cc: Andreas Färber <afaer...@suse.de>
> Cc: Daniel P. Berrange <berra...@redhat.com>
> Cc: Eduardo Habkost <ehabk...@redhat.com>
> ---
>  qmp.c | 19 +++
>  target-i386/cpu.c | 32 ++--
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/qmp.c b/qmp.c
> index 6ae7230..2721f16 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -516,6 +516,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const 
> char *typename,
> Error **errp)
>  {
>  ObjectClass *klass;
> +ObjectClass *cpu_klass;

Please use cpu_class, only "class" is a reserved identifier.

>  Object *obj;
>  ObjectProperty *prop;
>  ObjectPropertyIterator iter;
> @@ -580,6 +581,24 @@ DevicePropertyInfoList *qmp_device_list_properties(const 
> char *typename,
>  prop_list = entry;
>  }
>  
> +cpu_klass = object_class_dynamic_cast(klass, TYPE_CPU);
> +if (cpu_klass) {
> +CPUState *tmp_cpu = CPU(obj);
> +CPUState *cs = first_cpu;
> +
> +CPU_FOREACH(cs) {
> +if (tmp_cpu->cpu_index == cs->cpu_index) {
> +#if defined(CONFIG_USER_ONLY)
> +cpu_list_lock();
> +#endif
> +QTAILQ_REMOVE(, cs, node);
> +#if defined(CONFIG_USER_ONLY)
> +cpu_list_unlock();
> +#endif
> +}
> +}
> +}
> +
>  object_unref(obj);
>  
>  return prop_list;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3fa14bf..8c32794 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1479,7 +1479,7 @@ static void host_x86_cpu_class_init(ObjectClass *oc, 
> void *data)
>  
>  dc->props = host_x86_cpu_properties;
>  /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */
> -dc->cannot_destroy_with_object_finalize_yet = true;
> +dc->cannot_destroy_with_object_finalize_yet = false;
>  }
>  
>  static void host_x86_cpu_initfn(Object *obj)
> @@ -3225,11 +3225,39 @@ static void x86_cpu_common_class_init(ObjectClass 
> *oc, void *data)
>  cc->cpu_exec_enter = x86_cpu_exec_enter;
>  cc->cpu_exec_exit = x86_cpu_exec_exit;
>  
> +object_class_property_add(oc, "family", "int",
> +x86_cpuid_version_get_family,
> +x86_cpuid_version_set_family, NULL, NULL, NULL);

You add class properties here but I don't see you deleting the previous
instance properties anywhere?

> +object_class_property_add(oc, "model", "int",
> +x86_cpuid_version_get_model,
> +x86_cpuid_version_set_model, NULL, NULL, NULL);
> +object_class_property_add(oc, "stepping", "int",
> +x86_cpuid_version_get_stepping,
> +x86_cpuid_version_set_stepping, NULL, NULL, NULL);
> +object_class_property_add_str(oc, "vendor",
> +x86_cpuid_get_vendor,
> +x86_cpuid_set_vendor, NULL);
> +object_class_property_add_str(oc, "model-id",
> +x86_cpuid_get_model_id,
> +x86_cpuid_set_model_id, NULL);
> +object_class_property_add(oc, "tsc-frequency", "int",
> +x86_cpuid_get_tsc_freq,
> +x86_cpuid_set_tsc_fre

[Qemu-devel] [PATCH] MAINTAINERS: Drop target-i386 from CPU subsystem

[Andreas Färber]

X86CPU QOM type is in good hands and actively maintained these days, so
drop it from the generic QOM CPU subsystem.

Some refactorings and design questions will still intersect, but review
and discussions of individual series can still take place while opting out
of general X86CPU patch review.

Cc: Eduardo Habkost <ehabk...@redhat.com>
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 02710f8..c9b5893 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1032,7 +1032,6 @@ M: Andreas Färber <afaer...@suse.de>
 S: Supported
 F: qom/cpu.c
 F: include/qom/cpu.h
-F: target-i386/cpu.c
 
 ICC Bus
 M: Igor Mammedov <imamm...@redhat.com>
-- 
2.6.2

Re: [Qemu-devel] [PATCH 01/14] cpu: Clean up includes

[Andreas Färber]

Am 09.02.2016 um 16:24 schrieb Peter Maydell:
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
> 
> This commit was created with scripts/clean-includes.
> 
> Signed-off-by: Peter Maydell 
> ---
>  qom/cpu.c | 1 +
>  target-i386/cpu.c | 5 +
>  2 files changed, 2 insertions(+), 4 deletions(-)

Applied to qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] KVH call for agenda for 2016-02-16

[Andreas Färber]

Am 10.02.2016 um 16:48 schrieb Christian Borntraeger:
> On 02/10/2016 04:42 PM, Juan Quintela wrote:
>> Please, send any topic that you are interested in covering.
>>
>> At the end of Monday I will send an email with the agenda or the
>> cancellation of the call, so hurry up.
>>
>> After discussions on the QEMU Summit, we are going to have always open a
>> KVM call where you can add topics.
> 
> Since we seem to be stuck, I propose CPU hotplug as agenda item.

Thank you, confirming that I will be able to attend.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] target-arm: Fix MDCCSR_EL0 instruction encoding

[Andreas Färber]

Am 09.02.2016 um 21:57 schrieb Dirk Müller:
> See C5.1.5 of the ARMv8 Reference Manual
> 
> Signed-off-by: Dirk Mueller <dmuel...@suse.com>

Reviewed-by: Andreas Färber <afaer...@suse.de>

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] target-arm: Implement DBGDTRRX_EL0/DBGDTRTX_EL0 MSR

[Andreas Färber]

Hi Andreas,

Am 09.02.2016 um 21:59 schrieb Dirk Müller:
> This is used by the ARM JTAG DCC console in the Linux kernel,
> but can be ignored in order to continue booting.
> 
> Co-Authored-By: Andreas Schwab 

If this was co-authored by you, we need a proper Signed-off-by please.

> Signed-off-by: Dirk Mueller 
> ---
>  target-arm/helper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 954e6e8..abce416 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3704,6 +3704,9 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>  { .name = "DBGVCR",
>.cp = 14, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0,
>.access = PL1_RW, .type = ARM_CP_NOP },
> +{ .name = "DBGDTRxX_EL0", .state = ARM_CP_STATE_BOTH,
> +  .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 5, .opc2 = 0,
> +  .access = PL0_RW, .type = ARM_CP_NOP },
>  REGINFO_SENTINEL
>  };
>  

Otherwise this can have my Reviewed-by. The small x was suggested by me
since it's actually RX/TX depending on R/W.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 00/10] Allow hotplug of s390 CPUs

[Andreas Färber]

Am 10.02.2016 um 16:28 schrieb David Hildenbrand:
> For x86, cpu models are realized by making x86_64-cpu an abstract class and
> creating loads of new classes, e.g. host-x86_64-cpu or haswell-x86_64-cpu.
> 
> How does 'device_add ' play together with the x86 cpu model
> approach? And with cpu models specified via "-cpu" in general?

device_add needs to use an instantiatable type, like the ones you sketch
above.

> Or does that in return mean, that "making models own classes" is outdated? Or
> will some internal conversion happen that I am missing?
> 
> What is the plan for cpu models and cpu hotplug? How are cpu models to be
> defined in the future?

Someone at IBM was working on defining models for s390x - not sure what
the status is?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] qdev & hw/core owner? (was Re: [PATCH v19 7/9] machine: add properties to compat_props incrementaly)

[Andreas Färber]

Am 04.02.2016 um 12:31 schrieb Paolo Bonzini:
> On 03/02/2016 20:06, Michael S. Tsirkin wrote:
>> On Wed, Feb 03, 2016 at 03:55:04PM -0200, Eduardo Habkost wrote:
>>> On Thu, Jan 28, 2016 at 06:00:31PM +0100, Igor Mammedov wrote:
>>> [...]
 It looks like this series might go nowhere but this patch
 is not tied to it and useful to us in general
 so perhaps you could pick it up after ACKs from
 S390/SPAPR maintainers.

>
> Reviewed-by: Eduardo Habkost 
>>>
>>> We don't have a maintainer for hw/core/machine.c, hw/core/qdev*,
>>> and related files.
>>>
>>> Assuming we don't have a volunteer to maintain them officially,
>>> can we agree on a default destination for those patches so they
>>> don't linger on the list? Michael? Andreas?
>>
>> Not me please. Have too much on my plate.
>> Would you like to maintain it yourself?
> 
> That's my suggestion too.  I guess Igor and I could help with reviews,
> but testing and sending the pull requests would add too much work.
> Since you're the main one touching it, it makes sense for you to handle it.

You're talking about machine, right? Some time ago I had proposed Marcel
who initially worked on it, but I'm fine with anyone taking it.

For some (but not all) core qdev parts related to the (stalled) QOM
migration I've been taking care of via qom-next. Last time this came up
you didn't want anyone to be M: for qdev, so maybe we can use R: so that
at least people automatically get CC'ed and we avoid this recurring
discussion?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 3/4] usb-bot: hotplug support

[Andreas Färber]

Am 02.02.2016 um 14:36 schrieb Markus Armbruster:
> Gerd Hoffmann  writes:
> 
>> This patch marks usb-bot as hot-pluggable device, makes attached
>> property settable and turns off auto-attach in case the device
>> was hotplugged.
>>
>> Hot-plugging a usb-bot device with one or more scsi devices can be
>> done this way now:
>>
>>   (1) device-add usb-bot,id=foo
>>   (2) device-add scsi-{hd,cd},bus=foo.0,lun=0
>>   (2b) optionally add more devices (luns 0 ... 15).
>>   (3) qom-set foo.attached = true
> 
> This isn't exactly pretty, but it beats no hot plug.
> 
> A general solution for hot plugging composite devices could perhaps be
> prettier, but I'm not aware of any recent work in the area.  Andreas,
> Paolo?

Not aware, no. Essentially we'd need a DeviceClass::dont_realize flag,
right? Then foo.attached=true could become foo.realized=true. Question
is then whether the bus would be attachable prior to realization - to be
tested.

Haven't read the full series yet, so puzzled what kind of bot this is
supposed to be. ;)

Cheers,
Andreas

>> Signed-off-by: Gerd Hoffmann 
> 
> Assuming we want this because we can't have a general solution now:
> 
> Reviewed-by: Markus Armbruster 

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v5 01/10] qom: add helpers for UserCreatable object types

[Andreas Färber]

Am 03.02.2016 um 00:38 schrieb Eric Blake:
> On 02/02/2016 05:57 AM, Daniel P. Berrange wrote:
>> The QMP monitor code has two helper methods object_add
>> and qmp_object_del that are called from several places
>> in the code (QMP, HMP and main emulator startup).
>>
>> The HMP and main emulator startup code also share
>> further logic that extracts the qom-type & id
>> values from a qdict.
>>
>> We soon need to use this logic from qemu-img, qemu-io
>> and qemu-nbd too, but don't want those to depend on
>> the monitor, nor do we want to duplicate the code.
> 
> Yay - merge conflicts with my work pending on Markus' qapi-next branch:
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00254.html

Did I review the wrong patch then? I assumed this was the one I was
asked to look at just before FOSDEM...

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 01/10] qom: add helpers for UserCreatable object types

[Andreas Färber]

Am 02.02.2016 um 13:57 schrieb Daniel P. Berrange:
> The QMP monitor code has two helper methods object_add
> and qmp_object_del that are called from several places
> in the code (QMP, HMP and main emulator startup).
> 
> The HMP and main emulator startup code also share
> further logic that extracts the qom-type & id
> values from a qdict.
> 
> We soon need to use this logic from qemu-img, qemu-io
> and qemu-nbd too, but don't want those to depend on
> the monitor, nor do we want to duplicate the code.
> 
> To avoid this, move some code out of qmp.c and hmp.c
> adding new methods to qom/object_interfaces.c
> 
>  - user_creatable_add - takes a QDict holding a full
>object definition & instantiates it
>  - user_creatable_add_type - takes an ID, type name,
>and QDict holding object properties & instantiates
>it
>  - user_creatable_add_opts - takes a QemuOpts holding
>a full object definition & instantiates it
>  - user_creatable_add_opts_foreach - variant on
>user_creatable_add_opts which can be directly used
>in conjunction with qemu_opts_foreach.
>  - user_creatable_del - takes an ID and deletes the
>corresponding object
> 
> The existing code is updated to use these new methods.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  hmp.c   |  52 +++-
>  include/monitor/monitor.h   |   3 -
>  include/qom/object_interfaces.h |  92 
>  qmp.c   |  76 ++---
>  qom/object_interfaces.c | 180 
> 
>  vl.c|  66 ++-
>  6 files changed, 296 insertions(+), 173 deletions(-)

No objections from my side, looks sane, but I'd appreciate Paolo/Stefan
to ack since I wasn't really involved in that code.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PULL 1/3] qom: Allow properties to be registered against classes

[Andreas Färber]

From: "Daniel P. Berrange" <berra...@redhat.com>

When there are many instances of a given class, registering
properties against the instance is wasteful of resources. The
majority of objects have a statically defined list of possible
properties, so most of the properties are easily registerable
against the class. Only those properties which are conditionally
registered at runtime need be recorded against the klass.

Registering properties against classes also makes it possible
to provide static introspection of QOM - currently introspection
is only possible after creating an instance of a class, which
severely limits its usefulness.

This impl only supports simple scalar properties. It does not
attempt to allow child object / link object properties against
the class. There are ways to support those too, but it would
make this patch more complicated, so it is left as an exercise
for the future.

There is no equivalent to object_property_del() provided, since
classes must be immutable once they are defined.

Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 include/qom/object.h   |  46 -
 qom/object.c   | 236 ++---
 tests/check-qom-proplist.c |  31 --
 3 files changed, 287 insertions(+), 26 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 4509166..37d414a 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -381,6 +381,8 @@ struct ObjectClass
 const char *class_cast_cache[OBJECT_CLASS_CAST_CACHE];
 
 ObjectUnparent *unparent;
+
+GHashTable *properties;
 };
 
 /**
@@ -944,6 +946,13 @@ ObjectProperty *object_property_add(Object *obj, const 
char *name,
 
 void object_property_del(Object *obj, const char *name, Error **errp);
 
+ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name,
+  const char *type,
+  ObjectPropertyAccessor *get,
+  ObjectPropertyAccessor *set,
+  ObjectPropertyRelease *release,
+  void *opaque, Error **errp);
+
 /**
  * object_property_find:
  * @obj: the object
@@ -954,6 +963,8 @@ void object_property_del(Object *obj, const char *name, 
Error **errp);
  */
 ObjectProperty *object_property_find(Object *obj, const char *name,
  Error **errp);
+ObjectProperty *object_class_property_find(ObjectClass *klass, const char 
*name,
+   Error **errp);
 
 typedef struct ObjectPropertyIterator ObjectPropertyIterator;
 
@@ -962,7 +973,7 @@ typedef struct ObjectPropertyIterator 
ObjectPropertyIterator;
  * @obj: the object
  *
  * Initializes an iterator for traversing all properties
- * registered against an object instance.
+ * registered against an object instance, its class and all parent classes.
  *
  * It is forbidden to modify the property list while iterating,
  * whether removing or adding properties.
@@ -1371,6 +1382,12 @@ void object_property_add_str(Object *obj, const char 
*name,
  void (*set)(Object *, const char *, Error **),
  Error **errp);
 
+void object_class_property_add_str(ObjectClass *klass, const char *name,
+   char *(*get)(Object *, Error **),
+   void (*set)(Object *, const char *,
+   Error **),
+   Error **errp);
+
 /**
  * object_property_add_bool:
  * @obj: the object to add a property to
@@ -1387,6 +1404,11 @@ void object_property_add_bool(Object *obj, const char 
*name,
   void (*set)(Object *, bool, Error **),
   Error **errp);
 
+void object_class_property_add_bool(ObjectClass *klass, const char *name,
+bool (*get)(Object *, Error **),
+void (*set)(Object *, bool, Error **),
+Error **errp);
+
 /**
  * object_property_add_enum:
  * @obj: the object to add a property to
@@ -1406,6 +1428,13 @@ void object_property_add_enum(Object *obj, const char 
*name,
   void (*set)(Object *, int, Error **),
   Error **errp);
 
+void object_class_property_add_enum(ObjectClass *klass, const char *name,
+const char *typename,
+const char * const *strings,
+int (*get)(Object *, Error **),
+void (*set)(Object *, int, Error **),
+Error **errp);
+
 /**
  * object_property_add_tm:
  * @obj: the object to 

[Qemu-devel] [PULL 0/3] QOM devices patch queue 2016-01-18

[Andreas Färber]

Hello Peter,

This is my QOM (devices) patch queue. Please pull.

Regards,
Andreas

Cc: Peter Maydell 
Cc: Daniel P. Berrange 
Cc: Markus Armbruster 
Cc: Paolo Bonzini 

The following changes since commit 46188349ad7e23abef0e453d2b0406985f6552a1:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-ui-20160118-1' into 
staging (2016-01-18 16:00:47 +)

are available in the git repository at:

  git://github.com/afaerber/qemu-cpu.git tags/qom-devices-for-peter

for you to fetch changes up to abed886ec60cf239a03515cf0b30fb11fa964c44:

  qdev: Free QemuOpts when the QOM path goes away (2016-01-18 17:47:58 +0100)


QOM infrastructure fixes and device conversions

* Dynamic class properties
* Property iterator cleanup
* Device hot-unplug ID race fix


Daniel P. Berrange (2):
  qom: Allow properties to be registered against classes
  qom: Change object property iterator API contract

Paolo Bonzini (1):
  qdev: Free QemuOpts when the QOM path goes away

 hw/core/qdev.c |   4 +-
 hw/ppc/spapr_drc.c |   7 +-
 include/qom/object.h   |  76 +++---
 net/filter.c   |   7 +-
 qmp.c  |  14 ++-
 qom/object.c   | 252 +++--
 tests/check-qom-proplist.c |  38 ---
 vl.c   |   7 +-
 8 files changed, 323 insertions(+), 82 deletions(-)

[Qemu-devel] [PULL v2 1/4] qom: Allow properties to be registered against classes

[Andreas Färber]

From: "Daniel P. Berrange" <berra...@redhat.com>

When there are many instances of a given class, registering
properties against the instance is wasteful of resources. The
majority of objects have a statically defined list of possible
properties, so most of the properties are easily registerable
against the class. Only those properties which are conditionally
registered at runtime need be recorded against the klass.

Registering properties against classes also makes it possible
to provide static introspection of QOM - currently introspection
is only possible after creating an instance of a class, which
severely limits its usefulness.

This impl only supports simple scalar properties. It does not
attempt to allow child object / link object properties against
the class. There are ways to support those too, but it would
make this patch more complicated, so it is left as an exercise
for the future.

There is no equivalent to object_property_del() provided, since
classes must be immutable once they are defined.

Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 include/qom/object.h   |  46 -
 qom/object.c   | 236 ++---
 tests/check-qom-proplist.c |  31 --
 3 files changed, 287 insertions(+), 26 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 4509166..37d414a 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -381,6 +381,8 @@ struct ObjectClass
 const char *class_cast_cache[OBJECT_CLASS_CAST_CACHE];
 
 ObjectUnparent *unparent;
+
+GHashTable *properties;
 };
 
 /**
@@ -944,6 +946,13 @@ ObjectProperty *object_property_add(Object *obj, const 
char *name,
 
 void object_property_del(Object *obj, const char *name, Error **errp);
 
+ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name,
+  const char *type,
+  ObjectPropertyAccessor *get,
+  ObjectPropertyAccessor *set,
+  ObjectPropertyRelease *release,
+  void *opaque, Error **errp);
+
 /**
  * object_property_find:
  * @obj: the object
@@ -954,6 +963,8 @@ void object_property_del(Object *obj, const char *name, 
Error **errp);
  */
 ObjectProperty *object_property_find(Object *obj, const char *name,
  Error **errp);
+ObjectProperty *object_class_property_find(ObjectClass *klass, const char 
*name,
+   Error **errp);
 
 typedef struct ObjectPropertyIterator ObjectPropertyIterator;
 
@@ -962,7 +973,7 @@ typedef struct ObjectPropertyIterator 
ObjectPropertyIterator;
  * @obj: the object
  *
  * Initializes an iterator for traversing all properties
- * registered against an object instance.
+ * registered against an object instance, its class and all parent classes.
  *
  * It is forbidden to modify the property list while iterating,
  * whether removing or adding properties.
@@ -1371,6 +1382,12 @@ void object_property_add_str(Object *obj, const char 
*name,
  void (*set)(Object *, const char *, Error **),
  Error **errp);
 
+void object_class_property_add_str(ObjectClass *klass, const char *name,
+   char *(*get)(Object *, Error **),
+   void (*set)(Object *, const char *,
+   Error **),
+   Error **errp);
+
 /**
  * object_property_add_bool:
  * @obj: the object to add a property to
@@ -1387,6 +1404,11 @@ void object_property_add_bool(Object *obj, const char 
*name,
   void (*set)(Object *, bool, Error **),
   Error **errp);
 
+void object_class_property_add_bool(ObjectClass *klass, const char *name,
+bool (*get)(Object *, Error **),
+void (*set)(Object *, bool, Error **),
+Error **errp);
+
 /**
  * object_property_add_enum:
  * @obj: the object to add a property to
@@ -1406,6 +1428,13 @@ void object_property_add_enum(Object *obj, const char 
*name,
   void (*set)(Object *, int, Error **),
   Error **errp);
 
+void object_class_property_add_enum(ObjectClass *klass, const char *name,
+const char *typename,
+const char * const *strings,
+int (*get)(Object *, Error **),
+void (*set)(Object *, int, Error **),
+Error **errp);
+
 /**
  * object_property_add_tm:
  * @obj: the object to 

[Qemu-devel] [PULL v2 2/4] qom: Change object property iterator API contract

[Andreas Färber]

From: "Daniel P. Berrange" <berra...@redhat.com>

Currently the ObjectProperty iterator API works as follows:

  ObjectPropertyIterator *iter;

  iter = object_property_iter_init(obj);
  while ((prop = object_property_iter_next(iter))) {
 ...
  }
  object_property_iter_free(iter);

This has the benefit that the ObjectPropertyIterator struct
can be opaque, but has the downside that callers need to
explicitly call a free function. It is also not in keeping
with iterator style used elsewhere in QEMU/GLib2.

This patch changes the API to use stack allocation instead:

  ObjectPropertyIterator iter;

  object_property_iter_init(, obj);
  while ((prop = object_property_iter_next())) {
 ...
  }

Reviewed-by: Eric Blake <ebl...@redhat.com>
Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
Reviewed-by: Markus Armbruster <arm...@redhat.com>
[AF: Fused ObjectPropertyIterator struct with typedef]
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 hw/ppc/spapr_drc.c |  7 +++
 include/qom/object.h   | 30 ++
 net/filter.c   |  7 +++
 qmp.c  | 14 ++
 qom/object.c   | 22 --
 tests/check-qom-proplist.c |  7 +++
 vl.c   |  7 +++
 7 files changed, 36 insertions(+), 58 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 4fb86a6..dccb908 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -684,7 +684,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object 
*owner,
 {
 Object *root_container;
 ObjectProperty *prop;
-ObjectPropertyIterator *iter;
+ObjectPropertyIterator iter;
 uint32_t drc_count = 0;
 GArray *drc_indexes, *drc_power_domains;
 GString *drc_names, *drc_types;
@@ -708,8 +708,8 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object 
*owner,
  */
 root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
 
-iter = object_property_iter_init(root_container);
-while ((prop = object_property_iter_next(iter))) {
+object_property_iter_init(, root_container);
+while ((prop = object_property_iter_next())) {
 Object *obj;
 sPAPRDRConnector *drc;
 sPAPRDRConnectorClass *drck;
@@ -750,7 +750,6 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object 
*owner,
 spapr_drc_get_type_str(drc->type));
 drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
 }
-object_property_iter_free(iter);
 
 /* now write the drc count into the space we reserved at the
  * beginning of the arrays previously
diff --git a/include/qom/object.h b/include/qom/object.h
index 37d414a..d0dafe9 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -966,7 +966,10 @@ ObjectProperty *object_property_find(Object *obj, const 
char *name,
 ObjectProperty *object_class_property_find(ObjectClass *klass, const char 
*name,
Error **errp);
 
-typedef struct ObjectPropertyIterator ObjectPropertyIterator;
+typedef struct ObjectPropertyIterator {
+ObjectClass *nextclass;
+GHashTableIter iter;
+} ObjectPropertyIterator;
 
 /**
  * object_property_iter_init:
@@ -984,32 +987,27 @@ typedef struct ObjectPropertyIterator 
ObjectPropertyIterator;
  *   Using object property iterators
  *   
  *   ObjectProperty *prop;
- *   ObjectPropertyIterator *iter;
+ *   ObjectPropertyIterator iter;
  *
- *   iter = object_property_iter_init(obj);
- *   while ((prop = object_property_iter_next(iter))) {
+ *   object_property_iter_init(, obj);
+ *   while ((prop = object_property_iter_next())) {
  * ... do something with prop ...
  *   }
- *   object_property_iter_free(iter);
  *   
  * 
- *
- * Returns: the new iterator
  */
-ObjectPropertyIterator *object_property_iter_init(Object *obj);
-
-/**
- * object_property_iter_free:
- * @iter: the iterator instance
- *
- * Releases any resources associated with the iterator.
- */
-void object_property_iter_free(ObjectPropertyIterator *iter);
+void object_property_iter_init(ObjectPropertyIterator *iter,
+   Object *obj);
 
 /**
  * object_property_iter_next:
  * @iter: the iterator instance
  *
+ * Return the next available property. If no further properties
+ * are available, a %NULL value will be returned and the @iter
+ * pointer should not be used again after this point without
+ * re-initializing it.
+ *
  * Returns: the next property, or %NULL when all properties
  * have been traversed.
  */
diff --git a/net/filter.c b/net/filter.c
index f777ba2..5d90f83 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -137,7 +137,7 @@ static void netfilter_complete(UserCreatable *uc, Error 
**errp)
 Error *local_err = NULL;
 char *str, *info;
 ObjectProperty *prop;
-ObjectPropertyIterator *iter;
+ObjectPropertyIterator iter;
  

[Qemu-devel] [PULL v2 3/4] qdev: Free QemuOpts when the QOM path goes away

[Andreas Färber]

From: Paolo Bonzini <pbonz...@redhat.com>

Otherwise there is a race where the DEVICE_DELETED event has been sent but
attempts to reuse the ID will fail.

Note that similar races exist for other QemuOpts, which this patch
does not attempt to fix.

For example, if the device is a block device, then unplugging it also
deletes its backend.  However, this backend's get deleted in
drive_info_del(), which is only called when properties are
destroyed.  Just like device_finalize(), drive_info_del() is called
some time after DEVICE_DELETED is sent.  A separate patch series has
been sent to plug this other bug.  Character devices also have yet to
be fixed.

Reported-by: Michael S. Tsirkin <m...@redhat.com>
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
Reviewed-by: Markus Armbruster <arm...@redhat.com>
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 hw/core/qdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 2c7101d..44bf790 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1206,7 +1206,6 @@ static void device_finalize(Object *obj)
 NamedGPIOList *ngl, *next;
 
 DeviceState *dev = DEVICE(obj);
-qemu_opts_del(dev->opts);
 
 QLIST_FOREACH_SAFE(ngl, >gpios, node, next) {
 QLIST_REMOVE(ngl, node);
@@ -1254,6 +1253,9 @@ static void device_unparent(Object *obj)
 qapi_event_send_device_deleted(!!dev->id, dev->id, path, _abort);
 g_free(path);
 }
+
+qemu_opts_del(dev->opts);
+dev->opts = NULL;
 }
 
 static void device_class_init(ObjectClass *class, void *data)
-- 
2.6.2

[Qemu-devel] [PULL v2 4/4] MAINTAINERS: Fix sPAPR entry heading

[Andreas Färber]

get_maintainers.pl does not handle parenthesis in maintenance areas well
in connection with list emails (here: qemu-...@nongnu.org).

Resolve a recurring CC issue breaking git-send-email by reverting part
of commit 085eb217dfb3ee12e7985c11f71f8a038394735a ("Add David Gibson
for sPAPR in MAINTAINERS file").

Cc: David Gibson <da...@gibson.dropbear.id.au>
Signed-off-by: Andreas Färber <afaer...@suse.de>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8f44dca..4030e27 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -587,7 +587,7 @@ F: hw/ppc/prep.c
 F: hw/pci-host/prep.[hc]
 F: hw/isa/pc87312.[hc]
 
-sPAPR (pseries)
+sPAPR
 M: David Gibson <da...@gibson.dropbear.id.au>
 M: Alexander Graf <ag...@suse.de>
 L: qemu-...@nongnu.org
-- 
2.6.2

[Qemu-devel] [PULL v2 0/4] QOM devices patch queue 2016-01-18

[Andreas Färber]

Hello Peter,

This is my QOM (devices) patch queue. Please pull.

v2 fixes an annoying git-send-email RFC822 breakage by reverting a MAINTAINERS 
change.
I had reported it and already re-sent a previous pull; still not fixed, so 
fixing it myself.

Regards,
Andreas

Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Daniel P. Berrange <berra...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: David Gibson <da...@gibson.dropbear.id.au>

The following changes since commit 46188349ad7e23abef0e453d2b0406985f6552a1:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-ui-20160118-1' into 
staging (2016-01-18 16:00:47 +)

are available in the git repository at:

  git://github.com/afaerber/qemu-cpu.git tags/qom-devices-for-peter

for you to fetch changes up to 300b115ce804cb6a20acc0003cc17687545b728d:

  MAINTAINERS: Fix sPAPR entry heading (2016-01-18 18:19:35 +0100)


QOM infrastructure fixes and device conversions

* Dynamic class properties
* Property iterator cleanup
* Device hot-unplug ID race fix

------------
Andreas Färber (1):
  MAINTAINERS: Fix sPAPR entry heading

Daniel P. Berrange (2):
  qom: Allow properties to be registered against classes
  qom: Change object property iterator API contract

Paolo Bonzini (1):
  qdev: Free QemuOpts when the QOM path goes away

 MAINTAINERS|   2 +-
 hw/core/qdev.c |   4 +-
 hw/ppc/spapr_drc.c |   7 +-
 include/qom/object.h   |  76 +++---
 net/filter.c   |   7 +-
 qmp.c  |  14 ++-
 qom/object.c   | 252 +++--
 tests/check-qom-proplist.c |  38 ---
 vl.c   |   7 +-
 9 files changed, 324 insertions(+), 83 deletions(-)

Re: [Qemu-devel] [PATCH] sysbus: Remove ignored return value of FindSysbusDeviceFunc

[Andreas Färber]

Am 18.01.2016 um 05:39 schrieb David Gibson:
> Functions of type FindSysbusDeviceFunc currently return an integer.  I
> recently made an error in a patch because I assumed that this return value
> would control whether iteration of the function across devices continues
> or not.  In fact, the function's return value is always ignored.
> 
> This changes the function type to return void, so that others don't make
> the same mistake.

Have you considered implementing the behavior you expected? :)
Not necessary for your use case or too complicated?

> 
> Signed-off-by: David Gibson 
> ---
> 
> Please apply.

Patch looks okay, too short notice for today's pull though.
Usually we avoid "I" in a commit message.

Regards,
Andreas

> 
>  hw/arm/sysbus-fdt.c| 4 ++--
>  hw/core/machine.c  | 2 +-
>  hw/core/platform-bus.c | 8 ++--
>  hw/ppc/e500.c  | 4 +---
>  hw/ppc/spapr.c | 4 +---
>  include/hw/sysbus.h| 2 +-
>  6 files changed, 8 insertions(+), 16 deletions(-)

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] cpu: Clean up includes

[Andreas Färber]

Am 18.01.2016 um 18:35 schrieb Peter Maydell:
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
> 
> This commit was created with scripts/clean-includes.
> 
> Signed-off-by: Peter Maydell 
> ---
>  qom/cpu.c | 1 +
>  target-i386/cpu.c | 5 +
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 8f537a4..edd6b6a 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -18,6 +18,7 @@
>   * 
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu-common.h"

Shouldn't qemu-common.h include osdep.h?

Otherwise looks okay.

Regards,
Andreas

>  #include "qom/cpu.h"
>  #include "sysemu/kvm.h"
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0d447b5..1e3b98a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -16,10 +16,7 @@
>   * You should have received a copy of the GNU Lesser General Public
>   * License along with this library; if not, see 
> .
>   */
> -#include 
> -#include 
> -#include 
> -#include 
> +#include "qemu/osdep.h"
>  
>  #include "cpu.h"
>  #include "sysemu/kvm.h"

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Andreas Färber]

Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
> Paolo Bonzini <pbonz...@redhat.com> writes:
>> On 05/11/2015 13:06, Andreas Färber wrote:
>>>> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>>>>DEVICE_DELETED?  Like this:
>>>>
>>>> +++ b/hw/core/qdev.c
>>>> @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>>>>  dev->parent_bus = NULL;
>>>>  }
>>>>
>>>> +qemu_opts_del(dev->opts);
>>>> +dev->opts = NULL;
>>>> +
>>>>  /* Only send event if the device had been completely realized */
>>>>  if (dev->pending_deleted_event) {
>>>>  gchar *path = object_get_canonical_path(OBJECT(dev));
>>>
>>> To me this proposal sounds sane, but I did not get to tracing the code
>>> flow here. Paolo, which approach do you prefer and why?
>>
>> It doesn't really matter, because the BQL is being held here.
>>
>> On the other hand, if the opts are deleted in finalize, there is an
>> arbitrary delay because finalize is typically called after a
>> synchronize_rcu period.
>>
>>>>> 2. If the device is a block device, then unplugging it also deletes its
>>>>>backend (ugly wart we keep for backward compatibility; *not* for
>>>>>blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>>>>deleted in drive_info_del().  Just like device_finalize(), it runs
>>>>>within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>>>>different ID, or am I missing something?
>>>>>
>>>>>See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>>>
>>> If we can leave this patch decoupled from block layer and decide soonish
>>> on the desired approach, I'd be happy to include it in my upcoming
>>> qom-devices pull.
>>
>> I agree with you, the block layer bug is separate.
> 
> Related, but clearly separate.  Mentioning it in the commit message
> would be nice, though.

Paolo, pong: I gathered that I should queue the original patch without
Markus's proposed change, correct? And do you want to add some sentence
to the commit message as requested by Markus?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Andreas Färber]

Am 15.01.2016 um 18:16 schrieb Paolo Bonzini:
> On 15/01/2016 18:03, Andreas Färber wrote:
>> Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
>>> Paolo Bonzini <pbonz...@redhat.com> writes:
>>>> On 05/11/2015 13:06, Andreas Färber wrote:
>>>>>> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>>>>>>DEVICE_DELETED?  Like this:
>>>>>>
>>>>>> +++ b/hw/core/qdev.c
>>>>>> @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>>>>>>  dev->parent_bus = NULL;
>>>>>>  }
>>>>>>
>>>>>> +qemu_opts_del(dev->opts);
>>>>>> +dev->opts = NULL;
>>>>>> +
>>>>>>  /* Only send event if the device had been completely realized */
>>>>>>  if (dev->pending_deleted_event) {
>>>>>>  gchar *path = object_get_canonical_path(OBJECT(dev));
>>>>>
>>>>> To me this proposal sounds sane, but I did not get to tracing the code
>>>>> flow here. Paolo, which approach do you prefer and why?
>>>>
>>>> It doesn't really matter, because the BQL is being held here.
>>>>
>>>> On the other hand, if the opts are deleted in finalize, there is an
>>>> arbitrary delay because finalize is typically called after a
>>>> synchronize_rcu period.
>>>>
>>>>>>> 2. If the device is a block device, then unplugging it also deletes its
>>>>>>>backend (ugly wart we keep for backward compatibility; *not* for
>>>>>>>blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>>>>>>deleted in drive_info_del().  Just like device_finalize(), it runs
>>>>>>>within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>>>>>>different ID, or am I missing something?
>>>>>>>
>>>>>>>See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>>>>>
>>>>> If we can leave this patch decoupled from block layer and decide soonish
>>>>> on the desired approach, I'd be happy to include it in my upcoming
>>>>> qom-devices pull.
>>>>
>>>> I agree with you, the block layer bug is separate.
>>>
>>> Related, but clearly separate.  Mentioning it in the commit message
>>> would be nice, though.
>>
>> Paolo, pong: I gathered that I should queue the original patch without
>> Markus's proposed change, correct? And do you want to add some sentence
>> to the commit message as requested by Markus?
> 
> Yes, thanks:
> 
> 
> Note that similar races exist for other QemuOpts, which this patch
> does not attempt to fix.
> 
> For example, if the device is a block device, then unplugging it also
> deletes its backend.  However, this backend's get deleted in
> drive_info_del(), which is only called when properties are
> destroyed.  Just like device_finalize(), drive_info_del() is called
> some time after DEVICE_DELETED is sent.  A separate patch series has
> been sent to plug this other bug.  Character devices also have yet to
> be fixed.
> -

Thanks, queued on qom-next with that addition:
https://github.com/afaerber/qemu-cpu/commits/qom-next

I'll leave Markus and others time until Monday for *-by or comments, but
I really need to get out my pull with Daniel's class properties.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/2] qom: add object_class_get_instance_size()

[Andreas Färber]

Am 18.12.2015 um 16:30 schrieb Igor Mammedov:
> it will be used for allocating memory for a to be
> created new object in safe manner.
> 
> Signed-off-by: Igor Mammedov 
> ---
>  include/qom/object.h | 8 
>  qom/object.c | 5 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 4509166..8f61b0b 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -880,6 +880,14 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>  
> +/**
> + * object_class_get_instance_size:
> + * @klass: The #ObjectClass to obtain instance size
> + *
> + * Returns: instance size of @klass
> + */
> +size_t object_class_get_instance_size(ObjectClass *klass);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>const char *implements_type, bool include_abstract,
>void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index d751569..2f141ee 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -771,6 +771,11 @@ ObjectClass *object_class_get_parent(ObjectClass *class)
>  return type->class;
>  }
>  
> +size_t object_class_get_instance_size(ObjectClass *klass)
> +{
> +   return klass->type->instance_size;
> +}
> +
>  typedef struct OCFData
>  {
>  void (*fn)(ObjectClass *klass, void *opaque);

I had previously proposed a type_get_instance_size() but did not get any
feedback: http://patchwork.ozlabs.org/patch/269591/

It was using name -> type, here you are using class -> type.

One does not necessarily exclude the other. :)

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 2/2] qdev: safely fail device_add if unable to allocate device

[Andreas Färber]

Am 18.12.2015 um 22:15 schrieb Markus Armbruster:
> Eric Blake  writes:
>> On 12/18/2015 09:48 AM, Daniel P. Berrange wrote:
>>> On Fri, Dec 18, 2015 at 04:30:47PM +0100, Igor Mammedov wrote:
 qdev_device_add() currently uses object_new() which
 will abort if there memory allocation for device instance
 fails. While it's fine it startup, it is not desirable
 diring hotplug.

 Try to allocate memory for object first and fail safely
 if allocation fails.
>>
>>> This just avoids one small malloc failure.
>>>
 +object_initialize(dev, obj_size, driver);
>>>
>>> This is going to call g_new many more times, so you'll
>>> still hit OOM almost immediately. eg the call to
>>> g_hash_table_new_full() in object_initialize_with_type
>>> will abort on OOM, not to mention anything run in a
>>> instance constructor function registered against the
>>> class. There's no way to avoid this given that we have
>>> chosen to use GLib in QEMU, so I don't really see any
>>> point in replacing the 'object_new' call with g_try_malloc
> 
> Seconded.
> 
>> We just had a recent thread on it, and I tend to agree that this series
>> isn't helpful.
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/threads.html#01238
> 
> Plenty of arguments there why recovering from scattered random
> allocation failures isn't useful, and recovering from all of them isn't
> practical.  Limit recovery attempts to big allocations, and spend (some
> of) the saved cycles on actually testing the recovery code.

Jumping in late... I had done work into this direction around 2012/2013
and I believe that changing the hotplug allocation is the right thing to
do here.

http://patchwork.ozlabs.org/patch/269595/

Igor's error handling could still use some improvement (at the time we
did not have the out argument) and my suggested solution was a similar
one to error_abort (pre-allocation and special handling), to avoid
allocation of Error objects and strings on OOM.

Daniel, any allocations other than the core QOM API inside an
instance_init function are plain wrong, has been pointed out to patch
authors and is not an argument for not handling hot-plug errors/design
properly. My huge QOM conversions always had in mind that instance_init
cannot do random allocations and moved them to realize, which may fail,
including for OOM, and should be handled gracefully for hot-plug. For
startup I don't see it as critical - it may be inconvenient not to get a
nice error message but you don't risk losing the guest's data.

Now to the claim that this is just a small allocation. If some 20-char
string allocation fails, there's little QEMU can realistically do
recovery-wise. But a PowerPCCPU device for instance comes with huge
multi-level instruction-dispatch tables even in KVM mode. Therefore my
opinion that this user-initiated scenario and thus code location is
exactly the one we need to handle, even if many others remain unhandled.
It's also why CPU hot-plug needs to take QOM design into account, and
proposals parametrizing core count etc. make size precalculation tricky.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2] qom: change object property iterator API contract

[Andreas Färber]

Am 17.12.2015 um 09:54 schrieb Markus Armbruster:
> "Daniel P. Berrange"  writes:
> 
>> Currently the object property iterator API works as follows
>>
>>   ObjectPropertyIterator *iter;
>>
>>   iter = object_property_iter_init(obj);
>>   while ((prop = object_property_iter_next(iter))) {
>>  ...
>>   }
>>   object_property_iter_free(iter);
>>
>> This has the benefit that the ObjectPropertyIterator struct
>> can be opaque, but has the downside that callers need to
>> explicitly call a free function. It is also not in keeping
>> with iterator style used elsewhere in QEMU/glib2
>>
>> This patch changes the API to use stack allocation instead
>>
>>   ObjectPropertyIterator iter;
>>
>>   object_property_iter_init(, obj);
>>   while ((prop = object_property_iter_next())) {
>>  ...
>>   }
>>
>> Reviewed-by: Eric Blake 
>> Signed-off-by: Daniel P. Berrange 
> [...]
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 9630c5b..275a21b 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -971,6 +971,11 @@ ObjectProperty *object_class_property_find(ObjectClass 
>> *klass, const char *name,
>>  
>>  typedef struct ObjectPropertyIterator ObjectPropertyIterator;
>>  
>> +struct ObjectPropertyIterator {
>> +ObjectClass *nextclass;
>> +GHashTableIter iter;
>> +};
>> +
> 
> Please fuse the two declarations.  Perhaps Andreas can do that on
> commit.

Done.

>>  /**
>>   * object_property_iter_init:
>>   * @obj: the object
> 
> Reviewed-by: Markus Armbruster 

Queued on qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Andreas Färber]

Am 11.01.2016 um 18:37 schrieb Markus Armbruster:
> Paolo Bonzini  writes:
>> On 07/01/2016 10:53, Cao jin wrote:
>>> On 01/07/2016 05:38 PM, Paolo Bonzini wrote:
 On 06/01/2016 14:43, Cao jin wrote:
 These functions are called in the data path; changes to use SCSI_BUS()
 should come with performance data proving that it doesn't slow down I/O.
>>>
>>> I see. I am not familiar with the procedure of scsi i/o performance
>>> test, do have some guideline about it?
>>
>> Usually people test with FIO, but I think it's simpler to just keep
>> DO_UPCAST here.
> 
> DO_UPCAST() needs to die.
> 
> SCSI_BUS() is a readable wrapper around OBJECT_CHECK():
> 
> #define SCSI_BUS(obj) OBJECT_CHECK(SCSIBus, (obj), TYPE_SCSI_BUS)
> 
> OBJECT_CHECK() is semantically a type cast, but in actual code it does
> more:
> 
> #define OBJECT_CHECK(type, obj, name) \
> ((type *)object_dynamic_cast_assert(OBJECT(obj), (name), \
> __FILE__, __LINE__, __func__))
> 
> Object *object_dynamic_cast_assert(Object *obj, const char *typename,
>const char *file, int line, const char 
> *func)
> {
> trace_object_dynamic_cast_assert(obj ? obj->class->type->name : 
> "(null)",
>  typename, file, line, func);
> 
> #ifdef CONFIG_QOM_CAST_DEBUG
> [...]
> #endif
> return obj;
> }
> 
> If CONFIG_QOM_CAST_DEBUG is on, it checks.  That's a feature.
> 
> If CONFIG_QOM_CAST_DEBUG is off, it still calls to trace.  That might
> be a misfeature.

Ugh, I was not aware of that tracing!

Also, might it make sense to make that ..._assert() function inline?

> If OBJECT_CHECK() isn't fit for fast paths because of that, perhaps QOM
> should provide a conversion macro that is.  Andreas, what do you think?

We had a similar issue with virtio fast paths. I'm not sure whether we
kept DO_UPCAST() or used a direct (Foo *) cast though. I think mst was a
fan of upcast, and I used the latter in TCG innards.

The general idea (from Anthony) was that derived types should not know
about the implementation detail that the struct has a field of a certain
name (for example, a switch to C++ classes or something would make it go
away). So there's two issues mixed here, a) going from foo(x,y,z) to
just bar(x) and b) the type checking as part of bar(x).

If you want to propose a FOO_FAST() or whatever, I'm all ears.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] doc: Display i386 CPUID properties

[Andreas Färber]

Hi,

First of all this should be "target-i386:" and CC'ing Eduardo, too. Also
I think you meant "X86CPU properties". This is not documentation.

Am 04.01.2016 um 08:18 schrieb Valentin Rakush:
> When user invokes 'qemu -cpu help' all _features_ for CPUIDs are
> printed in stdout. In case of target-i386, Hyper-V CPUID _properties_ are
> _not_ printed. This patch fixes this and displays all properties.
> 
> I think it make sense to print properties along with features, because
> these properties can be used in the command line for the given CPUID.

Agreed; I disagree with your approach though.

You are baking into your code the wrong assumption that all properties
must be static and be present in one specific generic x86 array. QOM
handles all those details for you, both type inheritance and instance
vs. class properties (pull upcoming). -cpu help can be extended to list
some options but will never be able to list all possible options
accurately, as the exact list of properties might depend on the model.

Also, -cpu is a legacy option and this use case definitely does not
warrant moving all that code around in the first place (you could simply
go from object instance to class/properties though).

Regards,
Andreas

> 
> Signed-off-by: Valentin Rakush 
> 
> ---
>  target-i386/cpu.c | 132 
> +-
>  1 file changed, 70 insertions(+), 62 deletions(-)

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC PATCH v0 0/9] Generic cpu-core device

[Andreas Färber]

Am 15.12.2015 um 06:27 schrieb Zhu Guihua:
> 
>>> and allow individual targets to use its own way to build CPUs?
>>>
>>> For initial conversion of x86-cpus to device-add we could do pretty
>>> much the same like we do now, where cpu devices will appear under:
>>> /machine (pc-i440fx-2.5-machine)
>>>/unattached (container)
>>>  /device[x] (qemu64-x86_64-cpu)
>>>
>>> since we don't have to maintain/model dummy socket/core objects.
>>>
>>> PowerPC could do the similar only at core level since it has
>>> need for modeling core objects.
>>>
>>> It doesn't change anything wrt current introspection state, since
>>> cpus could be still found by mgmt tools that parse QOM tree.
>>>
>>> We probably should split 2 conflicting goals we are trying to meet here,
>>>
>>>   1. make device-add/dell work with cpus /
>>>   drop support for cpu-add in favor of device_add
>>>
>>>   2. how to model QOM tree view for CPUs in arch independent manner
>>>  to make mgmt layer life easier.
>>>
>>> and work on them independently instead of arguing for years,
>>> that would allow us to make progress in #1 while still thinking about
>>> how to do #2 the right way if we really need it.
>> Makes sense, s390 developer also recommends the same. Given that we have
>> CPU hotplug patchsets from x86, PowerPC and s390 all implementing
>> device_add
>> semantics pending on the list, can we hope to get them merged for
>> QEMU-2.6 ?
>>
>> So as seen below, the device is either "cpu_model-cpu_type" or just
>> "cpu_type".
>>
>> -device POWER8-powerpc64-cpu (pseries)
>> -device qemu64-x86_64-cpu (pc)
>> -device s390-cpu (s390)
>>
>> Is this going to be the final acceptable semantics ? Would libvirt be
>> able
>> to work with this different CPU device names for different guests ?
> 
> Is operating on core level not final decision ?

No, it is absolutely _not_ the conclusion from Seattle.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)