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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 4 Apr 2022 12:19:07 -0700
Tong Zhang <ztong0001@gmail.com> wrote:

> > 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

...

> > +
> > +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)?

Good point.  Silly mistake.  I'll fix for v10.

> 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.

I think we are fine without these two because in cxl-components-utils we
restrict the accesses to aligned only.

Jonathan


> 
> > +    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);
> > +    }
> > +}