Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)

[Gregory Price]

On Fri, Feb 17, 2023 at 04:16:17PM +0000, Jonathan Cameron via wrote:
> On Tue, 31 Jan 2023 16:38:47 +0000
> Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
> 
> > From: Gregory Price <gourry.memverge@gmail.com>
> > 
> > This commit enables each CXL Type-3 device to contain one volatile
> > memory region and one persistent region.
> > 
> > Two new properties have been added to cxl-type3 device initialization:
> >     [volatile-memdev] and [persistent-memdev]
> > 
> > The existing [memdev] property has been deprecated and will default the
> > memory region to a persistent memory region (although a user may assign
> > the region to a ram or file backed region). It cannot be used in
> > combination with the new [persistent-memdev] property.
> > 
> > Partitioning volatile memory from persistent memory is not yet supported.
> > 
> > Volatile memory is mapped at DPA(0x0), while Persistent memory is mapped
> > at DPA(vmem->size), per CXL Spec 8.2.9.8.2.0 - Get Partition Info.
> > 
> > Signed-off-by: Gregory Price <gregory.price@memverge.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> Hi Gregory,
> 
> I've added support for multiple HDM decoders and hence can now
> test both volatile and non volatile on same device.
> It very nearly all works. With one exception which is I couldn't
> poke the first byte of the non volatile region.
> 
> I think we have an off by one in a single check.
> 
> Interestingly it makes no difference when creating an FS on top
> (which was my standard test) so I only noticed when poking memory
> addresses directly to sanity check the HDM decoder setup.
> 
> I'll roll a v2 if no one shouts out that I'm wrong.
> 
> Note that adding multiple HDM decoders massively increases
> the number of test cases over what we had before to poke all the
> corners so I may well be missing stuff.  Hopefully can send an RFC
> of that support out next week.
> 
> Jonathan
> 

Very cool! Thanks for pushing this over the finishing line.

All my testing so far has been really smooth since getting the TCG issue
worked out.

> > -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> > -                           unsigned size, MemTxAttrs attrs)
[...]
> > +    if (vmr) {
> > +        if (*dpa_offset <= int128_get64(vmr->size)) {
> 
> Off by one I think.  < 
> 

Yes that makes sense, should be <.  Derp derp.

Though I think this may alludes to more off-by-one issues?  This says

if (dpa_offset < vmr->size)

but dpa_offset should be (hpa - memory_region_base),

The HPA is used by memory access routing for the whole system to determine
what device it should access.

If that corner case is being hit, doesn't it imply the higher level code
is also susceptible to this, and is routing accesses to the wrong device?

~Gregory