From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Gregory Price <gourry.memverge@gmail.com>
Cc: <qemu-devel@nongnu.org>, <linux-cxl@vger.kernel.org>,
<alison.schofield@intel.com>, <dave@stgolabs.net>,
<a.manzanares@samsung.com>, <bwidawsk@kernel.org>,
<gregory.price@memverge.com>, <mst@redhat.com>,
<hchkuo@avery-design.com.tw>, <cbrowy@avery-design.com>,
<ira.weiny@intel.com>
Subject: Re: [PATCH 2/5] hw/mem/cxl_type3: Pull validation checks ahead of functional code
Date: Thu, 13 Oct 2022 10:07:40 +0100 [thread overview]
Message-ID: <20221013100740.0000471b@huawei.com> (raw)
In-Reply-To: <20221012182120.174142-3-gregory.price@memverge.com>
On Wed, 12 Oct 2022 14:21:17 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:
> For style - pulling these validations ahead flattens the code.
True, but at the cost of separating the check from where it is
obvious why we have the check. I'd prefer to see it next to the
use.
Inverting the hostmem check is resonable so I'll make that change.
My original thinking is that doing so would make adding non volatile
support messier but given you plan to factor out most of this the
change won't be too bad anyway.
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
> hw/mem/cxl_type3.c | 193 ++++++++++++++++++++++-----------------------
> 1 file changed, 96 insertions(+), 97 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 94bc439d89..43b2b9e041 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -32,107 +32,106 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table,
> int dslbis_nonvolatile_num = 4;
> MemoryRegion *mr;
>
> + if (!ct3d->hostmem) {
> + return len;
> + }
> +
> + mr = host_memory_backend_get_memory(ct3d->hostmem);
> + if (!mr) {
> + return -EINVAL;
> + }
> +
> /* Non volatile aspects */
> - if (ct3d->hostmem) {
> - dsmas_nonvolatile = g_malloc(sizeof(*dsmas_nonvolatile));
> - if (!dsmas_nonvolatile) {
> - return -ENOMEM;
> - }
> - nonvolatile_dsmad = next_dsmad_handle++;
> - mr = host_memory_backend_get_memory(ct3d->hostmem);
> - if (!mr) {
> - return -EINVAL;
> - }
> - *dsmas_nonvolatile = (CDATDsmas) {
> - .header = {
> - .type = CDAT_TYPE_DSMAS,
> - .length = sizeof(*dsmas_nonvolatile),
> - },
> - .DSMADhandle = nonvolatile_dsmad,
> - .flags = CDAT_DSMAS_FLAG_NV,
> - .DPA_base = 0,
> - .DPA_length = int128_get64(mr->size),
> - };
> - len++;
> -
> - /* For now, no memory side cache, plausiblish numbers */
> - dslbis_nonvolatile =
> - g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num);
> - if (!dslbis_nonvolatile) {
> - return -ENOMEM;
> - }
> + dsmas_nonvolatile = g_malloc(sizeof(*dsmas_nonvolatile));
> + if (!dsmas_nonvolatile) {
> + return -ENOMEM;
> + }
> + nonvolatile_dsmad = next_dsmad_handle++;
> + *dsmas_nonvolatile = (CDATDsmas) {
> + .header = {
> + .type = CDAT_TYPE_DSMAS,
> + .length = sizeof(*dsmas_nonvolatile),
> + },
> + .DSMADhandle = nonvolatile_dsmad,
> + .flags = CDAT_DSMAS_FLAG_NV,
> + .DPA_base = 0,
> + .DPA_length = int128_get64(mr->size),
> + };
> + len++;
>
> - dslbis_nonvolatile[0] = (CDATDslbis) {
> - .header = {
> - .type = CDAT_TYPE_DSLBIS,
> - .length = sizeof(*dslbis_nonvolatile),
> - },
> - .handle = nonvolatile_dsmad,
> - .flags = HMAT_LB_MEM_MEMORY,
> - .data_type = HMAT_LB_DATA_READ_LATENCY,
> - .entry_base_unit = 10000, /* 10ns base */
> - .entry[0] = 15, /* 150ns */
> - };
> - len++;
> -
> - dslbis_nonvolatile[1] = (CDATDslbis) {
> - .header = {
> - .type = CDAT_TYPE_DSLBIS,
> - .length = sizeof(*dslbis_nonvolatile),
> - },
> - .handle = nonvolatile_dsmad,
> - .flags = HMAT_LB_MEM_MEMORY,
> - .data_type = HMAT_LB_DATA_WRITE_LATENCY,
> - .entry_base_unit = 10000,
> - .entry[0] = 25, /* 250ns */
> - };
> - len++;
> -
> - dslbis_nonvolatile[2] = (CDATDslbis) {
> - .header = {
> - .type = CDAT_TYPE_DSLBIS,
> - .length = sizeof(*dslbis_nonvolatile),
> - },
> - .handle = nonvolatile_dsmad,
> - .flags = HMAT_LB_MEM_MEMORY,
> - .data_type = HMAT_LB_DATA_READ_BANDWIDTH,
> - .entry_base_unit = 1000, /* GB/s */
> - .entry[0] = 16,
> - };
> - len++;
> -
> - dslbis_nonvolatile[3] = (CDATDslbis) {
> - .header = {
> - .type = CDAT_TYPE_DSLBIS,
> - .length = sizeof(*dslbis_nonvolatile),
> - },
> - .handle = nonvolatile_dsmad,
> - .flags = HMAT_LB_MEM_MEMORY,
> - .data_type = HMAT_LB_DATA_WRITE_BANDWIDTH,
> - .entry_base_unit = 1000, /* GB/s */
> - .entry[0] = 16,
> - };
> - len++;
> -
> - mr = host_memory_backend_get_memory(ct3d->hostmem);
> - if (!mr) {
> - return -EINVAL;
> - }
> - dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile));
> - *dsemts_nonvolatile = (CDATDsemts) {
> - .header = {
> - .type = CDAT_TYPE_DSEMTS,
> - .length = sizeof(*dsemts_nonvolatile),
> - },
> - .DSMAS_handle = nonvolatile_dsmad,
> - /* Reserved - the non volatile from DSMAS matters */
> - .EFI_memory_type_attr = 2,
> - .DPA_offset = 0,
> - .DPA_length = int128_get64(mr->size),
> - };
> - len++;
> + /* For now, no memory side cache, plausiblish numbers */
> + dslbis_nonvolatile =
> + g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num);
> + if (!dslbis_nonvolatile) {
> + return -ENOMEM;
> }
>
> + dslbis_nonvolatile[0] = (CDATDslbis) {
> + .header = {
> + .type = CDAT_TYPE_DSLBIS,
> + .length = sizeof(*dslbis_nonvolatile),
> + },
> + .handle = nonvolatile_dsmad,
> + .flags = HMAT_LB_MEM_MEMORY,
> + .data_type = HMAT_LB_DATA_READ_LATENCY,
> + .entry_base_unit = 10000, /* 10ns base */
> + .entry[0] = 15, /* 150ns */
> + };
> + len++;
> +
> + dslbis_nonvolatile[1] = (CDATDslbis) {
> + .header = {
> + .type = CDAT_TYPE_DSLBIS,
> + .length = sizeof(*dslbis_nonvolatile),
> + },
> + .handle = nonvolatile_dsmad,
> + .flags = HMAT_LB_MEM_MEMORY,
> + .data_type = HMAT_LB_DATA_WRITE_LATENCY,
> + .entry_base_unit = 10000,
> + .entry[0] = 25, /* 250ns */
> + };
> + len++;
> +
> + dslbis_nonvolatile[2] = (CDATDslbis) {
> + .header = {
> + .type = CDAT_TYPE_DSLBIS,
> + .length = sizeof(*dslbis_nonvolatile),
> + },
> + .handle = nonvolatile_dsmad,
> + .flags = HMAT_LB_MEM_MEMORY,
> + .data_type = HMAT_LB_DATA_READ_BANDWIDTH,
> + .entry_base_unit = 1000, /* GB/s */
> + .entry[0] = 16,
> + };
> + len++;
> +
> + dslbis_nonvolatile[3] = (CDATDslbis) {
> + .header = {
> + .type = CDAT_TYPE_DSLBIS,
> + .length = sizeof(*dslbis_nonvolatile),
> + },
> + .handle = nonvolatile_dsmad,
> + .flags = HMAT_LB_MEM_MEMORY,
> + .data_type = HMAT_LB_DATA_WRITE_BANDWIDTH,
> + .entry_base_unit = 1000, /* GB/s */
> + .entry[0] = 16,
> + };
> + len++;
> +
> + dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile));
> + *dsemts_nonvolatile = (CDATDsemts) {
> + .header = {
> + .type = CDAT_TYPE_DSEMTS,
> + .length = sizeof(*dsemts_nonvolatile),
> + },
> + .DSMAS_handle = nonvolatile_dsmad,
> + /* Reserved - the non volatile from DSMAS matters */
> + .EFI_memory_type_attr = 2,
> + .DPA_offset = 0,
> + .DPA_length = int128_get64(mr->size),
> + };
> + len++;
> +
> *cdat_table = g_malloc0(len * sizeof(*cdat_table));
> /* Header always at start of structure */
> if (dsmas_nonvolatile) {
next prev parent reply other threads:[~2022-10-13 9:07 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-07 15:21 [PATCH v7 0/5] QEMU PCIe DOE for PCIe 4.0/5.0 and CXL 2.0 Jonathan Cameron
2022-10-07 15:21 ` [PATCH v7 1/5] hw/pci: PCIe Data Object Exchange emulation Jonathan Cameron
2022-10-07 15:21 ` [PATCH v7 2/5] hw/mem/cxl-type3: Add MSIX support Jonathan Cameron
2022-10-07 15:21 ` [PATCH v7 3/5] hw/cxl/cdat: CXL CDAT Data Object Exchange implementation Jonathan Cameron
2022-10-13 11:04 ` Jonathan Cameron
2022-10-07 15:21 ` [PATCH v7 4/5] hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange Jonathan Cameron
2022-10-12 16:01 ` Gregory Price
2022-10-13 10:40 ` Jonathan Cameron
2022-10-13 10:56 ` Jonathan Cameron
2022-10-12 18:21 ` Gregory Price
2022-10-12 18:21 ` [PATCH 1/5] hw/mem/cxl_type3: fix checkpatch errors Gregory Price
2022-10-12 18:21 ` [PATCH 2/5] hw/mem/cxl_type3: Pull validation checks ahead of functional code Gregory Price
2022-10-13 9:07 ` Jonathan Cameron [this message]
2022-10-13 10:42 ` Jonathan Cameron
2022-10-12 18:21 ` [PATCH 3/5] hw/mem/cxl_type3: CDAT pre-allocate and check resources prior to work Gregory Price
2022-10-13 10:44 ` Jonathan Cameron
2022-10-12 18:21 ` [PATCH 4/5] hw/mem/cxl_type3: Change the CDAT allocation/free strategy Gregory Price
2022-10-13 10:45 ` Jonathan Cameron
2022-10-12 18:21 ` [PATCH 5/5] hw/mem/cxl_type3: Refactor CDAT sub-table entry initialization into a function Gregory Price
2022-10-13 10:47 ` Jonathan Cameron
2022-10-13 19:40 ` Gregory Price
2022-10-14 15:29 ` Jonathan Cameron
2022-10-13 8:57 ` [PATCH v7 4/5] hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange Jonathan Cameron
[not found] ` <CAD3UvdRYH2NVck-kLYLQcBym-5TY0WXWj7vCzcRi5yEuVfgzcQ@mail.gmail.com>
2022-10-13 11:53 ` Jonathan Cameron
2022-10-13 12:35 ` Gregory Price
2022-10-13 14:40 ` Jonathan Cameron
2022-10-07 15:21 ` [PATCH v7 5/5] hw/pci-bridge/cxl-upstream: Add a CDAT table access DOE Jonathan Cameron
2022-10-10 10:30 ` [PATCH v7 0/5] QEMU PCIe DOE for PCIe 4.0/5.0 and CXL 2.0 Jonathan Cameron
2022-10-11 21:19 ` [PATCH 0/5] Multi-Region and Volatile Memory support for CXL Type-3 Devices Gregory Price
2022-10-11 21:19 ` [PATCH 1/5] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL Gregory Price
2022-10-11 21:19 ` [PATCH 2/5] hw/cxl: Add CXL_CAPACITY_MULTIPLIER definition Gregory Price
2022-10-11 21:19 ` [PATCH 3/5] hw/mem/cxl_type: Generalize CDATDsmas initialization for Memory Regions Gregory Price
2022-10-12 14:10 ` Jonathan Cameron
2022-10-11 21:19 ` [PATCH 4/5] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent) Gregory Price
2022-10-11 21:19 ` [PATCH 5/5] cxl: update tests and documentation for new cxl properties Gregory Price
2022-10-11 22:20 ` [PATCH 0/5] Multi-Region and Volatile Memory support for CXL Type-3 Devices Michael S. Tsirkin
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=20221013100740.0000471b@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=a.manzanares@samsung.com \
--cc=alison.schofield@intel.com \
--cc=bwidawsk@kernel.org \
--cc=cbrowy@avery-design.com \
--cc=dave@stgolabs.net \
--cc=gourry.memverge@gmail.com \
--cc=gregory.price@memverge.com \
--cc=hchkuo@avery-design.com.tw \
--cc=ira.weiny@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=mst@redhat.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox