Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Sat, 7 Mar 2020, Mark Cave-Ayland wrote:
> On 06/03/2020 22:59, BALATON Zoltan wrote:
> > > > > I've done a quick grep of the source tree and AFAICT the only IDE controller 
> > > > > that
> > > > > tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this 
> > > > > should be
> > > > > fairly easy. In short:
> > > > >
> > > > > 1) Add qemu_irq legacy_irqs[2] into PCIIDEState
> > > > >
> > > > > (You could argue that it should belong in a separate VIAIDEState, however quite 
> > > > > a
> > > > > few
> > > > > of the BMDMA controllers in QEMU don't have their own device state and just use
> > > > > PCIIDEState. And whilst via-ide is the only one that currently needs support for
> > > > > legacy IRQs, I think it's good to put it there in case other controllers need 
> > > > > it in
> > > > > future)
> > > > >
> > > > > 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with 
> > > > > a
> > > > > name of "legacy-irq" to it
> > > >
> > > > I don't like this. This adds two via-ide specific data to to common PCI IDE code
> > > > where it does not belong and subclassing it just for this also seems to be more
> > > > changes than really needed. Reusing the existing CMD646 field and generalising 
> > > > it to
> > > > allow implementation specific feature flags seems much less intrusive and not 
> > > > less
> > > > clean than your proposal.
> > > It's not VIA-specific though: the ISA legacy and PCI buses have different 
> > > electrical
> > > characteristics and so by definition their signals must be driven by separate
> > > physical pins. Have a look at the CMD646 datasheet for example, and you will 
> > > see that
> > > separate pins exist for legacy and PCI native IRQs.
> >
> > For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy mode and 
> > thus
> > those pins are not modelled so not needed now.
>
> Correct. It seems to me that having it there in PCIIDEState provides a 
> convenient
> place for all controllers that support legacy mode to store the IRQ in case 
> someone
> would like to add legacy support to the device at a later date.

Why introduce it now and not only when someone would like to add legacy 
mode? I guess nobody needs legacy mode so we can avoid complexity 
modelling that unused detail now and only add it when needed.

> If you really object to this then as I mentioned above, you'll need to add a
> VIAIDEState or similar and have the legacy IRQs there for the qdev gpio 
> connector.
>
> > For via-ide we only use ISA interrupts
> > because even if we don't model legacy mode, boards expect ISA interrupts also in
> > native mode maybe because this controller is not a separate PCI device only 
> > found
> > embedded in southbridge/superio chips where they connect to the also embedded 
> > ISA
> > PICs so even in native mode it should raise one of the ISA IRQs.
>
> I certainly agree with your analysis here, and that is borne out by the fact 
> that you
> are able to boot your OSs using your current hack with no PCI interrupts.
>
> > My patch accesses
> > ISA irqs with isa_get_irq() so no gpios and legacy irqs in PCIIDEState is 
> > neeeded and
> > I don't see the need to introduce this complexity here.
>
> This isn't complexity though, it is just normal qdev usage. In fact if you look 
> at

Then qdev is probably overly complex for simple things.

> isa_get_irq() it contains this large comment above the function itself:
>
> /*
> * isa_get_irq() returns the corresponding qemu_irq entry for the i8259.
> *
> * This function is only for special cases such as the 'ferr', and
> * temporary use for normal devices until they are converted to qdev.
> */
>
> It was a temporary hack to allow old devices to access the 8259 IRQs before 
> devices
> were converted to qdev. And via-ide is a qdev device, so that's how you need to 
> wire
> it up.
>
> > Also newer PCI ATA and SATA
> > controllers such as sii3112 do not have a legacy mode so I'd keep things 
> > related to
> > that out of common PCI IDE code and model it instead in the controllers that 
> > have
> > this as this does not seem to belong to PCI IDE.
>
> Like I said above: if you really don't want to put the legacy IRQs there then 
> you
> need to create a VIAIDEState for the qdev gpio connector.

I still think what you propose is more complex than my patch and does not 
achieve any cleaner model. If you object to using isa_get_irq here I can 
go the piix way and embed the PIC array as well (or set a pointer to that 
with a property somehow) so I can use isa_irq[n] rather than adding 
non-existing pins for this. I won't do anything though until IDE and QEMU 
maintainers indicate what their preference is.

