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

[Isaku Yamahata]

On Tue, Apr 19, 2011 at 02:33:46PM +0200, Juan Quintela wrote:
> Isaku Yamahata <address@hidden> 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:

Have a good vacation. :-)


> 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.

I just followed the ACPI spec way. But I don't stick to it.
If necesarry, we can do it differently in coding details
(with a comment on the difference).
-- 
yamahata