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

[BALATON Zoltan]

On Mon, 20 Nov 2023, BALATON Zoltan wrote:
> On Mon, 20 Nov 2023, 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.
> >
> > 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.
>
> UDMA works with my original series, did not work with earlier versions of 
> this alternative from Mark but could be fixed up on top unless Mark can send 
> a v4 now.
>
> > 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?
>
> I could give that a try and see if that helps but all this via_ide_cfg_read() 
> seems like an unnecessary complication to me. Why can't we just set the BARs 
> (o for BAR1-3 and default for BAR4) then we don't need to override config 
> read?

Seems to work with this:

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 47223b1268..214ebe48a1 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -164,14 +164,17 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t 
addr, int len)
     uint8_t mode = pd->config[PCI_CLASS_PROG];

     if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
-                                              PCI_BASE_ADDRESS_0, 24)) {
+                                              PCI_BASE_ADDRESS_0, 16)) {
         /* BARs always read back zero in legacy mode */
         for (int i = addr; i < addr + len; i++) {
-            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
+            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
                 val &= ~(0xffULL << ((i - addr) << 3));
             }
         }
     }
+    if (addr == PCI_BASE_ADDRESS_4) {
+        val = 0xcc01;
+    }

     return val;
 }

But I still think all the above mess could be avoided and just set the BAR 
value to 0 for BAR0-3 and to 0xcc01 for BAR4 instead should be enough and 
would not mess up the device model unnecessarily.

Regards,
BALATON Zoltan