Message ID | 20220318150635.24600-33-Jonathan.Cameron@huawei.com |
---|---|
State | Superseded |
Headers | show |
Series | CXl 2.0 emulation Support | expand |
On 18/03/2022 15:06, Jonathan Cameron wrote: > From: Jonathan Cameron <jonathan.cameron@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-f4bug@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.Cameron@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)); } } > + > + 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. > 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.
On Sat, 19 Mar 2022 08:53:40 +0000 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 18/03/2022 15:06, Jonathan Cameron wrote: > > > From: Jonathan Cameron <jonathan.cameron@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-f4bug@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.Cameron@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.
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); + + if (ds->id) { + name = g_strdup_printf("cxl-type3-dpa-space-%s", ds->id); + } else { + name = g_strdup("cxl-type3-dpa-space"); + } + address_space_init(&ct3d->hostmem_as, mr, name); 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