Re: [PATCH v7 4/5] hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange

[Gregory Price <gourry.memverge@gmail.com>]

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.memverge@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.Cameron@huawei.com>
> > wrote:
> > 
> > > On Wed, 12 Oct 2022 14:21:15 -0400
> > > Gregory Price <gourry.memverge@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
> > >  
> > 
>