[PATCH 0/2] hw/arm/aspeed: Add /machine/labels container

[PATCH 0/2] hw/arm/aspeed: Add /machine/labels container

[Cédric Le Goater]

Hello,

Patch 1 adds a DT-style /machine/labels container where board code
registers link properties pointing to well-known devices. Tests can
then resolve a device with a single qom-get instead of walking the bus
hierarchy.

Patch 2 uses it to test the ADC128D818 on the Anacapa machine through
the kernel hwmon interface.

Thanks,

C.

Cédric Le Goater (1):
  hw/arm: Add /machine/labels container for Aspeed machines

Emmanuel Blot (1):
  test/functional: anacapa: test ADC128D818

 include/hw/arm/aspeed.h                     | 12 +++++
 hw/arm/aspeed.c                             | 10 +++++
 hw/arm/aspeed_ast2600_anacapa.c             | 11 +++--
 tests/functional/arm/test_aspeed_anacapa.py | 50 +++++++++++++++++++++
 4 files changed, 79 insertions(+), 4 deletions(-)

-- 
2.55.0

[PATCH 1/2] hw/arm: Add /machine/labels container for Aspeed machines

[Cédric Le Goater]

Add a DT-style alias container /machine/labels where board code registers
link<> properties pointing to well-known devices. Tests can then resolve
a device with a single QMP call (qom-get /machine/labels/<name>) instead
of walking the bus hierarchy.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed.h | 12 ++++++++++++
 hw/arm/aspeed.c         | 10 ++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 245d02e5f757..0b8b40262767 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -128,4 +128,16 @@ void aspeed_machine_ast2600_class_emmc_init(ObjectClass *oc);
  */
 void aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc);
 
+/*
+ * aspeed_machine_add_label:
+ * @bmc: pointer to the #AspeedMachineState.
+ * @label: the label name for the device.
+ * @target: the device object to register.
+ *
+ * Register a well-known device under /machine/labels/<label> as a
+ * read-only link.  Aborts on duplicate label names.
+ */
+void aspeed_machine_add_label(AspeedMachineState *bmc, const char *label,
+                              Object *target);
+
 #endif
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 1f8d5d3e132d..79b8d55d66da 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -127,6 +127,14 @@ void aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc)
     }
 }
 
