diff mbox series

[3/5] bloblist: Tidy up the data alignment

Message ID 20200920004930.2108887-4-sjg@chromium.org
State Accepted
Commit 751b7c79634003b4cb326b79e80202ea45b75fc8
Delegated to: Simon Glass
Headers show
Series bloblist: Add alignment control and a command | expand

Commit Message

Simon Glass Sept. 20, 2020, 12:49 a.m. UTC
The intention which bloblists is that each blob's data is aligned in
memory. At present it is only the headers that are aligned.

Update the code to correct this and add a little more documentation.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bloblist.c | 32 ++++++++++++++++++++++++++------
 test/bloblist.c   | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 7 deletions(-)

Comments

Simon Glass Oct. 5, 2020, 9:32 p.m. UTC | #1
The intention which bloblists is that each blob's data is aligned in
memory. At present it is only the headers that are aligned.

Update the code to correct this and add a little more documentation.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bloblist.c | 32 ++++++++++++++++++++++++++------
 test/bloblist.c   | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 7 deletions(-)

Applied to u-boot-dm/next, thanks!
Rasmus Villemoes Oct. 6, 2020, 5:41 a.m. UTC | #2
On 20/09/2020 02.49, Simon Glass wrote:
> The intention which bloblists is that each blob's data is aligned in
> memory. At present it is only the headers that are aligned.
> 
> Update the code to correct this and add a little more documentation.

Hi Simon

I haven't read this patch in detail, but I have a general request
regarding changes to the bloblist layout: Backwards compatibility - in
the sense that, if the bloblist is generated by SPL from 2020.04, that
should also be usable from a U-Boot proper as of 2021.01 etc.

While I/we don't actually use bloblist currently, we have some ideas for
what we might use it for on various boards. But, on at least one board,
our SPL is stored in read-only storage after production, while we have
redundant (and updatable) copies of U-Boot proper - so it's important
that a future U-Boot can also grok a bloblist generated by earlier code.

Is that guaranteed?

Thanks,
Rasmus
Simon Glass Oct. 6, 2020, 10:02 p.m. UTC | #3
Hi Rasmus,

On Mon, 5 Oct 2020 at 23:41, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 20/09/2020 02.49, Simon Glass wrote:
> > The intention which bloblists is that each blob's data is aligned in
> > memory. At present it is only the headers that are aligned.
> >
> > Update the code to correct this and add a little more documentation.
>
> Hi Simon
>
> I haven't read this patch in detail, but I have a general request
> regarding changes to the bloblist layout: Backwards compatibility - in
> the sense that, if the bloblist is generated by SPL from 2020.04, that
> should also be usable from a U-Boot proper as of 2021.01 etc.
>
> While I/we don't actually use bloblist currently, we have some ideas for
> what we might use it for on various boards. But, on at least one board,
> our SPL is stored in read-only storage after production, while we have
> redundant (and updatable) copies of U-Boot proper - so it's important
> that a future U-Boot can also grok a bloblist generated by earlier code.
>
> Is that guaranteed?

It is intended.

If we add a test for this we can provide that guarantee, yes. The test
will need to encode a pre-built binary bloblist though, not use the
code in master to generate it.

Regards,
Simon
diff mbox series

Patch

diff --git a/common/bloblist.c b/common/bloblist.c
index c86ea029c8d..173f28d8ec9 100644
--- a/common/bloblist.c
+++ b/common/bloblist.c
@@ -11,6 +11,20 @@ 
 #include <spl.h>
 #include <u-boot/crc.h>
 
