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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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

> -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> -                           unsigned size, MemTxAttrs attrs)
> +static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
> +                                       hwaddr host_addr,
> +                                       unsigned int size,
> +                                       AddressSpace **as,
> +                                       uint64_t *dpa_offset)
>  {
> -    CXLType3Dev *ct3d = CXL_TYPE3(d);
> -    uint64_t dpa_offset;
> -    MemoryRegion *mr;
> +    MemoryRegion *vmr = NULL, *pmr = NULL;
>  
> -    /* TODO support volatile region */
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> -        return MEMTX_ERROR;
> +    if (ct3d->hostvmem) {
> +        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> +    }
> +    if (ct3d->hostpmem) {
> +        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
>      }
>  
> -    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
> -        return MEMTX_ERROR;
> +    if (!vmr && !pmr) {
> +        return -ENODEV;
> +    }
> +
> +    if (!cxl_type3_dpa(ct3d, host_addr, dpa_offset)) {
> +        return -EINVAL;
> +    }
> +
> +    if (*dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
> +        return -EINVAL;
> +    }
> +
> +    if (vmr) {
> +        if (*dpa_offset <= int128_get64(vmr->size)) {

Off by one I think.  < 

> +            *as = &ct3d->hostvmem_as;
> +        } else {
> +            *as = &ct3d->hostpmem_as;
> +            *dpa_offset -= vmr->size;
> +        }
> +    } else {
> +        *as = &ct3d->hostpmem_as;
>      }
>  
> -    if (dpa_offset > int128_get64(mr->size)) {
> +    return 0;
> +}