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

[BALATON Zoltan]

On Mon, 20 Nov 2023, Kevin Wolf wrote:
> Am 20.11.2023 um 14:47 hat BALATON Zoltan geschrieben:
> > 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:
> > > > > 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?
>
> I would be fine with setting 0xcc00 as the default value for BAR 4, but
> as you said yourself, we can't do that in reset because it will be
> overwritten by the PCI core code. Where else could we meaningfully do
> that? As far as I understand, we don't have any hint that the
> native/compatibility mode switch resets it on real hardware, so I'm
> hesitant to do it there (and if the guest OS doesn't even switch, it
> would never get set).

Luckily machines which need legacy mode also seem to set it explicitly on 
startup so we can set the defaults there. The check to see if something 
changed the BARs before is enough to avoid breaking it when legacy mode is 
set after native mode which does not seem to reset BARs according to how 
pegasos2 Linux behaves that sets legacy mode after firmware set native and 
proframmed BARs but the keep using BAR addresses. The AmigaOne I think 
just uses the default values with setting legacy mode doing nothing as 
that's the default but we can detect this as setting legacy mode with BARs 
unset so that's a good place to set default values which is what my patch 
did and I added a lot of comments trying to explain this.

> As for BAR 0-3, didn't we conclude that the via device still accepts I/O
> to the configured addresses even though they read as zeros? Having
> inconsistent config space and PCIIORegion seems like a bad idea, the
> next call to pci_update_mappings() would break it.

I don't quite get this but then we could also just leave BARs alone and it 
would still work. It probably does not matter what it reads back when the 
device is in legacy mode. What would call pci_update_mappings() if device 
is in legacy and if something switches it to native it will very likely 
also program BARs. I can't imagine what would want to turn on native mode 
without trying to use a PCI driver and program BARs.

Regards,
BALATON Zoltan