+/*
+ * A bloblist is a single contiguous chunk of memory with a header
+ * (struct bloblist_hdr) and a number of blobs in it.
+ *
+ * Each blob starts on a BLOBLIST_ALIGN boundary relative to the start of the
+ * bloblist and consists of a struct bloblist_rec, some padding to the required
+ * alignment for the blog and then the actual data. The padding ensures that the
+ * start address of the data in each blob is aligned as required. Note that
+ * each blob's *data* is aligned to BLOBLIST_ALIGN regardless of the alignment
+ * of the bloblist itself or the blob header.
+ *
+ * So far, only BLOBLIST_ALIGN alignment is supported.
+ */
+
 DECLARE_GLOBAL_DATA_PTR;
 
 static const char *const tag_name[] = {
@@ -73,9 +87,14 @@  static int bloblist_addrec(uint tag, int size, struct bloblist_rec **recp)
 {
 	struct bloblist_hdr *hdr = gd->bloblist;
 	struct bloblist_rec *rec;
-	int new_alloced;
+	int data_start, new_alloced;
+
+	/* Figure out where the new data will start */
+	data_start = hdr->alloced + sizeof(*rec);
+	data_start = ALIGN(data_start, BLOBLIST_ALIGN);
 
-	new_alloced = hdr->alloced + sizeof(*rec) + ALIGN(size, BLOBLIST_ALIGN);
+	/* Calculate the new allocated total */
+	new_alloced = data_start + ALIGN(size, BLOBLIST_ALIGN);
 	if (new_alloced >= hdr->size) {
 		log(LOGC_BLOBLIST, LOGL_ERR,
 		    "Failed to allocate %x bytes size=%x, need size=%x\n",
@@ -83,15 +102,16 @@  static int bloblist_addrec(uint tag, int size, struct bloblist_rec **recp)
 		return log_msg_ret("bloblist add", -ENOSPC);
 	}
 	rec = (void *)hdr + hdr->alloced;
-	hdr->alloced = new_alloced;
 
 	rec->tag = tag;
-	rec->hdr_size = sizeof(*rec);
+	rec->hdr_size = data_start - hdr->alloced;
 	rec->size = size;
 	rec->spare = 0;
 
 	/* Zero the record data */
-	memset(rec + 1, '\0', rec->size);
+	memset((void *)rec + rec->hdr_size, '\0', rec->size);
+
+	hdr->alloced = new_alloced;
 	*recp = rec;
 
 	return 0;
@@ -139,7 +159,7 @@  void *bloblist_add(uint tag, int size)
 	if (bloblist_addrec(tag, size, &rec))
 		return NULL;
 
-	return rec + 1;
+	return (void *)rec + rec->hdr_size;
 }
 
 int bloblist_ensure_size(uint tag, int size, void **blobp)
diff --git a/test/bloblist.c b/test/bloblist.c
index 271fe9f5d7f..3493e681f39 100644
--- a/test/bloblist.c
+++ b/test/bloblist.c
@@ -29,6 +29,8 @@  enum {
 
 	TEST_ADDR		= CONFIG_BLOBLIST_ADDR,
 	TEST_BLOBLIST_SIZE	= 0x100,
+
+	ERASE_BYTE		= '\xff',
 };
 
 static struct bloblist_hdr *clear_bloblist(void)
@@ -41,7 +43,7 @@  static struct bloblist_hdr *clear_bloblist(void)
 	 * to 0xff for testing purposes.
 	 */
 	hdr = map_sysmem(CONFIG_BLOBLIST_ADDR, TEST_BLOBLIST_SIZE);
-	memset(hdr, '\xff', TEST_BLOBLIST_SIZE);
+	memset(hdr, ERASE_BYTE, TEST_BLOBLIST_SIZE);
 	memset(hdr, '\0', sizeof(*hdr));
 
 	return hdr;
@@ -292,6 +294,40 @@  static int bloblist_test_cmd_list(struct unit_test_state *uts)
 }
 BLOBLIST_TEST(bloblist_test_cmd_list, 0);
 
+/* Test alignment of bloblist blobs */
+static int bloblist_test_align(struct unit_test_state *uts)
+{
+	struct bloblist_hdr *hdr;
+	int i;
+
+	/* At the start there should be no records */
+	hdr = clear_bloblist();
+	ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0));
+	ut_assertnull(bloblist_find(TEST_TAG, TEST_BLOBLIST_SIZE));
+
+	/* Check the alignment */
+	for (i = 0; i < 3; i++) {
+		int size = i * 3;
+		ulong addr;
+		char *data;
+		int j;
+
+		data = bloblist_add(i, size);
+		ut_assertnonnull(data);
+		addr = map_to_sysmem(data);
+		ut_asserteq(0, addr & (BLOBLIST_ALIGN - 1));
+
+		/* Only the bytes in the blob data should be zeroed */
+		for (j = 0; j < size; j++)
+			ut_asserteq(0, data[j]);
+		for (; j < BLOBLIST_ALIGN; j++)
+			ut_asserteq(ERASE_BYTE, data[j]);
+	}
+
+	return 0;
+}
+BLOBLIST_TEST(bloblist_test_align, 0);
+
 int do_ut_bloblist(struct cmd_tbl *cmdtp, int flag, int argc,
 		   char *const argv[])
 {