Message ID | 20210929092843.2686234-4-laurent@vivier.eu |
---|---|
State | New |
Headers | show |
Series | [PULL,01/20] nubus: add comment indicating reference documents | expand |
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. > + > + 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. thanks -- PMM
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? >> + >> + 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
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 > > >
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h index 424309dd730d..89b0976aaa3d 100644 --- a/include/hw/nubus/nubus.h +++ b/include/hw/nubus/nubus.h @@ -43,6 +43,7 @@ struct NubusDevice { DeviceState qdev; int slot; + MemoryRegion super_slot_mem; MemoryRegion slot_mem; /* Format Block */ diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c index be0126956391..4e23df1280f9 100644 --- a/hw/nubus/nubus-device.c +++ b/hw/nubus/nubus-device.c @@ -168,26 +168,26 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) } nd->slot = nubus->current_slot++; - name = g_strdup_printf("nubus-slot-%d", nd->slot); - - if (nd->slot < NUBUS_FIRST_SLOT) { - /* Super */ - slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE; - - memory_region_init(&nd->slot_mem, OBJECT(dev), name, - NUBUS_SUPER_SLOT_SIZE); - memory_region_add_subregion(&nubus->super_slot_io, slot_offset, - &nd->slot_mem); - } else { - /* Normal */ - slot_offset = nd->slot * NUBUS_SLOT_SIZE; - - memory_region_init(&nd->slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE); - memory_region_add_subregion(&nubus->slot_io, slot_offset, - &nd->slot_mem); - } + /* Super */ + slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE; + + 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; + + name = g_strdup_printf("nubus-slot-%x", nd->slot); + memory_region_init(&nd->slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE); + memory_region_add_subregion(&nubus->slot_io, slot_offset, + &nd->slot_mem); g_free(name); + nubus_register_format_block(nd); }