> > > > > 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing 
> > > > > pci_create_simple()
> > > > > to pci_create() because the device shouldn't be realised immediately
> > > > >
> > > > > 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
> > > > > legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise 
> > > > > the
> > > > > device
> > > >
> > > > How do I connect gpios to 8259 interrupts? That seems to be internal state of 
> > > > 8259
> > > > that I'm not sure how to access cleanly from code instantiating it. Is this 
> > > > better
> > > > than my patch? It seems it achieves the same via-ide specific behaviour just in 
> > > > a
> > > > more complicated way and would still need the feature bit to know when to use
> > > > legacy_irq[1].
> > >
> > > We know from the PCI specification that the existing code for via-ide is 
> > > incorrect,
> > > and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE 
> > > in
> > > this way then both of these strongly suggest that current via-ide 
> > > implementation is
> > > wrong. Rather than add a hack on top of a hack then the simplest solution is to
> > > physically wire the IRQ pin using qdev at the board level, as is done on real
> > > hardware.
> >
> > But it's not done that way on real hardware and via-ide is not a PCI device but 
> > all
> > this is internal to the VT8231 chip, the PICs, via-ide and a lot of other things
> > which are modelled in QEMU by reusing existing components but I think we don't 
> > want
> > to model the internal of the chip down to the smallest detail.
>
> That's not what I'm suggesting though.

I think you did not like emulating the interrupt selection in via-ide that 
decides which irq to raise (which is really just one if() ) and would like 
to push it out to board code instead connecting it via gpios. But that's 
also not quite correct and would not even get rid of the feature bit you 
also disliked. In fact we don't actually have a qdev model for the 
VT82C686B where all this probably belongs but I don't plan to make a 
detailed qdevified model for that now (could be done later in a clean up 
patch maybe when I'll clean up pegasos2 patches for inclusion) and since 
via-ide is considered part of that southbridge (and reused in similar 
VT8231 model) we can include the interrupt controller emulation (this one 
if() ) as well for now and not worry about PCI specs that don't really 
apply to a "chip" that's not a standalone PCI IDE controller but part of a 
bigger southbridge chip. It's just reuses PCI IDE emulation from QEMU 
where appropriate.

> > In via-ide's case the
> > PCI_INTERRUPT_LINE is probably not used by the IDE controller function but is 
> > used by
> > some other function of the southbridge chip that implements interrupt controller
>
> What evidence do you have for this?

The datasheet (of the IDE function) says that PCI_INTERRUPT_LINE selects 
the ISA interrupt that's raised in native mode for both channels and 
guests work if we emulate that. Since via-ide is part of the southbridge 
that also includes the PICs they are somehow connected inside and 
something selects the interrupt line within the chip but I don't know how 
exactly that's implemented in hardware. If not the ide part directly as 
you propose then some other interrupt controller part has to do this and 
we need to do the same somewhere in our model. Since we don't emulate the 
southbridge in more detail I've just put it in via-ide which seems to be a 
good place as it's only used in these southbridges and this could be 
changed later if via-ide were used elsewhere. No need to be more complex 
now than that I think.

> > but
> > we don't have a separate model of that in QEMU so we can just emulate this 
> > function
> > within via-ide which I think is OK as this IDE controller is part of these
> > southbridges and not used anywhere else. This partly mixes IDE controller 
> > function
> > and interrupt controller function but probably OK until we want to model this
> > southbridge in more detail which could be done in later clean up patches. The 
> > piix
> > model seems to do the same embedding a 8259 which even less belongs to an IDE
> > controller and acceses irqs array directly.
> >
> > > Looking at the code it seems that i8259_init() returns the PIC IRQ array so it 
> > > should
> > > just be a case of returning the nth entry and using that with
> > > qdev_init_gpio_out_named().
> > >
> > > > > 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just 
> > > > > do
> > > > > qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] 
> > > > > but it
> > > > > seems that both drives on MIPS and Pegasos both use IRQ 14).
> > > >
> > > > According to the 8231 datasheet in legacy mode (and on pegasos2's half-native 
> > > > mode)
> > > > the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% 
> > > > native
> > > > mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 
> > > > 686B
> > > > datasheet does not detail this but I believe it works the same. Since we 
> > > > currently
> > > > fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU 
> > > > PCI
> > > > reset interactions using legacy_irq[0] might work but is not correct, the 
> > > > current way
> > > > using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion.
> > >
> > > This is not correct though: please re-read my previous email which quotes from 
> > > the
> > > PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon 
> > > the
> > > device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be
> > > placing a value other than 14 there, but if it does then that suggests the 
> > > board IRQs
> > > are physically wired differently which again should be handled at board level 
> > > by qdev.
> >
> > Correct or not or follows the spec or not this is how it works on real hardware 
> > and
> > to get guests running we need to emulate this.
>
> This may be a separate issue. As you mentioned in your email most guest OSs on
> Pegasos blindly ignore the PCI_INTERRUPT_LINE register since they know that the 
> IRQs
> are hardwired. However you may also be getting caught out by the bug that the 
> current
> implementation will try and use the PCI_INTERRUPT_LINE IRQ for routing which is
> incorrect.

