Re: [PULL 03/27] hw/cxl/cxl-mailbox-utils: Add support for Media operations discovery commands cxl r3.2 (8.2.10.9.5.3)

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

On Thu, 10 Jul 2025 14:26:07 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Wed, 14 May 2025 at 12:50, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Vinayak Holikatti <vinayak.kh@samsung.com>
> >
> > CXL spec 3.2 section 8.2.10.9.5.3 describes media operations commands.
> > CXL devices supports media operations discovery command.
> >
> > Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Message-Id: <20250305092501.191929-4-Jonathan.Cameron@huawei.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/cxl/cxl-mailbox-utils.c | 125 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 125 insertions(+)  
> 
> 
> 
> > +static CXLRetCode media_operations_discovery(uint8_t *payload_in,
> > +                                             size_t len_in,
> > +                                             uint8_t *payload_out,
> > +                                             size_t *len_out)
> > +{
> > +    struct {
> > +        uint8_t media_operation_class;
> > +        uint8_t media_operation_subclass;
> > +        uint8_t rsvd[2];
> > +        uint32_t dpa_range_count;
> > +        struct {
> > +            uint16_t start_index;
> > +            uint16_t num_ops;
> > +        } discovery_osa;
> > +    } QEMU_PACKED *media_op_in_disc_pl = (void *)payload_in;
> > +    struct media_op_discovery_out_pl *media_out_pl =
> > +        (struct media_op_discovery_out_pl *)payload_out;
> > +    int num_ops, start_index, i;
> > +    int count = 0;
> > +
> > +    if (len_in < sizeof(*media_op_in_disc_pl)) {
> > +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> > +    }
> > +
> > +    num_ops = media_op_in_disc_pl->discovery_osa.num_ops;
> > +    start_index = media_op_in_disc_pl->discovery_osa.start_index;
> > +
> > +    /*
> > +     * As per spec CXL r3.2 8.2.10.9.5.3 dpa_range_count should be zero and
> > +     * start index should not exceed the total number of entries for discovery
> > +     * sub class command.
> > +     */
> > +    if (media_op_in_disc_pl->dpa_range_count ||
> > +        start_index > ARRAY_SIZE(media_op_matrix)) {  
> 
> Coverity thinks this bounds check is wrong (CID 1610091):
> we allow start_index equal to the ARRAY_SIZE(media_op_matrix),
> which means that in the loop below we will index off the
> end of the array.
> 
> Don't we also need to be checking (start_index + num_ops)
> against the array size bounds, not just start_index ?

Agreed. I think this should be
start_index + num_ops > ARRAY_SIZE(media_op_matrix);


> 
> > +        return CXL_MBOX_INVALID_INPUT;
> > +    }
> > +
> > +    media_out_pl->dpa_range_granularity = CXL_CACHE_LINE_SIZE;
> > +    media_out_pl->total_supported_operations =
> > +                                     ARRAY_SIZE(media_op_matrix);
> > +    if (num_ops > 0) {
> > +        for (i = start_index; i < start_index + num_ops; i++) {
> > +            media_out_pl->entry[count].media_op_class =
> > +                    media_op_matrix[i].media_op_class;
> > +            media_out_pl->entry[count].media_op_subclass =
> > +                        media_op_matrix[i].media_op_subclass;
> > +            count++;
> > +            if (count == num_ops) {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    media_out_pl->num_of_supported_operations = count;
> > +    *len_out = sizeof(*media_out_pl) + count * sizeof(*media_out_pl->entry);
> > +    return CXL_MBOX_SUCCESS;
> > +}  
> 
> The functions in this patch also look like they aren't correctly
> handling the "guest and host endianness differ" case.

The fields in CXL mailbox commands are all defined as little endian.
There is a problem with littlendian vs guest.  That's true of a lot
of the CXL code and something that needs a thorough audit at some point.
I'll leave this for now.  Given how slow emulating an big endian system
is it's rather a marathon effort to test any such fixes.

I'll sort out a fix for the issue in previous patch as well.

Thanks,

Jonathan


> 
> thanks
> -- PMM