On Tue, Apr 19, 2011 at 02:33:46PM +0200, Juan Quintela wrote: > Isaku Yamahata <yamahata@xxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
