| Message ID | 20240304194331.1586191-7-nifan.cxl@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enabling DCD emulation support in Qemu | expand |
On Mon, 4 Mar 2024 11:34:01 -0800 nifan.cxl@gmail.com wrote: > From: Fan Ni <fan.ni@samsung.com> > > Add (file/memory backed) host backend, all the dynamic capacity regions > will share a single, large enough host backend. Set up address space for > DC regions to support read/write operations to dynamic capacity for DCD. > > With the change, following supports are added: > 1. Add a new property to type3 device "volatile-dc-memdev" to point to host > memory backend for dynamic capacity. Currently, all dc regions share one > host backend. > 2. Add namespace for dynamic capacity for read/write support; > 3. Create cdat entries for each dynamic capacity region; > 4. Fix dvsec range registers to include DC regions. > > Signed-off-by: Fan Ni <fan.ni@samsung.com> Hi Fan, This one has a few more significant comments inline. thanks, Jonathan > --- > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index c045fee32d..2b380a260b 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -45,7 +45,8 @@ enum { > > static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, > int dsmad_handle, uint64_t size, > - bool is_pmem, uint64_t dpa_base) > + bool is_pmem, bool is_dynamic, > + uint64_t dpa_base) > { > g_autofree CDATDsmas *dsmas = NULL; > g_autofree CDATDslbis *dslbis0 = NULL; There is a fixlet going through for these as the autofree doesn't do anything. Will require a rebase. I'll do it on my tree, but might not push that out for a few days so this is just a heads up for anyone using these. https://lore.kernel.org/qemu-devel/20240304104406.59855-1-thuth@redhat.com/ It went in clean for me, so may not even be something anyone notices! > @@ -61,7 +62,8 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, > .length = sizeof(*dsmas), > }, > .DSMADhandle = dsmad_handle, > - .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0, > + .flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) | > + (is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0), > .DPA_base = dpa_base, > .DPA_length = size, > }; > @@ -149,12 +151,13 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) > g_autofree CDATSubHeader **table = NULL; > > > @@ -176,21 +179,55 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) > pmr_size = memory_region_size(nonvolatile_mr); > } > > + if (ct3d->dc.num_regions) { > + if (ct3d->dc.host_dc) { > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > + if (!dc_mr) { > + return -EINVAL; > + } > + len += CT3_CDAT_NUM_ENTRIES * ct3d->dc.num_regions; > + } else { > + return -EINVAL; Flip logic to get the error out the way first and reduce indent. if (ct3d->dc.num_regions) { if (!ct3d->dc.host_dc) { return -EINVAL; } dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); if (!dc_mr) { return -EINVAL; } len += CT3... } > + } > + } > + > > *cdat_table = g_steal_pointer(&table); > @@ -300,11 +337,24 @@ static void build_dvsecs(CXLType3Dev *ct3d) > range2_size_hi = ct3d->hostpmem->size >> 32; > range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > (ct3d->hostpmem->size & 0xF0000000); > + } else if (ct3d->dc.host_dc) { > + range2_size_hi = ct3d->dc.host_dc->size >> 32; > + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > + (ct3d->dc.host_dc->size & 0xF0000000); > } > - } else { > + } else if (ct3d->hostpmem) { > range1_size_hi = ct3d->hostpmem->size >> 32; > range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | > (ct3d->hostpmem->size & 0xF0000000); > + if (ct3d->dc.host_dc) { > + range2_size_hi = ct3d->dc.host_dc->size >> 32; > + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > + (ct3d->dc.host_dc->size & 0xF0000000); > + } > + } else { > + range1_size_hi = ct3d->dc.host_dc->size >> 32; > + range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | > + (ct3d->dc.host_dc->size & 0xF0000000); I've forgotten if we ever closed out on the right thing to do with the legacy range registers. Maybe, just ignoring DC is the right option for now? So I'd drop this block of changes. Maybe Linux will do the wrong thing if we do, but then we should make Linux more flexible on this. If we did get a clarification that this is the right way to go then add a note here. > } > > dvsec = (uint8_t *)&(CXLDVSECDevice){ > @@ -579,11 +629,27 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp) > { > int i; > uint64_t region_base = 0; > - uint64_t region_len = 2 * GiB; > - uint64_t decode_len = 2 * GiB; > + uint64_t region_len; > + uint64_t decode_len; > uint64_t blk_size = 2 * MiB; > CXLDCRegion *region; > MemoryRegion *mr; > + uint64_t dc_size; > + > + mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > + dc_size = memory_region_size(mr); > + region_len = DIV_ROUND_UP(dc_size, ct3d->dc.num_regions); > + > + if (region_len * ct3d->dc.num_regions > dc_size) { This check had me scratching my head for a minute. Why not just check if (dc_size % (ct3d->dc.num_regions * CXL_CAPACITY_MULTIPLER) != 0) { error_setg(errp, "host backend must by a multiple of 256MiB and region len); return false; } > + error_setg(errp, "host backend size must be multiples of region len"); > + return false; > + } > + if (region_len % CXL_CAPACITY_MULTIPLIER != 0) { > + error_setg(errp, "DC region size is unaligned to %lx", > + CXL_CAPACITY_MULTIPLIER); > + return false; > + } > + decode_len = region_len; > @@ -868,16 +974,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d, > AddressSpace **as, > uint64_t *dpa_offset) > { > - MemoryRegion *vmr = NULL, *pmr = NULL; > + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; > + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; > > if (ct3d->hostvmem) { > vmr = host_memory_backend_get_memory(ct3d->hostvmem); > + vmr_size = memory_region_size(vmr); > } > if (ct3d->hostpmem) { > pmr = host_memory_backend_get_memory(ct3d->hostpmem); > + pmr_size = memory_region_size(pmr); > + } > + if (ct3d->dc.host_dc) { > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > + /* Do we want dc_size to be dc_mr->size or not?? */ Maybe - definitely don't want to leave this comment here unanswered and I think you enforce it above anyway. So if we get here ct3d->dc.total_capacity == memory_region_size(ct3d->dc.host_dc); As such we could just not stash total_capacity at all? > + dc_size = ct3d->dc.total_capacity; > }
On Wed, Mar 06, 2024 at 04:28:16PM +0000, Jonathan Cameron wrote: > On Mon, 4 Mar 2024 11:34:01 -0800 > nifan.cxl@gmail.com wrote: > > > From: Fan Ni <fan.ni@samsung.com> > > > > Add (file/memory backed) host backend, all the dynamic capacity regions > > will share a single, large enough host backend. Set up address space for > > DC regions to support read/write operations to dynamic capacity for DCD. > > > > With the change, following supports are added: > > 1. Add a new property to type3 device "volatile-dc-memdev" to point to host > > memory backend for dynamic capacity. Currently, all dc regions share one > > host backend. > > 2. Add namespace for dynamic capacity for read/write support; > > 3. Create cdat entries for each dynamic capacity region; > > 4. Fix dvsec range registers to include DC regions. > > > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > Hi Fan, > > This one has a few more significant comments inline. > > thanks, > > Jonathan > > > --- Hi Jonathan, Thanks for the review. See below, > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index c045fee32d..2b380a260b 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -45,7 +45,8 @@ enum { > > > > static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, > > int dsmad_handle, uint64_t size, > > - bool is_pmem, uint64_t dpa_base) > > + bool is_pmem, bool is_dynamic, > > + uint64_t dpa_base) > > { > > g_autofree CDATDsmas *dsmas = NULL; > > g_autofree CDATDslbis *dslbis0 = NULL; > > There is a fixlet going through for these as the autofree doesn't do anything. > Will require a rebase. I'll do it on my tree, but might not push that out for a > few days so this is just a heads up for anyone using these. > > https://lore.kernel.org/qemu-devel/20240304104406.59855-1-thuth@redhat.com/ > > It went in clean for me, so may not even be something anyone notices! > OK. So I will not rebase for v6 until there is a break. > > @@ -61,7 +62,8 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, > > .length = sizeof(*dsmas), > > }, > > .DSMADhandle = dsmad_handle, > > - .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0, > > + .flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) | > > + (is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0), > > .DPA_base = dpa_base, > > .DPA_length = size, > > }; > > @@ -149,12 +151,13 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) > > g_autofree CDATSubHeader **table = NULL; > > > > > > @@ -176,21 +179,55 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) > > pmr_size = memory_region_size(nonvolatile_mr); > > } > > > > + if (ct3d->dc.num_regions) { > > + if (ct3d->dc.host_dc) { > > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > + if (!dc_mr) { > > + return -EINVAL; > > + } > > + len += CT3_CDAT_NUM_ENTRIES * ct3d->dc.num_regions; > > + } else { > > + return -EINVAL; > > Flip logic to get the error out the way first and reduce indent. > > if (ct3d->dc.num_regions) { > if (!ct3d->dc.host_dc) { > return -EINVAL; > } > dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > if (!dc_mr) { > return -EINVAL; > } > len += CT3... > } Will do. > > > + } > > + } > > + > > > > > *cdat_table = g_steal_pointer(&table); > > @@ -300,11 +337,24 @@ static void build_dvsecs(CXLType3Dev *ct3d) > > range2_size_hi = ct3d->hostpmem->size >> 32; > > range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > (ct3d->hostpmem->size & 0xF0000000); > > + } else if (ct3d->dc.host_dc) { > > + range2_size_hi = ct3d->dc.host_dc->size >> 32; > > + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > + (ct3d->dc.host_dc->size & 0xF0000000); > > } > > - } else { > > + } else if (ct3d->hostpmem) { > > range1_size_hi = ct3d->hostpmem->size >> 32; > > range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > (ct3d->hostpmem->size & 0xF0000000); > > + if (ct3d->dc.host_dc) { > > + range2_size_hi = ct3d->dc.host_dc->size >> 32; > > + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > + (ct3d->dc.host_dc->size & 0xF0000000); > > + } > > + } else { > > + range1_size_hi = ct3d->dc.host_dc->size >> 32; > > + range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > + (ct3d->dc.host_dc->size & 0xF0000000); > > I've forgotten if we ever closed out on the right thing to do > with the legacy range registers. Maybe, just ignoring DC is the > right option for now? So I'd drop this block of changes. > Maybe Linux will do the wrong thing if we do, but then we should > make Linux more flexible on this. > > If we did get a clarification that this is the right way to go > then add a note here. > OK. Will drop the changes here. > > > } > > > > dvsec = (uint8_t *)&(CXLDVSECDevice){ > > @@ -579,11 +629,27 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp) > > { > > int i; > > uint64_t region_base = 0; > > - uint64_t region_len = 2 * GiB; > > - uint64_t decode_len = 2 * GiB; > > + uint64_t region_len; > > + uint64_t decode_len; > > uint64_t blk_size = 2 * MiB; > > CXLDCRegion *region; > > MemoryRegion *mr; > > + uint64_t dc_size; > > + > > + mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > + dc_size = memory_region_size(mr); > > + region_len = DIV_ROUND_UP(dc_size, ct3d->dc.num_regions); > > + > > + if (region_len * ct3d->dc.num_regions > dc_size) { > This check had me scratching my head for a minute. > Why not just check > > if (dc_size % (ct3d->dc.num_regions * CXL_CAPACITY_MULTIPLER) != 0) { > error_setg(errp, "host backend must by a multiple of 256MiB and region len); > return false; Your way is more straightforward, will follow your suggestion. > } > > + error_setg(errp, "host backend size must be multiples of region len"); > > + return false; > > + } > > + if (region_len % CXL_CAPACITY_MULTIPLIER != 0) { > > + error_setg(errp, "DC region size is unaligned to %lx", > > + CXL_CAPACITY_MULTIPLIER); > > + return false; > > + } > > + decode_len = region_len; > > > > > > @@ -868,16 +974,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d, > > AddressSpace **as, > > uint64_t *dpa_offset) > > { > > - MemoryRegion *vmr = NULL, *pmr = NULL; > > + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; > > + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; > > > > if (ct3d->hostvmem) { > > vmr = host_memory_backend_get_memory(ct3d->hostvmem); > > + vmr_size = memory_region_size(vmr); > > } > > if (ct3d->hostpmem) { > > pmr = host_memory_backend_get_memory(ct3d->hostpmem); > > + pmr_size = memory_region_size(pmr); > > + } > > + if (ct3d->dc.host_dc) { > > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > + /* Do we want dc_size to be dc_mr->size or not?? */ > > Maybe - definitely don't want to leave this comment here > unanswered and I think you enforce it above anyway. > > So if we get here ct3d->dc.total_capacity == memory_region_size(ct3d->dc.host_dc); > As such we could just not stash total_capacity at all? I cannot identify a case where these two will be different. But total_capacity is referenced at quite some places, it may be nice to have it so we do not need to call the function to get the value every time? Fan > > > > + dc_size = ct3d->dc.total_capacity; > > } >
> > > @@ -868,16 +974,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d, > > > AddressSpace **as, > > > uint64_t *dpa_offset) > > > { > > > - MemoryRegion *vmr = NULL, *pmr = NULL; > > > + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; > > > + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; > > > > > > if (ct3d->hostvmem) { > > > vmr = host_memory_backend_get_memory(ct3d->hostvmem); > > > + vmr_size = memory_region_size(vmr); > > > } > > > if (ct3d->hostpmem) { > > > pmr = host_memory_backend_get_memory(ct3d->hostpmem); > > > + pmr_size = memory_region_size(pmr); > > > + } > > > + if (ct3d->dc.host_dc) { > > > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > > + /* Do we want dc_size to be dc_mr->size or not?? */ > > > > Maybe - definitely don't want to leave this comment here > > unanswered and I think you enforce it above anyway. > > > > So if we get here ct3d->dc.total_capacity == memory_region_size(ct3d->dc.host_dc); > > As such we could just not stash total_capacity at all? > > I cannot identify a case where these two will be different. But > total_capacity is referenced at quite some places, it may be nice to have > it so we do not need to call the function to get the value every time? I kind of like having it via one path so that there is no confusion for the reader, but up to you on this one. The function called is trivial (other than some magic to handle very large memory regions) so this is just a readability question, not a perf one. Whatever, don't leave the question behind. Find to have something that says they are always the same size if you don't get rid of the total_capacity representation. Jonathan
On Thu, Mar 07, 2024 at 12:16:05PM +0000, Jonathan Cameron wrote: > > > > @@ -868,16 +974,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d, > > > > AddressSpace **as, > > > > uint64_t *dpa_offset) > > > > { > > > > - MemoryRegion *vmr = NULL, *pmr = NULL; > > > > + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; > > > > + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; > > > > > > > > if (ct3d->hostvmem) { > > > > vmr = host_memory_backend_get_memory(ct3d->hostvmem); > > > > + vmr_size = memory_region_size(vmr); > > > > } > > > > if (ct3d->hostpmem) { > > > > pmr = host_memory_backend_get_memory(ct3d->hostpmem); > > > > + pmr_size = memory_region_size(pmr); > > > > + } > > > > + if (ct3d->dc.host_dc) { > > > > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > > > + /* Do we want dc_size to be dc_mr->size or not?? */ > > > > > > Maybe - definitely don't want to leave this comment here > > > unanswered and I think you enforce it above anyway. > > > > > > So if we get here ct3d->dc.total_capacity == memory_region_size(ct3d->dc.host_dc); > > > As such we could just not stash total_capacity at all? > > > > I cannot identify a case where these two will be different. But > > total_capacity is referenced at quite some places, it may be nice to have > > it so we do not need to call the function to get the value every time? > > I kind of like having it via one path so that there is no confusion > for the reader, but up to you on this one. The function called is trivial > (other than some magic to handle very large memory regions) so > this is just a readability question, not a perf one. > > Whatever, don't leave the question behind. Find to have something > that says they are always the same size if you don't get rid > of the total_capacity representation. > I will fix it. For static capability, we have a variable static_mem_size, although we can calculate it from volatile and non-volatile memory region size. There are quite some places need to get the dynamic capacity, it is much more convenient to have a variable ready to use, I will keep it for now. Fan > > Jonathan
On Wed, Mar 06, 2024 at 04:28:16PM +0000, Jonathan Cameron wrote: > On Mon, 4 Mar 2024 11:34:01 -0800 > nifan.cxl@gmail.com wrote: > > > From: Fan Ni <fan.ni@samsung.com> > > > > Add (file/memory backed) host backend, all the dynamic capacity regions > > will share a single, large enough host backend. Set up address space for > > DC regions to support read/write operations to dynamic capacity for DCD. > > > > With the change, following supports are added: > > 1. Add a new property to type3 device "volatile-dc-memdev" to point to host > > memory backend for dynamic capacity. Currently, all dc regions share one > > host backend. > > 2. Add namespace for dynamic capacity for read/write support; > > 3. Create cdat entries for each dynamic capacity region; > > 4. Fix dvsec range registers to include DC regions. > > > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > Hi Fan, > > This one has a few more significant comments inline. > > thanks, > > Jonathan > > > --- > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index c045fee32d..2b380a260b 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -45,7 +45,8 @@ enum { > > > > static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, > > int dsmad_handle, uint64_t size, > > - bool is_pmem, uint64_t dpa_base) > > + bool is_pmem, bool is_dynamic, > > + uint64_t dpa_base) > > { > > g_autofree CDATDsmas *dsmas = NULL; > > g_autofree CDATDslbis *dslbis0 = NULL; > > There is a fixlet going through for these as the autofree doesn't do anything. > Will require a rebase. I'll do it on my tree, but might not push that out for a > few days so this is just a heads up for anyone using these. > > https://lore.kernel.org/qemu-devel/20240304104406.59855-1-thuth@redhat.com/ > > It went in clean for me, so may not even be something anyone notices! > > > @@ -61,7 +62,8 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, > > .length = sizeof(*dsmas), > > }, > > .DSMADhandle = dsmad_handle, > > - .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0, > > + .flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) | > > + (is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0), > > .DPA_base = dpa_base, > > .DPA_length = size, > > }; > > @@ -149,12 +151,13 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) > > g_autofree CDATSubHeader **table = NULL; > > > > > > @@ -176,21 +179,55 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) > > pmr_size = memory_region_size(nonvolatile_mr); > > } > > > > + if (ct3d->dc.num_regions) { > > + if (ct3d->dc.host_dc) { > > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > + if (!dc_mr) { > > + return -EINVAL; > > + } > > + len += CT3_CDAT_NUM_ENTRIES * ct3d->dc.num_regions; > > + } else { > > + return -EINVAL; > > Flip logic to get the error out the way first and reduce indent. > > if (ct3d->dc.num_regions) { > if (!ct3d->dc.host_dc) { > return -EINVAL; > } > dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > if (!dc_mr) { > return -EINVAL; > } > len += CT3... > } > > > + } > > + } > > + > > > > > *cdat_table = g_steal_pointer(&table); > > @@ -300,11 +337,24 @@ static void build_dvsecs(CXLType3Dev *ct3d) > > range2_size_hi = ct3d->hostpmem->size >> 32; > > range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > (ct3d->hostpmem->size & 0xF0000000); > > + } else if (ct3d->dc.host_dc) { > > + range2_size_hi = ct3d->dc.host_dc->size >> 32; > > + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > + (ct3d->dc.host_dc->size & 0xF0000000); > > } > > - } else { > > + } else if (ct3d->hostpmem) { > > range1_size_hi = ct3d->hostpmem->size >> 32; > > range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > (ct3d->hostpmem->size & 0xF0000000); > > + if (ct3d->dc.host_dc) { > > + range2_size_hi = ct3d->dc.host_dc->size >> 32; > > + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > + (ct3d->dc.host_dc->size & 0xF0000000); > > + } > > + } else { > > + range1_size_hi = ct3d->dc.host_dc->size >> 32; > > + range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | > > + (ct3d->dc.host_dc->size & 0xF0000000); > > I've forgotten if we ever closed out on the right thing to do > with the legacy range registers. Maybe, just ignoring DC is the > right option for now? So I'd drop this block of changes. > Maybe Linux will do the wrong thing if we do, but then we should > make Linux more flexible on this. > > If we did get a clarification that this is the right way to go > then add a note here. Hi Jonathan, I have noticed in the current kernel code, when checking whether the media is ready (in cxl_await_media_ready), we need to check the devsec range registers, for dcd device, if we leave dvsec range registers unset, the device cannot put into "ready" state, which will cause the device inactive. https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/tree/drivers/cxl/core/pci.c?h=fixes&id=d206a76d7d2726f3b096037f2079ce0bd3ba329b#n195 So we need to set it as above?? DO I miss anything? Fan > > > > } > > > > dvsec = (uint8_t *)&(CXLDVSECDevice){ > > @@ -579,11 +629,27 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp) > > { > > int i; > > uint64_t region_base = 0; > > - uint64_t region_len = 2 * GiB; > > - uint64_t decode_len = 2 * GiB; > > + uint64_t region_len; > > + uint64_t decode_len; > > uint64_t blk_size = 2 * MiB; > > CXLDCRegion *region; > > MemoryRegion *mr; > > + uint64_t dc_size; > > + > > + mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > + dc_size = memory_region_size(mr); > > + region_len = DIV_ROUND_UP(dc_size, ct3d->dc.num_regions); > > + > > + if (region_len * ct3d->dc.num_regions > dc_size) { > This check had me scratching my head for a minute. > Why not just check > > if (dc_size % (ct3d->dc.num_regions * CXL_CAPACITY_MULTIPLER) != 0) { > error_setg(errp, "host backend must by a multiple of 256MiB and region len); > return false; > } > > + error_setg(errp, "host backend size must be multiples of region len"); > > + return false; > > + } > > + if (region_len % CXL_CAPACITY_MULTIPLIER != 0) { > > + error_setg(errp, "DC region size is unaligned to %lx", > > + CXL_CAPACITY_MULTIPLIER); > > + return false; > > + } > > + decode_len = region_len; > > > > > > @@ -868,16 +974,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d, > > AddressSpace **as, > > uint64_t *dpa_offset) > > { > > - MemoryRegion *vmr = NULL, *pmr = NULL; > > + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; > > + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; > > > > if (ct3d->hostvmem) { > > vmr = host_memory_backend_get_memory(ct3d->hostvmem); > > + vmr_size = memory_region_size(vmr); > > } > > if (ct3d->hostpmem) { > > pmr = host_memory_backend_get_memory(ct3d->hostpmem); > > + pmr_size = memory_region_size(pmr); > > + } > > + if (ct3d->dc.host_dc) { > > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > + /* Do we want dc_size to be dc_mr->size or not?? */ > > Maybe - definitely don't want to leave this comment here > unanswered and I think you enforce it above anyway. > > So if we get here ct3d->dc.total_capacity == memory_region_size(ct3d->dc.host_dc); > As such we could just not stash total_capacity at all? > > > > + dc_size = ct3d->dc.total_capacity; > > } >
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 853dadba39..8309f27a2b 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -622,7 +622,8 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd, size_t *len_out, CXLCCI *cci) { - CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate; + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); + CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate; struct { uint8_t slots_supported; uint8_t slot_info; @@ -636,7 +637,8 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd, QEMU_BUILD_BUG_ON(sizeof(*fw_info) != 0x50); if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) || - (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) { + (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER) || + (ct3d->dc.total_capacity < CXL_CAPACITY_MULTIPLIER)) { return CXL_MBOX_INTERNAL_ERROR; } @@ -793,7 +795,8 @@ static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd, CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate; if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) || - (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) { + (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER)) || + (!QEMU_IS_ALIGNED(ct3d->dc.total_capacity, CXL_CAPACITY_MULTIPLIER))) { return CXL_MBOX_INTERNAL_ERROR; } @@ -835,9 +838,11 @@ static CXLRetCode cmd_ccls_get_partition_info(const struct cxl_cmd *cmd, uint64_t next_pmem; } QEMU_PACKED *part_info = (void *)payload_out; QEMU_BUILD_BUG_ON(sizeof(*part_info) != 0x20); + CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate); if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) || - (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) { + (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER)) || + (!QEMU_IS_ALIGNED(ct3d->dc.total_capacity, CXL_CAPACITY_MULTIPLIER))) { return CXL_MBOX_INTERNAL_ERROR; } @@ -1179,7 +1184,8 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd, struct clear_poison_pl *in = (void *)payload_in; dpa = ldq_le_p(&in->dpa); - if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->static_mem_size) { + if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->static_mem_size + + ct3d->dc.total_capacity) { return CXL_MBOX_INVALID_PA; } diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index c045fee32d..2b380a260b 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -45,7 +45,8 @@ enum { static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, int dsmad_handle, uint64_t size, - bool is_pmem, uint64_t dpa_base) + bool is_pmem, bool is_dynamic, + uint64_t dpa_base) { g_autofree CDATDsmas *dsmas = NULL; g_autofree CDATDslbis *dslbis0 = NULL; @@ -61,7 +62,8 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table, .length = sizeof(*dsmas), }, .DSMADhandle = dsmad_handle, - .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0, + .flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) | + (is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0), .DPA_base = dpa_base, .DPA_length = size, }; @@ -149,12 +151,13 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) g_autofree CDATSubHeader **table = NULL; CXLType3Dev *ct3d = priv; MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL; + MemoryRegion *dc_mr = NULL; uint64_t vmr_size = 0, pmr_size = 0; int dsmad_handle = 0; int cur_ent = 0; int len = 0; - if (!ct3d->hostpmem && !ct3d->hostvmem) { + if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) { return 0; } @@ -176,21 +179,55 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) pmr_size = memory_region_size(nonvolatile_mr); } + if (ct3d->dc.num_regions) { + if (ct3d->dc.host_dc) { + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); + if (!dc_mr) { + return -EINVAL; + } + len += CT3_CDAT_NUM_ENTRIES * ct3d->dc.num_regions; + } else { + return -EINVAL; + } + } + table = g_malloc0(len * sizeof(*table)); /* Now fill them in */ if (volatile_mr) { ct3_build_cdat_entries_for_mr(table, dsmad_handle++, vmr_size, - false, 0); + false, false, 0); cur_ent = CT3_CDAT_NUM_ENTRIES; } if (nonvolatile_mr) { uint64_t base = vmr_size; ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++, - pmr_size, true, base); + pmr_size, true, false, base); cur_ent += CT3_CDAT_NUM_ENTRIES; } + + if (dc_mr) { + int i; + uint64_t region_base = vmr_size + pmr_size; + + /* + * TODO: we assume the dynamic capacity to be volatile for now, + * non-volatile dynamic capacity will be added if needed in the + * future. + */ + for (i = 0; i < ct3d->dc.num_regions; i++) { + ct3_build_cdat_entries_for_mr(&(table[cur_ent]), + dsmad_handle++, + ct3d->dc.regions[i].len, + false, true, region_base); + ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1; + + cur_ent += CT3_CDAT_NUM_ENTRIES; + region_base += ct3d->dc.regions[i].len; + } + } + assert(len == cur_ent); *cdat_table = g_steal_pointer(&table); @@ -300,11 +337,24 @@ static void build_dvsecs(CXLType3Dev *ct3d) range2_size_hi = ct3d->hostpmem->size >> 32; range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | (ct3d->hostpmem->size & 0xF0000000); + } else if (ct3d->dc.host_dc) { + range2_size_hi = ct3d->dc.host_dc->size >> 32; + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | + (ct3d->dc.host_dc->size & 0xF0000000); } - } else { + } else if (ct3d->hostpmem) { range1_size_hi = ct3d->hostpmem->size >> 32; range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | (ct3d->hostpmem->size & 0xF0000000); + if (ct3d->dc.host_dc) { + range2_size_hi = ct3d->dc.host_dc->size >> 32; + range2_size_lo = (2 << 5) | (2 << 2) | 0x3 | + (ct3d->dc.host_dc->size & 0xF0000000); + } + } else { + range1_size_hi = ct3d->dc.host_dc->size >> 32; + range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | + (ct3d->dc.host_dc->size & 0xF0000000); } dvsec = (uint8_t *)&(CXLDVSECDevice){ @@ -579,11 +629,27 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp) { int i; uint64_t region_base = 0; - uint64_t region_len = 2 * GiB; - uint64_t decode_len = 2 * GiB; + uint64_t region_len; + uint64_t decode_len; uint64_t blk_size = 2 * MiB; CXLDCRegion *region; MemoryRegion *mr; + uint64_t dc_size; + + mr = host_memory_backend_get_memory(ct3d->dc.host_dc); + dc_size = memory_region_size(mr); + region_len = DIV_ROUND_UP(dc_size, ct3d->dc.num_regions); + + if (region_len * ct3d->dc.num_regions > dc_size) { + error_setg(errp, "host backend size must be multiples of region len"); + return false; + } + if (region_len % CXL_CAPACITY_MULTIPLIER != 0) { + error_setg(errp, "DC region size is unaligned to %lx", + CXL_CAPACITY_MULTIPLIER); + return false; + } + decode_len = region_len; if (ct3d->hostvmem) { mr = host_memory_backend_get_memory(ct3d->hostvmem); @@ -605,6 +671,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp) region->flags = 0; region_base += region->len; + ct3d->dc.total_capacity += region->len; } return true; @@ -614,7 +681,8 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) { DeviceState *ds = DEVICE(ct3d); - if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem) { + if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem + && !ct3d->dc.num_regions) { error_setg(errp, "at least one memdev property must be set"); return false; } else if (ct3d->hostmem && ct3d->hostpmem) { @@ -678,9 +746,41 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) g_free(p_name); } - if (!cxl_create_dc_regions(ct3d, errp)) { - error_setg(errp, "setup DC regions failed"); - return false; + ct3d->dc.total_capacity = 0; + if (ct3d->dc.num_regions) { + MemoryRegion *dc_mr; + char *dc_name; + + if (!ct3d->dc.host_dc) { + error_setg(errp, "dynamic capacity must have a backing device"); + return false; + } + + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); + if (!dc_mr) { + error_setg(errp, "dynamic capacity must have a backing device"); + return false; + } + + /* + * TODO: set dc as volatile for now, non-volatile support can be added + * in the future if needed. + */ + memory_region_set_nonvolatile(dc_mr, false); + memory_region_set_enabled(dc_mr, true); + host_memory_backend_set_mapped(ct3d->dc.host_dc, true); + if (ds->id) { + dc_name = g_strdup_printf("cxl-dcd-dpa-dc-space:%s", ds->id); + } else { + dc_name = g_strdup("cxl-dcd-dpa-dc-space"); + } + address_space_init(&ct3d->dc.host_dc_as, dc_mr, dc_name); + g_free(dc_name); + + if (!cxl_create_dc_regions(ct3d, errp)) { + error_setg(errp, "setup DC regions failed"); + return false; + } } return true; @@ -772,6 +872,9 @@ err_release_cdat: err_free_special_ops: g_free(regs->special_ops); err_address_space_free: + if (ct3d->dc.host_dc) { + address_space_destroy(&ct3d->dc.host_dc_as); + } if (ct3d->hostpmem) { address_space_destroy(&ct3d->hostpmem_as); } @@ -790,6 +893,9 @@ static void ct3_exit(PCIDevice *pci_dev) pcie_aer_exit(pci_dev); cxl_doe_cdat_release(cxl_cstate); g_free(regs->special_ops); + if (ct3d->dc.host_dc) { + address_space_destroy(&ct3d->dc.host_dc_as); + } if (ct3d->hostpmem) { address_space_destroy(&ct3d->hostpmem_as); } @@ -868,16 +974,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d, AddressSpace **as, uint64_t *dpa_offset) { - MemoryRegion *vmr = NULL, *pmr = NULL; + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; if (ct3d->hostvmem) { vmr = host_memory_backend_get_memory(ct3d->hostvmem); + vmr_size = memory_region_size(vmr); } if (ct3d->hostpmem) { pmr = host_memory_backend_get_memory(ct3d->hostpmem); + pmr_size = memory_region_size(pmr); + } + if (ct3d->dc.host_dc) { + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); + /* Do we want dc_size to be dc_mr->size or not?? */ + dc_size = ct3d->dc.total_capacity; } - if (!vmr && !pmr) { + if (!vmr && !pmr && !dc_mr) { return -ENODEV; } @@ -885,19 +999,18 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d, return -EINVAL; } - if (*dpa_offset > ct3d->cxl_dstate.static_mem_size) { + if (*dpa_offset >= vmr_size + pmr_size + dc_size) { return -EINVAL; } - if (vmr) { - if (*dpa_offset < memory_region_size(vmr)) { - *as = &ct3d->hostvmem_as; - } else { - *as = &ct3d->hostpmem_as; - *dpa_offset -= memory_region_size(vmr); - } - } else { + if (*dpa_offset < vmr_size) { + *as = &ct3d->hostvmem_as; + } else if (*dpa_offset < vmr_size + pmr_size) { *as = &ct3d->hostpmem_as; + *dpa_offset -= vmr_size; + } else { + *as = &ct3d->dc.host_dc_as; + *dpa_offset -= (vmr_size + pmr_size); } return 0; @@ -979,6 +1092,8 @@ static Property ct3_props[] = { DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL), DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename), DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0), + DEFINE_PROP_LINK("volatile-dc-memdev", CXLType3Dev, dc.host_dc, + TYPE_MEMORY_BACKEND, HostMemoryBackend *), DEFINE_PROP_END_OF_LIST(), }; @@ -1045,33 +1160,39 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size, static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data) { - MemoryRegion *vmr = NULL, *pmr = NULL; + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; AddressSpace *as; + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; if (ct3d->hostvmem) { vmr = host_memory_backend_get_memory(ct3d->hostvmem); + vmr_size = memory_region_size(vmr); } if (ct3d->hostpmem) { pmr = host_memory_backend_get_memory(ct3d->hostpmem); + pmr_size = memory_region_size(pmr); } + if (ct3d->dc.host_dc) { + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); + dc_size = ct3d->dc.total_capacity; + } - if (!vmr && !pmr) { + if (!vmr && !pmr && !dc_mr) { return false; } - if (dpa_offset + CXL_CACHE_LINE_SIZE > ct3d->cxl_dstate.static_mem_size) { + if (dpa_offset + CXL_CACHE_LINE_SIZE > vmr_size + pmr_size + dc_size) { return false; } - if (vmr) { - if (dpa_offset < memory_region_size(vmr)) { - as = &ct3d->hostvmem_as; - } else { - as = &ct3d->hostpmem_as; - dpa_offset -= memory_region_size(vmr); - } - } else { + if (dpa_offset < vmr_size) { + as = &ct3d->hostvmem_as; + } else if (dpa_offset < vmr_size + pmr_size) { as = &ct3d->hostpmem_as; + dpa_offset -= vmr_size; + } else { + as = &ct3d->dc.host_dc_as; + dpa_offset -= (vmr_size + pmr_size); } address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data, diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h index f82d018422..265679302c 100644 --- a/include/hw/cxl/cxl_device.h +++ b/include/hw/cxl/cxl_device.h @@ -467,6 +467,10 @@ struct CXLType3Dev { uint64_t poison_list_overflow_ts; struct dynamic_capacity { + HostMemoryBackend *host_dc; + AddressSpace host_dc_as; + uint64_t total_capacity; /* 256M aligned */ + uint8_t num_regions; /* 0-8 regions */ CXLDCRegion regions[DCD_MAX_NUM_REGION]; } dc;