| Message ID | 20250331142627.BAA2F4E6029@zero.eik.bme.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ppc/vof: Make nextprop behave more like Open Firmware | expand |
On Tue Apr 1, 2025 at 12:26 AM AEST, 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 <balaton@eik.bme.hu> > --- > I've tested this with pegasos2 but don't know how to test spapr. Boot a pseries machine with pseries (book3s 64-bit) Linux kernel with x-vof=on option. AFAIKS the two places Linux calls nextprop look like this if (call_prom("nextprop", 3, 1, node, prev_name, pname) != 1) break; /* skip "name" */ if (prom_strcmp(pname, "name") == 0) { prev_name = "name"; continue; } So, seems like skipping name is okay? After iterating through properties it also has this: /* Add a "phandle" property if none already exist */ if (!has_phandle) { soff = dt_find_string("phandle"); if (soff == 0) prom_printf("WARNING: Can't find string index for <phandle> node %s\n", path); That warning does not seem to fire after your patch. spapr *seems* to be okay booting, but I would not be inclined to take this for 10.0 at least without review from someone who knows more than I do about OF since there can be subtle breakage. What actual problem is it causing for pegasos? Thanks, Nick
On Tue, 1 Apr 2025, Nicholas Piggin wrote: > On Tue Apr 1, 2025 at 12:26 AM AEST, 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 <balaton@eik.bme.hu> >> --- >> I've tested this with pegasos2 but don't know how to test spapr. > > Boot a pseries machine with pseries (book3s 64-bit) Linux kernel > with x-vof=on option. > > AFAIKS the two places Linux calls nextprop look like this > > if (call_prom("nextprop", 3, 1, node, prev_name, > pname) != 1) > break; > > /* skip "name" */ > if (prom_strcmp(pname, "name") == 0) { > prev_name = "name"; > continue; > } > > So, seems like skipping name is okay? For Linux maybe OK to not have name but other OSes use it to identify devices so we need it. I tried to match what the real pegasos firmware returns but likely SLOF does the same if you do .properties in a node, I have not tried SLOF but I can try if that helps. I've seen some OpenFirmware output from Macs which also have name in property list and OF specification says each node should have a name property so I think it's not OK to skip it and not returning it from nextprop does not work for pegasos which now has to add explicit name properties in pegasos2.c to fix this. > After iterating through properties it also has this: > > /* Add a "phandle" property if none already exist */ > if (!has_phandle) { > soff = dt_find_string("phandle"); > if (soff == 0) > prom_printf("WARNING: Can't find string index for <phandle> node %s\n", path); > > That warning does not seem to fire after your patch. Is that good or bad? Was it firing before? If so, getting rid of it may be good but I can leave phandle there if it helps Linux. Other OSes don't seem to care but it does not seem to appear on real OF results that's why I also removed it but I could leave it in if you think it's better that way. > spapr *seems* to be okay booting, but I would not be inclined to > take this for 10.0 at least without review from someone who knows > more than I do about OF since there can be subtle breakage. > > What actual problem is it causing for pegasos? Sorry I did not make it clear this is for 10.1 not 10.0, that's why it does not say what does it fix. I want to clean up pegasos2.c a bit after the freeze and this allows me to remove all the explicit name properties which are now only needed because while the name property works with getprop, it is not queried due to missing from nextprop. I've just submitted this patch in advance to get it reviewed and hopefully merged after the freeze so I can submit the patches that depend on it later. I don't have any fixes for 10.0 so that should be fine for now, these are for after the freeze. Regards, BALATON Zoltan
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. + * Do that first and then skip it if seen later. Also skip phandle which is + * an internal value we added in vof_build_dt but should not appear here. + */ + if (prev[0] == '\0') { + tmp = "name"; + } else { + if (strcmp(prev, "name") == 0) { + prev[0] = '\0'; } - if (prev[0] == '\0' || strcmp(prev, tmp) == 0) { - if (prev[0] != '\0') { - offset = fdt_next_property_offset(fdt, offset); - if (offset < 0) { - return 0; - } - } + fdt_for_each_property_offset(offset, fdt, nodeoff) { if (!fdt_getprop_by_offset(fdt, offset, &tmp, NULL)) { return 0; } - - if (VOF_MEM_WRITE(nameaddr, tmp, strlen(tmp) + 1) != MEMTX_OK) { - return PROM_ERROR; + if (strcmp(tmp, "name") == 0 || strcmp(tmp, "phandle") == 0) { + continue; + } + if (match) { + break; } - return 1; + if (strcmp(prev, tmp) == 0) { + match = true; + continue; + } + if (prev[0] == '\0') { + break; + } + } + if (offset < 0) { + return 0; } } - + if (tmp) { + if (VOF_MEM_WRITE(nameaddr, tmp, strlen(tmp) + 1) != MEMTX_OK) { + return PROM_ERROR; + } + return 1; + } return 0; }
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 <balaton@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(-)