Re: [PULL 03/20] nubus-device: expose separate super slot memory region

[BALATON Zoltan]

On Mon, 4 Oct 2021, Laurent Vivier wrote:
> Le 02/10/2021 à 12:33, Peter Maydell a écrit :
> > On Wed, 29 Sept 2021 at 10:49, Laurent Vivier <laurent@vivier.eu> wrote:
> > > From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > >
> > > According to "Designing Cards and Drivers for the Macintosh Family" each 
> > > physical
> > > nubus slot can access 2 separate address ranges: a super slot memory region 
> > > which
> > > is 256MB and a standard slot memory region which is 16MB.
> > >
> > > Currently a Nubus device uses the physical slot number to determine whether it 
> > > is
> > > using a standard slot memory region or a super slot memory region rather than
> > > exposing both memory regions for use as required.
> >
> >
> > > +    /* Super */
> > > +    slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
> >
> > Hi; Coverity thinks this multiply might overflow, because
> > we're calculating a hw_addr (64-bits) but the multiply is only
> > done at 32-bits. Adding an explicit cast or using 'ULL' in the
> > constant #define rather than just 'U' would fix this.
> > This is CID 1464070.
>
> I'm wondering if adding "assert(nd->slot < NUBUS_SUPER_SLOT_NB)" would help 
> coverity to avoid the
> error without using 64bit arithmetic?

Using ULL in constant is simpler and better, assert is an unnecessary 
condition evaluation in cases where it can't happen (that's not a 
performance problem here but could be in some frequently called code).

Regards,
BALATON Zoltan

> > > +
> > > +    name = g_strdup_printf("nubus-super-slot-%x", nd->slot);
> > > +    memory_region_init(&nd->super_slot_mem, OBJECT(dev), name,
> > > +                       NUBUS_SUPER_SLOT_SIZE);
> > > +    memory_region_add_subregion(&nubus->super_slot_io, slot_offset,
> > > +                                &nd->super_slot_mem);
> > > +    g_free(name);
> > > +
> > > +    /* Normal */
> > > +    slot_offset = nd->slot * NUBUS_SLOT_SIZE;
> >
> > Same with this one.
>
> assert(nb->slot < NUBUS_SLOT_NB)
>
> > thanks
> > -- PMM
>
> Laurent