DCD region base address

DCD region base address

[Shesha Bhushan Sreenivasamurthy]

Hello All,

I am adding FM-API support to add DCD extents in QEMU. The current code is not making sense to me.  According to CXL3.0 spec (Table 8-126), region_base should be the DPA and not the size, decode_length is the total region length which is used to program HDM decoders, region_len is the usable length meaning as extents are added the region_len increases up to a max of decode_length. So should be 0 before we add any extents. 

This is what it should look like in my opinion. Am I correct ?

--- hw/mem/cxl_type3_old.c    2023-07-19 18:18:54.211748475 -0700
+++ hw/mem/cxl_type3.c  2023-07-19 18:27:13.468602825 -0700
@@ -747,10 +747,10 @@
 static int cxl_create_toy_regions(CXLType3Dev *ct3d)
 {
      int i;
-     uint64_t region_base = ct3d->hostvmem?ct3d->hostvmem->size
-           :ct3d->hostpmem->size;
-     uint64_t region_len = 1024*1024*1024;
-     uint64_t decode_len = 4; /* 4*256MB */
+    MemoryRegion *mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
+    uint64_t region_base = (uint64_t)memory_region_get_ram_ptr(mr);
+     uint64_t region_len = 0;
+     uint64_t decode_len = ct3d->dc.host_dc->size / ct3d->dc.num_regions / (256*1024*1024);
      uint64_t blk_size = 2*1024*1024;
      struct CXLDCD_Region *region;

Re: DCD region base address

[Jonathan Cameron]

On Thu, 20 Jul 2023 01:43:34 +0000
Shesha Bhushan Sreenivasamurthy <sheshas@marvell.com> wrote:

> Hello All,
> 
> I am adding FM-API support to add DCD extents in QEMU. The current code is not making sense to me.  According to CXL3.0 spec (Table 8-126), region_base should be the DPA and not the size, decode_length is the total region length which is used to program HDM decoders, region_len is the usable length meaning as extents are added the region_len increases up to a max of decode_length. So should be 0 before we add any extents. 
> 
> This is what it should look like in my opinion. Am I correct ?
> 
> --- hw/mem/cxl_type3_old.c    2023-07-19 18:18:54.211748475 -0700
> +++ hw/mem/cxl_type3.c  2023-07-19 18:27:13.468602825 -0700
> @@ -747,10 +747,10 @@
>  static int cxl_create_toy_regions(CXLType3Dev *ct3d)
>  {
>       int i;
> -     uint64_t region_base = ct3d->hostvmem?ct3d->hostvmem->size
> -           :ct3d->hostpmem->size;
> -     uint64_t region_len = 1024*1024*1024;
> -     uint64_t decode_len = 4; /* 4*256MB */
> +    MemoryRegion *mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
> +    uint64_t region_base = (uint64_t)memory_region_get_ram_ptr(mr);
> +     uint64_t region_len = 0;
> +     uint64_t decode_len = ct3d->dc.host_dc->size / ct3d->dc.num_regions / (256*1024*1024);
>       uint64_t blk_size = 2*1024*1024;
>       struct CXLDCD_Region *region;

What code are we looking at here?

Anyhow, the DPA here is relative to the device which isn't connected to the QEMU implementation
at all.  The address decoder stuff in QEMU will map to the right region wherever it is, we
just need to pretend the go:

[RAM region] [PMem Region] [DC Region]
where any of these are optional and where the Qemu emulation doesn't currently support
sizes that require any padding.

So the offsets above look wrong anyway as it seems to be assuming we either have
volatile or persistent regions but not both.

Jonathan

Re: DCD region base address

[Fan Ni]

On Mon, Jul 24, 2023 at 10:03:42AM +0100, Jonathan Cameron wrote:

> On Thu, 20 Jul 2023 01:43:34 +0000
> Shesha Bhushan Sreenivasamurthy <sheshas@marvell.com> wrote:
> 
> > Hello All,
> > 
> > I am adding FM-API support to add DCD extents in QEMU. The current code is not making sense to me.  According to CXL3.0 spec (Table 8-126), region_base should be the DPA and not the size, decode_length is the total region length which is used to program HDM decoders, region_len is the usable length meaning as extents are added the region_len increases up to a max of decode_length. So should be 0 before we add any extents. 
> > 
> > This is what it should look like in my opinion. Am I correct ?
> > 
> > --- hw/mem/cxl_type3_old.c    2023-07-19 18:18:54.211748475 -0700
> > +++ hw/mem/cxl_type3.c  2023-07-19 18:27:13.468602825 -0700
> > @@ -747,10 +747,10 @@
> >  static int cxl_create_toy_regions(CXLType3Dev *ct3d)
> >  {
> >       int i;
> > -     uint64_t region_base = ct3d->hostvmem?ct3d->hostvmem->size
> > -           :ct3d->hostpmem->size;
> > -     uint64_t region_len = 1024*1024*1024;
> > -     uint64_t decode_len = 4; /* 4*256MB */
> > +    MemoryRegion *mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
> > +    uint64_t region_base = (uint64_t)memory_region_get_ram_ptr(mr);
> > +     uint64_t region_len = 0;
> > +     uint64_t decode_len = ct3d->dc.host_dc->size / ct3d->dc.num_regions / (256*1024*1024);
> >       uint64_t blk_size = 2*1024*1024;
> >       struct CXLDCD_Region *region;
> 
> What code are we looking at here?
> 
> Anyhow, the DPA here is relative to the device which isn't connected to the QEMU implementation
> at all.  The address decoder stuff in QEMU will map to the right region wherever it is, we
> just need to pretend the go:
> 
> [RAM region] [PMem Region] [DC Region]
> where any of these are optional and where the Qemu emulation doesn't currently support
> sizes that require any padding.
> 
> So the offsets above look wrong anyway as it seems to be assuming we either have
> volatile or persistent regions but not both.
> 
> Jonathan
> 

FYI.
The issue was fixed in the new patch as below:

+static int cxl_create_dc_regions(CXLType3Dev *ct3d)
+{
+    int i;
+    uint64_t region_base = (ct3d->hostvmem ? ct3d->hostvmem->size : 0)
+        + (ct3d->hostpmem ? ct3d->hostpmem->size : 0);
+    uint64_t region_len = (uint64_t)2 * 1024 * 1024 * 1024;

https://lore.kernel.org/linux-cxl/20230724162313.34196-5-fan.ni@samsung.com/T/#u

Also, I suggest to try the new patches as it fixed quite a few things.
https://lore.kernel.org/linux-cxl/20230724162313.34196-1-fan.ni@samsung.com/T/#t

Fan