[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: hw: Audit of qdev properties with same name but different types
From: |
Markus Armbruster |
Subject: |
Re: hw: Audit of qdev properties with same name but different types |
Date: |
Fri, 22 Dec 2023 11:23:06 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> Hi,
>
> I just did an audit of QDev properties from different
> models sharing the same name, but of different types
> (as of v8.2.0-rc1).
>
> Nothing wrong, but some having the same meaning could
> use the same type :)
Yes.
> Just sharing:
>
> ---
> 2 addr
>
> hw/core/generic-loader.c:183: DEFINE_PROP_UINT64("addr",
> GenericLoaderState, addr, 0),
> hw/core/guest-loader.c:115: DEFINE_PROP_UINT64("addr", GuestLoaderState,
> addr, 0),
> hw/ide/macio.c:469: DEFINE_PROP_UINT32("addr", MACIOIDEState, addr, -1),
> hw/pci/pci.c:71: DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
"addr" is kind of vague, because the address space is anybody's guess.
Width depends on the address space.
Similar naming sins in the QAPI schema at least come with documentation,
e.g.
##
# @PCDIMMDeviceInfo:
[...]
# @addr: physical address, where device is mapped
> 2 base
>
> hw/arm/armv7m.c:630: DEFINE_PROP_UINT32("base", BitBandState, base, 0),
> hw/dma/i8257.c:589: DEFINE_PROP_INT32("base", I8257State, base, 0x00),
"base" is even more vague than "addr".
If it's a base *address*, width again depends on the address space, and
use of signed is fishy.
> 2 bus_nr
>
> hw/pci-bridge/pci_expander_bridge.c:412: DEFINE_PROP_UINT8("bus_nr",
> PXBDev, bus_nr, 0),
> hw/pci-host/xilinx-pcie.c:160: DEFINE_PROP_UINT32("bus_nr",
> XilinxPCIEHost, bus_nr, 0),
If these are PCI bus numbers: correct width is 8 bits.
> 2 clock-frequency
>
> hw/timer/altera_timer.c:218: DEFINE_PROP_UINT32("clock-frequency",
> AlteraTimer, freq_hz, 0),
> hw/timer/mss-timer.c:284: DEFINE_PROP_UINT32("clock-frequency",
> MSSTimerState, freq_hz,
> hw/timer/sifive_pwm.c:409: DEFINE_PROP_UINT64("clock-frequency", struct
> SiFivePwmState,
> hw/timer/stm32f2xx_timer.c:302: DEFINE_PROP_UINT64("clock-frequency",
> struct STM32F2XXTimerState,
> hw/timer/xilinx_timer.c:246: DEFINE_PROP_UINT32("clock-frequency",
> XpsTimerState, freq_hz, 62 * 1000000),
Appropriate width depends on valid frequency range.
> 2 config
>
> hw/intc/pnv_xive2.c:1939: DEFINE_PROP_UINT64("config", PnvXive2, config,
> hw/isa/pc87312.c:332: DEFINE_PROP_UINT8("config", PC87312State, config, 1),
Are these two related? "config" could be anything...
> 2 core-id
>
> target/i386/cpu.c:7788: DEFINE_PROP_INT32("core-id", X86CPU, core_id, 0),
> target/i386/cpu.c:7794: DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
> target/s390x/cpu.c:302: DEFINE_PROP_UINT32("core-id", S390CPU,
> env.core_id, 0),
CPU core IDs are non-negative. Why signed? Perhaps to encode "N/A" as
-1?
I think we can see already any improvements would need to be made case
by case.
[Quite a few more...]
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: hw: Audit of qdev properties with same name but different types,
Markus Armbruster <=