Re: hw: Audit of qdev properties with same name but different types

[Markus Armbruster <armbru@redhat.com>]

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...]