malta: Fix PCI IRQ levels to be preserved during migration

[PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

Posted by Bernhard Beschow 3 years, 12 months ago

This is a follow-up on patch "malta: Move PCI interrupt handling from
gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State
to make the code movement more obvious. However, i8259[] seems redundant
to *isa, so remove it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix4.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index a9322851db..692fa8d1f9 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -43,7 +43,6 @@ struct PIIX4State {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa;
-    qemu_irq i8259[ISA_NUM_IRQS];
 
     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
 
@@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
                 pic_level |= s->pci_irq_levels[i];
             }
         }
-        qemu_set_irq(s->i8259[pic_irq], pic_level);
+        qemu_set_irq(s->isa[pic_irq], pic_level);
     }
 }
 
@@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 
     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
 
-    for (int i = 0; i < ISA_NUM_IRQS; i++) {
-        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
-    }
-
     return dev;
 }
-- 
2.35.1

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

Posted by BALATON Zoltan 3 years, 12 months ago

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> This is a follow-up on patch "malta: Move PCI interrupt handling from
> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State
> to make the code movement more obvious. However, i8259[] seems redundant
> to *isa, so remove it.

Should this be squashed in patch 1 or at least come after it? Or are there 
some other changes needed before this patch?

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/piix4.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index a9322851db..692fa8d1f9 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -43,7 +43,6 @@ struct PIIX4State {
>     PCIDevice dev;
>     qemu_irq cpu_intr;
>     qemu_irq *isa;
> -    qemu_irq i8259[ISA_NUM_IRQS];
>
>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>
> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>                 pic_level |= s->pci_irq_levels[i];
>             }
>         }
> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>     }
> }
>
> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>
>     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>
> -    for (int i = 0; i < ISA_NUM_IRQS; i++) {
> -        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
> -    }
> -
>     return dev;
> }
>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

Posted by BB 3 years, 12 months ago


Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State
>> to make the code movement more obvious. However, i8259[] seems redundant
>> to *isa, so remove it.
>
>Should this be squashed in patch 1 or at least come after it? Or are there 
>some other changes needed before this patch?

I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3.

Regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/piix4.c | 7 +------
>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index a9322851db..692fa8d1f9 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>     PCIDevice dev;
>>     qemu_irq cpu_intr;
>>     qemu_irq *isa;
>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>
>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>
>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>>                 pic_level |= s->pci_irq_levels[i];
>>             }
>>         }
>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>     }
>> }
>>
>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>>
>>     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>>
>> -    for (int i = 0; i < ISA_NUM_IRQS; i++) {
>> -        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
>> -    }
>> -
>>     return dev;
>> }
>>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

Posted by Bernhard Beschow 3 years, 12 months ago

Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State
>> to make the code movement more obvious. However, i8259[] seems redundant
>> to *isa, so remove it.
>
>Should this be squashed in patch 1 or at least come after it? Or are there 
>some other changes needed before this patch?

I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3.

Regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/piix4.c | 7 +------
>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index a9322851db..692fa8d1f9 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>     PCIDevice dev;
>>     qemu_irq cpu_intr;
>>     qemu_irq *isa;
>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>
>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>
>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>>                 pic_level |= s->pci_irq_levels[i];
>>             }
>>         }
>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>     }
>> }
>>
>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>>
>>     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>>
>> -    for (int i = 0; i < ISA_NUM_IRQS; i++) {
>> -        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
>> -    }
>> -
>>     return dev;
>> }
>>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

Posted by BALATON Zoltan 3 years, 12 months ago

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>>> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State
>>> to make the code movement more obvious. However, i8259[] seems redundant
>>> to *isa, so remove it.
>>
>> Should this be squashed in patch 1 or at least come after it? Or are there
>> some other changes needed before this patch?
>
> I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3.

I had another comment in the bottom of this message, not sure you've 
found that too, I forgot to put a warning that there are more comments 
below here. Changing the patch order or patches according to review is OK 
and adding a new patch between already reviewed ones is OK too then you 
can keep Reviewed-by on patches that did not change.

Regards,
BALATON Zoltan

> Regards,
> Bernhard
>
>>
>> Regards,
>> BALATON Zoltan
>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/piix4.c | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>>> index a9322851db..692fa8d1f9 100644
>>> --- a/hw/isa/piix4.c
>>> +++ b/hw/isa/piix4.c
>>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>>     PCIDevice dev;
>>>     qemu_irq cpu_intr;
>>>     qemu_irq *isa;
>>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>>
>>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>>
>>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>>>                 pic_level |= s->pci_irq_levels[i];
>>>             }
>>>         }
>>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>>     }
>>> }
>>>
>>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>>>
>>>     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>>>
>>> -    for (int i = 0; i < ISA_NUM_IRQS; i++) {
>>> -        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
>>> -    }
>>> -
>>>     return dev;
>>> }
>>>
>
>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

Posted by BALATON Zoltan 3 years, 12 months ago

On Sat, 12 Feb 2022, BALATON Zoltan wrote:
> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan 
>> <balaton@eik.bme.hu>:
>>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>>>> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State
>>>> to make the code movement more obvious. However, i8259[] seems redundant
>>>> to *isa, so remove it.
>>> 
>>> Should this be squashed in patch 1 or at least come after it? Or are there
>>> some other changes needed before this patch?
>> 
>> I didn't want to change the patch order since they've been reviewed 
>> already. But yeah, you're right: It makes sense to get this out of the way 
>> early in the patch series. I will do a v3.
>
> I had another comment in the bottom of this message, not sure you've found 
> that too, I forgot to put a warning that there are more comments below here.

I mean at the end of patch 1 not this one, sorry was too quick to send it.

> Changing the patch order or patches according to review is OK and adding a 
> new patch between already reviewed ones is OK too then you can keep 
> Reviewed-by on patches that did not change.
>
> Regards,
> BALATON Zoltan
>
>> Regards,
>> Bernhard
>> 
>>> 
>>> Regards,
>>> BALATON Zoltan
>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> hw/isa/piix4.c | 7 +------
>>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>> 
>>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>>>> index a9322851db..692fa8d1f9 100644
>>>> --- a/hw/isa/piix4.c
>>>> +++ b/hw/isa/piix4.c
>>>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>>>     PCIDevice dev;
>>>>     qemu_irq cpu_intr;
>>>>     qemu_irq *isa;
>>>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>>>
>>>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>>> 
>>>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, 
>>>> int level)
>>>>                 pic_level |= s->pci_irq_levels[i];
>>>>             }
>>>>         }
>>>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>>>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>>>     }
>>>> }
>>>> 
>>>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
>>>> **isa_bus, I2CBus **smbus)
>>>>
>>>>     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 
>>>> PIIX_NUM_PIRQS);
>>>> 
>>>> -    for (int i = 0; i < ISA_NUM_IRQS; i++) {
>>>> -        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
>>>> -    }
>>>> -
>>>>     return dev;
>>>> }
>>>> 
>> 
>> 
>