On Wed, Mar 20, 2019 at 05:36:15PM -0700, Frank Rowand wrote:
> On 3/20/19 8:10 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> >
> > The new fdt_generate_phandle() function can be used to generate a new,
> > unused phandle given a specific device tree blob. The implementation is
> > somewhat naive in that it simply walks the entire device tree to find
> > the highest phandle value and then returns a phandle value one higher
> > than that. A more clever implementation might try to find holes in the
> > current set of phandle values and fill them. But this implementation is
> > relatively simple and works reliably.
> >
> > Also add a test that validates that phandles generated by this new API
> > are indeed unique.
>
> I don't see any issues.
>
> I am curious as to what the envisioned use is for fdt_generate_phandle().
The idea here is to allow firmware to pass information about carveout
areas to the Linux kernel (and potentially other OSes). To specify the
carveouts, it needs to create subnodes in the top-level /reserved-memory
node and attach a phandle to those nodes so that they can later be
referenced by users of the carveouts.
One such example would be a framebuffer carveout set up by early
firmware to show a boot splash. In order to prevent the OS kernel from
recycling the framebuffer carveout for its allocator, the firmware needs
to marks it as reserved memory. This is what the child nodes in the top-
level /reserved-memory node do. Then the display controller that's
scanning out the framebuffer from that carveout needs to be passed a
reference to the reserved memory node (via its phandle, which is stored
in the display controller node's memory-region property).
The display controller driver can then use that memory-region property
to look up the reserved memory region and reuse it, copy out data, ...
This information can also be used on early OS boot to setup things like
IOMMU page tables so that there's initially a 1:1 mapping of physical
to IO virtual addresses for the carveout. This is necessary because the
display controller may otherwise cause a lot of IOMMU faults trying to
access a physical memory address through the IOMMU.
Does that clarify the use-case?
Thanks,
Thierry
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - add fdt_generate_phandle() to version.lds
> >
> > Changes in v2:
> > - Handle the case where the number of phandles has been exhausted. Also
> > add more testing to exercise this corner case. Note that this is tied
> > to the current, naive implementation of fdt_generate_phandle(). If it
> > is ever changed to be more clevel, the tests will have to be updated.
> > ---
> > libfdt/fdt_ro.c | 31 +++++++++++++++++++++++++++++++
> > libfdt/libfdt.h | 19 +++++++++++++++++++
> > libfdt/libfdt_env.h | 1 +
> > libfdt/version.lds | 1 +
> > tests/get_phandle.c | 31 ++++++++++++++++++++++++++++++-
> > tests/multilabel.dts | 5 +++++
> > tests/multilabel_merge.dts | 5 +++++
> > tests/references.c | 19 +++++++++++++++++--
> > tests/references.dts | 5 +++++
> > 9 files changed, 114 insertions(+), 3 deletions(-)
> >
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index eafc14282892..97cdcda0dbec 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -170,6 +170,37 @@ uint32_t fdt_get_max_phandle(const void *fdt)
> > return 0;
> > }
> >
> > +int fdt_generate_phandle(const void *fdt, uint32_t *phandle)
> > +{
> > + uint32_t max = 0;
> > + int offset = -1;
> > +
> > + while (true) {
> > + uint32_t value;
> > +
> > + offset = fdt_next_node(fdt, offset, NULL);
> > + if (offset < 0) {
> > + if (offset == -FDT_ERR_NOTFOUND)
> > + break;
> > +
> > + return offset;
> > + }
> > +
> > + value = fdt_get_phandle(fdt, offset);
> > +
> > + if (value > max)
> > + max = value;
> > + }
> > +
> > + if (max == FDT_MAX_PHANDLE)
> > + return -FDT_ERR_NOPHANDLES;
> > +
> > + if (phandle)
> > + *phandle = max + 1;
> > +
> > + return 0;
> > +}
> > +
> > static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
> > {
> > int offset = n * sizeof(struct fdt_reserve_entry);
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index a470d1df6d2a..7e102d188e86 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -140,6 +140,10 @@
> >
> > #define FDT_ERR_MAX 17
> >
> > +/* constants */
> > +#define FDT_MAX_PHANDLE 0xfffffffe
> > + /* Valid values for phandles range from 1 to 2^32-2. */
> > +
> > /**********************************************************************/
> > /* Low-level functions (you probably don't need these) */
> > /**********************************************************************/
> > @@ -376,6 +380,21 @@ const char *fdt_string(const void *fdt, int stroffset);
> > */
> > uint32_t fdt_get_max_phandle(const void *fdt);
> >
> > +/**
> > + * fdt_generate_phandle - return a new, unused phandle for a device tree blob
> > + * @fdt: pointer to the device tree blob
> > + * @phandle: return location for the new phandle
> > + *
> > + * Walks the device tree blob and looks for the highest phandle value. On
> > + * success, the new, unused phandle value (one higher than the previously
> > + * highest phandle value in the device tree blob) will be returned in the
> > + * @phandle parameter.
> > + *
> > + * Returns:
> > + * 0 on success or a negative error-code on failure
> > + */
> > +int fdt_generate_phandle(const void *fdt, uint32_t *phandle);
> > +
> > /**
> > * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
> > * @fdt: pointer to the device tree blob
> > diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h
> > index eb2053845c9c..4d1cdfa58547 100644
> > --- a/libfdt/libfdt_env.h
> > +++ b/libfdt/libfdt_env.h
> > @@ -52,6 +52,7 @@
> > * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > */
> >
> > +#include <stdbool.h>
> > #include <stddef.h>
> > #include <stdint.h>
> > #include <stdlib.h>
> > diff --git a/libfdt/version.lds b/libfdt/version.lds
> > index 9f5d70880f24..5897f1e288a3 100644
> > --- a/libfdt/version.lds
> > +++ b/libfdt/version.lds
> > @@ -66,6 +66,7 @@ LIBFDT_1.2 {
> > fdt_resize;
> > fdt_overlay_apply;
> > fdt_get_string;
> > + fdt_generate_phandle;
> > local:
> > *;
> > };
> > diff --git a/tests/get_phandle.c b/tests/get_phandle.c
> > index 22bd7b81b3f0..d87aee50b065 100644
> > --- a/tests/get_phandle.c
> > +++ b/tests/get_phandle.c
> > @@ -17,6 +17,7 @@
> > * License along with this library; if not, write to the Free Software
> > * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> > */
> > +#include <stdbool.h>
> > #include <stdlib.h>
> > #include <stdio.h>
> > #include <string.h>
> > @@ -42,10 +43,32 @@ static void check_phandle(void *fdt, const char *path, uint32_t checkhandle)
> > path, phandle, checkhandle);
> > }
> >
> > +static void check_phandle_unique(const void *fdt, uint32_t checkhandle)
> > +{
> > + uint32_t phandle;
> > + int offset = -1;
> > +
> > + while (true) {
> > + offset = fdt_next_node(fdt, offset, NULL);
> > + if (offset < 0) {
> > + if (offset == -FDT_ERR_NOTFOUND)
> > + break;
> > +
> > + FAIL("error looking for phandle %#x", checkhandle);
> > + }
> > +
> > + phandle = fdt_get_phandle(fdt, offset);
> > +
> > + if (phandle == checkhandle)
> > + FAIL("generated phandle already exists");
> > + }
> > +}
> > +
> > int main(int argc, char *argv[])
> > {
> > - uint32_t max;
> > + uint32_t max, phandle;
> > void *fdt;
> > + int err;
> >
> > test_init(argc, argv);
> > fdt = load_blob_arg(argc, argv);
> > @@ -59,5 +82,11 @@ int main(int argc, char *argv[])
> > FAIL("fdt_get_max_phandle returned 0x%x instead of 0x%x\n",
> > max, PHANDLE_2);
> >
> > + err = fdt_generate_phandle(fdt, &phandle);
> > + if (err < 0)
> > + FAIL("failed to generate phandle: %d", err);
> > +
> > + check_phandle_unique(fdt, phandle);
> > +
> > PASS();
> > }
> > diff --git a/tests/multilabel.dts b/tests/multilabel.dts
> > index 77da06cc4174..d80ebe1fb082 100644
> > --- a/tests/multilabel.dts
> > +++ b/tests/multilabel.dts
> > @@ -41,4 +41,9 @@ m1: mq: /memreserve/ 0 0x1000;
> > n2 = &n2;
> > n3 = &n3;
> > };
> > +
> > + node6 {
> > + linux,phandle = <0xfffffffe>;
> > + phandle = <0xfffffffe>;
> > + };
> > };
> > diff --git a/tests/multilabel_merge.dts b/tests/multilabel_merge.dts
> > index 3e8029897405..a27d856f80dd 100644
> > --- a/tests/multilabel_merge.dts
> > +++ b/tests/multilabel_merge.dts
> > @@ -39,6 +39,11 @@ m1: mq: /memreserve/ 0 0x1000;
> > n2 = &n2;
> > n3 = &n3;
> > };
> > +
> > + node6 {
> > + linux,phandle = <0xfffffffe>;
> > + phandle = <0xfffffffe>;
> > + };
> > };
> >
> > / {
> > diff --git a/tests/references.c b/tests/references.c
> > index cab70f6579af..5b233a4bda20 100644
> > --- a/tests/references.c
> > +++ b/tests/references.c
> > @@ -76,8 +76,8 @@ static void check_rref(const void *fdt)
> > int main(int argc, char *argv[])
> > {
> > void *fdt;
> > - int n1, n2, n3, n4, n5;
> > - uint32_t h1, h2, h4, h5;
> > + int n1, n2, n3, n4, n5, n6, err;
> > + uint32_t h1, h2, h4, h5, h6, hn;
> >
> > test_init(argc, argv);
> > fdt = load_blob_arg(argc, argv);
> > @@ -97,11 +97,15 @@ int main(int argc, char *argv[])
> > n5 = fdt_path_offset(fdt, "/node5");
> > if (n5 < 0)
> > FAIL("fdt_path_offset(/node5): %s", fdt_strerror(n5));
> > + n6 = fdt_path_offset(fdt, "/node6");
> > + if (n6 < 0)
> > + FAIL("fdt_path_offset(/node6): %s", fdt_strerror(n6));
> >
> > h1 = fdt_get_phandle(fdt, n1);
> > h2 = fdt_get_phandle(fdt, n2);
> > h4 = fdt_get_phandle(fdt, n4);
> > h5 = fdt_get_phandle(fdt, n5);
> > + h6 = fdt_get_phandle(fdt, n6);
> >
> > if (h1 != 0x2000)
> > FAIL("/node1 has wrong phandle, 0x%x instead of 0x%x",
> > @@ -109,6 +113,9 @@ int main(int argc, char *argv[])
> > if (h2 != 0x1)
> > FAIL("/node2 has wrong phandle, 0x%x instead of 0x%x",
> > h2, 0x1);
> > + if (h6 != FDT_MAX_PHANDLE)
> > + FAIL("/node6 has wrong phandle, 0x%x instead of 0x%x",
> > + h6, FDT_MAX_PHANDLE);
> > if ((h4 == 0x2000) || (h4 == 0x1) || (h4 == 0))
> > FAIL("/node4 has bad phandle, 0x%x", h4);
> >
> > @@ -117,6 +124,14 @@ int main(int argc, char *argv[])
> > if ((h5 == h4) || (h5 == h2) || (h5 == h1))
> > FAIL("/node5 has duplicate phandle, 0x%x", h5);
> >
> > + /*
> > + * /node6 has phandle FDT_MAX_PHANDLE, so fdt_generate_phandle() is
> > + * expected to fail.
> > + */
> > + err = fdt_generate_phandle(fdt, &hn);
> > + if (err != -FDT_ERR_NOPHANDLES)
> > + FAIL("generated invalid phandle 0x%x\n", hn);
> > +
> > check_ref(fdt, n1, h2);
> > check_ref(fdt, n2, h1);
> > check_ref(fdt, n3, h4);
> > diff --git a/tests/references.dts b/tests/references.dts
> > index f783e8bc8643..b39063999189 100644
> > --- a/tests/references.dts
> > +++ b/tests/references.dts
> > @@ -33,4 +33,9 @@
> > linux,phandle = <&n5>;
> > phandle = <&n5>;
> > };
> > +
> > + node6 {
> > + linux,phandle = <0xfffffffe>;
> > + phandle = <0xfffffffe>;
> > + };
> > };
> >
>
Attachment:
signature.asc
Description: PGP signature
