Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[BALATON Zoltan]

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> Now that pci_irq_levels[] is part of PIIX4State, the PCI IRQ levels can
> be saved and restored via the VMState mechanism. This fixes migrated VMs
> to start with PCI IRQ levels zeroed, which is a bug.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/piix4.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 964e09cf7f..a9322851db 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -45,7 +45,7 @@ struct PIIX4State {
>     qemu_irq *isa;
>     qemu_irq i8259[ISA_NUM_IRQS];
>
> -    int pci_irq_levels[PIIX_NUM_PIRQS];
> +    int32_t pci_irq_levels[PIIX_NUM_PIRQS];

Do you really need 32 bits to store a value of 0 or 1? I saw piix3 has 
that too but do we need to do the same here? Could this be just an uint8_t 
array? That's still an overkill to store 4 bits of information but less so 
than with int32_t.

By the way the corresponding member in struct PIIXState in 
include/hw/southbridge/piix.h has a comment saying:

    /* This member isn't used. Just for save/load compatibility */
    int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];

and only seems to be filled in piix3_pre_save() but never used. So what's 
the point of this then? Maybe piix3 also uses a bitmap to store these 
levels instead? There's a uint64_t pic_levels member above the unused 
array but I haven't checked the implementation.

Regards,
BALATON Zoltan

>     RTCState rtc;
>     /* Reset Control Register */
> @@ -128,12 +128,14 @@ static int piix4_ide_post_load(void *opaque, int 
> version_id)
>
> static const VMStateDescription vmstate_piix4 = {
>     .name = "PIIX4",
> -    .version_id = 3,
> +    .version_id = 4,
>     .minimum_version_id = 2,
>     .post_load = piix4_ide_post_load,
>     .fields = (VMStateField[]) {
>         VMSTATE_PCI_DEVICE(dev, PIIX4State),
>         VMSTATE_UINT8_V(rcr, PIIX4State, 3),
> +        VMSTATE_INT32_ARRAY_V(pci_irq_levels, PIIX4State,
> +                              PIIX_NUM_PIRQS, 4),
>         VMSTATE_END_OF_LIST()
>     }
> };