| Message ID | 7e892c9654f16e4c163607222e1554394235ecf4.1303135172.git.yamahata@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
Isaku Yamahata <yamahata@valinux.co.jp> wrote: > It's vmstate parameter was wrong. This patch fixes it. > > Reported-by: Avi Kivity <avi@redhat.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/acpi_piix4.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 96f5222..3a8fece 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -214,10 +214,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id) > { \ > .name = (stringify(_field)), \ > .version_id = 0, \ > - .num = GPE_LEN, \ > .info = &vmstate_info_uint16, \ > .size = sizeof(uint16_t), \ > - .flags = VMS_ARRAY | VMS_POINTER, \ > + .flags = VMS_SINGLE | VMS_POINTER, \ > .offset = vmstate_offset_pointer(_state, _field, uint8_t), > \ shouldn't last one still be uint16_t? 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 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. Thanks, Juan.
On Mon, Apr 18, 2011 at 06:26:08PM +0200, Juan Quintela wrote: > Isaku Yamahata <yamahata@valinux.co.jp> wrote: > > It's vmstate parameter was wrong. This patch fixes it. > > > > Reported-by: Avi Kivity <avi@redhat.com> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > --- > > hw/acpi_piix4.c | 3 +-- > > 1 files changed, 1 insertions(+), 2 deletions(-) > > > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > > index 96f5222..3a8fece 100644 > > --- a/hw/acpi_piix4.c > > +++ b/hw/acpi_piix4.c > > @@ -214,10 +214,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id) > > { \ > > .name = (stringify(_field)), \ > > .version_id = 0, \ > > - .num = GPE_LEN, \ > > .info = &vmstate_info_uint16, \ > > .size = sizeof(uint16_t), \ > > - .flags = VMS_ARRAY | VMS_POINTER, \ > > + .flags = VMS_SINGLE | VMS_POINTER, \ > > .offset = vmstate_offset_pointer(_state, _field, uint8_t), > > \ > > > shouldn't last one still be uint16_t? It results in an error by type_check_pointer. > 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. > 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. 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. thanks, the acpi_ich9.c --------------------------------------------------------------------------- /* * ACPI implementation * * Copyright (c) 2006 Fabrice Bellard * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public * License version 2 as published by the Free Software Foundation. * * This library is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, see <http://www.gnu.org/licenses/> */ /* * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp> * VA Linux Systems Japan K.K. * * This is based on acpi.c. */ #include "hw.h" #include "pc.h" #include "pci.h" #include "qemu-timer.h" #include "sysemu.h" #include "acpi.h" #include "q35.h" //#define DEBUG #ifdef DEBUG #define ICH9_DEBUG(fmt, ...) do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0) #else #define ICH9_DEBUG(fmt, ...) do { } while (0) #endif static void pm_ioport_write_fallback(void *opaque, uint32_t addr, int len, uint32_t val); static uint32_t pm_ioport_read_fallback(void *opaque, uint32_t addr, int len); static void pm_update_sci(ICH9_LPCPmRegs *pm) { int sci_level, pm1a_sts; pm1a_sts = acpi_pm1_evt_get_sts(&pm->pm1a, pm->tmr.overflow_time); sci_level = (((pm1a_sts & pm->pm1a.en) & (ACPI_BITMASK_RT_CLOCK_ENABLE | ACPI_BITMASK_POWER_BUTTON_ENABLE | ACPI_BITMASK_GLOBAL_LOCK_ENABLE | ACPI_BITMASK_TIMER_ENABLE)) != 0); qemu_set_irq(pm->irq, sci_level); /* schedule a timer interruption if needed */ acpi_pm_tmr_update(&pm->tmr, (pm->pm1a.en & ACPI_BITMASK_TIMER_ENABLE) && !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS)); } static void ich9_pm_update_sci_fn(ACPIPMTimer *tmr) { ICH9_LPCPmRegs *pm = container_of(tmr, ICH9_LPCPmRegs, tmr); pm_update_sci(pm); } static void pm_ioport_writeb(void *opaque, uint32_t addr, uint32_t val) { ICH9_LPCPmRegs *pm = opaque; switch (addr & ICH9_PMIO_MASK) { case ICH9_PMIO_GPE0_STS ... (ICH9_PMIO_GPE0_STS + ICH9_PMIO_GPE0_LEN - 1): acpi_gpe_ioport_writeb(&pm->gpe0, addr, val); break; default: break; } ICH9_DEBUG("port=0x%04x val=0x%04x\n", addr, val); } static uint32_t pm_ioport_readb(void *opaque, uint32_t addr) { ICH9_LPCPmRegs *pm = opaque; uint32_t val = 0; switch(addr & ICH9_PMIO_MASK) { case ICH9_PMIO_GPE0_STS ... (ICH9_PMIO_GPE0_STS + ICH9_PMIO_GPE0_LEN - 1): val = acpi_gpe_ioport_readb(&pm->gpe0, addr); break; default: val = 0; break; } ICH9_DEBUG("port=0x%04x val=0x%04x\n", addr, val); return val; } static void pm_ioport_writew(void *opaque, uint32_t addr, uint32_t val) { ICH9_LPCPmRegs *pm = opaque; switch(addr & ICH9_PMIO_MASK) { case ICH9_PMIO_PM1_STS: acpi_pm1_evt_write_sts(&pm->pm1a, &pm->tmr, val); pm_update_sci(pm); break; case ICH9_PMIO_PM1_EN: pm->pm1a.en = val; pm_update_sci(pm); break; case ICH9_PMIO_PM1_CNT: acpi_pm1_cnt_write(&pm->pm1a, &pm->pm1_cnt, val); break; default: pm_ioport_write_fallback(opaque, addr, 2, val); break; } ICH9_DEBUG("port=0x%04x val=0x%04x\n", addr, val); } static uint32_t pm_ioport_readw(void *opaque, uint32_t addr) { ICH9_LPCPmRegs *pm = opaque; uint32_t val; switch(addr & ICH9_PMIO_MASK) { case ICH9_PMIO_PM1_STS: val = acpi_pm1_evt_get_sts(&pm->pm1a, pm->tmr.overflow_time); break; case ICH9_PMIO_PM1_EN: val = pm->pm1a.en; break; case ICH9_PMIO_PM1_CNT: val = pm->pm1_cnt.cnt; break; default: val = pm_ioport_read_fallback(opaque, addr, 2); break; } ICH9_DEBUG("port=0x%04x val=0x%04x\n", addr, val); return val; } static void pm_ioport_writel(void *opaque, uint32_t addr, uint32_t val) { ICH9_LPCPmRegs *pm = opaque; switch(addr & ICH9_PMIO_MASK) { case ICH9_PMIO_SMI_EN: pm->smi_en = val; break; default: pm_ioport_write_fallback(opaque, addr, 4, val); break; } ICH9_DEBUG("port=0x%04x val=0x%08x\n", addr, val); } static uint32_t pm_ioport_readl(void *opaque, uint32_t addr) { ICH9_LPCPmRegs *pm = opaque; uint32_t val; switch(addr & ICH9_PMIO_MASK) { case ICH9_PMIO_PM1_TMR: val = acpi_pm_tmr_get(&pm->tmr); break; case ICH9_PMIO_SMI_EN: val = pm->smi_en; break; default: val = pm_ioport_read_fallback(opaque, addr, 4); break; } ICH9_DEBUG("port=0x%04x val=0x%08x\n", addr, val); return val; } static void pm_ioport_write_fallback(void *opaque, uint32_t addr, int len, uint32_t val) { int subsize = (len == 4)? 2: 1; IOPortWriteFunc *ioport_write = (subsize == 2)? pm_ioport_writew: pm_ioport_writeb; int i; for (i = 0; i < len; i += subsize) { ioport_write(opaque, addr, val); val >>= 8 * subsize; } } static uint32_t pm_ioport_read_fallback(void *opaque, uint32_t addr, int len) { int subsize = (len == 4)? 2: 1; IOPortReadFunc *ioport_read = (subsize == 2)? pm_ioport_readw: pm_ioport_readb; uint32_t val; int i; val = 0; for (i = 0; i < len; i += subsize) { val <<= 8 * subsize; val |= ioport_read(opaque, addr); } return val; } void ich9_pm_iospace_update(ICH9_LPCPmRegs *pm, uint32_t pm_io_base) { ICH9_DEBUG("to 0x%x\n", pm_io_base); assert((pm_io_base & ICH9_PMIO_MASK) == 0); if (pm->pm_io_base != 0) { isa_unassign_ioport(pm->pm_io_base, ICH9_PMIO_SIZE); } /* don't map at 0 */ if (pm_io_base == 0) { return; } register_ioport_write(pm_io_base, ICH9_PMIO_SIZE, 1, pm_ioport_writeb, pm); register_ioport_read(pm_io_base, ICH9_PMIO_SIZE, 1, pm_ioport_readb, pm); register_ioport_write(pm_io_base, ICH9_PMIO_SIZE, 2, pm_ioport_writew, pm); register_ioport_read(pm_io_base, ICH9_PMIO_SIZE, 2, pm_ioport_readw, pm); register_ioport_write(pm_io_base, ICH9_PMIO_SIZE, 4, pm_ioport_writel, pm); register_ioport_read(pm_io_base, ICH9_PMIO_SIZE, 4, pm_ioport_readl, pm); pm->pm_io_base = pm_io_base; acpi_gpe_blk(&pm->gpe0, pm_io_base + ICH9_PMIO_GPE0_STS); } static int ich9_pm_post_load(void *opaque, int version_id) { ICH9_LPCPmRegs *pm = opaque; uint32_t pm_io_base = pm->pm_io_base; pm->pm_io_base = 0; ich9_pm_iospace_update(pm, pm_io_base); return 0; } #define VMSTATE_GPE_ARRAY(_field, _state) \ { \ .name = (stringify(_field)), \ .version_id = 0, \ .num = ICH9_PMIO_GPE0_LEN / 2, \ .info = &vmstate_info_uint8, \ .size = sizeof(uint8_t), \ .flags = VMS_ARRAY | VMS_POINTER, \ .offset = vmstate_offset_pointer(_state, _field, uint8_t), \ } const VMStateDescription vmstate_ich9_pm = { .name = "ich9_pm", .post_load = ich9_pm_post_load, .fields = (VMStateField[]) { VMSTATE_UINT16(pm1a.sts, ICH9_LPCPmRegs), VMSTATE_UINT16(pm1a.en, ICH9_LPCPmRegs), VMSTATE_UINT16(pm1_cnt.cnt, ICH9_LPCPmRegs), VMSTATE_TIMER(tmr.timer, ICH9_LPCPmRegs), VMSTATE_INT64(tmr.overflow_time, ICH9_LPCPmRegs), VMSTATE_GPE_ARRAY(gpe0.sts, ICH9_LPCPmRegs), VMSTATE_GPE_ARRAY(gpe0.en, ICH9_LPCPmRegs), VMSTATE_UINT32(smi_en, ICH9_LPCPmRegs), VMSTATE_UINT32(smi_sts, ICH9_LPCPmRegs), VMSTATE_END_OF_LIST() } }; static void pm_reset(void *opaque) { ICH9_LPCPmRegs *pm = opaque; ich9_pm_iospace_update(pm, 0); acpi_pm1_evt_reset(&pm->pm1a); acpi_pm1_cnt_reset(&pm->pm1_cnt); acpi_pm_tmr_reset(&pm->tmr); acpi_gpe_reset(&pm->gpe0); pm_update_sci(pm); } static void pm_powerdown(void *opaque, int irq, int power_failing) { ICH9_LPCPmRegs *pm = opaque; ACPIPM1EVT *pm1a = pm? &pm->pm1a: NULL; ACPIPMTimer *tmr = pm? &pm->tmr: NULL; acpi_pm1_evt_power_down(pm1a, tmr); } void ich9_pm_init(ICH9_LPCPmRegs *pm, qemu_irq sci_irq, qemu_irq cmos_s3) { acpi_pm_tmr_init(&pm->tmr, ich9_pm_update_sci_fn); acpi_pm1_cnt_init(&pm->pm1_cnt, cmos_s3); acpi_gpe_init(&pm->gpe0, ICH9_PMIO_GPE0_LEN); pm->irq = sci_irq; qemu_register_reset(pm_reset, pm); qemu_system_powerdown = *qemu_allocate_irqs(pm_powerdown, pm, 1); }
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.
On Tue, Apr 19, 2011 at 02:33:46PM +0200, Juan Quintela wrote: > 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: 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).
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 96f5222..3a8fece 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -214,10 +214,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id) { \ .name = (stringify(_field)), \ .version_id = 0, \ - .num = GPE_LEN, \ .info = &vmstate_info_uint16, \ .size = sizeof(uint16_t), \ - .flags = VMS_ARRAY | VMS_POINTER, \ + .flags = VMS_SINGLE | VMS_POINTER, \ .offset = vmstate_offset_pointer(_state, _field, uint8_t), \ }
It's vmstate parameter was wrong. This patch fixes it. Reported-by: Avi Kivity <avi@redhat.com> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/acpi_piix4.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)