+void aspeed_machine_add_label(AspeedMachineState *bmc, const char *label,
+                              Object *target)
+{
+    Object *labels = object_resolve_path_component(OBJECT(bmc), "labels");
+
+    object_property_add_const_link(labels, label, target);
+}
+
 static void aspeed_machine_init(MachineState *machine)
 {
     AspeedMachineState *bmc = ASPEED_MACHINE(machine);
@@ -137,6 +145,8 @@ static void aspeed_machine_init(MachineState *machine)
     DriveInfo *emmc0 = NULL;
     bool boot_emmc;
 
+    object_property_add_new_container(OBJECT(machine), "labels");
+
     bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
     object_unref(OBJECT(bmc->soc));
-- 
2.55.0

[PATCH 2/2] test/functional: anacapa: test ADC128D818

[Cédric Le Goater]

From: Emmanuel Blot <emmanuel.blot@free.fr>

Verify the ADC128D818 on the Anacapa R-PDB (i2c8 mux channel 0): two
channels are driven with distinct voltages and each is checked to be
reported back independently through the kernel hwmon interface, and the
channel 0 limit registers are validated.

Signed-off-by: Emmanuel Blot <emmanuel.blot@free.fr>
Link: https://lore.kernel.org/qemu-devel/20260707094413.17539-1-emmanuel.blot@free.fr
[ clg: - adjusted on upstream
       - added /machine/labels support ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/arm/aspeed_ast2600_anacapa.c             | 11 +++--
 tests/functional/arm/test_aspeed_anacapa.py | 50 +++++++++++++++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed_ast2600_anacapa.c b/hw/arm/aspeed_ast2600_anacapa.c
index 65d6b0faa2de..0b8d9cc822ca 100644
--- a/hw/arm/aspeed_ast2600_anacapa.c
+++ b/hw/arm/aspeed_ast2600_anacapa.c
@@ -221,14 +221,15 @@ static const uint8_t hpm_brd_id_eeprom[] = {
 };
 static const size_t hpm_brd_id_eeprom_len = sizeof(hpm_brd_id_eeprom);
 
-static void anacapa_add_adc128d818(I2CBus *bus, uint8_t addr,
+static void anacapa_add_adc128d818(AspeedMachineState *bmc,
+                                   I2CBus *bus, uint8_t addr,
                                    const char *description)
 {
     DeviceState *dev = DEVICE(i2c_slave_new(TYPE_ADC128D818, addr));
     g_autofree char *childname = g_strdup_printf("0x%02x", addr);
 
     qdev_prop_set_string(dev, "description", description);
-    object_property_add_child(OBJECT(bus), childname, OBJECT(dev));
+    aspeed_machine_add_label(bmc, description, OBJECT(dev));
     i2c_slave_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
 }
 
@@ -271,7 +272,8 @@ static void anacapa_bmc_i2c_init(AspeedMachineState *bmc)
 
     /* i2c8mux ch0 */
     /* adc128d818@1f - R-PDB ADC (mode 1: 8 voltage channels) */
-    anacapa_add_adc128d818(pca954x_i2c_get_bus(i2c_mux, 0), 0x1f, "i2c8:0:1f");
+    anacapa_add_adc128d818(bmc, pca954x_i2c_get_bus(i2c_mux, 0),
+                           0x1f, "i2c8:0:1f");
     /* pca9555@22 */
     i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 0),
                             TYPE_PCA9552, 0x22);
@@ -333,7 +335,8 @@ static void anacapa_bmc_i2c_init(AspeedMachineState *bmc)
 
     /* i2c13mux ch3 */
     /* adc128d818@1f - MB ADC (mode 1: 8 voltage channels) */
-    anacapa_add_adc128d818(pca954x_i2c_get_bus(i2c_mux, 3), 0x1f, "i2c13:3:1f");
+    anacapa_add_adc128d818(bmc, pca954x_i2c_get_bus(i2c_mux, 3),
+                           0x1f, "i2c13:3:1f");
 
     /* i2c13mux ch4 */
     /* eeprom@51 */
diff --git a/tests/functional/arm/test_aspeed_anacapa.py b/tests/functional/arm/test_aspeed_anacapa.py
index 363b4c2a1d09..0aa00c6f8c86 100644
--- a/tests/functional/arm/test_aspeed_anacapa.py
+++ b/tests/functional/arm/test_aspeed_anacapa.py
@@ -2,10 +2,16 @@
 #
 # Functional test that boots the ASPEED machines
 #
+# Copyright (c) 2026 Meta Platforms, Inc. and affiliates.
+#
 # SPDX-License-Identifier: GPL-2.0-or-later
 
+import re
+import time
+
 from qemu_test import Asset
 from aspeed import AspeedTest
+from qemu_test import exec_command_and_wait_for_pattern
 
 
 class AnacapaMachine(AspeedTest):
@@ -14,6 +20,10 @@ class AnacapaMachine(AspeedTest):
         'https://github.com/legoater/qemu-aspeed-boot/raw/refs/heads/master/images/anacapa-bmc/openbmc-20260616025349/obmc-phosphor-image-anacapa-20260616025349.static.mtd.xz',
         'de3841fb6ed3085aec6424358ee6efc4b8ee85688361e5aa1987fd1acb7d3fb4')
 
+    ADC128D818_QOM_PATH = "/machine/labels/i2c8:0:1f"
+    ADC128D818_MUX_CHANNEL = "/sys/bus/i2c/devices/8-0072/channel-0"
+    PROMPT = "root@anacapa:~#"
+
     def test_arm_ast2600_anacapa_openbmc(self):
         image_path = self.uncompress(self.ASSET_ANACAPA_FLASH)
 
@@ -21,5 +31,45 @@ def test_arm_ast2600_anacapa_openbmc(self):
                                         uboot='2019.04', cpu_id='0xf00',
                                         soc='AST2600 rev A3')
 
