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

[Kevin Wolf]

Am 20.11.2023 um 16:11 hat BALATON Zoltan geschrieben:
> 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.

With machines, you mean the QEMU board code? Where does this happen? Or
just what guests usually do?

> 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.

I guess it's a question of your philosophy if you focus on just making a
specific set of OSes work, or if you focus on trying to do what hardware
actually does. Of course, figuring out what hardware actually does
proved a bit harder than I would have hoped in this case.

> > 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.

Didn't we have config space dumps from real hardware that showed that
this isn't what it does? Otherwise, this would be simpler indeed.

Of course, the spec also mandates that the values in the first four BARs
are ignored in compatibility mode (i.e. we would have to unregister the
memory regions), but we already figured that the via controller doesn't
do this. So that's something we would only have to solve if we want pci.c
code to be actually generic, but not for via.

> 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.

For example, I see vmstate_info_pci_config -> get_pci_config_device ->
pci_update_mappings. Would live migration be enough to trigger this?

Maybe the relevant machines don't even support live migration (I haven't
checked), but it just doesn't feel robust to have inconsistent values in
data structures that are supposed to be in sync.

Kevin