From: "Michael S. Tsirkin" <mst@redhat.com>
To: Ani Sinha <ani@anisinha.ca>
Cc: berrange@redhat.com, ehabkost@redhat.com, konrad.wilk@oracle.com,
qemu-devel@nongnu.org, pbonzini@redhat.com, imammedo@redhat.com,
boris.ostrovsky@oracle.com,
Eric DeVolder <eric.devolder@oracle.com>,
rth@twiddle.net
Subject: Re: [PATCH v13 06/10] ACPI ERST: build the ACPI ERST table
Date: Tue, 25 Jan 2022 07:05:31 -0500 [thread overview]
Message-ID: <20220125070221-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2201251600060.1134355@anisinha-lenovo>
On Tue, Jan 25, 2022 at 04:23:49PM +0530, Ani Sinha wrote:
>
>
> On Mon, 24 Jan 2022, Eric DeVolder wrote:
>
> > This builds the ACPI ERST table to inform OSPM how to communicate
> > with the acpi-erst device.
> >
> > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > ---
> > hw/acpi/erst.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 188 insertions(+)
> >
> > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > index fe9ba51..b0c7539 100644
> > --- a/hw/acpi/erst.c
> > +++ b/hw/acpi/erst.c
> > @@ -59,6 +59,27 @@
> > #define STATUS_RECORD_STORE_EMPTY 0x04
> > #define STATUS_RECORD_NOT_FOUND 0x05
> >
> > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > +#define INST_READ_REGISTER 0x00
> > +#define INST_READ_REGISTER_VALUE 0x01
> > +#define INST_WRITE_REGISTER 0x02
> > +#define INST_WRITE_REGISTER_VALUE 0x03
> > +#define INST_NOOP 0x04
> > +#define INST_LOAD_VAR1 0x05
> > +#define INST_LOAD_VAR2 0x06
> > +#define INST_STORE_VAR1 0x07
> > +#define INST_ADD 0x08
> > +#define INST_SUBTRACT 0x09
> > +#define INST_ADD_VALUE 0x0A
> > +#define INST_SUBTRACT_VALUE 0x0B
> > +#define INST_STALL 0x0C
> > +#define INST_STALL_WHILE_TRUE 0x0D
> > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > +#define INST_GOTO 0x0F
> > +#define INST_SET_SRC_ADDRESS_BASE 0x10
> > +#define INST_SET_DST_ADDRESS_BASE 0x11
> > +#define INST_MOVE_DATA 0x12
> > +
> > /* UEFI 2.1: Appendix N Common Platform Error Record */
> > #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > @@ -172,6 +193,173 @@ typedef struct {
> >
> > /*******************************************************************/
> > /*******************************************************************/
> > +
> > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > +static void build_serialization_instruction_entry(GArray *table_data,
> > + uint8_t serialization_action,
> > + uint8_t instruction,
> > + uint8_t flags,
> > + uint8_t register_bit_width,
> > + uint64_t register_address,
> > + uint64_t value)
> > +{
> > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> > + struct AcpiGenericAddress gas;
> > + uint64_t mask;
> > +
> > + /* Serialization Action */
> > + build_append_int_noprefix(table_data, serialization_action, 1);
> > + /* Instruction */
> > + build_append_int_noprefix(table_data, instruction , 1);
> > + /* Flags */
> > + build_append_int_noprefix(table_data, flags , 1);
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0 , 1);
> > + /* Register Region */
> > + gas.space_id = AML_SYSTEM_MEMORY;
> > + gas.bit_width = register_bit_width;
> > + gas.bit_offset = 0;
> > + gas.access_width = ctz32(register_bit_width) - 2;
> > + gas.address = register_address;
> > + build_append_gas_from_struct(table_data, &gas);
> > + /* Value */
> > + build_append_int_noprefix(table_data, value , 8);
> > + /* Mask */
> > + mask = (1ULL << (register_bit_width - 1) << 1) - 1;
> > + build_append_int_noprefix(table_data, mask , 8);
> > +}
> > +
> > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> > + const char *oem_id, const char *oem_table_id)
> > +{
> > + GArray *table_instruction_data;
> > + unsigned action;
> > + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> > + .oem_table_id = oem_table_id };
> > +
> > + trace_acpi_erst_pci_bar_0(bar0);
> > +
> > + /*
> > + * Serialization Action Table
> > + * The serialization action table must be generated first
> > + * so that its size can be known in order to populate the
> > + * Instruction Entry Count field.
> > + */
> > + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> > +
> > + /*
> > + * Macros for use with construction of the action instructions
> > + */
> > +#define BUILD_READ_REGISTER(width_in_bits, reg) \
> > + build_serialization_instruction_entry(table_instruction_data, \
> > + action, INST_READ_REGISTER, 0, width_in_bits, \
> > + bar0 + reg, 0)
> > +
> > +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \
> > + build_serialization_instruction_entry(table_instruction_data, \
> > + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \
> > + bar0 + reg, value)
> > +
> > +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \
> > + build_serialization_instruction_entry(table_instruction_data, \
> > + action, INST_WRITE_REGISTER, 0, width_in_bits, \
> > + bar0 + reg, value)
> > +
> > +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \
> > + build_serialization_instruction_entry(table_instruction_data, \
> > + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \
> > + bar0 + reg, value)
I think these macros which in a hidden way use the bar0 variable really
should be replaced with inline functions, improving type safety.
> > +
> > + /* Serialization Instruction Entries */
> > + action = ACTION_BEGIN_WRITE_OPERATION;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > +
> > + action = ACTION_BEGIN_READ_OPERATION;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > +
> > + action = ACTION_BEGIN_CLEAR_OPERATION;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > +
> > + action = ACTION_END_OPERATION;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > +
> > + action = ACTION_SET_RECORD_OFFSET;
> > + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0);
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > +
> > + action = ACTION_EXECUTE_OPERATION;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET,
> > + ERST_EXECUTE_OPERATION_MAGIC);
>
> except here, on all cases we have
> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
>
> We should treat the above as special case and simplify the rest of the
> calls (eliminate repeated common arguments).
>
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > +
> > + action = ACTION_CHECK_BUSY_STATUS;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01);
> > +
> > + action = ACTION_GET_COMMAND_STATUS;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET);
> > +
> > + action = ACTION_GET_RECORD_IDENTIFIER;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
> > +
> > + action = ACTION_SET_RECORD_IDENTIFIER;
> > + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0);
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
>
> This one seems reverted. Should this be
> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0);
>
> like others?
>
> > +
> > + action = ACTION_GET_RECORD_COUNT;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET);
> > +
> > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > +
> > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
> > +
> > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
> > +
> > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET);
> > +
> > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
> > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
> > +
>
> BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second
> argument. WE should eliminate this repeated passing of same argument.
>
>
> > + /* Serialization Header */
> > + acpi_table_begin(&table, table_data);
> > +
> > + /* Serialization Header Size */
> > + build_append_int_noprefix(table_data, 48, 4);
> > +
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0, 4);
> > +
> > + /*
> > + * Instruction Entry Count
> > + * Each instruction entry is 32 bytes
> > + */
> > + g_assert((table_instruction_data->len) % 32 == 0);
> > + build_append_int_noprefix(table_data,
> > + (table_instruction_data->len / 32), 4);
> > +
> > + /* Serialization Instruction Entries */
> > + g_array_append_vals(table_data, table_instruction_data->data,
> > + table_instruction_data->len);
> > + g_array_free(table_instruction_data, TRUE);
> > +
> > + acpi_table_end(linker, &table);
> > +}
> > +
> > +/*******************************************************************/
> > +/*******************************************************************/
> > static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > {
> > uint8_t *rc = NULL;
> > --
> > 1.8.3.1
> >
> >
next prev parent reply other threads:[~2022-01-25 12:13 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-24 17:16 [PATCH v13 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
2022-01-24 17:16 ` [PATCH v13 01/10] ACPI ERST: bios-tables-test.c steps 1 and 2 Eric DeVolder
2022-01-24 17:16 ` [PATCH v13 02/10] ACPI ERST: specification for ERST support Eric DeVolder
2022-01-24 17:16 ` [PATCH v13 03/10] ACPI ERST: PCI device_id for ERST Eric DeVolder
2022-01-24 17:16 ` [PATCH v13 04/10] ACPI ERST: header file " Eric DeVolder
2022-01-24 17:16 ` [PATCH v13 05/10] ACPI ERST: support for ACPI ERST feature Eric DeVolder
2022-01-24 17:16 ` [PATCH v13 06/10] ACPI ERST: build the ACPI ERST table Eric DeVolder
2022-01-25 10:53 ` Ani Sinha
2022-01-25 12:05 ` Michael S. Tsirkin [this message]
2022-01-25 12:21 ` Ani Sinha
2022-01-25 16:32 ` Eric DeVolder
2022-01-25 17:29 ` Michael S. Tsirkin
2022-01-25 16:24 ` Eric DeVolder
2022-01-25 17:58 ` Michael S. Tsirkin
2022-01-26 7:05 ` Ani Sinha
2022-01-26 16:23 ` Eric DeVolder
2022-01-26 7:41 ` Michael S. Tsirkin
2022-01-24 17:16 ` [PATCH v13 07/10] ACPI ERST: create ACPI ERST table for pc/x86 machines Eric DeVolder
2022-01-24 17:16 ` [PATCH v13 08/10] ACPI ERST: qtest for ERST Eric DeVolder
2022-01-24 17:17 ` [PATCH v13 09/10] ACPI ERST: bios-tables-test testcase Eric DeVolder
2022-01-24 17:17 ` [PATCH v13 10/10] ACPI ERST: step 6 of bios-tables-test.c Eric DeVolder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220125070221-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=ani@anisinha.ca \
--cc=berrange@redhat.com \
--cc=boris.ostrovsky@oracle.com \
--cc=ehabkost@redhat.com \
--cc=eric.devolder@oracle.com \
--cc=imammedo@redhat.com \
--cc=konrad.wilk@oracle.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.