mbox series

[0/9] hw/sysbus/platform-bus: Introduce TYPE_DYNAMIC_SYS_BUS_DEVICE

Message ID 20250125181343.59151-1-philmd@linaro.org
Headers show
Series hw/sysbus/platform-bus: Introduce TYPE_DYNAMIC_SYS_BUS_DEVICE | expand

Message

Philippe Mathieu-Daudé Jan. 25, 2025, 6:13 p.m. UTC
Some SysBus devices can optionally be dynamically plugged onto
the sysbus-platform-bus (then virtual guests are aware of
mmio mapping and IRQs via device tree / ACPI rules).

This series makes these devices explicit by having them implement
the DYNAMIC_SYS_BUS_DEVICE class, which only sets 'user_creatable'
flag to %true but makes the codebase a bit clearer (IMHO, at least
now we can grep for DYNAMIC_SYS_BUS_DEVICE).

Philippe Mathieu-Daudé (9):
  hw/sysbus: Use sizeof(BusState) in main_system_bus_create()
  hw/sysbus: Declare QOM types using DEFINE_TYPES() macro
  hw/sysbus: Introduce TYPE_DYNAMIC_SYS_BUS_DEVICE
  hw/vfio: Have VFIO_PLATFORM devices inherit from
    DYNAMIC_SYS_BUS_DEVICE
  hw/display: Have RAMFB device inherit from DYNAMIC_SYS_BUS_DEVICE
  hw/i386: Have X86_IOMMU devices inherit from DYNAMIC_SYS_BUS_DEVICE
  hw/net: Have eTSEC device inherit from DYNAMIC_SYS_BUS_DEVICE
  hw/tpm: Have TPM TIS sysbus device inherit from DYNAMIC_SYS_BUS_DEVICE
  hw/xen: Have legacy Xen backend inherit from DYNAMIC_SYS_BUS_DEVICE

 include/hw/sysbus.h           |  2 ++
 include/hw/xen/xen_pvdev.h    |  3 +-
 hw/core/sysbus.c              | 53 ++++++++++++++++++++---------------
 hw/display/ramfb-standalone.c |  3 +-
 hw/i386/amd_iommu.c           |  2 --
 hw/i386/intel_iommu.c         |  2 --
 hw/i386/x86-iommu.c           |  2 +-
 hw/net/fsl_etsec/etsec.c      |  4 +--
 hw/tpm/tpm_tis_sysbus.c       |  3 +-
 hw/vfio/amd-xgbe.c            |  2 --
 hw/vfio/calxeda-xgmac.c       |  2 --
 hw/vfio/platform.c            |  4 +--
 hw/xen/xen-legacy-backend.c   |  7 ++---
 13 files changed, 42 insertions(+), 47 deletions(-)

Comments

Richard Henderson Jan. 26, 2025, 9:42 p.m. UTC | #1
On 1/25/25 10:13, Philippe Mathieu-Daudé wrote:
> Rather than using the obscure system_bus_info.instance_size,
> directly use sizeof(BusState).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/core/sysbus.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Nice.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Richard Henderson Jan. 26, 2025, 9:47 p.m. UTC | #2
On 1/25/25 10:13, Philippe Mathieu-Daudé wrote:
> Some TYPE_SYS_BUS_DEVICEs can be optionally dynamically
> plugged on the TYPE_PLATFORM_BUS_DEVICE.
> Rather than sometimes noting that with comment around
> the 'user_creatable = true' line in each DeviceRealize
> handler, introduce an abstract TYPE_DYNAMIC_SYS_BUS_DEVICE
> class.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/sysbus.h |  2 ++
>   hw/core/sysbus.c    | 14 ++++++++++++++
>   2 files changed, 16 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Richard Henderson Jan. 26, 2025, 9:52 p.m. UTC | #3
On 1/25/25 10:13, Philippe Mathieu-Daudé wrote:
> Do not explain why VFIO_PLATFORM devices are user_creatable,
> have them inherit TYPE_DYNAMIC_SYS_BUS_DEVICE, to explicit
> they can optionally be plugged on TYPE_PLATFORM_BUS_DEVICE.