+        exec_command_and_wait_for_pattern(self, "root", "Password:")
+        exec_command_and_wait_for_pattern(self, "0penBmc", "#")
+
+        self.adc_hwmon = self.resolve_adc128d818_hwmon()
+        self.assertIn(b"adc128d818", self.read_adc128d818("name"))
+
+        adc = self.ADC128D818_QOM_PATH
+        for ch0_mv, ch1_mv in ((108, 2000), (1280, 500)):
+            self.vm.cmd("qom-set", path=adc, property="ain0", value=ch0_mv)
+            self.vm.cmd("qom-set", path=adc, property="ain1", value=ch1_mv)
+            self.wait_adc128d818_value("in0_input", ch0_mv)
+            self.wait_adc128d818_value("in1_input", ch1_mv)
+
+        self.assertIn(b"2551", self.read_adc128d818("in0_max"))
+        self.assertRegex(self.read_adc128d818("in0_min"), rb"(?m)^0\r*$")
+
+    def resolve_adc128d818_hwmon(self):
+        out = self.read_adc128d818_console(
+            f"basename $(readlink {self.ADC128D818_MUX_CHANNEL})"
+        )
+        match = re.search(rb"i2c-(\d+)", out)
+        self.assertIsNotNone(match, "could not resolve ADC128D818 i2c bus")
+        bus = int(match.group(1))
+        return f"/sys/bus/i2c/devices/{bus}-001f/hwmon/hwmon*"
+
+    def read_adc128d818_console(self, command):
+        return exec_command_and_wait_for_pattern(self, command, self.PROMPT)
+
+    def read_adc128d818(self, attr):
+        return self.read_adc128d818_console(f"cat {self.adc_hwmon}/{attr}")
+
+    def wait_adc128d818_value(self, attr, expected):
+        needle = str(expected).encode()
+        if needle in self.read_adc128d818(attr):
+            return
+        time.sleep(2)
+        if needle not in self.read_adc128d818(attr):
+            self.fail(f"{attr} did not reach {expected}")
+
+
 if __name__ == '__main__':
     AspeedTest.main()
-- 
2.55.0

Re: [PATCH 1/2] hw/arm: Add /machine/labels container for Aspeed machines

[Mark Cave-Ayland]

On 04/09/2026 06:04, Cédric Le Goater wrote:

> Add a DT-style alias container /machine/labels where board code registers
> link<> properties pointing to well-known devices. Tests can then resolve
> a device with a single QMP call (qom-get /machine/labels/<name>) instead
> of walking the bus hierarchy.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/arm/aspeed.h | 12 ++++++++++++
>   hw/arm/aspeed.c         | 10 ++++++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index 245d02e5f757..0b8b40262767 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -128,4 +128,16 @@ void aspeed_machine_ast2600_class_emmc_init(ObjectClass *oc);
>    */
>   void aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc);
>   
> +/*
> + * aspeed_machine_add_label:
> + * @bmc: pointer to the #AspeedMachineState.
> + * @label: the label name for the device.
> + * @target: the device object to register.
> + *
> + * Register a well-known device under /machine/labels/<label> as a
> + * read-only link.  Aborts on duplicate label names.
> + */
> +void aspeed_machine_add_label(AspeedMachineState *bmc, const char *label,
> +                              Object *target);
> +
>   #endif
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 1f8d5d3e132d..79b8d55d66da 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -127,6 +127,14 @@ void aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>       }
>   }
>   
> +void aspeed_machine_add_label(AspeedMachineState *bmc, const char *label,
> +                              Object *target)
> +{
> +    Object *labels = object_resolve_path_component(OBJECT(bmc), "labels");
> +
> +    object_property_add_const_link(labels, label, target);
> +}
> +
>   static void aspeed_machine_init(MachineState *machine)
>   {
>       AspeedMachineState *bmc = ASPEED_MACHINE(machine);
> @@ -137,6 +145,8 @@ static void aspeed_machine_init(MachineState *machine)
>       DriveInfo *emmc0 = NULL;
>       bool boot_emmc;
>   
> +    object_property_add_new_container(OBJECT(machine), "labels");
> +
>       bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
>       object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
>       object_unref(OBJECT(bmc->soc));

Thanks for the proposal, Cédric! A few comments from me below:

1) Is there any reason it should be called labels as opposed to aliases? 
The aliases name as used in Open Firmware feels more intuitive to me.

2) If there is agreement in this approach, is there any reason why we 
shouldn't create /machine/labels (or equivalent) for all QOM trees? I 
certainly think it would be a useful addition going forward.

3) Is there any reason why we need to provide the machine object 
directly to the aspeed_machine_add_label() function? If possible I think 
it makes sense to avoid the direct machine reference, in case the 
underlying implementation changes i.e.


   void machine_add_alias(const char *alias, Object *target)
   {
      Object *aliases = object_resolve_path("/machine/aliases",  NULL);

      object_property_add_const_link(aliases, alias, target);
   }


