Re: [PATCH v4 08/12] bloblist: Handle alignment with a void entry
On Thu, 28 Dec 2023 at 18:04, Raymond Mao wrote:
>
> Hi Ilias,
>
> On Thu, 28 Dec 2023 at 06:32, Ilias Apalodimas
> wrote:
>>
>> Hi Raymond,
>>
>> On Wed, 27 Dec 2023 at 23:08, Raymond Mao wrote:
>> >
>> > From: Simon Glass
>> >
>> > Rather than setting the alignment using the header size, add an entirely
>> > new entry to cover the gap left by the alignment.
>> >
>> > Signed-off-by: Simon Glass
>> > Co-developed-by: Raymond Mao
>> > Signed-off-by: Raymond Mao
>> > Reviewed-by: Simon Glass
>> > ---
>> > common/bloblist.c | 23 +++
>> > 1 file changed, 19 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/common/bloblist.c b/common/bloblist.c
>> > index 705d9c6ae9..73dbbc01c0 100644
>> > --- a/common/bloblist.c
>> > +++ b/common/bloblist.c
>> > @@ -142,7 +142,7 @@ static int bloblist_addrec(uint tag, int size, int
>> > align_log2,
>> > {
>> > struct bloblist_hdr *hdr = gd->bloblist;
>> > struct bloblist_rec *rec;
>> > - int data_start, new_alloced;
>> > + int data_start, aligned_start, new_alloced;
>> >
>> > if (!align_log2)
>> > align_log2 = BLOBLIST_ALIGN_LOG2;
>> > @@ -151,10 +151,25 @@ static int bloblist_addrec(uint tag, int size, int
>> > align_log2,
>> > data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*rec);
>> >
>> > /* Align the address and then calculate the offset from ->alloced
>> > */
>> > - data_start = ALIGN(data_start, 1U << align_log2) -
>> > map_to_sysmem(hdr);
>> > + aligned_start = ALIGN(data_start, 1U << align_log2) - data_start;
>> > +
>> > + /* If we need to create a dummy record, create it */
>> > + if (aligned_start) {
>> > + int void_size = aligned_start - sizeof(*rec);
>> > + struct bloblist_rec *vrec;
>> > + int ret;
>> > +
>> > + ret = bloblist_addrec(BLOBLISTT_VOID, void_size, 0, );
>>
>> I just noticed the 'BLOBLISTT'. Is that on purpose? or a typo?
>>
>> > + if (ret)
>> > + return log_msg_ret("void", ret);
>> > +
>> > + /* start the record after that */
>> > + data_start = map_to_sysmem(hdr) + hdr->alloced +
>> > sizeof(*vrec);
>> > + }
>> >
>> > /* Calculate the new allocated total */
>> > - new_alloced = data_start + ALIGN(size, 1U << align_log2);
>> > + new_alloced = data_start - map_to_sysmem(hdr) +
>> > + ALIGN(size, 1U << align_log2);
>>
Writing this as
new_alloced = hdr->alloced + sizeof(*rec) + ALIGN(size, 1U << align_log2);
is much more readable
>> So, wouldn't it make more sense to add the dummy record and align the
>> whole thing *after* we've added the entry?
>> Doing it like this might leave the last entry on an unaligned boundary, no?
>>
> The spec just cares about the TE *data* starts at an aligned address but not
> the TE header.
> Not sure if I fully understand your question but each TE *data* will start at
> an aligned address
> after calling `bloblist_addrec()`.
> And each TE is allowed to have its own alignment value, so we have to do the
> padding when
> a TE is being added but cannot predict the alignment value for the next TE -
> that means we
> cannot do the padding after each TE is added.
>
Ah thanks, this makes sense
> [...]
>
> Thanks and regards,
> Raymond
With the change above
Reviewed-by: Ilias Apalodimas
Re: [PATCH v4 08/12] bloblist: Handle alignment with a void entry
Hi Ilias,
On Thu, 28 Dec 2023 at 06:32, Ilias Apalodimas
wrote:
> Hi Raymond,
>
> On Wed, 27 Dec 2023 at 23:08, Raymond Mao wrote:
> >
> > From: Simon Glass
> >
> > Rather than setting the alignment using the header size, add an entirely
> > new entry to cover the gap left by the alignment.
> >
> > Signed-off-by: Simon Glass
> > Co-developed-by: Raymond Mao
> > Signed-off-by: Raymond Mao
> > Reviewed-by: Simon Glass
> > ---
> > common/bloblist.c | 23 +++
> > 1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/common/bloblist.c b/common/bloblist.c
> > index 705d9c6ae9..73dbbc01c0 100644
> > --- a/common/bloblist.c
> > +++ b/common/bloblist.c
> > @@ -142,7 +142,7 @@ static int bloblist_addrec(uint tag, int size, int
> align_log2,
> > {
> > struct bloblist_hdr *hdr = gd->bloblist;
> > struct bloblist_rec *rec;
> > - int data_start, new_alloced;
> > + int data_start, aligned_start, new_alloced;
> >
> > if (!align_log2)
> > align_log2 = BLOBLIST_ALIGN_LOG2;
> > @@ -151,10 +151,25 @@ static int bloblist_addrec(uint tag, int size, int
> align_log2,
> > data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*rec);
> >
> > /* Align the address and then calculate the offset from
> ->alloced */
> > - data_start = ALIGN(data_start, 1U << align_log2) -
> map_to_sysmem(hdr);
> > + aligned_start = ALIGN(data_start, 1U << align_log2) - data_start;
> > +
> > + /* If we need to create a dummy record, create it */
> > + if (aligned_start) {
> > + int void_size = aligned_start - sizeof(*rec);
> > + struct bloblist_rec *vrec;
> > + int ret;
> > +
> > + ret = bloblist_addrec(BLOBLISTT_VOID, void_size, 0,
> );
>
> I just noticed the 'BLOBLISTT'. Is that on purpose? or a typo?
>
> > + if (ret)
> > + return log_msg_ret("void", ret);
> > +
> > + /* start the record after that */
> > + data_start = map_to_sysmem(hdr) + hdr->alloced +
> sizeof(*vrec);
> > + }
> >
> > /* Calculate the new allocated total */
> > - new_alloced = data_start + ALIGN(size, 1U << align_log2);
> > + new_alloced = data_start - map_to_sysmem(hdr) +
> > + ALIGN(size, 1U << align_log2);
>
> So, wouldn't it make more sense to add the dummy record and align the
> whole thing *after* we've added the entry?
> Doing it like this might leave the last entry on an unaligned boundary, no?
>
> The spec just cares about the TE *data* starts at an aligned address but
not the TE header.
Not sure if I fully understand your question but each TE *data* will start
at an aligned address
after calling `bloblist_addrec()`.
And each TE is allowed to have its own alignment value, so we have to do
the padding when
a TE is being added but cannot predict the alignment value for the next TE
- that means we
cannot do the padding after each TE is added.
[...]
Thanks and regards,
Raymond
Re: [PATCH v4 08/12] bloblist: Handle alignment with a void entry
Hi Ilias,
On Thu, Dec 28, 2023 at 11:32 AM Ilias Apalodimas
wrote:
>
> Hi Raymond,
>
> On Wed, 27 Dec 2023 at 23:08, Raymond Mao wrote:
> >
> > From: Simon Glass
> >
> > Rather than setting the alignment using the header size, add an entirely
> > new entry to cover the gap left by the alignment.
> >
> > Signed-off-by: Simon Glass
> > Co-developed-by: Raymond Mao
> > Signed-off-by: Raymond Mao
> > Reviewed-by: Simon Glass
> > ---
> > common/bloblist.c | 23 +++
> > 1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/common/bloblist.c b/common/bloblist.c
> > index 705d9c6ae9..73dbbc01c0 100644
> > --- a/common/bloblist.c
> > +++ b/common/bloblist.c
> > @@ -142,7 +142,7 @@ static int bloblist_addrec(uint tag, int size, int
> > align_log2,
> > {
> > struct bloblist_hdr *hdr = gd->bloblist;
> > struct bloblist_rec *rec;
> > - int data_start, new_alloced;
> > + int data_start, aligned_start, new_alloced;
> >
> > if (!align_log2)
> > align_log2 = BLOBLIST_ALIGN_LOG2;
> > @@ -151,10 +151,25 @@ static int bloblist_addrec(uint tag, int size, int
> > align_log2,
> > data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*rec);
> >
> > /* Align the address and then calculate the offset from ->alloced */
> > - data_start = ALIGN(data_start, 1U << align_log2) -
> > map_to_sysmem(hdr);
> > + aligned_start = ALIGN(data_start, 1U << align_log2) - data_start;
> > +
> > + /* If we need to create a dummy record, create it */
> > + if (aligned_start) {
> > + int void_size = aligned_start - sizeof(*rec);
> > + struct bloblist_rec *vrec;
> > + int ret;
> > +
> > + ret = bloblist_addrec(BLOBLISTT_VOID, void_size, 0, );
>
> I just noticed the 'BLOBLISTT'. Is that on purpose? or a typo?
It means 'bloblist tag'
>
> > + if (ret)
> > + return log_msg_ret("void", ret);
> > +
> > + /* start the record after that */
> > + data_start = map_to_sysmem(hdr) + hdr->alloced +
> > sizeof(*vrec);
> > + }
> >
> > /* Calculate the new allocated total */
> > - new_alloced = data_start + ALIGN(size, 1U << align_log2);
> > + new_alloced = data_start - map_to_sysmem(hdr) +
> > + ALIGN(size, 1U << align_log2);
>
> So, wouldn't it make more sense to add the dummy record and align the
> whole thing *after* we've added the entry?
> Doing it like this might leave the last entry on an unaligned boundary, no?
I will let Raymond respond as I'm not sure what this means.
>
> Thanks
> /Ilias
> >
> > if (new_alloced > hdr->size) {
> > log_err("Failed to allocate %x bytes size=%x, need
> > size=%x\n",
> > @@ -164,7 +179,7 @@ static int bloblist_addrec(uint tag, int size, int
> > align_log2,
> > rec = (void *)hdr + hdr->alloced;
> >
> > rec->tag = tag;
> > - rec->hdr_size = data_start - hdr->alloced;
> > + rec->hdr_size = sizeof(struct bloblist_rec);
> > rec->size = size;
> >
> > /* Zero the record data */
> > --
> > 2.25.1
> >
Regards,
Simon
Re: [PATCH v4 08/12] bloblist: Handle alignment with a void entry
Hi Raymond,
On Wed, 27 Dec 2023 at 23:08, Raymond Mao wrote:
>
> From: Simon Glass
>
> Rather than setting the alignment using the header size, add an entirely
> new entry to cover the gap left by the alignment.
>
> Signed-off-by: Simon Glass
> Co-developed-by: Raymond Mao
> Signed-off-by: Raymond Mao
> Reviewed-by: Simon Glass
> ---
> common/bloblist.c | 23 +++
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/common/bloblist.c b/common/bloblist.c
> index 705d9c6ae9..73dbbc01c0 100644
> --- a/common/bloblist.c
> +++ b/common/bloblist.c
> @@ -142,7 +142,7 @@ static int bloblist_addrec(uint tag, int size, int
> align_log2,
> {
> struct bloblist_hdr *hdr = gd->bloblist;
> struct bloblist_rec *rec;
> - int data_start, new_alloced;
> + int data_start, aligned_start, new_alloced;
>
> if (!align_log2)
> align_log2 = BLOBLIST_ALIGN_LOG2;
> @@ -151,10 +151,25 @@ static int bloblist_addrec(uint tag, int size, int
> align_log2,
> data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*rec);
>
> /* Align the address and then calculate the offset from ->alloced */
> - data_start = ALIGN(data_start, 1U << align_log2) - map_to_sysmem(hdr);
> + aligned_start = ALIGN(data_start, 1U << align_log2) - data_start;
> +
> + /* If we need to create a dummy record, create it */
> + if (aligned_start) {
> + int void_size = aligned_start - sizeof(*rec);
> + struct bloblist_rec *vrec;
> + int ret;
> +
> + ret = bloblist_addrec(BLOBLISTT_VOID, void_size, 0, );
I just noticed the 'BLOBLISTT'. Is that on purpose? or a typo?
> + if (ret)
> + return log_msg_ret("void", ret);
> +
> + /* start the record after that */
> + data_start = map_to_sysmem(hdr) + hdr->alloced +
> sizeof(*vrec);
> + }
>
> /* Calculate the new allocated total */
> - new_alloced = data_start + ALIGN(size, 1U << align_log2);
> + new_alloced = data_start - map_to_sysmem(hdr) +
> + ALIGN(size, 1U << align_log2);
So, wouldn't it make more sense to add the dummy record and align the
whole thing *after* we've added the entry?
Doing it like this might leave the last entry on an unaligned boundary, no?
Thanks
/Ilias
>
> if (new_alloced > hdr->size) {
> log_err("Failed to allocate %x bytes size=%x, need size=%x\n",
> @@ -164,7 +179,7 @@ static int bloblist_addrec(uint tag, int size, int
> align_log2,
> rec = (void *)hdr + hdr->alloced;
>
> rec->tag = tag;
> - rec->hdr_size = data_start - hdr->alloced;
> + rec->hdr_size = sizeof(struct bloblist_rec);
> rec->size = size;
>
> /* Zero the record data */
> --
> 2.25.1
>
