Re: [PATCH v9 18/45] hw/cxl/device: Implement MMIO HDM decoding (8.2.5.12)

[Tong Zhang]

> On Apr 4, 2022, at 8:14 AM, Jonathan Cameron via <qemu-devel@nongnu.org> 
> wrote:
> 
> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> A device's volatile and persistent memory are known Host Defined Memory
> (HDM) regions. The mechanism by which the device is programmed to claim
> the addresses associated with those regions is through dedicated logic
> known as the HDM decoder. In order to allow the OS to properly program
> the HDMs, the HDM decoders must be modeled.
> 
> There are two ways the HDM decoders can be implemented, the legacy
> mechanism is through the PCIe DVSEC programming from CXL 1.1 (8.1.3.8),
> and MMIO is found in 8.2.5.12 of the spec. For now, 8.1.3.8 is not
> implemented.
> 
> Much of CXL device logic is implemented in cxl-utils. The HDM decoder
> however is implemented directly by the device implementation.
> Whilst the implementation currently does no validity checks on the
> encoder set up, future work will add sanity checking specific to
> the type of cxl component.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> hw/mem/cxl_type3.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 329a6ea2a9..5c93fbbd9b 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -50,6 +50,48 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>                                GPF_DEVICE_DVSEC_REVID, dvsec);
> }
> 
> +static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
> +{
> +    ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
> +    uint32_t *cache_mem = cregs->cache_mem_registers;
> +
> +    assert(which == 0);
> +
> +    /* TODO: Sanity checks that the decoder is possible */
> +    ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> +    ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +
> +    ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> +}
> +
> +static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    CXLComponentState *cxl_cstate = opaque;
> +    ComponentRegisters *cregs = &cxl_cstate->crb;
> +    CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
> +    uint32_t *cache_mem = cregs->cache_mem_registers;
> +    bool should_commit = false;
> +    int which_hdm = -1;
> +
> +    assert(size == 4);
> +    g_assert(offset <= CXL2_COMPONENT_CM_REGION_SIZE);
> +

Looks like this will allow offset == CXL2_COMPONENT_CM_REGION_SIZE to pass the 
check, and cause a buffer overrun.
Shouldn’t this be g_assert(offset< CXL2_COMPONENT_CM_REGION_SIZE)?
We also need to make sure (offset + 4<= CXL2_COMPONENT_CM_REGION_SIZE).
Or maybe we just need offset +4 <= CXL2_COMPONENT_CM_REGION_SIZE here, if 
offset < CXL2_COMPONENT_CM_REGION_SIZE is already checked somewhere else.

> +    switch (offset) {
> +    case A_CXL_HDM_DECODER0_CTRL:
> +        should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        which_hdm = 0;
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    stl_le_p((uint8_t *)cache_mem + offset, value);
> +    if (should_commit) {
> +        hdm_decoder_commit(ct3d, which_hdm);
> +    }
> +}
> +
> static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> {
>     MemoryRegion *mr;
> @@ -93,6 +135,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>     ct3d->cxl_cstate.pdev = pci_dev;
>     build_dvsecs(ct3d);
> 
> +    regs->special_ops = g_new0(MemoryRegionOps, 1);
> +    regs->special_ops->write = ct3d_reg_write;
> +
>     cxl_component_register_block_init(OBJECT(pci_dev), cxl_cstate,
>                                       TYPE_CXL_TYPE3);
> 
> @@ -107,6 +152,15 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>                      &ct3d->cxl_dstate.device_registers);
> }
> 
> +static void ct3_exit(PCIDevice *pci_dev)
> +{
> +    CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
> +    CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
> +    ComponentRegisters *regs = &cxl_cstate->crb;
> +
> +    g_free(regs->special_ops);
> +}
> +
> static void ct3d_reset(DeviceState *dev)
> {
>     CXLType3Dev *ct3d = CXL_TYPE3(dev);
> @@ -128,6 +182,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
>     PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
> 
>     pc->realize = ct3_realize;
> +    pc->exit = ct3_exit;
>     pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
>     pc->vendor_id = PCI_VENDOR_ID_INTEL;
>     pc->device_id = 0xd93; /* LVF for now */
> -- 
> 2.32.0
> 
> 
>