Even better perhaps we should also generate an error if the alias 
already exists to ensure they are always unique i.e.


   bool machine_add_alias(const char *alias, Object *target,
                          Error **errp)
   {
      Object *aliases = object_resolve_path("/machine/aliases",  NULL);

      if (object_resolve_path_component(aliases, alias)) {
          error_setg(errp, "machine alias '%s' already exists");
          return false;
      }

      return (object_property_add_const_link(aliases, alias, target) !=
              NULL);
   }


4) Should we create a page in the documentation explaining which 
devices/objects should be included in the alias list, which ones are 
added automatically, and what naming conventions should be used for 
devices e.g. serial0, net0 for a network device etc.?

5) What should happen to aliases for devices that are 
hot-plugged/hot-unplugged?


ATB,

Mark.

Re: [PATCH 1/2] hw/arm: Add /machine/labels container for Aspeed machines

[Cédric Le Goater]

Hello,

On 9/9/26 12:35, Mark Cave-Ayland wrote:
> On 04/09/2026 06:04, Cédric Le Goater wrote:
> 
>> Add a DT-style alias container /machine/labels where board code registers
>> link<> properties pointing to well-known devices. Tests can then resolve
>> a device with a single QMP call (qom-get /machine/labels/<name>) instead
>> of walking the bus hierarchy.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/arm/aspeed.h | 12 ++++++++++++
>>   hw/arm/aspeed.c         | 10 ++++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index 245d02e5f757..0b8b40262767 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -128,4 +128,16 @@ void aspeed_machine_ast2600_class_emmc_init(ObjectClass *oc);
>>    */
>>   void aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc);
>> +/*
>> + * aspeed_machine_add_label:
>> + * @bmc: pointer to the #AspeedMachineState.
>> + * @label: the label name for the device.
>> + * @target: the device object to register.
>> + *
>> + * Register a well-known device under /machine/labels/<label> as a
>> + * read-only link.  Aborts on duplicate label names.
>> + */
>> +void aspeed_machine_add_label(AspeedMachineState *bmc, const char *label,
>> +                              Object *target);
>> +
>>   #endif
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 1f8d5d3e132d..79b8d55d66da 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -127,6 +127,14 @@ void aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>>       }
>>   }
>> +void aspeed_machine_add_label(AspeedMachineState *bmc, const char *label,
>> +                              Object *target)
>> +{
>> +    Object *labels = object_resolve_path_component(OBJECT(bmc), "labels");
>> +
>> +    object_property_add_const_link(labels, label, target);
>> +}
>> +
>>   static void aspeed_machine_init(MachineState *machine)
>>   {
>>       AspeedMachineState *bmc = ASPEED_MACHINE(machine);
>> @@ -137,6 +145,8 @@ static void aspeed_machine_init(MachineState *machine)
>>       DriveInfo *emmc0 = NULL;
>>       bool boot_emmc;
>> +    object_property_add_new_container(OBJECT(machine), "labels");
>> +
>>       bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
>>       object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
>>       object_unref(OBJECT(bmc->soc));
> 
> Thanks for the proposal, Cédric! A few comments from me below:
> 
> 1) Is there any reason it should be called labels as opposed to aliases? 

none

> The aliases name as used in Open Firmware feels more intuitive to me.

yes. It it just a name. aliases is fine for me unless it introduces
some confusion with object_property_add_alias().

> 
> 2) If there is agreement in this approach, is there any reason why we shouldn't create /machine/labels (or equivalent) for all QOM trees? I certainly think it would be a useful addition going forward.

It is a trivial addition. We would need more maintainers to Ack
the concept though.

> 
> 3) Is there any reason why we need to provide the machine object directly to the aspeed_machine_add_label() function?

Only because it was selfishly introduced as a Aspeed machine helper !

>  If possible I think it makes sense to avoid the direct machine reference, in case the underlying implementation changes i.e.
> 
> 
>    void machine_add_alias(const char *alias, Object *target)
>    {
>       Object *aliases = object_resolve_path("/machine/aliases",  NULL);
> 
>       object_property_add_const_link(aliases, alias, target);
>    }

