On Fri, 4 Apr 2025, Alexey Kardashevskiy wrote:
On Tue, 1 Apr 2025, at 01:26, BALATON Zoltan wrote:
The FDT does not normally store name properties but reconstructs it
from path but each node in Open Firmware should at least have this
property. This is correctly handled in getprop but nextprop should
also return it even if not present as a property. This patch fixes
that and also skips phandle which does not appear in Open Firmware
and only added for internal use by VOF.
Explicit name properties are still allowed because they are needed
e.g. on the root node that guests expect to have specific names as
seen on real machines instead of being empty so sometimes the node
name may need to be overriden.
Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
---
I've tested this with pegasos2 but don't know how to test spapr.
v2:
Fixed a typo in commit message
Simplified loop to get next property name
hw/ppc/vof.c | 51 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 17 deletions(-)
diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
index 09cb77de93..790d67c096 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -353,34 +353,51 @@ static uint32_t vof_nextprop(const void *fdt, uint32_t
phandle,
{
int offset, nodeoff = fdt_node_offset_by_phandle(fdt, phandle);
char prev[OF_PROPNAME_LEN_MAX + 1];
- const char *tmp;
+ const char *tmp = NULL;
+ bool match = false;
if (readstr(prevaddr, prev, sizeof(prev))) {
return PROM_ERROR;
}
-
- fdt_for_each_property_offset(offset, fdt, nodeoff) {
- if (!fdt_getprop_by_offset(fdt, offset, &tmp, NULL)) {
- return 0;
+ /*
+ * "name" may or may not be present in fdt but we should still return it.
yeah we should, at least, to match "getprop". I also wonder if VOF does
not add "name", then what would do so, do we really expect to see such
properties anywhere? Because if not, then we do not need to skip it as
we won't find it.
I have to add it to fdt where needed. For example on pegasos MorphOS
checks the name of "/" and expects to find bplan,Pegasos2 which is how it
identifies the machine. So we need a specific name property there which is
one example when there will be explicit name property in the fdt. Maybe
it's needed on some other nodes sometimes but normally the default will be
sufficient but not always.
+ * Do that first and then skip it if seen later. Also skip phandle which is
(a nit) appears to me that if handling of a missing "name" was done
after the last property, the patch would look simpler, but not sure and
do not insist.
I put name first to match what OpenFirmware does which returns name first.
SLOF seems to do everything backwards (maybe a result of parsing the fdt
to build the device tree) and returns properties with inverted order so
name is last on SLOF but even then the order is matched by putting name
first when we return properties in the normal order otherwise it would not
be in same order when reversed. I don't know if it's significant, some
guests may expect to get a name first but most would probably look for the
name not its position. The order now matches OpenFirmware and pegasos2
SmartFirmware and SLOF backwards so I think name is now where it should be
so I'd leave it first. The loop may become simpler if we don't skip
phandle only name, the complexity is mostly from sometimes we need to skip
both in a row.
+ * an internal value we added in vof_build_dt but should not appear here.
I would not hide anything though, unless it breaks something. Thanks,
I did some tests with SLOF. This is what I get from SLOF:
package 0x1e64a890 /vdevice/v-scsi@71000003:
slof,from-fdt 0
reg 71000003
device_type "vscsi"
compatible "IBM,v-scsi"
interrupts [0x8 bytes, 2 cells]
[000] 00001103 00000000
ibm,#dma-address-cells 2
ibm,#dma-size-cells 2
ibm,my-dma-window "q"
#address-cells 2
#size-cells 0
name "v-scsi"
This is VOF before patch:
package 0x00001122 /vdevice/v-scsi@71000003:
phandle 1122
#size-cells 0
#address-cells 2
ibm,my-dma-window "q"
ibm,#dma-size-cells 2
ibm,#dma-address-cells 2
interrupts [0x8 bytes, 2 cells]
[000] 00001103 00000000
compatible "IBM,v-scsi"
device_type "vscsi"
reg 71000003
and this is VOF after patch:
package 0x00001122 /vdevice/v-scsi@71000003:
name "v-scsi"
#size-cells 0
#address-cells 2
ibm,my-dma-window "q"
ibm,#dma-size-cells 2
ibm,#dma-address-cells 2
interrupts [0x8 bytes, 2 cells]
[000] 00001103 00000000
compatible "IBM,v-scsi"
device_type "vscsi"
reg 71000003
Apart from SLOF returning properties backwards this now matches better.
SLOF or other Open Firmware implementations don't return phandle property
because that's what you pass to nextprop or getprop to get the property in
the first place (listed next to package above) and it is returned by
finddevice so not normally stored as a property. But if Linux would add it
and it helps Linux to have it there already we can keep it, it did not
break OSes on pegasos as they only parse properties they need and ignore
the rest. So I can skip skipping phandle and add that back if it would be
better for Linux but didn't removing it fixed a warning about it too?
By the way, phandle is identifying the node so can't we use the fdt offset
for it so we don't have to add phandle properties? Or does offset change
when editing the fdt? I think libfdt also uses offsets to identify nodes
so maybe these should be somewhat stable.
Regards,
BALATON Zoltan
