This device is part of a superio/ISA bridge chip and IRQs from it are
routed to an ISA interrupt set by the Interrupt Line PCI config
register. Change uhci_update_irq() to allow this and implement it in
vt82c686-uhci-pci.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v3: Do it more differently using qemu_irq instead as suggested by Gerd
v2: Do it differently to confine isa reference to vt82c686-uhci-pci as
hcd-uhci is also used on machines that don't have isa. Left Jiaxun's
R-b there as he checked it's the same for VT82C686B and gave R-b for
the 82c686b case which still holds but speak up if you tink otherwise.

 hw/usb/hcd-uhci.c          | 11 +++++------
 hw/usb/hcd-uhci.h          |  2 +-
 hw/usb/vt82c686-uhci-pci.c | 12 ++++++++++++
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 0cb02a6432..7201cd0ae7 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -31,6 +31,7 @@
 #include "hw/usb/uhci-regs.h"
 #include "migration/vmstate.h"
 #include "hw/pci/pci.h"
+#include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/timer.h"
@@ -290,7 +291,7 @@ static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr)
 
 static void uhci_update_irq(UHCIState *s)
 {
-    int level;
+    int level = 0;
     if (((s->status2 & 1) && (s->intr & (1 << 2))) ||
         ((s->status2 & 2) && (s->intr & (1 << 3))) ||
         ((s->status & UHCI_STS_USBERR) && (s->intr & (1 << 0))) ||
@@ -298,10 +299,8 @@ static void uhci_update_irq(UHCIState *s)
         (s->status & UHCI_STS_HSERR) ||
         (s->status & UHCI_STS_HCPERR)) {
         level = 1;
-    } else {
-        level = 0;
     }
-    pci_set_irq(&s->dev, level);
+    qemu_set_irq(s->irq, level);
 }
 
 static void uhci_reset(DeviceState *dev)
@@ -1170,9 +1169,9 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
 
     pci_conf[PCI_CLASS_PROG] = 0x00;
     /* TODO: reset value should be 0. */
-    pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
-
+    pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
     pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