to make it explicit that they can be

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Richard Henderson Jan. 26, 2025, 9:53 p.m. UTC | #4
On 1/25/25 10:13, Philippe Mathieu-Daudé wrote:
> Because the RAM FB device can be optionally plugged on the
> TYPE_PLATFORM_BUS_DEVICE, have it inherit TYPE_DYNAMIC_SYS_BUS_DEVICE.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/display/ramfb-standalone.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Richard Henderson Jan. 26, 2025, 9:54 p.m. UTC | #5
On 1/25/25 10:13, Philippe Mathieu-Daudé wrote:
> Do not explain why _X86_IOMMU devices are user_creatable,
> have them inherit TYPE_DYNAMIC_SYS_BUS_DEVICE, to explicit
> they can optionally be plugged on TYPE_PLATFORM_BUS_DEVICE.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/i386/amd_iommu.c   | 2 --
>   hw/i386/intel_iommu.c | 2 --
>   hw/i386/x86-iommu.c   | 2 +-
>   3 files changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Richard Henderson Jan. 26, 2025, 9:54 p.m. UTC | #6
On 1/25/25 10:13, Philippe Mathieu-Daudé wrote:
> Because the network eTSEC device can be optionally plugged on the
> TYPE_PLATFORM_BUS_DEVICE, have it inherit TYPE_DYNAMIC_SYS_BUS_DEVICE.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/net/fsl_etsec/etsec.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Alexander Graf Jan. 27, 2025, 12:29 a.m. UTC | #7
On 25.01.25 10:13, Philippe Mathieu-Daudé wrote:
> Some SysBus devices can optionally be dynamically plugged onto
> the sysbus-platform-bus (then virtual guests are aware of
> mmio mapping and IRQs via device tree / ACPI rules).
>
> This series makes these devices explicit by having them implement
> the DYNAMIC_SYS_BUS_DEVICE class, which only sets 'user_creatable'
> flag to %true but makes the codebase a bit clearer (IMHO, at least
> now we can grep for DYNAMIC_SYS_BUS_DEVICE).


I love it. Thank you! This is clearly much more readable than the 
magical hint swizzling I did :).

Reviewed-by: Alexander Graf <graf@amazon.com>


Alex
CLEMENT MATHIEU--DRIF Jan. 27, 2025, 6:14 a.m. UTC | #8
Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>

Thanks phil

On 25/01/2025 19:13, Philippe Mathieu-Daudé wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> 
> 
> Do not explain why _X86_IOMMU devices are user_creatable,
> have them inherit TYPE_DYNAMIC_SYS_BUS_DEVICE, to explicit
> they can optionally be plugged on TYPE_PLATFORM_BUS_DEVICE.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/amd_iommu.c   | 2 --
>   hw/i386/intel_iommu.c | 2 --
>   hw/i386/x86-iommu.c   | 2 +-
>   3 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 6b13ce894b1..e8e084c7cf8 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1687,8 +1687,6 @@ static void amdvi_sysbus_class_init(ObjectClass *klass, void *data)
>       dc->hotpluggable = false;
>       dc_class->realize = amdvi_sysbus_realize;
>       dc_class->int_remap = amdvi_int_remap;
> -    /* Supported by the pc-q35-* machine types */
> -    dc->user_creatable = true;
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>       dc->desc = "AMD IOMMU (AMD-Vi) DMA Remapping device";
>       device_class_set_props(dc, amdvi_properties);
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index f366c223d0e..7fde0603bfe 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4871,8 +4871,6 @@ static void vtd_class_init(ObjectClass *klass, void *data)
>       dc->hotpluggable = false;
>       x86_class->realize = vtd_realize;
>       x86_class->int_remap = vtd_int_remap;
> -    /* Supported by the pc-q35-* machine types */
> -    dc->user_creatable = true;
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>       dc->desc = "Intel IOMMU (VT-d) DMA Remapping device";
>   }
> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
> index fed34b2fcfa..5cdd165af0d 100644
> --- a/hw/i386/x86-iommu.c
> +++ b/hw/i386/x86-iommu.c
> @@ -146,7 +146,7 @@ bool x86_iommu_ir_supported(X86IOMMUState *s)
> 
>   static const TypeInfo x86_iommu_info = {
>       .name          = TYPE_X86_IOMMU_DEVICE,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_DYNAMIC_SYS_BUS_DEVICE,
>       .instance_size = sizeof(X86IOMMUState),
>       .class_init    = x86_iommu_class_init,
>       .class_size    = sizeof(X86IOMMUClass),
> --
> 2.47.1
>
CLEMENT MATHIEU--DRIF Jan. 27, 2025, 6:17 a.m. UTC | #9
Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>




On 25/01/2025 19:13, Philippe Mathieu-Daudé wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Because the RAM FB device can be optionally plugged on the
> TYPE_PLATFORM_BUS_DEVICE, have it inherit TYPE_DYNAMIC_SYS_BUS_DEVICE.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/display/ramfb-standalone.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index 6c35028965d..1be106b57f2 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -72,13 +72,12 @@ static void ramfb_class_initfn(ObjectClass *klass, void *data)
>       dc->vmsd = &ramfb_dev_vmstate;
>       dc->realize = ramfb_realizefn;
>       dc->desc = "ram framebuffer standalone device";
> -    dc->user_creatable = true;
>       device_class_set_props(dc, ramfb_properties);
>   }
>
>   static const TypeInfo ramfb_info = {
>       .name          = TYPE_RAMFB_DEVICE,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_DYNAMIC_SYS_BUS_DEVICE,
>       .instance_size = sizeof(RAMFBStandaloneState),
>       .class_init    = ramfb_class_initfn,
>   };
> --
> 2.47.1
>
CLEMENT MATHIEU--DRIF Jan. 27, 2025, 6:19 a.m. UTC | #10
Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>




