On Sat, 19 Mar 2022 08:53:40 +0000 Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> wrote:
> On 18/03/2022 15:06, Jonathan Cameron wrote: > > > From: Jonathan Cameron <jonathan.came...@huawei.com> > > > > Once a read or write reaches a CXL type 3 device, the HDM decoders > > on the device are used to establish the Device Physical Address > > which should be accessed. These functions peform the required maths > > and then use a device specific address space to access the > > hostmem->mr to fullfil the actual operation. Note that failed writes > > are silent, but failed reads return poison. Note this is based > > loosely on: > > > > https://lore.kernel.org/qemu-devel/20200817161853.593247-6-f4...@amsat.org/ > > [RFC PATCH 0/9] hw/misc: Add support for interleaved memory accesses > > > > Only lightly tested so far. More complex test cases yet to be written. > > > > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com> > > --- > > hw/mem/cxl_type3.c | 88 +++++++++++++++++++++++++++++++++++++ > > include/hw/cxl/cxl_device.h | 6 +++ > > 2 files changed, 94 insertions(+) > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index 244eb5dc91..225155dac5 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -100,7 +100,9 @@ static void ct3_finalize(Object *obj) > > > > static void cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) > > { > > + DeviceState *ds = DEVICE(ct3d); > > MemoryRegion *mr; > > + g_autofree char *name = NULL; > > > > if (!ct3d->hostmem) { > > error_setg(errp, "memdev property must be set"); > > @@ -115,6 +117,13 @@ static void cxl_setup_memory(CXLType3Dev *ct3d, Error > > **errp) > > memory_region_set_nonvolatile(mr, true); > > memory_region_set_enabled(mr, true); > > host_memory_backend_set_mapped(ct3d->hostmem, true); > > There is an existing example for generating names for PCI devices in SPAPR > which you > can borrow which looks something like this (not compile tested!): > > static char *cxl_type3_get_id(CXLType3Dev *ct3d) > { > uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(ct3d)))); > PCIDevice *pd = PCI_DEVICE(ct3d); > DeviceState *ds = DEVICE(ct3d); > > if (ds->id) { > return g_strdup_printf("%s:%02x:%02x.%x", ds->id, busnr, > PCI_SLOT(pd->devfn), PCI_FUNC(pd->devfn)); > } else { > return g_strdup_printf("%02x:%02x.%x", busnr, > PCI_SLOT(pd->devfn), PCI_FUNC(pd->devfn)); > } > } The snag is at this point the PCI bus hasn't been enumerated so all of these numbers are 0 for endpoints. They can change at any point (in theory) so using them to help identify a device is unstable. I switched to using a : as the separator as that is clear than yet another - > > > + > > + if (ds->id) { > > + name = g_strdup_printf("cxl-type3-dpa-space-%s", ds->id); > > + } else { > > + name = g_strdup("cxl-type3-dpa-space"); > > + } > > This then becomes: > > char *id, *name; > ... > > id = cxl_type3_get_id(ct3d); > name = g_strdup_printf("cxl-type3-dpa-space:%s", id); > address_space_init(&ct3d->hostmem_as, mr, name); > g_free(id); > g_free(name); > > > + address_space_init(&ct3d->hostmem_as, mr, name); > > There is an address_space_init() here but no associated > address_space_destroy() - > you'll need to add a ct3_finalize() function to remove the address space, > otherwise > there will be a memory leak when the device is removed because of the > dangling reference. I was lazy on this for two reasons: a) I could actually figure out to do a finalize for a PCI device. I think after digging more it's via the pc->exit callback (which is very oddly named - I guess for historic reasons - why is the unwind of realize called exit? Ah well. I had previously had it in instance_finalize which resulted in a qtest crash as it would destroy an address space we hadn't created. b) Only a tiny percentage of all the address spaces in qemu are ever destroyed (or at least I couldn't find where they were destroyed). I guess using bad practice elsewhere isn't the best way to write code :) Having chased it through it seems to me that what I had in instance_finalize in 23 should have been in pc->exit as it is unwinding stuff done in pc->realize() not the instance_init. Thanks, Jonathan > > > ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size; > > > > if (!ct3d->lsa) { > > @@ -160,6 +169,85 @@ static void ct3_realize(PCIDevice *pci_dev, Error > > **errp) > > &ct3d->cxl_dstate.device_registers); > > } > > > > +/* TODO: Support multiple HDM decoders and DPA skip */ > > +static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t > > *dpa) > > +{ > > + uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers; > > + uint64_t decoder_base, decoder_size, hpa_offset; > > + uint32_t hdm0_ctrl; > > + int ig, iw; > > + > > + decoder_base = (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI] << > > 32) | > > + cache_mem[R_CXL_HDM_DECODER0_BASE_LO]); > > + if ((uint64_t)host_addr < decoder_base) { > > + return false; > > + } > > + > > + hpa_offset = (uint64_t)host_addr - decoder_base; > > + > > + decoder_size = ((uint64_t)cache_mem[R_CXL_HDM_DECODER0_SIZE_HI] << 32) > > | > > + cache_mem[R_CXL_HDM_DECODER0_SIZE_LO]; > > + if (hpa_offset >= decoder_size) { > > + return false; > > + } > > + > > + hdm0_ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL]; > > + iw = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IW); > > + ig = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IG); > > + > > + *dpa = (MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) | > > + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset) >> > > iw); > > + > > + return true; > > +} > > + > > +MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data, > > + unsigned size, MemTxAttrs attrs) > > +{ > > + CXLType3Dev *ct3d = CT3(d); > > + uint64_t dpa_offset; > > + MemoryRegion *mr; > > + > > + /* TODO support volatile region */ > > + mr = host_memory_backend_get_memory(ct3d->hostmem); > > + if (!mr) { > > + return MEMTX_ERROR; > > + } > > + > > + if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) { > > + return MEMTX_ERROR; > > + } > > + > > + if (dpa_offset > int128_get64(mr->size)) { > > + return MEMTX_ERROR; > > + } > > + > > + return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data, > > size); > > +} > > + > > +MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data, > > + unsigned size, MemTxAttrs attrs) > > +{ > > + CXLType3Dev *ct3d = CT3(d); > > + uint64_t dpa_offset; > > + MemoryRegion *mr; > > + > > + mr = host_memory_backend_get_memory(ct3d->hostmem); > > + if (!mr) { > > + return MEMTX_OK; > > + } > > + > > + if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) { > > + return MEMTX_OK; > > + } > > + > > + if (dpa_offset > int128_get64(mr->size)) { > > + return MEMTX_OK; > > + } > > + return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs, > > + &data, size); > > +} > > + > > static void ct3d_reset(DeviceState *dev) > > { > > CXLType3Dev *ct3d = CT3(dev); > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > > index 288cc11772..eb998791d7 100644 > > --- a/include/hw/cxl/cxl_device.h > > +++ b/include/hw/cxl/cxl_device.h > > @@ -235,6 +235,7 @@ typedef struct cxl_type3_dev { > > PCIDevice parent_obj; > > > > /* Properties */ > > + AddressSpace hostmem_as; > > uint64_t size; > > HostMemoryBackend *hostmem; > > HostMemoryBackend *lsa; > > @@ -262,4 +263,9 @@ struct CXLType3Class { > > uint64_t offset); > > }; > > > > +MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data, > > + unsigned size, MemTxAttrs attrs); > > +MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data, > > + unsigned size, MemTxAttrs attrs); > > + > > #endif > > > ATB, > > Mark.
