Re: [PATCH] acpi_piix4: fix save/load of PIIX4PMState

[Juan Quintela <quintela@redhat.com>]

Isaku Yamahata <yamahata@valinux.co.jp> wrote:
>> shouldn't last one still be uint16_t?
>
> It results in an error by type_check_pointer.

You are right.  We are just lying.  Will think about how to fix this
properly (basically move the whole thing to a uint8_t array, and work
from there.

>> I guess that on ich9, GPE becomes one array, do you have that code handy
>> somewhere, just to see what you want to do?
>
> I attached acpi_ich9.c.
> For the full source tree please see the thread of q35 patchset.
> http://lists.nongnu.org/archive/html/qemu-devel/2011-03/msg01656.html
>
> You may want to see only struct ACPIGFP and vmstate_ich9_pm.

Thanks.

>
>> I think that best thing to do at this point is just to revert this whole
>> patch.  We are creating a new type for uint8_t, that becomes a pointer.
>> We are not sending the length of that array, so we need to add a new
>> version/subsection when we add ICH9 anyways.
>> 
>> Seeing what you want to do would help me trying to figure out the best
>> vmstate aproach.
>
> What I want to do eventually is to implement ACPI GPE logic generally,
> to replace the current acpi_piix4's which was very specific to piix4,
> and to implement ich9 GPE with new GPE code.
> Thus we can avoid code/logic duplication of ACPI GPE between piix4 and
> ich9.
> struct ACPIGPE is used for both piix4 and ich9 with different
> initialization parameter.

Do you mean

> According to the ACPI spec, GPE register length is hw specific.
> In fact PIIX4 has 4 bytes length gpe
> (Before patch PIIX4 GPE implementation used uint16_t sts + uint16_t en
> which is very hw-specific. With the patch they became
> uint8_t* sts + uint8_t *en which are dynamically allocated).
> ICH9 has 8 bytes length gpe. (newer chipsets tend to have longer
> GPE length as they support more and more events)
>
> Regarding to save/load, what I want to do is to save/load
> the dynamically allocated array whose size is determined dynamically,
> but the size is chip-constant.
>
> struct ACPIGPE {
>     uint32_t blk;
>     uint8_t len;        <<<<< this is determined by chip.
>                         <<<<< 4 for piix4
>                         <<<<< 8 for ich9
>
>     uint8_t *sts;       <<<<< ACPI GPE status register
>                         <<<< uint8_t array whose size is determined
>                         <<<< dynamically
>                         <<<< 2 bytes for piix4
>                         <<<< 4 bytes for ich9
>
>     uint8_t *en;        <<<< ACPI GPE enable register
>                         <<<< uint8_t array whose size is determined
>                         <<<< dynamically
>                         <<<< 2 bytes for piix4
>                         <<<< 4 bytes for ich9
> };
>
> In order to keep the save/load compatibility of piix4(big endian uint16_t),
> I had to add ugly hack as above.
> If you don't like it, only acpi_piix4 part should be reverted.

I am on vacation Thrusday & Friday, but will try to do something for
this.  Things would be easiare if we change that struct to something
like:

struct ACPIGPE {
       unti32_t blk;
       uint8_t len;
       uint8_t data[8]; /* We can change struct to bigger values when we
                           implement new chipsets */
       uint8_t *sts; /* pointer to previous array);
       uint8_t *en;  /* another pointer to previous array */
}

my problem here is that you have a len field that means we have 2 arrays
of len/2 each.  That is very difficult to describe (althought possible).
This other way, we just separate the logical interface and how things
are stored.  as an added bonos, we remove two allocations.

Later, Juan.