It's not incorrect. It follows what the datasheet says at least for 
VT8231, the 686B does not detail this but the Linux driver also works on 
fulong2e with this so I think it's correct for that as well.

> Both the 8231 and 636B datasheets mark PCI_INTERRUPT_LINE as read-only with a 
> default
> value of 0xe, so I believe that once you've done the qdev gpio part, keeping 
> this
> part of your patchset as a separate patch should fix this.

I don't get this. My patch emulates that already, what do you want to be 
changed here?

> > Again, this controller is embedded in
> > the 868B and 8231 southbridge chips so they may not completely confirm to PCI 
> > spec
> > but their own datasheets. We can argue in how much detail do we want to model 
> > the
> > internals of these chips (which we don't know for sure) but I think this patch 
> > is
> > good enough for now and could be refined later or we likely won't be able to 
> > finish
> > this before the freeze.
> >
> > Another way to look at it is that this patch gets some guests running and does 
> > not
> > break anything as far as I know so is there anything in it that's unacceptable 
> > now
> > and needs to be changed for it to be merged? Unless you can propose a way to 
> > achieve
> > the same in a simpler way now I think we could go with this and you can submit 
> > follow
> > up patches to clean it up as you like later.
>
> I don't know what else to say except other than what I have already said: both 
> the
> current via-ide IRQ implementation and the changes in your patchset are 
> incorrect.

Why do you think so? You base this on PCI docs but those don't necessarily 
apply to via-ide which is not a standalone PCI device but embedded in 
integrated southbridges whose datasheets say what I've implemented and 
guests are happy with that. So I don't think it's incorrect maybe just not 
to your taste.

> I've given you the outline of what you need to do to switch the device over to 
> qdev
> which agrees with the datasheet, the PCI specification, comments within the 
> QEMU code
> itself and the results of your experiments. The proposed change is also simple 
> to
> implement (less than an hour's work) using standard APIs.

Then please do that and submit alternative patches. I don't understand all 
of your proposals so it's easier if you can do it in less than an hour. It 
would probably take me more time. I'll test them with the clients if you 
provide patches or I've pushed my working version now to

https://osdn.net/projects/qmiga/scm/git/qemu/tree/pegasos2/

so you can try as well.

> At this time I don't see any point in continuing to repeat myself over and over
> again: if you can make the qdev change and post an updated patchset then I'm 
> happy to
> review and continue this discussion, but otherwise I don't see any purpose in
> continuing this thread.

I also think we're not getting anywhere so I hope John and Peter can give 
some advice here what is needed to get these patches merged. I've run out 
of time for now so I won't have time to make extensive changes to these 
patches and can only make reasonable changes or test what you provide.

You seem to want to force me again to qdevify everything now which I don't 
have time for. You did the same to my OpenBIOS patches too where you 
wanted me to clean up PCI code and implement handling of multiple PCI 
buses when I just wanted to include a simple workaround to get MorphOS 
running until the bigger clean up is eventually done. Even though some 
years have passed you did still not come around to do the OpenBIOS PCI 
clean up but also not accepted my workaround in the meantime so I need to 
use patched OpenBIOS for MorphOS ever since. I think we'd end up in the 
same situation with these patches. (It would be OK if OpenBIOS were 
otherwise very clean but it's full of other workarounds like patching 
device tree and installing QEMU VGA driver to any VGA card it sees and 
others that are much worse than what I've proposed and you don't seem to 
mind those which come from you just those that someone else propose.) 
Don't get this wrong, we don't have any personal problem with each other, 
at least I don't have anything against you but your habit of forcing 
people to do much more additional work just to satisfy your pedantry is 
not something I have time to comply with. If you propose changes that make 
the patches simpler or in some way better I'd do that but otherwise I'll 
only do what QEMU maintainers also think is necessary.

Regards,
BALATON Zoltan