| Message ID | DM6PR18MB2844A8A869CFB8932782DD67AF3EA@DM6PR18MB2844.namprd18.prod.outlook.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DCD region base address | expand |
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
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
--- 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;