mbox series

[RFC,0/5] qdev-properties: Try to improve use of dynamic property introspection

Message ID 20240102160455.68612-1-philmd@linaro.org
Headers show
Series qdev-properties: Try to improve use of dynamic property introspection | expand

Message

Philippe Mathieu-Daudé Jan. 2, 2024, 4:04 p.m. UTC 
Hi,

This RFC series tries to work over some limitations exposed in
https://lore.kernel.org/qemu-devel/9293e63b-8032-4ea0-b516-9db6949fb607@linaro.org/

Eventually all QDev objects would use static QOM properties,
but in some cases we can not. ARMv7MState is a such example
adding properties that might end up irrelevant.

This is just an example, but thinking long term (in particular
in the context of dynamic machines) I'm looking at how this
could be improved. Thus this series. I don't like much this
current approach (because more boiler place and complexity)
however this seems to DTRT for the user.

Philippe Mathieu-Daudé (5):
  qdev-properties: Add qdev_property_del_static()
  qdev-properties: Add OptionalBool QAPI type
  hw/arm/armv7m: Convert ARMv7MState::vfp from bool to OptionalBool
  hw/arm/armv7m: Error if trying to set unavailable ARMCPU::vfp property
  hw/arm/armv7m: Do not expose 'vfp' property if ARM CPU doesn't have it

 qapi/common.json             | 16 ++++++++++++++++
 include/hw/arm/armv7m.h      |  2 +-
 include/hw/qdev-properties.h |  7 +++++++
 hw/arm/armsse.c              |  2 +-
 hw/arm/armv7m.c              | 12 +++++++++++-
 hw/core/qdev-properties.c    | 17 +++++++++++++++++
 6 files changed, 53 insertions(+), 3 deletions(-)

Comments

Kevin Wolf Jan. 9, 2024, 10:47 a.m. UTC | #1 
Am 02.01.2024 um 17:04 hat Philippe Mathieu-Daudé geschrieben:
> Hi,
> 
> This RFC series tries to work over some limitations exposed in
> https://lore.kernel.org/qemu-devel/9293e63b-8032-4ea0-b516-9db6949fb607@linaro.org/
> 
> Eventually all QDev objects would use static QOM properties,
> but in some cases we can not. ARMv7MState is a such example
> adding properties that might end up irrelevant.
> 
> This is just an example, but thinking long term (in particular
> in the context of dynamic machines) I'm looking at how this
> could be improved. Thus this series. I don't like much this
> current approach (because more boiler place and complexity)
> however this seems to DTRT for the user.

This doesn't feel like the right approach to me. If you were to describe
the properties of armv7m in the QAPI schema language without looking at
the implementation, you would never add something like OptionalBool.

I'm not entirely sure if I understand the details of the problem, so if
I make wrong assumptions below, please correct me.

I think what you would get instead is a union with the different CPU
types as union variants. You would have few fields in the base type
(cpu-type, memory, etc.), and everything specific to the variant, you
wouldn't even want to look at in armv7m, but just forward it to the CPU.

Now of course actually getting this QAPIfied is not something to expect
in the next few days, so let's get back to the QOM level, but keep the
general idea in mind.

Based on the above, I'd argue that armv7m shouldn't even have the
properties that it only uses to forward them. Instead, we should let the
CPU grab its properties directly from the configuration. The way we
create objects currently is not really designed for this. But let's get
back to the QOM/QAPI integration patches [1] I sent two years ago and
suddently it becomes quite easy: armv7m's .instance_config simply calls
the CPU's .instance_config and passes its visitor. Then the CPU takes
the options it needs for its properties and that's it.

I discussed this series with Markus recently, and I actually assume that
he at least mentioned it to you when you discussed things with him. I
believe the part that you would need here (only up to patch 4 really) is
pretty much uncontroversial.

Of course, I ignored several "details" here, but I think the idea should
still be workable. Some of those details:

* This approach requires that the CPUs are first converted to have
  static properties, but given that making everything static is your
  goal, I assume that's not a problem.

* The CPU must actually exist when you want to set properties for it.
  This probably means moving creating the CPU from realize to the new
  instance_config.

* My patches are for object-add, but we're coming from board code here.
  So the callers would have to be changed to call .instance_config
  (probalby indirectly through object_configure()) instead of setting
  individual properties.

All of this is additional work, but it doesn't look like exceedingly
hard work.

Kevin

[1] https://patchew.org/QEMU/20211103173002.209906-1-kwolf@redhat.com/