Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 5781 bytes --]

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