On Thu, 10 Jul 2025 14:26:07 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Wed, 14 May 2025 at 12:50, Michael S. Tsirkin <m...@redhat.com> wrote:
> >
> > From: Vinayak Holikatti <vinayak...@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...@samsung.com>
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > Message-Id: <20250305092501.191929-4-jonathan.came...@huawei.com>
> > Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <m...@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