ok.
> Even better perhaps we should also generate an error if the alias already exists to ensure they are always unique i.e.
> 
> 
>    bool machine_add_alias(const char *alias, Object *target,
>                           Error **errp)
>    {
>       Object *aliases = object_resolve_path("/machine/aliases",  NULL);
> 
>       if (object_resolve_path_component(aliases, alias)) {
>           error_setg(errp, "machine alias '%s' already exists");
>           return false;
>       }

I don't think this is useful since object_property_add_const_link()
should abort in case of duplicate, which would be a modeling error
anyhow.

> 
>       return (object_property_add_const_link(aliases, alias, target) !=
>               NULL);
>    }
> 
> 
> 4) Should we create a page in the documentation explaining which devices/objects should be included in the alias list,

Any device in the QOM tree could have an alias. Why would you want
to limit the possible aliases? a part from dynamic ones.

> which ones are added automatically, 

That's dangerous. I would say none.

> and what naming conventions should be used for devices e.g. serial0, net0 for a network device etc.?

I would leave the choice to the person in charge of the SoC or the
machine.
  
> 5) What should happen to aliases for devices that are hot-plugged/hot-unplugged?
IMO, aliases are intended for devices "soldered" on the board,
created by the machine. Devices created via the command line or
QMP/HMP are excluded.


Thanks,

C.

Re: [PATCH 1/2] hw/arm: Add /machine/labels container for Aspeed machines

[Mark Cave-Ayland]

On 09/09/2026 12:15, Cédric Le Goater wrote:

> Hello,
> 
> On 9/9/26 12:35, Mark Cave-Ayland wrote:
>> On 04/09/2026 06:04, Cédric Le Goater wrote:
>>
>>> Add a DT-style alias container /machine/labels where board code 
>>> registers
>>> link<> properties pointing to well-known devices. Tests can then resolve
>>> a device with a single QMP call (qom-get /machine/labels/<name>) instead
>>> of walking the bus hierarchy.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   include/hw/arm/aspeed.h | 12 ++++++++++++
>>>   hw/arm/aspeed.c         | 10 ++++++++++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>>> index 245d02e5f757..0b8b40262767 100644
>>> --- a/include/hw/arm/aspeed.h
>>> +++ b/include/hw/arm/aspeed.h
>>> @@ -128,4 +128,16 @@ void 
>>> aspeed_machine_ast2600_class_emmc_init(ObjectClass *oc);
>>>    */
>>>   void aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc);
>>> +/*
>>> + * aspeed_machine_add_label:
>>> + * @bmc: pointer to the #AspeedMachineState.
>>> + * @label: the label name for the device.
>>> + * @target: the device object to register.
>>> + *
>>> + * Register a well-known device under /machine/labels/<label> as a
>>> + * read-only link.  Aborts on duplicate label names.
>>> + */
>>> +void aspeed_machine_add_label(AspeedMachineState *bmc, const char 
>>> *label,
>>> +                              Object *target);
>>> +
>>>   #endif
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index 1f8d5d3e132d..79b8d55d66da 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -127,6 +127,14 @@ void 
>>> aspeed_connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>>>       }
>>>   }
>>> +void aspeed_machine_add_label(AspeedMachineState *bmc, const char 
>>> *label,
>>> +                              Object *target)
>>> +{
>>> +    Object *labels = object_resolve_path_component(OBJECT(bmc), 
>>> "labels");
>>> +
>>> +    object_property_add_const_link(labels, label, target);
>>> +}
>>> +
>>>   static void aspeed_machine_init(MachineState *machine)
>>>   {
>>>       AspeedMachineState *bmc = ASPEED_MACHINE(machine);
>>> @@ -137,6 +145,8 @@ static void aspeed_machine_init(MachineState 
>>> *machine)
>>>       DriveInfo *emmc0 = NULL;
>>>       bool boot_emmc;
>>> +    object_property_add_new_container(OBJECT(machine), "labels");
>>> +
>>>       bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
>>>       object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc- 
>>> >soc));
>>>       object_unref(OBJECT(bmc->soc));
>>
>> Thanks for the proposal, Cédric! A few comments from me below:
>>
>> 1) Is there any reason it should be called labels as opposed to aliases? 
> 
> none
> 
>> The aliases name as used in Open Firmware feels more intuitive to me.
> 
> yes. It it just a name. aliases is fine for me unless it introduces
> some confusion with object_property_add_alias().

