Philippe,

Could this be merged for 8.2 as it fixes USB on the amigaone machine?
This would be useful as usb-storage is the simplest way to share data
with the host with these machines.

This is a slight change from v2 adding more comments and improving
commit messages and clean things a bit but otherwise should be the
same as previous versions. Even v1 worked the same as this one and v2,
the additional check to avoid stuck bits is just paranoia, it does not
happen in practice as IRQ mappings are quite static, they are set once
at boot and don't change afterwards.

The rest is just some explanation on how we got here but can be
skipped if not interested in history.

This is going back to my original implementation of this IRQ routing
that I submitted already for 8.0 in the beginning of this year
(https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/)
but Mark had concerns about that because he wanted to use PCI
interrupt routing instead. I've already told back then that won't work
but I could not convince reviewers about that. Now with the amigaone
machine this can also be seen and that's why this series is needed now.

The routing code in PCIBus cannot be used as that only considers the 4
PCI interrupts but there are about 12 interrupt sources in this chip
that need to be routed to ISA IRQs (the embedded chip functions and
the 4 PCI interrupts that are coming in through 4 pins of the chip).
Also the chip does not own the PCIBus, that's part of the north bridge
so it should not change the PCI interrupt routing of a bus it does not
own. (Piix calling pci_bus_irqs() I think is also wrong because the
PCI bus in that machine is also owned by the north bridge and piix
should not take over routing of IRQs on a bus it's connected to.)
Another concern from Mark was that this makes chip functions specific
to the chip and they cannot be used as individual PCI devices. Well,
yes, they are chip functions, are not user creatable and don't exist
as individual devices so it does not make sense to use them without
the actual VIA chip they are a function of so that's not a real
concern. These functions are also not actual PCI devices, they are
PCIDevice because they appear as PCI functions of the chip but are
connected internally and this series also models that correctly. This
seems to be supported by comments in Linux about how these VIA chip
function aren't following PCI standards and use ISA IRQs instead:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/pci/quirks.c?h=v6.5.6#n1172

Therefore I think Mark's proposals are not improving this model so I
went back to the original approach which was tested to work and is
also simpler and easier to understand than trying to reuse PCI
intrrupt routing which does not work and would be more complex anyway
for no good reason.

Regards,
BALATON Zoltan

BALATON Zoltan (4):
  hw/isa/vt82c686: Bring back via_isa_set_irq()
  hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
  hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq()
  hw/audio/via-ac97: Route interrupts using via_isa_set_irq()

 hw/audio/via-ac97.c        |  8 ++--
 hw/isa/vt82c686.c          | 79 +++++++++++++++++++++++++-------------
 hw/usb/vt82c686-uhci-pci.c |  9 +++++
 include/hw/isa/vt82c686.h  |  2 +
 4 files changed, 67 insertions(+), 31 deletions(-)

-- 
2.30.9

27.11.2023 01:49, BALATON Zoltan:
> Philippe,
> 
> Could this be merged for 8.2 as it fixes USB on the amigaone machine?
> This would be useful as usb-storage is the simplest way to share data
> with the host with these machines.
> 
> This is a slight change from v2 adding more comments and improving
> commit messages and clean things a bit but otherwise should be the
> same as previous versions. Even v1 worked the same as this one and v2,
> the additional check to avoid stuck bits is just paranoia, it does not
> happen in practice as IRQ mappings are quite static, they are set once
> at boot and don't change afterwards.

Should this patchset be picked up for stable-8.1?


Thanks,

/mjt

On Wed, 29 Nov 2023, Michael Tokarev wrote:
> 27.11.2023 01:49, BALATON Zoltan:
>> Philippe,
>> 
>> Could this be merged for 8.2 as it fixes USB on the amigaone machine?
>> This would be useful as usb-storage is the simplest way to share data
>> with the host with these machines.
>> 
>> This is a slight change from v2 adding more comments and improving
>> commit messages and clean things a bit but otherwise should be the
>> same as previous versions. Even v1 worked the same as this one and v2,
>> the additional check to avoid stuck bits is just paranoia, it does not
>> happen in practice as IRQ mappings are quite static, they are set once
>> at boot and don't change afterwards.
>
> Should this patchset be picked up for stable-8.1?

The amigaone machine was added now in 8.2 so the problem does not occur in 
8.1 yet (the different usage of this chip by amigaone firmware uncovered 
the issue that made this fix necessary). So 8.1 should still work without 
this therefore I don't think this is needed in stable.

Regards,
BALATON Zoltan

On Sun, 26 Nov 2023, BALATON Zoltan wrote:
> Philippe,
>
> Could this be merged for 8.2 as it fixes USB on the amigaone machine?
> This would be useful as usb-storage is the simplest way to share data
> with the host with these machines.

Philippe, do you have some time to look at this now for 8.2 please? I 
still hope this could be fixed for the amigaone machine on release and 
dont' have to wait until the next one for USB to work on that machine.

Regards,
BALATON Zoltan

> This is a slight change from v2 adding more comments and improving
> commit messages and clean things a bit but otherwise should be the
> same as previous versions. Even v1 worked the same as this one and v2,
> the additional check to avoid stuck bits is just paranoia, it does not
> happen in practice as IRQ mappings are quite static, they are set once
> at boot and don't change afterwards.
>
> The rest is just some explanation on how we got here but can be
> skipped if not interested in history.
>
> This is going back to my original implementation of this IRQ routing
> that I submitted already for 8.0 in the beginning of this year
> (https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/)
> but Mark had concerns about that because he wanted to use PCI
> interrupt routing instead. I've already told back then that won't work
> but I could not convince reviewers about that. Now with the amigaone
> machine this can also be seen and that's why this series is needed now.
>
> The routing code in PCIBus cannot be used as that only considers the 4
> PCI interrupts but there are about 12 interrupt sources in this chip
> that need to be routed to ISA IRQs (the embedded chip functions and
> the 4 PCI interrupts that are coming in through 4 pins of the chip).
> Also the chip does not own the PCIBus, that's part of the north bridge
> so it should not change the PCI interrupt routing of a bus it does not
> own. (Piix calling pci_bus_irqs() I think is also wrong because the
> PCI bus in that machine is also owned by the north bridge and piix
> should not take over routing of IRQs on a bus it's connected to.)
> Another concern from Mark was that this makes chip functions specific
> to the chip and they cannot be used as individual PCI devices. Well,
> yes, they are chip functions, are not user creatable and don't exist
> as individual devices so it does not make sense to use them without
> the actual VIA chip they are a function of so that's not a real
> concern. These functions are also not actual PCI devices, they are
> PCIDevice because they appear as PCI functions of the chip but are
> connected internally and this series also models that correctly. This
> seems to be supported by comments in Linux about how these VIA chip
> function aren't following PCI standards and use ISA IRQs instead:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/pci/quirks.c?h=v6.5.6#n1172
>
> Therefore I think Mark's proposals are not improving this model so I
> went back to the original approach which was tested to work and is
> also simpler and easier to understand than trying to reuse PCI
> intrrupt routing which does not work and would be more complex anyway
> for no good reason.
>
> Regards,
> BALATON Zoltan
>
> BALATON Zoltan (4):
>  hw/isa/vt82c686: Bring back via_isa_set_irq()
>  hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
>  hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq()
>  hw/audio/via-ac97: Route interrupts using via_isa_set_irq()
>
> hw/audio/via-ac97.c        |  8 ++--
> hw/isa/vt82c686.c          | 79 +++++++++++++++++++++++++-------------
> hw/usb/vt82c686-uhci-pci.c |  9 +++++
> include/hw/isa/vt82c686.h  |  2 +
> 4 files changed, 67 insertions(+), 31 deletions(-)
>
>

Hi Zoltan,

On 28/11/23 13:47, BALATON Zoltan wrote:
> On Sun, 26 Nov 2023, BALATON Zoltan wrote:
>> Philippe,
>>
>> Could this be merged for 8.2 as it fixes USB on the amigaone machine?
>> This would be useful as usb-storage is the simplest way to share data
>> with the host with these machines.
> 
> Philippe, do you have some time to look at this now for 8.2 please? I 
> still hope this could be fixed for the amigaone machine on release and 
> dont' have to wait until the next one for USB to work on that machine.

Thanks for your detailed cover and patch descriptions.

I just finished to run my tests and they all passed.

I couldn't spend much time reviewing the patches, but having a quick
look I don't think the way you model it is correct. This is a tricky
setup and apparently we don't fully understand it (I understand what
you explained, but some pieces don't make sense to me). That said,
I understand it help you and the AmigaOne users, and nobody objected.
So, while being a bit reluctant, I am queuing this series; and will
send a PR in a few. We'll have time to improve this model later.

Regards,

Phil.

On Tue, 28 Nov 2023, Philippe Mathieu-Daudé wrote:
> On 28/11/23 13:47, BALATON Zoltan wrote:
>> On Sun, 26 Nov 2023, BALATON Zoltan wrote:
>>> Philippe,
>>> 
>>> Could this be merged for 8.2 as it fixes USB on the amigaone machine?
>>> This would be useful as usb-storage is the simplest way to share data
>>> with the host with these machines.
>> 
>> Philippe, do you have some time to look at this now for 8.2 please? I still 
>> hope this could be fixed for the amigaone machine on release and dont' have 
>> to wait until the next one for USB to work on that machine.
>
> Thanks for your detailed cover and patch descriptions.
>
> I just finished to run my tests and they all passed.
>
> I couldn't spend much time reviewing the patches, but having a quick
> look I don't think the way you model it is correct. This is a tricky
> setup and apparently we don't fully understand it (I understand what
> you explained, but some pieces don't make sense to me). That said,
> I understand it help you and the AmigaOne users, and nobody objected.
> So, while being a bit reluctant, I am queuing this series; and will
> send a PR in a few. We'll have time to improve this model later.

Thanks very much. I'm open to further discussion and improving this model, 
just wanted to have something working in master now. The discussion about 
this seemed never ending, it started before 8.0 and still could not get to 
a conclusion yet so until then this should work for now and allow users to 
use it and does not prevent improving it later. So I'm still interested in 
your review and why do you think this is not modelling it correctly but we 
have more time for that now and can change this further as a follow up.

I think the current way makes it easier to add Bernhard's SCI interrupt as 
well for which I had a review proposal before. The main disagreement 
seemsd to be if the chip functions should be PCI devices or not. I think 
they aren't like regular PCI devices and clearly don't use PCI interrupts 
so Mark's and Bernhard's idea to use PCI bus interrupt routing for these 
does not work because they need to be independently routable to ISA 
interrupts. So whatever we do we'll need to distinguish the interrupt 
sources and keep track of their state individually because more than one 
of them can control a single ISA IRQ. Doing this in the ISA bridge seems 
like the best place because that already owns the ISA interrupts so no 
other component will need access to them and it can keep track of state of 
IRQ sources at one place.

Regards,
BALATON Zoltan