Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Mark Cave-Ayland]

On 20/11/2023 13:42, Kevin Wolf wrote:

> Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben:
> > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
> > > On 19/11/2023 21:43, BALATON Zoltan wrote:
> > > > On Thu, 16 Nov 2023, Mark Cave-Ayland wrote:
> > > > > This series adds a simple implementation of legacy/native mode
> > > > > switching for PCI
> > > > > IDE controllers and updates the via-ide device to use it.
> > > > >
> > > > > The approach I take here is to add a new pci_ide_update_mode()
> > > > > function which handles
> > > > > management of the PCI BARs and legacy IDE ioports for each mode
> > > > > to avoid exposing
> > > > > details of the internal logic to individual PCI IDE controllers.
> > > > >
> > > > > As noted in [1] this is extracted from a local WIP branch I have
> > > > > which contains
> > > > > further work in this area. However for the moment I've kept it simple (and
> > > > > restricted it to the via-ide device) which is good enough for Zoltan's PPC
> > > > > images whilst paving the way for future improvements after 8.2.
> > > > >
> > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > >
> > > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
> > > > >
> > > > > v3:
> > > > > - Rebase onto master
> > > > > - Move ide_portio_list[] and ide_portio_list2[] to IDE core to
> > > > > prevent duplication in
> > > > >   hw/ide/pci.c
> > > > > - Don't zero BARs when switching from native mode to legacy
> > > > > mode, instead always force
> > > > >   them to read zero as suggested in the PCI IDE specification
> > > > > (note: this also appears
> > > > >   to fix the fuloong2e machine booting from IDE)
> > > >
> > > > Not sure you're getting this, see also:
> > > > https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html
> > > > but this seems to break latest version of the AmigaOS driver for
> > > > some reason. I assume this is the BAR zeroing that causes this as it
> > > > works with v2 series and nothing else changed in v3 that could cause
> > > > this. Testing was done by Rene Engel, cc'd so maybe he can add more
> > > > info. It seems to work with my patch that sets BARs to legacy values
> > > > and with v2 that sets them to 0 but not with v3 which should also
> > > > read 0 but maybe something is off here.
> > >
> > > I've been AFK for a few days, so just starting to catch up on various
> > > bits and pieces.
> >
> > OK just wasn't sure if you saw my emails at all as it happened before that
> > some spam filters disliked my mail server and put messages in the spam
> > folder.
> >
> > > The only difference I can think of regarding the BAR zeroing is that the
> > > BMDMA BAR is zeroed here. Does the following diff fix things?
> >
> > This helps, with this the latest driver does not crash but still reads BAR4
> > as 0 instead of 0xcc00 so UDMA won't work but at least it boots.
>
> And disabling only the first four BARs is actually what the spec says,
> too. So I'll make this change to the queued patches.

That was definitely something that I thought about: what should happen to BARs 
outside of the ones mentioned in the PCI IDE controller specification? It seems 
reasonable to me just to consider BARS 0-3 for zeroing here.

> If I understand correctly, UDMA didn't work before this series either,
> so it's a separate goal and doing it in its own patch is best anyway.
>
> As we don't seem to have a good place to set a default, maybe just
> overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in
> compatibility mode is enough?

It's difficult to know whether switching to legacy mode on the via-ide device resets 
BAR4 to its default value, or whether it is simply left unaltered. For 8.2 I don't 
mind too much as long as the logic is separate from the BAR zeroing logic (which will 
eventually be lifted up into hw/ide/pci.c).


ATB,

Mark.