Hmmm good point. It seems an obvious distinction here, although maybe 
others would find it confusing? I'd be interested to hear other opinions 
here.
>> 2) If there is agreement in this approach, is there any reason why we 
>> shouldn't create /machine/labels (or equivalent) for all QOM trees? I 
>> certainly think it would be a useful addition going forward.
> 
> It is a trivial addition. We would need more maintainers to Ack
> the concept though.

Agreed.

>> 3) Is there any reason why we need to provide the machine object 
>> directly to the aspeed_machine_add_label() function?
> 
> Only because it was selfishly introduced as a Aspeed machine helper !

:)
>>  If possible I think it makes sense to avoid the direct machine 
>> reference, in case the underlying implementation changes i.e.
>>
>>
>>    void machine_add_alias(const char *alias, Object *target)
>>    {
>>       Object *aliases = object_resolve_path("/machine/aliases",  NULL);
>>
>>       object_property_add_const_link(aliases, alias, target);
>>    }
> 
> ok.
>> Even better perhaps we should also generate an error if the alias 
>> already exists to ensure they are always unique i.e.
>>
>>
>>    bool machine_add_alias(const char *alias, Object *target,
>>                           Error **errp)
>>    {
>>       Object *aliases = object_resolve_path("/machine/aliases",  NULL);
>>
>>       if (object_resolve_path_component(aliases, alias)) {
>>           error_setg(errp, "machine alias '%s' already exists");
>>           return false;
>>       }
> 
> I don't think this is useful since object_property_add_const_link()
> should abort in case of duplicate, which would be a modeling error
> anyhow.

Okay I didn't realise that - in that case it will already prevent 
duplicate aliases from being added.

>>       return (object_property_add_const_link(aliases, alias, target) !=
>>               NULL);
>>    }
>>
>>
>> 4) Should we create a page in the documentation explaining which 
>> devices/objects should be included in the alias list,
> 
> Any device in the QOM tree could have an alias. Why would you want
> to limit the possible aliases? a part from dynamic ones.

I was thinking in terms of automatically populating some entries e.g. 
for PCI buses, but from below I see this is not considered a good idea :)

When you say any *device* above, are you thinking of a specific 
restriction to devices, since I can see this would be useful for some 
buses too?
>> which ones are added automatically, 
> 
> That's dangerous. I would say none.

Fair enough. Should we enforce some basic rules e.g. all lower-case, no 
spaces, mandatory index suffix to try and keep things a bit consistent? 
(basically as they would appear in Open Firmware). Perhaps these should 
be enforced programmatically?
>> and what naming conventions should be used for devices e.g. serial0, 
>> net0 for a network device etc.?
> 
> I would leave the choice to the person in charge of the SoC or the
> machine.

Okay.

>> 5) What should happen to aliases for devices that are hot-plugged/hot- 
>> unplugged?
> IMO, aliases are intended for devices "soldered" on the board,
> created by the machine. Devices created via the command line or
> QMP/HMP are excluded.
Makes sense, but I thought I'd ask just in case the inevitable question 
comes up so at least it is documented somewhere ;)


ATB,

Mark.

Re: [PATCH 1/2] hw/arm: Add /machine/labels container for Aspeed machines

[Cédric Le Goater]

>> Any device in the QOM tree could have an alias. Why would you want
>> to limit the possible aliases? a part from dynamic ones.
> 
> I was thinking in terms of automatically populating some entries e.g. for PCI buses, but from below I see this is not considered a good idea :)

Ah. I imagine that once the container is available, the machine, the SoC,
buses, devices could populate it however they see fit.

> When you say any *device* above, are you thinking of a specific restriction to devices, since I can see this would be useful for some buses too?

The const link should apply to any QOM object I think. Give it a try :)

>>> which ones are added automatically, 
>>
>> That's dangerous. I would say none.
> 
> Fair enough. Should we enforce some basic rules e.g. all lower-case, no spaces, mandatory index suffix to try and keep things a bit consistent? (basically as they would appear in Open Firmware).

I don't think QEMU necessarily needs to follow the naming convention
of Open Firmware specification. A PPC or ARM machine could make sure
the names are compatible though or would you like the machine_add_alias()
helper to implement some dt fixup ?

> Perhaps these should be enforced programmatically?

Well, if it is a key requirement for a machine, perhaps it can be
implemented at the machine level. What do you have in mind ?

Cheers,

C.