Re: [PULL 53/60] hw/cxl: Standardize all references on CXL r3.1 and minor updates

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 8 Mar 2024 14:38:55 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Fri, 8 Mar 2024 at 14:34, Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Fri, 8 Mar 2024 13:47:47 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
> > > Is there a way we could write this that would catch this error?
> > > I'm thinking maybe something like
> > >
> > > #define CXL_CREATE_DVSEC(CXL, DEVTYPE, TYPE, DATA) do { \
> > >      assert(sizeof(*DATA) == TYPE##_LENGTH); \
> > >      cxl_component_create_dvsec(CXL, DEVTYPE, TYPE##_LENGTH, \
> > >                                 TYPE, TYPE##_REVID, (uint8_t*)DATA); \
> > >      } while (0)  
> >
> > We should be able to use the length definitions in the original assert.
> > I'm not sure why that wasn't done before.  I think there were some cases
> > where we supported multiple versions and so the length can be shorter
> > than the structure defintion but that doesn't matter on this one.
> >
> > So I think minimal fix is u16 of padding and update the assert.
> > Can circle back to tidy up the multiple places the value is defined.
> > Any mismatch in which the wrong length define is used should be easy
> > enough to spot so not sure we need the macro you suggest.  
> 
> Well, I mean, you didn't in fact spot the mismatch between
> the struct type you were passing and the length value you
> were using. That's why I think it would be helpful to
> assert() that the size of the struct really does match
> the length value you're passing in. At the moment the
> code completely throws away the type information the compiler
> has by casting the pointer to the struct to a uint8_t*.

True, but the original assert at the structure definition would have
fired if I'd actually used the define rather than a number :(

There is definitely more to do here - but fix wants to be on the light
side of all the options.

cxl_component_create_dvsec() is an odd function in general as it has
more code that varies depending on cxl_dev_type than is shared.

So it might just make sense to split it up and provide some more
trivial functions for the header writing. This is a case of code
that has evolved and ended up as a far from ideal solution.

We only carry the DVSECHeader in the structures so that the sizes can
be read against the spec. It makes the code more complex though
so maybe should consider dropping it and making the asserts next
to the structure definitions more complex.

The asserts in existing function can go (checking it fits etc is done
by pcie_add_capability()).

If not need something more like
//awkward naming is because the second cxl needs to be there to match spec.
void cxl_create_pcie_cxl_device_dvsec(CXLComponentState *cxl,
				      CXLDVSECDevice *dvsec)
{
    PCIDevice *pdev = cxl->pdev;
    uint16_t offset = cxl->dvsec_offset;
    uint16_t length = sizeof(*dvsec);
    uint8_t *wmask = pdev->wmask;

    ///next block can probably be a helper or done in a simpler way.
    /// A lot of what we have here is just to let us reuse this first call.
    pcie_add_capability(pdev, PCI_EXT_CAP_ID_DVSEC, 1, offset, length);

    ///These could be done by writing into dvsec, and memcpy ing more
    ///but the offset will be even stranger if we do that.
    pci_set_long(pdev->config + offset + PCIE_DVSEC_HEADER1_OFFSET,
                 (length << 20) | (rev << 16) | CXL_VENDOR_ID);
    pci_set_word(pdev->config + offset + PCIE_DEVSEC_ID_OFFSET,
                 PCIE_CXL_DEVICE_DEVSEC);

    memcpy(pdev->config + offset + sizeof(DVSEC_HEADER),
           (uint8_t *)dvsec + sizeof(DVSECHeader),
           length - sizeof(DVSECHEAEDR));

// all the wmask stuff for this structure.
}


So I'm aiming for more drastic surgery than you were suggesting but
not in the fix!

Jonathan

> 
> thanks
> -- PMM