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

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Thu, 24 Sep 2020 23:01:37 +0200
schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:

> Le 24/09/2020 à 22:15, Thomas Huth a écrit :
> > Am Thu, 24 Sep 2020 21:57:15 +0300
> > schrieb Eero Tamminen <oak@xxxxxxxxxxxxxx>:
> >   
> >> On 9/24/20 4:50 PM, Nicolas Pomarède wrote:  
> >>> Le 24/09/2020 à 15:29, Nicolas Pomarède a écrit :  
> >>>> 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.  
> >>
> >> Original ide.c code didn't do any checks for
> >> Falcon, only for IDE drives.
> >>
> >>  
> >>>> 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  
> >>
> >> Exiting there means that opaque_ide_if structure contains bogus
> >> (zero) values, and I think that to
> >> mean IDE register get/put functions not to work
> >> correctly.
> >>
> >>  
> >>   > (else there was another crash due to
> >>   > bdrv_get_geometry() doing a division by 0
> >>   > because "bs->sector_size==0")  
> >>
> >> I think that's expected when there's no disk...
> >>
> >>
> >> What is supposed to happen on Falcon if when there's no disk but
> >> IDE registers are accessed?
> >>
> >> (Original code would in that case give bus error in register
> >> get/put functions.)  
> > 
> > Sorry for the late reply, it's yet another busy week for me...
> > 
> > Original code caused a bus error in Falcon mode, too - but that was
> > clearly wrong. It happened to work since TOS 4.04 catches the bus
> > errors and ignores IDE in that case - but some hard disk drivers for
> > the Falcon crashed (I think it was Cecile IIRC) because they
> > accessed the registers without checking for bus errors.
> > The correct behavior is of course that we should always emulate an
> > IDE controller on the Falcon, which of course should not crash the
> > emulator if there is no IDE drive attached.
> > I can maybe have a look at this at the weekend, but if somebody else
> > currently has some spare time to fix the issues, you're welcome of
> > course, too.  
> 
> Hi
> 
> I tried to fix this ide.c because it was a consequence of an STE demo 
> crashing with bus error when blitter accessed the ide memory region
> and because region 0xF0xxxx was always mapped to ide registers, even
> for plain ST/STE with no ide drive (instead of mapping the whole
> region directly to bus error)
> 
> As if pulling a thread, trying to fix this bus error region showed
> more side effect than expected in the ide.c code itself ;)
> 
> I will try to give another look at the code I changed, it might not 
> require that much change to make it work in all cases. Don't have
> much time too at the moment, so if you have any idea, don't hesitate
> to change my commits if you have a working solution :)

I've now had a look, and I think I've successfully fixed the issue
reported by Uwe, and another crash that occurred when there was only a
secondary IDE drive attached, without a primary one.

BTW, the hard disk driver Cecile now also seems to work fine in Falcon
mode when no IDE drive has been specified (it used to crash with a bus
error before) :-)

 Thomas