From: Fan Ni <fan.ni@samsung.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: "Michael Tokarev" <mjt@tls.msk.ru>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"Michael Tsirkin" <mst@redhat.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"linuxarm@huawei.com" <linuxarm@huawei.com>
Subject: Re: [PATCH v2 4/4] hw/cxl: Line length reductions
Date: Mon, 18 Sep 2023 16:48:45 +0000 [thread overview]
Message-ID: <20230918164844.GB3331103@sjcvldevvm72> (raw)
In-Reply-To: <20230915170418.21337-5-Jonathan.Cameron@huawei.com>
On Fri, Sep 15, 2023 at 06:04:18PM +0100, Jonathan Cameron wrote:
> Michael Tsirkin observed that there were some unnecessarily
> long lines in the CXL code in a recent review.
> This patch is intended to rectify that where it does not
> hurt readability.
>
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
One minor comment inline. Other than that, looks good to me.
> include/hw/cxl/cxl_component.h | 3 ++-
> include/hw/cxl/cxl_device.h | 5 +++--
> include/hw/cxl/cxl_events.h | 3 ++-
> hw/cxl/cxl-cdat.c | 3 ++-
> hw/cxl/cxl-component-utils.c | 10 ++++++----
> hw/cxl/cxl-events.c | 9 ++++++---
> hw/cxl/cxl-mailbox-utils.c | 21 ++++++++++++++-------
> hw/mem/cxl_type3.c | 31 +++++++++++++++++++------------
> hw/mem/cxl_type3_stubs.c | 5 +++--
> 9 files changed, 57 insertions(+), 33 deletions(-)
>
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index 3c795a6278..e52dd8d2b9 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -175,7 +175,8 @@ HDM_DECODER_INIT(3);
> (CXL_IDE_REGISTERS_OFFSET + CXL_IDE_REGISTERS_SIZE)
> #define CXL_SNOOP_REGISTERS_SIZE 0x8
>
> -QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET + CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
> +QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET +
> + CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
> "No space for registers");
>
> typedef struct component_registers {
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 51cd0d9ce3..007ddaf078 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -192,7 +192,7 @@ void cxl_device_register_init_common(CXLDeviceState *dev);
> * Documented as a 128 bit register, but 64 bit accesses and the second
> * 64 bits are currently reserved.
> */
> -REG64(CXL_DEV_CAP_ARRAY, 0) /* Documented as 128 bit register but 64 byte accesses */
> +REG64(CXL_DEV_CAP_ARRAY, 0)
> FIELD(CXL_DEV_CAP_ARRAY, CAP_ID, 0, 16)
> FIELD(CXL_DEV_CAP_ARRAY, CAP_VERSION, 16, 8)
> FIELD(CXL_DEV_CAP_ARRAY, CAP_COUNT, 32, 16)
> @@ -361,7 +361,8 @@ struct CXLType3Class {
> uint64_t offset);
> void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size,
> uint64_t offset);
> - bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data);
> + bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset,
> + uint8_t *data);
> };
>
> MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> index 089ba2091f..d778487b7e 100644
> --- a/include/hw/cxl/cxl_events.h
> +++ b/include/hw/cxl/cxl_events.h
> @@ -92,7 +92,8 @@ typedef enum CXLEventIntMode {
> CXL_INT_RES = 0x03,
> } CXLEventIntMode;
> #define CXL_EVENT_INT_MODE_MASK 0x3
> -#define CXL_EVENT_INT_SETTING(vector) ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
> +#define CXL_EVENT_INT_SETTING(vector) \
> + ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
> typedef struct CXLEventInterruptPolicy {
> uint8_t info_settings;
> uint8_t warn_settings;
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index d246d6885b..639a2db3e1 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -60,7 +60,8 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> return;
> }
>
> - cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf, cdat->private);
> + cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> + cdat->private);
>
> if (!cdat->built_buf_len) {
> /* Build later as not all data available yet */
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index c8d632d540..915c208209 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -240,7 +240,8 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT, 1);
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_256B, 1);
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_4K, 1);
> - ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, POISON_ON_ERR_CAP, 0);
> + ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> + POISON_ON_ERR_CAP, 0);
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_GLOBAL_CONTROL,
> HDM_DECODER_ENABLE, 0);
> write_msk[R_CXL_HDM_DECODER_GLOBAL_CONTROL] = 0x3;
> @@ -263,13 +264,14 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> }
> }
>
> -void cxl_component_register_init_common(uint32_t *reg_state, uint32_t *write_msk,
> +void cxl_component_register_init_common(uint32_t *reg_state,
> + uint32_t *write_msk,
> enum reg_type type)
> {
> int caps = 0;
>
> /*
> - * In CXL 2.0 the capabilities required for each CXL component are such that,
> + * In CXL2.0 the capabilities required for each CXL component are such that,
It seems we always leave a space between CXL and 2.0 in other
places.
Fan
> * with the ordering chosen here, a single number can be used to define
> * which capabilities should be provided.
> */
> @@ -448,7 +450,7 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
> default: /* Registers are RO for other component types */
> break;
> }
> - /* There are rw1cs bits in the status register but never set currently */
> + /* There are rw1cs bits in the status register but never set */
> break;
> }
>
> diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> index d161d57456..10a4531904 100644
> --- a/hw/cxl/cxl-events.c
> +++ b/hw/cxl/cxl-events.c
> @@ -170,8 +170,10 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
> if (log->overflow_err_count) {
> pl->flags |= CXL_GET_EVENT_FLAG_OVERFLOW;
> pl->overflow_err_count = cpu_to_le16(log->overflow_err_count);
> - pl->first_overflow_timestamp = cpu_to_le64(log->first_overflow_timestamp);
> - pl->last_overflow_timestamp = cpu_to_le64(log->last_overflow_timestamp);
> + pl->first_overflow_timestamp =
> + cpu_to_le64(log->first_overflow_timestamp);
> + pl->last_overflow_timestamp =
> + cpu_to_le64(log->last_overflow_timestamp);
> }
>
> pl->record_count = cpu_to_le16(nr);
> @@ -180,7 +182,8 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
> return CXL_MBOX_SUCCESS;
> }
>
> -CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds, CXLClearEventPayload *pl)
> +CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
> + CXLClearEventPayload *pl)
> {
> CXLEventLog *log;
> uint8_t log_type;
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 02f9b5a870..3977af23f1 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -366,9 +366,12 @@ static CXLRetCode cmd_identify_memory_device(struct cxl_cmd *cmd,
>
> snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
>
> - stq_le_p(&id->total_capacity, cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
> - stq_le_p(&id->persistent_capacity, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> - stq_le_p(&id->volatile_capacity, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&id->total_capacity,
> + cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&id->persistent_capacity,
> + cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&id->volatile_capacity,
> + cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
> stl_le_p(&id->lsa_size, cvc->get_lsa_size(ct3d));
> /* 256 poison records */
> st24_le_p(id->poison_list_max_mer, 256);
> @@ -396,13 +399,15 @@ static CXLRetCode cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
> return CXL_MBOX_INTERNAL_ERROR;
> }
>
> - stq_le_p(&part_info->active_vmem, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&part_info->active_vmem,
> + cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
> /*
> * When both next_vmem and next_pmem are 0, there is no pending change to
> * partitioning.
> */
> stq_le_p(&part_info->next_vmem, 0);
> - stq_le_p(&part_info->active_pmem, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&part_info->active_pmem,
> + cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> stq_le_p(&part_info->next_pmem, 0);
>
> *len = sizeof(*part_info);
> @@ -681,8 +686,10 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
> cmd_firmware_update_get_info, 0, 0 },
> [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
> - [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, IMMEDIATE_POLICY_CHANGE },
> - [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 0, 0 },
> + [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
> + 8, IMMEDIATE_POLICY_CHANGE },
> + [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported,
> + 0, 0 },
> [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
> [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
> cmd_identify_memory_device, 0, 0 },
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 1658e0cc59..c5855d4e7d 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -197,10 +197,9 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> }
>
> if (nonvolatile_mr) {
> + uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
> rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
> - nonvolatile_mr, true,
> - (volatile_mr ?
> - memory_region_size(volatile_mr) : 0));
> + nonvolatile_mr, true, base);
> if (rc < 0) {
> goto error_cleanup;
> }
> @@ -503,7 +502,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> case A_CXL_RAS_UNC_ERR_STATUS:
> {
> uint32_t capctrl = ldl_le_p(cache_mem + R_CXL_RAS_ERR_CAP_CTRL);
> - uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL, FIRST_ERROR_POINTER);
> + uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL,
> + FIRST_ERROR_POINTER);
> CXLError *cxl_err;
> uint32_t unc_err;
>
> @@ -522,7 +522,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> * closest to behavior of hardware not capable of multiple
> * header recording.
> */
> - QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node, cxl_next) {
> + QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node,
> + cxl_next) {
> if ((1 << cxl_err->type) & value) {
> QTAILQ_REMOVE(&ct3d->error_list, cxl_err, node);
> g_free(cxl_err);
> @@ -1061,7 +1062,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
> if (((start >= p->start) && (start < p->start + p->length)) ||
> ((start + length > p->start) &&
> (start + length <= p->start + p->length))) {
> - error_setg(errp, "Overlap with existing poisoned region not supported");
> + error_setg(errp,
> + "Overlap with existing poisoned region not supported");
> return;
> }
> }
> @@ -1074,7 +1076,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
> p = g_new0(CXLPoison, 1);
> p->length = length;
> p->start = start;
> - p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
> + /* Different from injected via the mbox */
> + p->type = CXL_POISON_TYPE_INTERNAL;
>
> QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
> ct3d->poison_list_cnt++;
> @@ -1211,7 +1214,8 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
> return;
> }
> /* If the error is masked, nothting to do here */
> - if (!((1 << cxl_err_type) & ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
> + if (!((1 << cxl_err_type) &
> + ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
> return;
> }
>
> @@ -1361,7 +1365,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
> bool has_bank, uint8_t bank,
> bool has_row, uint32_t row,
> bool has_column, uint16_t column,
> - bool has_correction_mask, uint64List *correction_mask,
> + bool has_correction_mask,
> + uint64List *correction_mask,
> Error **errp)
> {
> Object *obj = object_resolve_path(path, NULL);
> @@ -1462,7 +1467,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> int16_t temperature,
> uint32_t dirty_shutdown_count,
> uint32_t corrected_volatile_error_count,
> - uint32_t corrected_persistent_error_count,
> + uint32_t corrected_persist_error_count,
> Error **errp)
> {
> Object *obj = object_resolve_path(path, NULL);
> @@ -1502,8 +1507,10 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> module.life_used = life_used;
> stw_le_p(&module.temperature, temperature);
> stl_le_p(&module.dirty_shutdown_count, dirty_shutdown_count);
> - stl_le_p(&module.corrected_volatile_error_count, corrected_volatile_error_count);
> - stl_le_p(&module.corrected_persistent_error_count, corrected_persistent_error_count);
> + stl_le_p(&module.corrected_volatile_error_count,
> + corrected_volatile_error_count);
> + stl_le_p(&module.corrected_persistent_error_count,
> + corrected_persist_error_count);
>
> if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&module)) {
> cxl_event_irq_assert(ct3d);
> diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
> index f3e4a9fa72..56d60b4fd8 100644
> --- a/hw/mem/cxl_type3_stubs.c
> +++ b/hw/mem/cxl_type3_stubs.c
> @@ -23,7 +23,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
> bool has_bank, uint8_t bank,
> bool has_row, uint32_t row,
> bool has_column, uint16_t column,
> - bool has_correction_mask, uint64List *correction_mask,
> + bool has_correction_mask,
> + uint64List *correction_mask,
> Error **errp) {}
>
> void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> @@ -35,7 +36,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> int16_t temperature,
> uint32_t dirty_shutdown_count,
> uint32_t corrected_volatile_error_count,
> - uint32_t corrected_persistent_error_count,
> + uint32_t corrected_persist_error_count,
> Error **errp) {}
>
> void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
> --
> 2.39.2
>
next prev parent reply other threads:[~2023-09-18 16:50 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-15 17:04 [PATCH v2 0/4] hw/cxl: Line length reduction and related Jonathan Cameron via
2023-09-15 17:04 ` [PATCH v2 1/4] hw/cxl: Use a switch to explicitly check size in caps_reg_read() Jonathan Cameron via
2023-09-18 16:23 ` Fan Ni
2023-09-15 17:04 ` [PATCH v2 2/4] hw/cxl: Use switch statements for read and write of cachemem registers Jonathan Cameron via
2023-09-18 16:32 ` Fan Ni
2023-09-15 17:04 ` [PATCH v2 3/4] hw/cxl: CXLDVSECPortExtensions renamed to CXLDVSECPortExt Jonathan Cameron via
2023-09-18 16:34 ` Fan Ni
2023-09-15 17:04 ` [PATCH v2 4/4] hw/cxl: Line length reductions Jonathan Cameron via
2023-09-18 16:48 ` Fan Ni [this message]
2023-09-19 9:11 ` Jonathan Cameron via
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=20230918164844.GB3331103@sjcvldevvm72 \
--to=fan.ni@samsung.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=linuxarm@huawei.com \
--cc=mjt@tls.msk.ru \
--cc=mst@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
/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.