Avoid using isa_get_irq in vt82c686 model

[PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by BALATON Zoltan 4 years, 3 months ago

Use via_isa_set_irq() which better encapsulates irq handling in the
vt82xx model and avoids using isa_get_irq() that has a comment saying
it should not be used.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 94cc2142c7..252d18f4ac 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -29,7 +29,7 @@
 #include "migration/vmstate.h"
 #include "qemu/module.h"
 #include "sysemu/dma.h"
-
+#include "hw/isa/vt82c686.h"
 #include "hw/ide/pci.h"
 #include "trace.h"
 
@@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
         d->config[0x70 + n * 8] &= ~0x80;
     }
 
-    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
+    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
 }
 
 static void via_ide_reset(DeviceState *dev)
-- 
2.21.4

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by Philippe Mathieu-Daudé 4 years, 3 months ago

On 10/15/21 03:06, BALATON Zoltan wrote:
> Use via_isa_set_irq() which better encapsulates irq handling in the
> vt82xx model and avoids using isa_get_irq() that has a comment saying
> it should not be used.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ide/via.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 94cc2142c7..252d18f4ac 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -29,7 +29,7 @@
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
>  #include "sysemu/dma.h"
> -
> +#include "hw/isa/vt82c686.h"
>  #include "hw/ide/pci.h"
>  #include "trace.h"
>  
> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>          d->config[0x70 + n * 8] &= ~0x80;
>      }
>  
> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);

Since pci_get_function_0() is expensive, we should cache
'PCIDevice *func0' in PCIIDEState, setting the pointer in
via_ide_realize(). Do you mind sending a follow-up patch?

>  }

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by BALATON Zoltan 4 years, 3 months ago

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/15/21 03:06, BALATON Zoltan wrote:
>> Use via_isa_set_irq() which better encapsulates irq handling in the
>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>> it should not be used.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 94cc2142c7..252d18f4ac 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -29,7 +29,7 @@
>>  #include "migration/vmstate.h"
>>  #include "qemu/module.h"
>>  #include "sysemu/dma.h"
>> -
>> +#include "hw/isa/vt82c686.h"
>>  #include "hw/ide/pci.h"
>>  #include "trace.h"
>>
>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>>          d->config[0x70 + n * 8] &= ~0x80;
>>      }
>>
>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>
> Since pci_get_function_0() is expensive, we should cache
> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
> via_ide_realize(). Do you mind sending a follow-up patch?

I can do that but waiting for a decision on how to proceed. Will Gerd take 
my first series this is based on as is then this should be a separate 
series doing the clean up using pci_get_function_0 or should these two 
series be merged? I'd also squash setting user_creatable = false into this 
patch (and do similar for the usb one) unless you guys think it should be 
a separate patch?

Regards,
BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by Philippe Mathieu-Daudé 4 years, 3 months ago

On 10/17/21 20:44, BALATON Zoltan wrote:
> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>> it should not be used.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ide/via.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index 94cc2142c7..252d18f4ac 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -29,7 +29,7 @@
>>>  #include "migration/vmstate.h"
>>>  #include "qemu/module.h"
>>>  #include "sysemu/dma.h"
>>> -
>>> +#include "hw/isa/vt82c686.h"
>>>  #include "hw/ide/pci.h"
>>>  #include "trace.h"
>>>
>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>> int level)
>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>      }
>>>
>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>
>> Since pci_get_function_0() is expensive, we should cache
>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>> via_ide_realize(). Do you mind sending a follow-up patch?
> 
> I can do that but waiting for a decision on how to proceed. Will Gerd
> take my first series this is based on as is then this should be a
> separate series doing the clean up using pci_get_function_0 or should
> these two series be merged? I'd also squash setting user_creatable =
> false into this patch (and do similar for the usb one) unless you guys
> think it should be a separate patch?

