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

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

Nicolas, I think IDE would work correctly if
memory.c would just check for:
Config_IsMachineFalcon() || ConfigureParams.Ide[0].bUseDevice

And all ide.c changes were reverted, as they break the other 
expectations that code has.

(Or you could add separate check function to ide.c
instead of mangling the existing internal one.)


	- Eero

On 9/24/20 9:57 PM, Eero Tamminen wrote:
> 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.)
>
>
> > Note : not related to these changes, but bdrv_get_geometry() should 
> > really check if "bs->sector_size==0" else hatari will crash also
>
> If disk image was successfully opened, I'm pretty sure it has 
> sector_size. :-)
>
>
> > 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 ?
>
>
>      - Eero