fwiw this is what my function looked like after the prior changes, very
similar to yours proposed below
static int ct3_build_cdat_table(CDATSubHeader ***cdat_table,
void *priv)
{
CXLType3Dev *ct3d = priv;
MemoryRegion *vmr = NULL, *pmr = NULL;
uint64_t dpa_base = 0;
int dsmad_handle = 0;
int num_ents = 0;
int cur_ent = 0;
int ret = 0;
if (ct3d->hostvmem) {
vmr = host_memory_backend_get_memory(ct3d->hostvmem);
if (!vmr)
return -EINVAL;
num_ents += CT3_CDAT_SUBTABLE_SIZE;
}
if (ct3d->hostpmem) {
pmr = host_memory_backend_get_memory(ct3d->hostpmem);
if (!pmr)
return -EINVAL;
num_ents += CT3_CDAT_SUBTABLE_SIZE;
}
if (!num_ents) {
return 0;
}
*cdat_table = g_malloc0(num_ents * sizeof(*cdat_table));
if (!*cdat_table) {
return -ENOMEM;
}
/* Volatile aspects are mapped first */
if (vmr) {
ret = ct3_build_cdat_subtable(*cdat_table, vmr, dsmad_handle++,
false, dpa_base);
if (ret < 0) {
goto error_cleanup;
}
dpa_base = vmr->size;
cur_ent += ret;
}
/* Non volatile aspects */
if (pmr) {
/* non-volatile entries follow the volatile entries */
ret = ct3_build_cdat_subtable(&(*cdat_table)[cur_ent], pmr,
dsmad_handle, true, dpa_base);
if (ret < 0) {
goto error_cleanup;
}
cur_ent += ret;
}
assert(cur_ent == num_ents);
return ret;
error_cleanup:
int i;
for (i = 0; i < num_ents; i++) {
g_free(*cdat_table[i]);
}
g_free(*cdat_table);
return ret;
}
On Thu, Oct 13, 2022 at 12:53:13PM +0100, Jonathan Cameron wrote:
> On Thu, 13 Oct 2022 07:36:28 -0400
> Gregory Price <gourry.memve...@gmail.com> wrote:
>
> > Reading through your notes, everything seems reasonable, though I'm not
> > sure I agree with the two pass notion, though I'll wait to see the patch
> > set.
> >
> > The enum is a good idea, *forehead slap*, I should have done it. If we
> > have a local enum, why not just make it global (within the file) and
> > allocate the table as I have once we know how many MRs are present?
>
> It's not global as we need the entries to be packed. So if just one mr
> (which ever one) the entries for that need to be at the beginning of
> cdat_table. I also don't want to bake into the outer caller that the
> entries will always be the same size for different MRs.
>
> For the two pass case...
>
> I'll send code in a few mins, but in meantime my thought is that
> the extended code for volatile + non volatile will looks something like:
> (variable names made up)
>
> if (ct3d->volatile_mem) {
> volatile_mr =
> host_memory_backend_get_memory(ct3d->volatile_mem....);
> if (!volatile_mr) {
> return -ENINVAL;
> }
> rc = ct3_build_cdat_entries_for_mr(NULL, dsmad++, volatile_mr);
> if (rc < 0) {
> return rc;
> }
> volatile_len = rc;
> }
>
> if (ct3d->nonvolatile_mem) {
> nonvolatile_mr =
> host_memory_backend_get_memory(ct3d->nonvolatile_mem);
> if (!nonvolatile_mr) {
> return -ENINVAL;
> }
> rc = ct3_build_cdat_entries_for_mr(NULL, dmsmad++,
> nonvolatile_mr....);
> if (rc < 0) {
> return rc;
> }
> nonvolatile_len = rc;
> }
>
> dsmad = 0;
>
> table = g_malloc(0, (volatile_len + nonvolatile_len) * sizeof(*table));
> if (!table) {
> return -ENOMEM;
> }
>
> if (volatile_len) {
> rc = ct3_build_cdat_entries_for_mr(&table[0], dmsad++,
> volatile_mr....);
> if (rc < 0) {
> return rc;
> }
> }
> if (nonvolatile_len) {
> rc = ct3_build_cdat_entries_for_mr(&table[volatile_len],
> dsmad++, nonvolatile_mr...);
> if (rc < 0) {
> /* Only place we need error handling. Could make it
> more generic of course */
> for (i = 0; i < volatile_len; i++) {
> g_free(cdat_table[i]);
> }
> return rc;
> }
> }
>
> *cdat_table = g_steal_pointer(&table);
>
>
> Jonathan
>
> >
> > 6 eggs/half dozen though, I'm ultimately fine with either.
> >
> > On Thu, Oct 13, 2022, 4:58 AM Jonathan Cameron <jonathan.came...@huawei.com>
> > wrote:
> >
> > > On Wed, 12 Oct 2022 14:21:15 -0400
> > > Gregory Price <gourry.memve...@gmail.com> wrote:
> > >
> > > > Included in this response is a recommended patch set on top of this
> > > > patch that resolves a number of issues, including style and a heap
> > > > corruption bug.
> > > >
> > > > The purpose of this patch set is to refactor the CDAT initialization
> > > > code to support future patch sets that will introduce multi-region
> > > > support in CXL Type3 devices.
> > > >
> > > > 1) Checkpatch errors in the immediately prior patch
> > > > 2) Flatting of code in cdat initialization
> > > > 3) Changes in allocation and error checking for cleanliness
> > > > 4) Change in the allocation/free strategy of CDAT sub-tables to simplify
> > > > multi-region allocation in the future. Also resolves a heap
> > > > corruption bug
> > > > 5) Refactor of CDAT initialization code into a function that initializes
> > > > sub-tables per memory-region.
> > > >
> > > > Gregory Price (5):
> > > > hw/mem/cxl_type3: fix checkpatch errors
> > > > hw/mem/cxl_type3: Pull validation checks ahead of functional code
> > > > hw/mem/cxl_type3: CDAT pre-allocate and check resources prior to work
> > > > hw/mem/cxl_type3: Change the CDAT allocation/free strategy
> > > > hw/mem/cxl_type3: Refactor CDAT sub-table entry initialization into a
> > > > function
> > > >
> > > > hw/mem/cxl_type3.c | 240 +++++++++++++++++++++++----------------------
> > > > 1 file changed, 122 insertions(+), 118 deletions(-)
> > > >
> > >
> > > Thanks, I'm going to roll this stuff into the original patch set for v8.
> > > Some of this I already have (like the check patch stuff).
> > > Some I may disagree with in which case I'll reply to the patches - note
> > > I haven't looked at them in detail yet!
> > >
> > > Jonathan
> > >
> >
>