I don't know what Gerd will do with the USB patches.
Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
and extra user_creatable) via mips-next.

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by BALATON Zoltan 4 years, 3 months ago

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/17/21 20:44, BALATON Zoltan wrote:
>> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>>> it should not be used.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ide/via.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index 94cc2142c7..252d18f4ac 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -29,7 +29,7 @@
>>>>  #include "migration/vmstate.h"
>>>>  #include "qemu/module.h"
>>>>  #include "sysemu/dma.h"
>>>> -
>>>> +#include "hw/isa/vt82c686.h"
>>>>  #include "hw/ide/pci.h"
>>>>  #include "trace.h"
>>>>
>>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>>> int level)
>>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>>      }
>>>>
>>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>>
>>> Since pci_get_function_0() is expensive, we should cache
>>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>>> via_ide_realize(). Do you mind sending a follow-up patch?
>>
>> I can do that but waiting for a decision on how to proceed. Will Gerd
>> take my first series this is based on as is then this should be a
>> separate series doing the clean up using pci_get_function_0 or should
>> these two series be merged? I'd also squash setting user_creatable =
>> false into this patch (and do similar for the usb one) unless you guys
>> think it should be a separate patch?
>
> I don't know what Gerd will do with the USB patches.
> Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
> and extra user_creatable) via mips-next.

I'm confused. So you don't need another version from the last two at 
least? And patch 2 is useless without the rest of the series that's why I 
said you can cherry pick 1.

Regards,
BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by Gerd Hoffmann 4 years, 3 months ago

  Hi,

> > I can do that but waiting for a decision on how to proceed. Will Gerd
> > take my first series this is based on as is then this should be a
> > separate series doing the clean up using pci_get_function_0 or should
> > these two series be merged? I'd also squash setting user_creatable =
> > false into this patch (and do similar for the usb one) unless you guys
> > think it should be a separate patch?
> 
> I don't know what Gerd will do with the USB patches.

Latest revision of usb patches is fine.  I'll go stick them into the
next usb pull request ...

> Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
> and extra user_creatable) via mips-next.

.. unless someone else is faster with merging ;)

Feel free to add my "Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>"
to the usb patches and merge the complete pack via mips-next.

take care,
  Gerd

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by BALATON Zoltan 4 years, 3 months ago

Hello,

On Mon, 18 Oct 2021, Gerd Hoffmann wrote:
>  Hi,
>
>>> I can do that but waiting for a decision on how to proceed. Will Gerd
>>> take my first series this is based on as is then this should be a
>>> separate series doing the clean up using pci_get_function_0 or should
>>> these two series be merged? I'd also squash setting user_creatable =
>>> false into this patch (and do similar for the usb one) unless you guys
>>> think it should be a separate patch?
>>
>> I don't know what Gerd will do with the USB patches.
>
> Latest revision of usb patches is fine.  I'll go stick them into the
> next usb pull request ...
>
>> Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
>> and extra user_creatable) via mips-next.
>
> .. unless someone else is faster with merging ;)
>
> Feel free to add my "Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>"
> to the usb patches and merge the complete pack via mips-next.

Wait, the usb patch needs adding user_creatable = false too to prevent 
crashing when user adds the device without having function 0. I'll resend 
with that.

Regards,
BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by BALATON Zoltan 4 years, 3 months ago

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/15/21 03:06, BALATON Zoltan wrote:
>> Use via_isa_set_irq() which better encapsulates irq handling in the
>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>> it should not be used.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 94cc2142c7..252d18f4ac 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -29,7 +29,7 @@
>>  #include "migration/vmstate.h"
>>  #include "qemu/module.h"
>>  #include "sysemu/dma.h"
>> -
>> +#include "hw/isa/vt82c686.h"
>>  #include "hw/ide/pci.h"
>>  #include "trace.h"
>>
>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>>          d->config[0x70 + n * 8] &= ~0x80;
>>      }
>>
>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>
> Since pci_get_function_0() is expensive, we should cache
> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
> via_ide_realize(). Do you mind sending a follow-up patch?

On the other hand, IMO PCIIDEState should be about PCI IDE stuff so to 
keep this clean this would need subclassing it to VIAIDEState and put the 
func0 pointer there. But then we probably need to cast between VIAIDE and 
PCIIDE and likely we're back to the same level of expensiveness. The main 
source why of pci_get_function_0 is expensive is probably the QOM cast to 
PCI_BUS in pci_get_bus() the rest is just pointer and array index 
dereferences which should not be too bad. And the reason why QOM casts are 
expensive is because we have --enable-qom-debug enabled by default. I've 
tried to send a patch before to disable this on release builds and only 
enable it with --enable-debug or when explicitly asked but it was rejected 
saying that these asserts are useful. Maybe we can just live with 
qemu_set_irq and not bother and drop this series. (You can cherry pick the 
first patch removing code duplication from via isa if you want.)

