Attention is currently required from: Furquan Shaikh, Jack Rosenthal.
Ricardo Quesada has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56987 )
Change subject: cbfstool: add buffer_get() to common.h
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Hi, could you PTAL? ty!
--
To view, visit https://review.coreboot.org/c/coreboot/+/56987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I380eecbc89c13f5fe5ab4c31d7a4fef97690a791
Gerrit-Change-Number: 56987
Gerrit-PatchSet: 1
Gerrit-Owner: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Comment-Date: Mon, 16 Aug 2021 23:45:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Jack Rosenthal.
Ricardo Quesada has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56985 )
Change subject: elog: move functionality to commonlib/bsd
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Hi, could you PTAL? ty!
--
To view, visit https://review.coreboot.org/c/coreboot/+/56985
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I67d5ad6e7c4d486b3d4ebb25be77998173cee5a9
Gerrit-Change-Number: 56985
Gerrit-PatchSet: 1
Gerrit-Owner: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Comment-Date: Mon, 16 Aug 2021 23:44:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh.
Ricardo Quesada has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56883 )
Change subject: elogtool: add "clear" command
......................................................................
Patch Set 5:
(17 comments)
Patchset:
PS5:
fixed. could you PTALA? ty!
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/51b80d41_32c98859
PS1, Line 50: static inline uint8_t mybin2bcd(uint8_t val)
: {
: return ((val / 10) << 4) | (val % 10);
: }
:
> Already defined here: […]
Done
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/56c13852_b657feb4
PS2, Line 79: void elog_update_checksum(struct event_header *event, uint8_t checksum)
: {
: uint8_t *event_data = (uint8_t *)event;
: event_data[event->length - 1] = checksum;
: }
> assuming that the two checksum functions where created by Googler's and it is Ok to relicense them u […]
Done
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/f62f795b_516541d9
PS4, Line 50:
> I think it would be best to split this CL into two: […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/54161087_416bb84c
PS4, Line 51: /* Populate timestamp in event header with given time. */
> I think these comments would be very helpful in elog.h.
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/c231c0a9_42343924
PS4, Line 82: struct
> const
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/eb74f58b_bc6b67e1
PS4, Line 85: uint8_t
> const
Done
File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/35a2fb21_6e29b888
PS4, Line 821: #if CONFIG(RTC)
> I don't think this needs to be a CPP check: […]
Done
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/a3bcf6c8_40fb1bf4
PS4, Line 16: #define RW_ELOG_REGION "RW_ELOG"
> I think this should also be added to bsd/elog.h. […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/15614a6e_33485c5f
PS4, Line 133: data_size = buf->size - sizeof(struct elog_header);
: data_offset = buf->data + sizeof(struct elog_header);
> ``` […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/bcf24100_0551aece
PS4, Line 137: if (data_size < sizeof(struct event_header) + sizeof(used_data_size) + 1)
: return ELOGTOOL_EXIT_INVALID_ELOG_FORMAT;
> Rather than adding this check here, I think it would be helpful to just pass in the buffer to `event […]
do you mean replacing the `struct event header*` with ` struct buffer*`? e.g:
void eventlog_init_event(struct buffer* buf, uint8_t type, ....)
?
I can do that, but looks kind-of-strange to pass a "struct buffer*" to a function that expects a event_header*.
Perhaps a I can create an additional function that calls the original one. e.g:
void eventlog_init_event_with_buf(struct buffer* buf, ....)
{
/* check size */
void* b = buffer_get(buf);
return eventlog_init_event(b, ...);
}
WDYT?
https://review.coreboot.org/c/coreboot/+/56883/comment/7e0ade74_dbefb8a1
PS4, Line 145: (const struct event_header *)
> I don't think this typecast is required. […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/4b3b2b20_cc6fc399
PS4, Line 147: buf->data + buf->size
> I think you can add a helper for this in a separate change: `buffer_end()`
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/f8d0f07f_bd20fd1d
PS4, Line 175: ELOGTOOL_EXIT_BAD_INPUT_PATH
> If the read was successful but write back failed, it might not really be a problem with bad path?
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/8353e507_1dff4ed8
PS4, Line 181: (uint8_t *) buf->data, buf->size
> ``` […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/05fa5d3d_29129061
PS4, Line 194: /* Returned buffer must be freed. */
: struct buffer buf;
: ret = elog_read(filename, &buf);
: if (ret != 0)
: return ret;
:
> I think this operation is now going to be required for each elog subcommand. […]
yay, much better!
File util/cbfstool/eventlog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/84e5eb7b_778ea91f
PS4, Line 654: (uint32_t *)&event[1];
> event_get_data(event);
good point.
Should I rename the current `event_get_data()` to `event_get_data_const()` (which receives and returns a `const*`), and add a new one that receives and returns a `non-const*` ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56883
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia28a6eb34c82103ab078a0841b022e2e5e430585
Gerrit-Change-Number: 56883
Gerrit-PatchSet: 5
Gerrit-Owner: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Comment-Date: Mon, 16 Aug 2021 23:43:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-MessageType: comment
Ricardo Quesada has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/56987 )
Change subject: cbfstool: add buffer_get() to common.h
......................................................................
cbfstool: add buffer_get() to common.h
Add buffer_get() function to common.h.
This function returns a pointer to the end of the buffer.
This is needed by elogtool util. (See the next CL in the chain).
BUG=b:172210863
Change-Id: I380eecbc89c13f5fe5ab4c31d7a4fef97690a791
Signed-off-by: Ricardo Quesada <ricardoq(a)google.com>
---
M util/cbfstool/common.h
1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/56987/1
diff --git a/util/cbfstool/common.h b/util/cbfstool/common.h
index 1c25a66..07ffdf8 100644
--- a/util/cbfstool/common.h
+++ b/util/cbfstool/common.h
@@ -54,6 +54,11 @@
return b->offset;
}
+static inline void *buffer_end(const struct buffer *b)
+{
+ return b->data + b->size;
+}
+
/*
* Shrink a buffer toward the beginning of its previous space.
* Afterward, buffer_delete() remains the means of cleaning it up. */
--
To view, visit https://review.coreboot.org/c/coreboot/+/56987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I380eecbc89c13f5fe5ab4c31d7a4fef97690a791
Gerrit-Change-Number: 56987
Gerrit-PatchSet: 1
Gerrit-Owner: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-MessageType: newchange