On 25/01/2025 19:13, Philippe Mathieu-Daudé wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Some TYPE_SYS_BUS_DEVICEs can be optionally dynamically
> plugged on the TYPE_PLATFORM_BUS_DEVICE.
> Rather than sometimes noting that with comment around
> the 'user_creatable = true' line in each DeviceRealize
> handler, introduce an abstract TYPE_DYNAMIC_SYS_BUS_DEVICE
> class.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/sysbus.h |  2 ++
>   hw/core/sysbus.c    | 14 ++++++++++++++
>   2 files changed, 16 insertions(+)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index c9b1e0e90e3..81bbda10d37 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -19,6 +19,8 @@ DECLARE_INSTANCE_CHECKER(BusState, SYSTEM_BUS,
>   OBJECT_DECLARE_TYPE(SysBusDevice, SysBusDeviceClass,
>                       SYS_BUS_DEVICE)
>
> +#define TYPE_DYNAMIC_SYS_BUS_DEVICE "dynamic-sysbus-device"
> +
>   /**
>    * SysBusDeviceClass:
>    *
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 306f98406c0..e8d03fd28d9 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -321,6 +321,14 @@ BusState *sysbus_get_default(void)
>       return main_system_bus;
>   }
>
> +static void dynamic_sysbus_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *k = DEVICE_CLASS(klass);
> +
> +    k->user_creatable = true;
> +    k->hotpluggable = false;
> +}
> +
>   static const TypeInfo sysbus_types[] = {
>       {
>           .name           = TYPE_SYSTEM_BUS,
> @@ -336,6 +344,12 @@ static const TypeInfo sysbus_types[] = {
>           .class_size     = sizeof(SysBusDeviceClass),
>           .class_init     = sysbus_device_class_init,
>       },
> +    {
> +        .name           = TYPE_DYNAMIC_SYS_BUS_DEVICE,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .class_init     = dynamic_sysbus_device_class_init,
> +        .abstract       = true,
> +    }
>   };
>
>   DEFINE_TYPES(sysbus_types)
> --
> 2.47.1
>
CLEMENT MATHIEU--DRIF Jan. 27, 2025, 6:21 a.m. UTC | #11
Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>



On 25/01/2025 19:13, Philippe Mathieu-Daudé wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Rather than using the obscure system_bus_info.instance_size,
> directly use sizeof(BusState).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/core/sysbus.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 9355849ff0a..f713bbfe04f 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -323,8 +323,8 @@ static void main_system_bus_create(void)
>        * assign main_system_bus before qbus_init()
>        * in order to make "if (bus != sysbus_get_default())" work
>        */
> -    main_system_bus = g_malloc0(system_bus_info.instance_size);
> -    qbus_init(main_system_bus, system_bus_info.instance_size,
> +    main_system_bus = g_new0(BusState, 1);
> +    qbus_init(main_system_bus, sizeof(BusState),
>                 TYPE_SYSTEM_BUS, NULL, "main-system-bus");
>       OBJECT(main_system_bus)->free = g_free;
>   }
> --
> 2.47.1
>
CLEMENT MATHIEU--DRIF Jan. 27, 2025, 6:21 a.m. UTC | #12
Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>



On 25/01/2025 19:13, Philippe Mathieu-Daudé wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Because the network eTSEC device can be optionally plugged on the
> TYPE_PLATFORM_BUS_DEVICE, have it inherit TYPE_DYNAMIC_SYS_BUS_DEVICE.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/net/fsl_etsec/etsec.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 781b9003954..3ce4fa2662d 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -425,14 +425,12 @@ static void etsec_class_init(ObjectClass *klass, void *data)
>       dc->realize = etsec_realize;
>       device_class_set_legacy_reset(dc, etsec_reset);
>       device_class_set_props(dc, etsec_properties);
> -    /* Supported by ppce500 machine */
> -    dc->user_creatable = true;
>   }
>
>   static const TypeInfo etsec_types[] = {
>       {
>           .name          = TYPE_ETSEC_COMMON,
> -        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .parent        = TYPE_DYNAMIC_SYS_BUS_DEVICE,
>           .instance_size = sizeof(eTSEC),
>           .class_init    = etsec_class_init,
>           .instance_init = etsec_instance_init,
> --
> 2.47.1
>
Bernhard Beschow Jan. 27, 2025, 9:21 a.m. UTC | #13
Am 25. Januar 2025 18:13:41 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Because the network eTSEC device can be optionally plugged on the
>TYPE_PLATFORM_BUS_DEVICE, have it inherit TYPE_DYNAMIC_SYS_BUS_DEVICE.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Tested-by: Bernhard Beschow <shentey@gmail.com>
Acked-by: Bernhard Beschow <shentey@gmail.com>

>---
> hw/net/fsl_etsec/etsec.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
>index 781b9003954..3ce4fa2662d 100644
>--- a/hw/net/fsl_etsec/etsec.c
>+++ b/hw/net/fsl_etsec/etsec.c
>@@ -425,14 +425,12 @@ static void etsec_class_init(ObjectClass *klass, void *data)
>     dc->realize = etsec_realize;
>     device_class_set_legacy_reset(dc, etsec_reset);
>     device_class_set_props(dc, etsec_properties);
>-    /* Supported by ppce500 machine */
>-    dc->user_creatable = true;
> }
> 
> static const TypeInfo etsec_types[] = {
>     {
>         .name          = TYPE_ETSEC_COMMON,
>-        .parent        = TYPE_SYS_BUS_DEVICE,
>+        .parent        = TYPE_DYNAMIC_SYS_BUS_DEVICE,
>         .instance_size = sizeof(eTSEC),
>         .class_init    = etsec_class_init,
>         .instance_init = etsec_instance_init,
Gerd Hoffmann Jan. 28, 2025, 10:41 a.m. UTC | #14
On Sat, Jan 25, 2025 at 07:13:34PM +0100, Philippe Mathieu-Daudé wrote:
> Some SysBus devices can optionally be dynamically plugged onto
> the sysbus-platform-bus (then virtual guests are aware of
> mmio mapping and IRQs via device tree / ACPI rules).

Do we have some sane way to have user-pluggable sysbus devices on arm?

I've played around with that a bit, with the uefi variable service I'm
working on.  Specifically I'd prefer to *not* have a patch wiring things
up in machine type code like this ...

https://lore.kernel.org/qemu-devel/20250107153353.1144978-20-kraxel@redhat.com/

... and just use 'qemu -device uefi-vars-sysbus' instead.

Something like AcpiDevAmlIfClass but for device tree seems to not exist
though.  Also apparently AcpiDevAmlIfClass is not used.

take care,
  Gerd
Peter Maydell Jan. 28, 2025, 10:50 a.m. UTC | #15
On Tue, 28 Jan 2025 at 10:42, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Sat, Jan 25, 2025 at 07:13:34PM +0100, Philippe Mathieu-Daudé wrote:
> > Some SysBus devices can optionally be dynamically plugged onto
> > the sysbus-platform-bus (then virtual guests are aware of
> > mmio mapping and IRQs via device tree / ACPI rules).
>
> Do we have some sane way to have user-pluggable sysbus devices on arm?

The answer in a general sense is "no, because user pluggable
sysbus is a weird idea". "sysbus" means "it's wired into a
specific bit of the memory map and to specific IRQs, and whoever
does that needs to know what IRQs and bits of memory are usable,
and the guest OS needs to know it's there". "user-pluggable" means
"it's all automatic and the guest can just do some kind of
probing for what is or isn't present". All the platform bus stuff
is a nasty mess that's working around the things people want
to plug in not being clean devices on probeable buses :-(
And the platform bus is only supported on the "virt" board,
because that's the only one where QEMU is generating its
own dtb or ACPI tables where it can tell the guest "hey,
there's some device here".

-- PMM
BALATON Zoltan Jan. 28, 2025, 12:57 p.m. UTC | #16
On Tue, 28 Jan 2025, Peter Maydell wrote:
> On Tue, 28 Jan 2025 at 10:42, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> On Sat, Jan 25, 2025 at 07:13:34PM +0100, Philippe Mathieu-Daudé wrote:
>>> Some SysBus devices can optionally be dynamically plugged onto
>>> the sysbus-platform-bus (then virtual guests are aware of
>>> mmio mapping and IRQs via device tree / ACPI rules).
>>
>> Do we have some sane way to have user-pluggable sysbus devices on arm?
>
> The answer in a general sense is "no, because user pluggable
> sysbus is a weird idea". "sysbus" means "it's wired into a
> specific bit of the memory map and to specific IRQs, and whoever
> does that needs to know what IRQs and bits of memory are usable,
> and the guest OS needs to know it's there". "user-pluggable" means
> "it's all automatic and the guest can just do some kind of
> probing for what is or isn't present". All the platform bus stuff
> is a nasty mess that's working around the things people want
> to plug in not being clean devices on probeable buses :-(
> And the platform bus is only supported on the "virt" board,
> because that's the only one where QEMU is generating its
> own dtb or ACPI tables where it can tell the guest "hey,
> there's some device here".

There are some SoCs that have memory mapped devices but different versions 
in the same family have different devices. Either older ones missing some 
devices or have less USB or network ports while newer SoCs have more of 
those or they have PCIe instead of PCI. Modelling these could use 
pluggable sysbus devices so one could add the devices needed for a SoC 
version without having to write or modify a board code. I think Bernhard's 
attempt to try creating e500 SoCs from a device tree goes in that 
direction too. We could also model this by having a SoC that can 
instantiate devices based on some properties but maybe pluggable devices 
could be more generic for this. The issue seems to be how to tell the 
board or SoC where to map it and what IRQ to connect it as this is done by 
the board and not the device so properties on the device to set these does 
not really help unless the board can somehow query it and instantiate the 
devices based on that. Otherwise whatever handles the -device option to 
create the device would need knowledge about the board. (E.g. the e500 
devices are mapped in the CCSR memory region so one can't just use system 
address space for them.)

Regards,
BALATON Zoltan
Philippe Mathieu-Daudé Jan. 28, 2025, 3:10 p.m. UTC | #17
On 28/1/25 13:57, BALATON Zoltan wrote:
> On Tue, 28 Jan 2025, Peter Maydell wrote:
>> On Tue, 28 Jan 2025 at 10:42, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>
>>> On Sat, Jan 25, 2025 at 07:13:34PM +0100, Philippe Mathieu-Daudé wrote:
>>>> Some SysBus devices can optionally be dynamically plugged onto
>>>> the sysbus-platform-bus (then virtual guests are aware of
>>>> mmio mapping and IRQs via device tree / ACPI rules).
>>>
>>> Do we have some sane way to have user-pluggable sysbus devices on arm?
>>
>> The answer in a general sense is "no, because user pluggable
>> sysbus is a weird idea". "sysbus" means "it's wired into a
>> specific bit of the memory map and to specific IRQs, and whoever
>> does that needs to know what IRQs and bits of memory are usable,
>> and the guest OS needs to know it's there". "user-pluggable" means
>> "it's all automatic and the guest can just do some kind of
>> probing for what is or isn't present". All the platform bus stuff
>> is a nasty mess that's working around the things people want
>> to plug in not being clean devices on probeable buses :-(
>> And the platform bus is only supported on the "virt" board,
>> because that's the only one where QEMU is generating its
>> own dtb or ACPI tables where it can tell the guest "hey,
>> there's some device here".
> 
> There are some SoCs that have memory mapped devices but different 
> versions in the same family have different devices. Either older ones 
> missing some devices or have less USB or network ports while newer SoCs 
> have more of those or they have PCIe instead of PCI. Modelling these 
> could use pluggable sysbus devices so one could add the devices needed 
> for a SoC version without having to write or modify a board code. I 
> think Bernhard's attempt to try creating e500 SoCs from a device tree 
> goes in that direction too. We could also model this by having a SoC 
> that can instantiate devices based on some properties but maybe 
> pluggable devices could be more generic for this. The issue seems to be 
> how to tell the board or SoC where to map it and what IRQ to connect it 
> as this is done by the board and not the device so properties on the 
> device to set these does not really help unless the board can somehow 
> query it and instantiate the devices based on that. Otherwise whatever 
> handles the -device option to create the device would need knowledge 
> about the board. (E.g. the e500 devices are mapped in the CCSR memory 
> region so one can't just use system address space for them.)

IIRC Bernard's series takes a DTB as input and create the machine
matching this DTB.

As Peter explained, sysbus-platform-bus fits TYPE_DYNAMIC_SYS_BUS_DEVICE
in free slots, then generates the corresponding ACPI/DTB.

What you describe seems closer to the QEMU Dynamic Machine project,
following Damien's idea:
https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.hedde@greensocs.com/
We are not quite there yet...
Bernhard Beschow Jan. 28, 2025, 7:13 p.m. UTC | #18
Am 28. Januar 2025 15:10:18 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 28/1/25 13:57, BALATON Zoltan wrote:
>> On Tue, 28 Jan 2025, Peter Maydell wrote:
>>> On Tue, 28 Jan 2025 at 10:42, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>> 
>>>> On Sat, Jan 25, 2025 at 07:13:34PM +0100, Philippe Mathieu-Daudé wrote:
>>>>> Some SysBus devices can optionally be dynamically plugged onto
>>>>> the sysbus-platform-bus (then virtual guests are aware of
>>>>> mmio mapping and IRQs via device tree / ACPI rules).
>>>> 
>>>> Do we have some sane way to have user-pluggable sysbus devices on arm?
>>> 
>>> The answer in a general sense is "no, because user pluggable
>>> sysbus is a weird idea". "sysbus" means "it's wired into a
>>> specific bit of the memory map and to specific IRQs, and whoever
>>> does that needs to know what IRQs and bits of memory are usable,
>>> and the guest OS needs to know it's there". "user-pluggable" means
>>> "it's all automatic and the guest can just do some kind of
>>> probing for what is or isn't present". All the platform bus stuff
>>> is a nasty mess that's working around the things people want
>>> to plug in not being clean devices on probeable buses :-(
>>> And the platform bus is only supported on the "virt" board,
>>> because that's the only one where QEMU is generating its
>>> own dtb or ACPI tables where it can tell the guest "hey,
>>> there's some device here".
>> 
>> There are some SoCs that have memory mapped devices but different versions in the same family have different devices. Either older ones missing some devices or have less USB or network ports while newer SoCs have more of those or they have PCIe instead of PCI. Modelling these could use pluggable sysbus devices so one could add the devices needed for a SoC version without having to write or modify a board code. I think Bernhard's attempt to try creating e500 SoCs from a device tree goes in that direction too. We could also model this by having a SoC that can instantiate devices based on some properties but maybe pluggable devices could be more generic for this. The issue seems to be how to tell the board or SoC where to map it and what IRQ to connect it as this is done by the board and not the device so properties on the device to set these does not really help unless the board can somehow query it and instantiate the devices based on that. Otherwise whatever handles the -device option to create the device would need knowledge about the board. (E.g. the e500 devices are mapped in the CCSR memory region so one can't just use system address space for them.)
>
>IIRC Bernard's series takes a DTB as input and create the machine
>matching this DTB.

That's correct. It's still on my todo list to send an RFC. I first wanted to gain some experience implementing a machine in the classic way which I've now done by means of the imx8mp-evk series. Once I clean up the e500-fdt branch I'd send an RFC.

Best regards,
Bernhard

>
>As Peter explained, sysbus-platform-bus fits TYPE_DYNAMIC_SYS_BUS_DEVICE
>in free slots, then generates the corresponding ACPI/DTB.
>
>What you describe seems closer to the QEMU Dynamic Machine project,
>following Damien's idea:
>https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.hedde@greensocs.com/
>We are not quite there yet...