Regards,
BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by Philippe Mathieu-Daudé 4 years, 3 months ago

On 10/17/21 21:39, BALATON Zoltan wrote:
> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>> it should not be used.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ide/via.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index 94cc2142c7..252d18f4ac 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -29,7 +29,7 @@
>>>  #include "migration/vmstate.h"
>>>  #include "qemu/module.h"
>>>  #include "sysemu/dma.h"
>>> -
>>> +#include "hw/isa/vt82c686.h"
>>>  #include "hw/ide/pci.h"
>>>  #include "trace.h"
>>>
>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>> int level)
>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>      }
>>>
>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>
>> Since pci_get_function_0() is expensive, we should cache
>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>> via_ide_realize(). Do you mind sending a follow-up patch?
> 
> On the other hand, IMO PCIIDEState should be about PCI IDE stuff so to
> keep this clean this would need subclassing it to VIAIDEState and put
> the func0 pointer there.

I expect any multi-function PCI embedding an IDE controller to route
its IRQs via Func#0, but I'm not a PCI expert.

> But then we probably need to cast between
> VIAIDE and PCIIDE and likely we're back to the same level of
> expensiveness.

realize() is called once, get_irq() multiple times.

> The main source why of pci_get_function_0 is expensive is
> probably the QOM cast to PCI_BUS in pci_get_bus() the rest is just
> pointer and array index dereferences which should not be too bad. And
> the reason why QOM casts are expensive is because we have
> --enable-qom-debug enabled by default. I've tried to send a patch before
> to disable this on release builds and only enable it with --enable-debug
> or when explicitly asked but it was rejected saying that these asserts
> are useful. Maybe we can just live with qemu_set_irq and not bother and
> drop this series. (You can cherry pick the first patch removing code
> duplication from via isa if you want.)
> 
> Regards,
> BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

Posted by BALATON Zoltan 4 years, 3 months ago

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/17/21 21:39, BALATON Zoltan wrote:
>> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>>> it should not be used.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ide/via.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index 94cc2142c7..252d18f4ac 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -29,7 +29,7 @@
>>>>  #include "migration/vmstate.h"
>>>>  #include "qemu/module.h"
>>>>  #include "sysemu/dma.h"
>>>> -
>>>> +#include "hw/isa/vt82c686.h"
>>>>  #include "hw/ide/pci.h"
>>>>  #include "trace.h"
>>>>
>>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>>> int level)
>>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>>      }
>>>>
>>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>>
>>> Since pci_get_function_0() is expensive, we should cache
>>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>>> via_ide_realize(). Do you mind sending a follow-up patch?
>>
>> On the other hand, IMO PCIIDEState should be about PCI IDE stuff so to
>> keep this clean this would need subclassing it to VIAIDEState and put
>> the func0 pointer there.
>
> I expect any multi-function PCI embedding an IDE controller to route
> its IRQs via Func#0, but I'm not a PCI expert.

We don't have many in QEMU, I think only via and piix and not sure what 
piix does (currently using isa_get_irq, that's where I got this from for 
via).

>> But then we probably need to cast between
>> VIAIDE and PCIIDE and likely we're back to the same level of
>> expensiveness.
>
> realize() is called once, get_irq() multiple times.

Sure but we need to pass something to set_irq which will be then 
VIAIDEState that maybe we'll need to cast to something else but now we 
also cast it to PCI_DEVICE so maybe we could cache that too?

Regards,
BALATON Zoltan

>> The main source why of pci_get_function_0 is expensive is
>> probably the QOM cast to PCI_BUS in pci_get_bus() the rest is just
>> pointer and array index dereferences which should not be too bad. And
>> the reason why QOM casts are expensive is because we have
>> --enable-qom-debug enabled by default. I've tried to send a patch before
>> to disable this on release builds and only enable it with --enable-debug
>> or when explicitly asked but it was rejected saying that these asserts
>> are useful. Maybe we can just live with qemu_set_irq and not bother and
>> drop this series. (You can cherry pick the first patch removing code
>> duplication from via isa if you want.)
>>
>> Regards,
>> BALATON Zoltan
>
>