+    s->irq = pci_allocate_irq(dev);
 
     if (s->masterbus) {
         USBPort *ports[NB_PORTS];
diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
index e61d8fcb19..1f8ee04186 100644
--- a/hw/usb/hcd-uhci.h
+++ b/hw/usb/hcd-uhci.h
@@ -60,7 +60,7 @@ typedef struct UHCIState {
     uint32_t frame_bandwidth;
     bool completions_only;
     UHCIPort ports[NB_PORTS];
-
+    qemu_irq irq;
     /* Interrupts that should be raised at the end of the current frame.  */
     uint32_t pending_int_mask;
 
diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index b109c21603..e70e739409 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -1,6 +1,16 @@
 #include "qemu/osdep.h"
+#include "hw/irq.h"
 #include "hcd-uhci.h"
 
+static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
+{
+    UHCIState *s = opaque;
+    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
+    if (irq > 0 && irq < 15) {
+        qemu_set_irq(isa_get_irq(NULL, irq), level);
+    }
+}
+
 static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
 {
     UHCIState *s = UHCI(dev);
@@ -14,6 +24,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
     pci_set_long(pci_conf + 0xc0, 0x00002000);
 
     usb_uhci_common_realize(dev, errp);
+    object_unref(s->irq);
+    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
 }
 
 static UHCIInfo uhci_info[] = {
-- 
2.21.4

On Wed, Oct 13, 2021 at 02:13:09PM +0200, BALATON Zoltan wrote:
> This device is part of a superio/ISA bridge chip and IRQs from it are
> routed to an ISA interrupt set by the Interrupt Line PCI config
> register. Change uhci_update_irq() to allow this and implement it in
> vt82c686-uhci-pci.

Looks good.  There are some unrelated changes in though (whitespace,
comments, ...), and the vt82c686-uhci-pci.c changes should be a
separate patch.

thanks,
  Gerd

On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
> On Wed, Oct 13, 2021 at 02:13:09PM +0200, BALATON Zoltan wrote:
>> This device is part of a superio/ISA bridge chip and IRQs from it are
>> routed to an ISA interrupt set by the Interrupt Line PCI config
>> register. Change uhci_update_irq() to allow this and implement it in
>> vt82c686-uhci-pci.
>
> Looks good.  There are some unrelated changes in though (whitespace,
> comments, ...), and the vt82c686-uhci-pci.c changes should be a
> separate patch.

So you mean split it into a series of three small patches? Should I do a 
w4 with that?

Regards,
BALATON Zoltan

On Thu, Oct 14, 2021 at 12:22:58PM +0200, BALATON Zoltan wrote:
> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
> > On Wed, Oct 13, 2021 at 02:13:09PM +0200, BALATON Zoltan wrote:
> > > This device is part of a superio/ISA bridge chip and IRQs from it are
> > > routed to an ISA interrupt set by the Interrupt Line PCI config
> > > register. Change uhci_update_irq() to allow this and implement it in
> > > vt82c686-uhci-pci.
> > 
> > Looks good.  There are some unrelated changes in though (whitespace,
> > comments, ...), and the vt82c686-uhci-pci.c changes should be a
> > separate patch.
> 
> So you mean split it into a series of three small patches? Should I do a w4
> with that?

I was thinking about two patches: drop the unrelated stuff, one patch
for the irq signaling change, and one for the vt82c686 changes.

But of course you can add more patches for the other changes, i.e.
dropping the else branch for level = 0 and other small tweaks.

take care,
  Gerd

On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
> On Thu, Oct 14, 2021 at 12:22:58PM +0200, BALATON Zoltan wrote:
>> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>>> On Wed, Oct 13, 2021 at 02:13:09PM +0200, BALATON Zoltan wrote:
>>>> This device is part of a superio/ISA bridge chip and IRQs from it are
>>>> routed to an ISA interrupt set by the Interrupt Line PCI config
>>>> register. Change uhci_update_irq() to allow this and implement it in
>>>> vt82c686-uhci-pci.
>>>
>>> Looks good.  There are some unrelated changes in though (whitespace,
>>> comments, ...), and the vt82c686-uhci-pci.c changes should be a
>>> separate patch.
>>
>> So you mean split it into a series of three small patches? Should I do a w4
>> with that?
>
> I was thinking about two patches: drop the unrelated stuff, one patch
> for the irq signaling change, and one for the vt82c686 changes.
>
> But of course you can add more patches for the other changes, i.e.
> dropping the else branch for level = 0 and other small tweaks.

The tewak for the comment is needed for checkpach, dropping the else is 
just to make the function shorter and IMO more readable. I can do this in 
a previous patch so checkpatch won't complain on the pci irq change or 
need at least the comment change in that patch. I'm thinking about an 
alternative way for the vt82c686 part to avoid using isa_get_irq which has 
a comment saying it should be dropped but not sure about that yet, I'll 
try it and submit a v4 with at least breaking it up to smaller patches.

Thanks,
BALATON Zoltan

On Thu, 14 Oct 2021, BALATON Zoltan wrote:
> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>> On Thu, Oct 14, 2021 at 12:22:58PM +0200, BALATON Zoltan wrote:
>>> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>>>> On Wed, Oct 13, 2021 at 02:13:09PM +0200, BALATON Zoltan wrote:
>>>>> This device is part of a superio/ISA bridge chip and IRQs from it are
>>>>> routed to an ISA interrupt set by the Interrupt Line PCI config
>>>>> register. Change uhci_update_irq() to allow this and implement it in
>>>>> vt82c686-uhci-pci.
>>>> 
>>>> Looks good.  There are some unrelated changes in though (whitespace,
>>>> comments, ...), and the vt82c686-uhci-pci.c changes should be a
>>>> separate patch.
>>> 
>>> So you mean split it into a series of three small patches? Should I do a 
>>> w4
>>> with that?
>> 
>> I was thinking about two patches: drop the unrelated stuff, one patch
>> for the irq signaling change, and one for the vt82c686 changes.
>> 
>> But of course you can add more patches for the other changes, i.e.
>> dropping the else branch for level = 0 and other small tweaks.
>
> The tewak for the comment is needed for checkpach, dropping the else is just 
> to make the function shorter and IMO more readable. I can do this in a 
> previous patch so checkpatch won't complain on the pci irq change or need at 
> least the comment change in that patch. I'm thinking about an alternative way 
> for the vt82c686 part to avoid using isa_get_irq which has a comment saying 
> it should be dropped but not sure about that yet, I'll try it and submit a v4 
> with at least breaking it up to smaller patches.

I've tried it and it's just not worth it. By moving isa reference to 
vt82c686.c we end up doing the same, just with a lot more code changes 
(which is harder to understand) and less efficiently (at every interrupt 
need to jump through hoops and do some QOM casts and additional calls to 
reach the same effect). These are basically modelling functions of the 
same device just split up in parts to categorise and reuse parts of 
similar models so accesing isa parts of the chip from usb part does not 
seem to be that bad and it's simple this way. So I ended up just splitting 
the patch for v4.

Thanks,
BALATON Zoltan

On 10/13/21 14:13, BALATON Zoltan wrote:
> This device is part of a superio/ISA bridge chip and IRQs from it are
> routed to an ISA interrupt set by the Interrupt Line PCI config
> register. Change uhci_update_irq() to allow this and implement it in
> vt82c686-uhci-pci.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> v3: Do it more differently using qemu_irq instead as suggested by Gerd
> v2: Do it differently to confine isa reference to vt82c686-uhci-pci as
> hcd-uhci is also used on machines that don't have isa. Left Jiaxun's
> R-b there as he checked it's the same for VT82C686B and gave R-b for
> the 82c686b case which still holds but speak up if you tink otherwise.
> 
>  hw/usb/hcd-uhci.c          | 11 +++++------
>  hw/usb/hcd-uhci.h          |  2 +-
>  hw/usb/vt82c686-uhci-pci.c | 12 ++++++++++++
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index 0cb02a6432..7201cd0ae7 100644
> --- a/hw/usb/hcd-uhci.c
> +++ b/hw/usb/hcd-uhci.c
> @@ -31,6 +31,7 @@
>  #include "hw/usb/uhci-regs.h"
>  #include "migration/vmstate.h"
>  #include "hw/pci/pci.h"
> +#include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  #include "qapi/error.h"
>  #include "qemu/timer.h"
> @@ -290,7 +291,7 @@ static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr)
>  
>  static void uhci_update_irq(UHCIState *s)
>  {
> -    int level;
> +    int level = 0;
>      if (((s->status2 & 1) && (s->intr & (1 << 2))) ||
>          ((s->status2 & 2) && (s->intr & (1 << 3))) ||
>          ((s->status & UHCI_STS_USBERR) && (s->intr & (1 << 0))) ||
> @@ -298,10 +299,8 @@ static void uhci_update_irq(UHCIState *s)
>          (s->status & UHCI_STS_HSERR) ||
>          (s->status & UHCI_STS_HCPERR)) {
>          level = 1;
> -    } else {
> -        level = 0;
>      }
> -    pci_set_irq(&s->dev, level);
> +    qemu_set_irq(s->irq, level);
>  }

^ OK.

>  static void uhci_reset(DeviceState *dev)
> @@ -1170,9 +1169,9 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
>  
>      pci_conf[PCI_CLASS_PROG] = 0x00;
>      /* TODO: reset value should be 0. */
> -    pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
> -
> +    pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
>      pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
> +    s->irq = pci_allocate_irq(dev);
>  
>      if (s->masterbus) {
>          USBPort *ports[NB_PORTS];

usb_uhci_common_realize() should be refactored making it PCI-agnostic.

> diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
> index e61d8fcb19..1f8ee04186 100644
> --- a/hw/usb/hcd-uhci.h
> +++ b/hw/usb/hcd-uhci.h
> @@ -60,7 +60,7 @@ typedef struct UHCIState {
>      uint32_t frame_bandwidth;
>      bool completions_only;
>      UHCIPort ports[NB_PORTS];
> -
> +    qemu_irq irq;
>      /* Interrupts that should be raised at the end of the current frame.  */
>      uint32_t pending_int_mask;

OK.

> diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
> index b109c21603..e70e739409 100644
> --- a/hw/usb/vt82c686-uhci-pci.c
> +++ b/hw/usb/vt82c686-uhci-pci.c
> @@ -1,6 +1,16 @@
>  #include "qemu/osdep.h"
> +#include "hw/irq.h"
>  #include "hcd-uhci.h"
>  
> +static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
> +{
> +    UHCIState *s = opaque;
> +    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
> +    if (irq > 0 && irq < 15) {
> +        qemu_set_irq(isa_get_irq(NULL, irq), level);
> +    }
> +}

OK.

>  static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>  {
>      UHCIState *s = UHCI(dev);
> @@ -14,6 +24,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>      pci_set_long(pci_conf + 0xc0, 0x00002000);
>  
>      usb_uhci_common_realize(dev, errp);
> +    object_unref(s->irq);
> +    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);

This can be avoided by refactoring usb_uhci_common_realize(),
uhci_pci_type_info and uhci_data_class_init().

Current TYPE_UHCI becomes TYPE_PCI_UHCI.

Not sure why UHCI has been implemented that way, we already
have USB_OHCI_PCI / USB_EHCI_PCI / USB_XHCI_PCI.

Maybe look at how TYPE_SYSBUS_OHCI is implemented VS TYPE_PCI_OHCI
to be able to implement the similar TYPE_SYSBUS_UHCI?

On Wed, 13 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/13/21 14:13, BALATON Zoltan wrote:
>> This device is part of a superio/ISA bridge chip and IRQs from it are
>> routed to an ISA interrupt set by the Interrupt Line PCI config
>> register. Change uhci_update_irq() to allow this and implement it in
>> vt82c686-uhci-pci.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>> v3: Do it more differently using qemu_irq instead as suggested by Gerd
>> v2: Do it differently to confine isa reference to vt82c686-uhci-pci as
>> hcd-uhci is also used on machines that don't have isa. Left Jiaxun's
>> R-b there as he checked it's the same for VT82C686B and gave R-b for
>> the 82c686b case which still holds but speak up if you tink otherwise.
>>
>>  hw/usb/hcd-uhci.c          | 11 +++++------
>>  hw/usb/hcd-uhci.h          |  2 +-
>>  hw/usb/vt82c686-uhci-pci.c | 12 ++++++++++++
>>  3 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
>> index 0cb02a6432..7201cd0ae7 100644
>> --- a/hw/usb/hcd-uhci.c
>> +++ b/hw/usb/hcd-uhci.c
>> @@ -31,6 +31,7 @@
>>  #include "hw/usb/uhci-regs.h"
>>  #include "migration/vmstate.h"
>>  #include "hw/pci/pci.h"
>> +#include "hw/irq.h"
>>  #include "hw/qdev-properties.h"
>>  #include "qapi/error.h"
>>  #include "qemu/timer.h"
>> @@ -290,7 +291,7 @@ static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr)
>>
>>  static void uhci_update_irq(UHCIState *s)
>>  {
>> -    int level;
>> +    int level = 0;
>>      if (((s->status2 & 1) && (s->intr & (1 << 2))) ||
>>          ((s->status2 & 2) && (s->intr & (1 << 3))) ||
>>          ((s->status & UHCI_STS_USBERR) && (s->intr & (1 << 0))) ||
>> @@ -298,10 +299,8 @@ static void uhci_update_irq(UHCIState *s)
>>          (s->status & UHCI_STS_HSERR) ||
>>          (s->status & UHCI_STS_HCPERR)) {
>>          level = 1;
>> -    } else {
>> -        level = 0;
>>      }
>> -    pci_set_irq(&s->dev, level);
>> +    qemu_set_irq(s->irq, level);
>>  }
>
> ^ OK.
>
>>  static void uhci_reset(DeviceState *dev)
>> @@ -1170,9 +1169,9 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
>>
>>      pci_conf[PCI_CLASS_PROG] = 0x00;
>>      /* TODO: reset value should be 0. */
>> -    pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
>> -
>> +    pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
>>      pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
>> +    s->irq = pci_allocate_irq(dev);
>>
>>      if (s->masterbus) {
>>          USBPort *ports[NB_PORTS];
>
> usb_uhci_common_realize() should be refactored making it PCI-agnostic.
>
>> diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
>> index e61d8fcb19..1f8ee04186 100644
>> --- a/hw/usb/hcd-uhci.h
>> +++ b/hw/usb/hcd-uhci.h
>> @@ -60,7 +60,7 @@ typedef struct UHCIState {
>>      uint32_t frame_bandwidth;
>>      bool completions_only;
>>      UHCIPort ports[NB_PORTS];
>> -
>> +    qemu_irq irq;
>>      /* Interrupts that should be raised at the end of the current frame.  */
>>      uint32_t pending_int_mask;
>
> OK.
>
>> diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
>> index b109c21603..e70e739409 100644
>> --- a/hw/usb/vt82c686-uhci-pci.c
>> +++ b/hw/usb/vt82c686-uhci-pci.c
>> @@ -1,6 +1,16 @@
>>  #include "qemu/osdep.h"
>> +#include "hw/irq.h"
>>  #include "hcd-uhci.h"
>>
>> +static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
>> +{
>> +    UHCIState *s = opaque;
>> +    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
>> +    if (irq > 0 && irq < 15) {
>> +        qemu_set_irq(isa_get_irq(NULL, irq), level);
>> +    }
>> +}
>
> OK.
>
>>  static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>>  {
>>      UHCIState *s = UHCI(dev);
>> @@ -14,6 +24,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>>      pci_set_long(pci_conf + 0xc0, 0x00002000);
>>
>>      usb_uhci_common_realize(dev, errp);
>> +    object_unref(s->irq);
>> +    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
>
> This can be avoided by refactoring usb_uhci_common_realize(),
> uhci_pci_type_info and uhci_data_class_init().
>
> Current TYPE_UHCI becomes TYPE_PCI_UHCI.
>
> Not sure why UHCI has been implemented that way, we already
> have USB_OHCI_PCI / USB_EHCI_PCI / USB_XHCI_PCI.
>
> Maybe look at how TYPE_SYSBUS_OHCI is implemented VS TYPE_PCI_OHCI
> to be able to implement the similar TYPE_SYSBUS_UHCI?

That doesn't seem to be part of fixing this bug with vt82c686-uhci-pci. Do 
I really have to do that much refactoring of UHCI model just to make it 
work with the case I care about? If this was good up to now it should be 
good enough until somebody can do this refactoring independent of this 
patch as a follow up. I may not have time for that. I'd like to improve 
pegasos2 emulation by fixing this bug for 6.2 but there's a limit on how 
much unrelated stuff I'm willing to do for that. Let's say this bug 
uncovered a possible improvement in the uhci model so note it somewhere 
(like bite-sized task on wiki) then let somebody who has time handle it. 
This should not be reason to block fixing a bug if the fix is otherwise 
acceptable.

Regards,
BALATON Zoltan

On Wed, 13 Oct 2021, BALATON Zoltan wrote:
> On Wed, 13 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/13/21 14:13, BALATON Zoltan wrote:
>>> This device is part of a superio/ISA bridge chip and IRQs from it are
>>> routed to an ISA interrupt set by the Interrupt Line PCI config
>>> register. Change uhci_update_irq() to allow this and implement it in
>>> vt82c686-uhci-pci.
>>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>> ---
>>> v3: Do it more differently using qemu_irq instead as suggested by Gerd
>>> v2: Do it differently to confine isa reference to vt82c686-uhci-pci as
>>> hcd-uhci is also used on machines that don't have isa. Left Jiaxun's
>>> R-b there as he checked it's the same for VT82C686B and gave R-b for
>>> the 82c686b case which still holds but speak up if you tink otherwise.
>>>
>>>  hw/usb/hcd-uhci.c          | 11 +++++------
>>>  hw/usb/hcd-uhci.h          |  2 +-
>>>  hw/usb/vt82c686-uhci-pci.c | 12 ++++++++++++
>>>  3 files changed, 18 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
>>> index 0cb02a6432..7201cd0ae7 100644
>>> --- a/hw/usb/hcd-uhci.c
>>> +++ b/hw/usb/hcd-uhci.c
>>> @@ -31,6 +31,7 @@
>>>  #include "hw/usb/uhci-regs.h"
>>>  #include "migration/vmstate.h"
>>>  #include "hw/pci/pci.h"
>>> +#include "hw/irq.h"
>>>  #include "hw/qdev-properties.h"
>>>  #include "qapi/error.h"
>>>  #include "qemu/timer.h"
>>> @@ -290,7 +291,7 @@ static UHCIAsync *uhci_async_find_td(UHCIState *s, 
>>> uint32_t td_addr)
>>>
>>>  static void uhci_update_irq(UHCIState *s)
>>>  {
>>> -    int level;
>>> +    int level = 0;
>>>      if (((s->status2 & 1) && (s->intr & (1 << 2))) ||
>>>          ((s->status2 & 2) && (s->intr & (1 << 3))) ||
>>>          ((s->status & UHCI_STS_USBERR) && (s->intr & (1 << 0))) ||
>>> @@ -298,10 +299,8 @@ static void uhci_update_irq(UHCIState *s)
>>>          (s->status & UHCI_STS_HSERR) ||
>>>          (s->status & UHCI_STS_HCPERR)) {
>>>          level = 1;
>>> -    } else {
>>> -        level = 0;
>>>      }
>>> -    pci_set_irq(&s->dev, level);
>>> +    qemu_set_irq(s->irq, level);
>>>  }
>> 
>> ^ OK.
>>
>>>  static void uhci_reset(DeviceState *dev)
>>> @@ -1170,9 +1169,9 @@ void usb_uhci_common_realize(PCIDevice *dev, Error 
>>> **errp)
>>>
>>>      pci_conf[PCI_CLASS_PROG] = 0x00;
>>>      /* TODO: reset value should be 0. */
>>> -    pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
>>> -
>>> +    pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
>>>      pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
>>> +    s->irq = pci_allocate_irq(dev);
>>>
>>>      if (s->masterbus) {
>>>          USBPort *ports[NB_PORTS];
>> 
>> usb_uhci_common_realize() should be refactored making it PCI-agnostic.
>> 
>>> diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
>>> index e61d8fcb19..1f8ee04186 100644
>>> --- a/hw/usb/hcd-uhci.h
>>> +++ b/hw/usb/hcd-uhci.h
>>> @@ -60,7 +60,7 @@ typedef struct UHCIState {
>>>      uint32_t frame_bandwidth;
>>>      bool completions_only;
>>>      UHCIPort ports[NB_PORTS];
>>> -
>>> +    qemu_irq irq;
>>>      /* Interrupts that should be raised at the end of the current frame. 
>>> */
>>>      uint32_t pending_int_mask;
>> 
>> OK.
>> 
>>> diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
>>> index b109c21603..e70e739409 100644
>>> --- a/hw/usb/vt82c686-uhci-pci.c
>>> +++ b/hw/usb/vt82c686-uhci-pci.c
>>> @@ -1,6 +1,16 @@
>>>  #include "qemu/osdep.h"
>>> +#include "hw/irq.h"
>>>  #include "hcd-uhci.h"
>>> 
>>> +static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
>>> +{
>>> +    UHCIState *s = opaque;
>>> +    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
>>> +    if (irq > 0 && irq < 15) {
>>> +        qemu_set_irq(isa_get_irq(NULL, irq), level);
>>> +    }
>>> +}
>> 
>> OK.
>>
>>>  static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>>>  {
>>>      UHCIState *s = UHCI(dev);
>>> @@ -14,6 +24,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, 
>>> Error **errp)
>>>      pci_set_long(pci_conf + 0xc0, 0x00002000);
>>>
>>>      usb_uhci_common_realize(dev, errp);
>>> +    object_unref(s->irq);
>>> +    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
>> 
>> This can be avoided by refactoring usb_uhci_common_realize(),
>> uhci_pci_type_info and uhci_data_class_init().
>> 
>> Current TYPE_UHCI becomes TYPE_PCI_UHCI.
>> 
>> Not sure why UHCI has been implemented that way, we already
>> have USB_OHCI_PCI / USB_EHCI_PCI / USB_XHCI_PCI.
>> 
>> Maybe look at how TYPE_SYSBUS_OHCI is implemented VS TYPE_PCI_OHCI
>> to be able to implement the similar TYPE_SYSBUS_UHCI?
>
> That doesn't seem to be part of fixing this bug with vt82c686-uhci-pci. Do I 
> really have to do that much refactoring of UHCI model just to make it work 
> with the case I care about? If this was good up to now it should be good 
> enough until somebody can do this refactoring independent of this patch as a 
> follow up. I may not have time for that. I'd like to improve pegasos2 
> emulation by fixing this bug for 6.2 but there's a limit on how much 
> unrelated stuff I'm willing to do for that. Let's say this bug uncovered a 
> possible improvement in the uhci model so note it somewhere (like bite-sized 
> task on wiki) then let somebody who has time handle it. This should not be 
> reason to block fixing a bug if the fix is otherwise acceptable.

If you don't like replacing irq with object_unref another possible way is 
adding it to UHCIInfo simlar to how realize is set, I can do that if you 
think that would be better but changing qdev stuff is not something I want 
to do as that tends to get complex and takes hours.

Regards,
BALATON Zoltan

On Wed, 13 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/13/21 14:13, BALATON Zoltan wrote:
>> This device is part of a superio/ISA bridge chip and IRQs from it are
>> routed to an ISA interrupt set by the Interrupt Line PCI config
>> register. Change uhci_update_irq() to allow this and implement it in
>> vt82c686-uhci-pci.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>> v3: Do it more differently using qemu_irq instead as suggested by Gerd
>> v2: Do it differently to confine isa reference to vt82c686-uhci-pci as
>> hcd-uhci is also used on machines that don't have isa. Left Jiaxun's
>> R-b there as he checked it's the same for VT82C686B and gave R-b for
>> the 82c686b case which still holds but speak up if you tink otherwise.
>>
>>  hw/usb/hcd-uhci.c          | 11 +++++------
>>  hw/usb/hcd-uhci.h          |  2 +-
>>  hw/usb/vt82c686-uhci-pci.c | 12 ++++++++++++
>>  3 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
>> index 0cb02a6432..7201cd0ae7 100644
>> --- a/hw/usb/hcd-uhci.c
>> +++ b/hw/usb/hcd-uhci.c
>> @@ -31,6 +31,7 @@
>>  #include "hw/usb/uhci-regs.h"
>>  #include "migration/vmstate.h"
>>  #include "hw/pci/pci.h"
>> +#include "hw/irq.h"
>>  #include "hw/qdev-properties.h"
>>  #include "qapi/error.h"
>>  #include "qemu/timer.h"
>> @@ -290,7 +291,7 @@ static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr)
>>
>>  static void uhci_update_irq(UHCIState *s)
>>  {
>> -    int level;
>> +    int level = 0;
>>      if (((s->status2 & 1) && (s->intr & (1 << 2))) ||
>>          ((s->status2 & 2) && (s->intr & (1 << 3))) ||
>>          ((s->status & UHCI_STS_USBERR) && (s->intr & (1 << 0))) ||
>> @@ -298,10 +299,8 @@ static void uhci_update_irq(UHCIState *s)
>>          (s->status & UHCI_STS_HSERR) ||
>>          (s->status & UHCI_STS_HCPERR)) {
>>          level = 1;
>> -    } else {
>> -        level = 0;
>>      }
>> -    pci_set_irq(&s->dev, level);
>> +    qemu_set_irq(s->irq, level);
>>  }
>
> ^ OK.
>
>>  static void uhci_reset(DeviceState *dev)
>> @@ -1170,9 +1169,9 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
>>
>>      pci_conf[PCI_CLASS_PROG] = 0x00;
>>      /* TODO: reset value should be 0. */
>> -    pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
>> -
>> +    pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
>>      pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
>> +    s->irq = pci_allocate_irq(dev);
>>
>>      if (s->masterbus) {
>>          USBPort *ports[NB_PORTS];
>
> usb_uhci_common_realize() should be refactored making it PCI-agnostic.
>
>> diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
>> index e61d8fcb19..1f8ee04186 100644
>> --- a/hw/usb/hcd-uhci.h
>> +++ b/hw/usb/hcd-uhci.h
>> @@ -60,7 +60,7 @@ typedef struct UHCIState {
>>      uint32_t frame_bandwidth;
>>      bool completions_only;
>>      UHCIPort ports[NB_PORTS];
>> -
>> +    qemu_irq irq;
>>      /* Interrupts that should be raised at the end of the current frame.  */
>>      uint32_t pending_int_mask;
>
> OK.
>
>> diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
>> index b109c21603..e70e739409 100644
>> --- a/hw/usb/vt82c686-uhci-pci.c
>> +++ b/hw/usb/vt82c686-uhci-pci.c
>> @@ -1,6 +1,16 @@
>>  #include "qemu/osdep.h"
>> +#include "hw/irq.h"
>>  #include "hcd-uhci.h"
>>
>> +static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
>> +{
>> +    UHCIState *s = opaque;
>> +    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
>> +    if (irq > 0 && irq < 15) {
>> +        qemu_set_irq(isa_get_irq(NULL, irq), level);
>> +    }
>> +}
>
> OK.
>
>>  static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>>  {
>>      UHCIState *s = UHCI(dev);
>> @@ -14,6 +24,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>>      pci_set_long(pci_conf + 0xc0, 0x00002000);
>>
>>      usb_uhci_common_realize(dev, errp);
>> +    object_unref(s->irq);
>> +    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
>
> This can be avoided by refactoring usb_uhci_common_realize(),
> uhci_pci_type_info and uhci_data_class_init().
>
> Current TYPE_UHCI becomes TYPE_PCI_UHCI.
>
> Not sure why UHCI has been implemented that way, we already
> have USB_OHCI_PCI / USB_EHCI_PCI / USB_XHCI_PCI.

The reason for that may be that those have sysbus versions and pci 
versions and maybe there was no need for a sysbus UHCI version as all of 
those we emulate are PCI? I think those were probably also like UHCI 
before but when a sysbus version was needed they were split.

Regards,
BALATON Zoltan

  Hi,

> >      if (s->masterbus) {
> >          USBPort *ports[NB_PORTS];
> 
> usb_uhci_common_realize() should be refactored making it PCI-agnostic.

Not sure this is needed here.  This seems to be more a platform-specific
oddity in IRQ routing than a non-pci UHCI device.

take care,
  Gerd

On 10/14/21 11:12, Gerd Hoffmann wrote:
>   Hi,
> 
>>>      if (s->masterbus) {
>>>          USBPort *ports[NB_PORTS];
>>
>> usb_uhci_common_realize() should be refactored making it PCI-agnostic.
> 
> Not sure this is needed here.  This seems to be more a platform-specific
> oddity in IRQ routing than a non-pci UHCI device.

OK, fine then.