Re: [hatari-devel] IDE IO register range access commit

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 24/09/2020 à 15:29, Nicolas Pomarède a écrit :
> Le 24/09/2020 à 15:10, Eero Tamminen a écrit :
> > Hi,
> >
> > On 9/24/20 3:26 PM, Uwe Seimet wrote:
> > > Looks as if there is still an issue with the IDE handling. I don't know
> > > whether this is a new issue or has always been there > In Falcon030 
> > > mode Hatari crashes when no IDE images are assigned:
> > >
> > > Thread 1 "hatari" received signal SIGSEGV, Segmentation fault.
> > > 0x00005555558f7258 in Ide_Mem_bget ()
> > > (gdb) bt
> > > #0  0x00005555558f7258 in Ide_Mem_bget ()
> > > #1  0x00005555559c5a99 in op_4a39_0_ff ()
> > > #2  0x000055555597d092 in m68k_run_2_020 ()
> > > #3  0x00005555559758c2 in m68k_go ()
> > > #4  0x00005555558ba703 in main ()
> > >
> > > In TT mode everything is fine, i.e. the IDE interface is not present.
> >
> > I can reproduce this just with:
> >      $ hatari --machine falcon --dsp none
> >
> > ---------------------------------------------
> > Thread 1 "hatari" received signal SIGSEGV, Segmentation fault.
> > Ide_Mem_bget (addr=15728669) at src/ide.c:107
> > 107            retval = ide_ioport_read(opaque_ide_if, ideport);
> > (gdb) print opaque_ide_if
> > $2 = (struct IDEState *) 0x0
> > ---------------------------------------------
> >
> > It's due to Nicolas' Ide_MmioIsAvailable()
> > function change claiming that IDE is available
> > (on Falcon) even when there are no IDE devices
> > present.
> >
> > That (previously internal) function is used by IDE
> > data access functions which chrash when there's no
> > actual IDE device...
>
> Hi
>
> I'm looking at it, but IdeInit() seems strange to me ; it doesn't create 
> the "opaque_ide_if" struct if machine is falcon, only if 
> Ide[0].bUseDevice (else it returns immediately)
>
> But Ide should be enabled on Falcon even if there's no disk on Ide[0].
> It seems that sometimes IDE depends on Ide[0].bUseDevice and sometimes 
> it depends on machine==falcon.
> I think it should check both in every places.

I pushed a patch to fix this ; I'm not too familiar with ide.c code, but 
I think the following needed to be changed/fixed :

 - In IdeInit, we should not return if machine==falcon, else 
opaque_ide_if is not malloc'ed and hatari will crash later in Ide_Mem_bget

 - Once we don't return in IdeInit() in case of falcon, ide_init2 
should not assume that hd_table[n] is initialized, because it can be 
skipped in the "for" loop in IdeInit if Ide[i].bUseDevice == false. So, 
I added a check for Ide[i].bUseDevice in ide_init2 too (else there was 
another crash due to bdrv_get_geometry() doing a division by 0 because 
"bs->sector_size==0")

Note : not related to these changes, but bdrv_get_geometry() should 
really check if "bs->sector_size==0" else hatari will crash also

Hopefully, this should now work as before for IDE for Falcon/TT/..., at 
least it doesn't crash in falcon mode when there's no ide disk inserted.

Can someone test with images in ide disk 0 or 1 if everything is OK ?

Nicolas