| Message ID | 20140826071519.1672.54843.stgit@PASHA-ISP |
|---|---|
| State | New |
| Headers | show |
Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto: > This patch adds virtual clock-dependent timers to VMState to allow correct > saving and restoring the state of RTL8139 network controller. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > hw/net/rtl8139.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 files changed, 48 insertions(+), 2 deletions(-) Again, this is only needed in your record/replay system (and you haven't yet quite explained why the design has this limitation), so it should not be a part of this series. I'll review the other patches separately, no need to resubmit. Paolo
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini > Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto: > > This patch adds virtual clock-dependent timers to VMState to allow correct > > saving and restoring the state of RTL8139 network controller. > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > --- > > hw/net/rtl8139.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 files changed, 48 insertions(+), 2 deletions(-) > > Again, this is only needed in your record/replay system (and you haven't > yet quite explained why the design has this limitation), so it should > not be a part of this series. I see. Updating s->timer and s->TimerExpire in post_load will be enough? Pavel Dovgalyuk
Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto: >> > Again, this is only needed in your record/replay system (and you haven't >> > yet quite explained why the design has this limitation), so it should >> > not be a part of this series. > I see. Updating s->timer and s->TimerExpire in post_load will be enough? In theory it should be done already, I guess it's not deterministic enough though. Have you tried my patch to rewrite the timer stuff? Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto: > >> > Again, this is only needed in your record/replay system (and you haven't > >> > yet quite explained why the design has this limitation), so it should > >> > not be a part of this series. > > I see. Updating s->timer and s->TimerExpire in post_load will be enough? > > In theory it should be done already, I guess it's not deterministic > enough though. I split existing function into the two parts: one sets irq (and calls another). And the second part sets only timer and TimerExpire fields. The second function is also called from post_load. > Have you tried my patch to rewrite the timer stuff? What patch do you mean? Pavel Dovgalyuk
Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto: >>>>> Again, this is only needed in your record/replay system (and you haven't >>>>> yet quite explained why the design has this limitation), so it should >>>>> not be a part of this series. >>> I see. Updating s->timer and s->TimerExpire in post_load will be enough? >> >> In theory it should be done already, I guess it's not deterministic >> enough though. > > I split existing function into the two parts: one sets irq (and calls another). > And the second part sets only timer and TimerExpire fields. The second function > is also called from post_load. > >> Have you tried my patch to rewrite the timer stuff? > > What patch do you mean? This one: http://article.gmane.org/gmane.comp.emulators.qemu/288521 Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto: > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto: > >>>>> Again, this is only needed in your record/replay system (and you haven't > >>>>> yet quite explained why the design has this limitation), so it should > >>>>> not be a part of this series. > >>> I see. Updating s->timer and s->TimerExpire in post_load will be enough? > >> > >> In theory it should be done already, I guess it's not deterministic > >> enough though. > > > > I split existing function into the two parts: one sets irq (and calls another). > > And the second part sets only timer and TimerExpire fields. The second function > > is also called from post_load. > > > >> Have you tried my patch to rewrite the timer stuff? > > > > What patch do you mean? > > This one: > > http://article.gmane.org/gmane.comp.emulators.qemu/288521 It will solve the problem, because it removes raising an irq from rtl8139_set_next_tctr_time function. Ok then, I'll remove my rtl8139 patch from the series. Pavel Dovgalyuk
Il 27/08/2014 12:48, Pavel Dovgaluk ha scritto: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto: >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>>> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto: >>>>>>> Again, this is only needed in your record/replay system (and you haven't >>>>>>> yet quite explained why the design has this limitation), so it should >>>>>>> not be a part of this series. >>>>> I see. Updating s->timer and s->TimerExpire in post_load will be enough? >>>> >>>> In theory it should be done already, I guess it's not deterministic >>>> enough though. >>> >>> I split existing function into the two parts: one sets irq (and calls another). >>> And the second part sets only timer and TimerExpire fields. The second function >>> is also called from post_load. >>> >>>> Have you tried my patch to rewrite the timer stuff? >>> >>> What patch do you mean? >> >> This one: >> >> http://article.gmane.org/gmane.comp.emulators.qemu/288521 > > It will solve the problem, because it removes raising an irq from > rtl8139_set_next_tctr_time function. > > Ok then, I'll remove my rtl8139 patch from the series. Please include that patch in your record/replay work though. I'm not sure if I'll have time to test it and submit it. Paolo
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini > Il 27/08/2014 12:48, Pavel Dovgaluk ha scritto: > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >> Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto: > >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >>>> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto: > >>>>>>> Again, this is only needed in your record/replay system (and you haven't > >>>>>>> yet quite explained why the design has this limitation), so it should > >>>>>>> not be a part of this series. > >>>>> I see. Updating s->timer and s->TimerExpire in post_load will be enough? > >>>> > >>>> In theory it should be done already, I guess it's not deterministic > >>>> enough though. > >>> > >>> I split existing function into the two parts: one sets irq (and calls another). > >>> And the second part sets only timer and TimerExpire fields. The second function > >>> is also called from post_load. > >>> > >>>> Have you tried my patch to rewrite the timer stuff? > >>> > >>> What patch do you mean? > >> > >> This one: > >> > >> http://article.gmane.org/gmane.comp.emulators.qemu/288521 > > > > It will solve the problem, because it removes raising an irq from > > rtl8139_set_next_tctr_time function. > > > > Ok then, I'll remove my rtl8139 patch from the series. > > Please include that patch in your record/replay work though. I'm not > sure if I'll have time to test it and submit it. Ok. Now I've fixed all issues and ready to submit the second version. Will you review any of the other patches from the series? Pavel Dovgalyuk
Il 28/08/2014 10:31, Pavel Dovgaluk ha scritto: >> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini >> Il 27/08/2014 12:48, Pavel Dovgaluk ha scritto: >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>>> Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto: >>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>>>>> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto: >>>>>>>>> Again, this is only needed in your record/replay system (and you haven't >>>>>>>>> yet quite explained why the design has this limitation), so it should >>>>>>>>> not be a part of this series. >>>>>>> I see. Updating s->timer and s->TimerExpire in post_load will be enough? >>>>>> >>>>>> In theory it should be done already, I guess it's not deterministic >>>>>> enough though. >>>>> >>>>> I split existing function into the two parts: one sets irq (and calls another). >>>>> And the second part sets only timer and TimerExpire fields. The second function >>>>> is also called from post_load. >>>>> >>>>>> Have you tried my patch to rewrite the timer stuff? >>>>> >>>>> What patch do you mean? >>>> >>>> This one: >>>> >>>> http://article.gmane.org/gmane.comp.emulators.qemu/288521 >>> >>> It will solve the problem, because it removes raising an irq from >>> rtl8139_set_next_tctr_time function. >>> >>> Ok then, I'll remove my rtl8139 patch from the series. >> >> Please include that patch in your record/replay work though. I'm not >> sure if I'll have time to test it and submit it. > > Ok. Now I've fixed all issues and ready to submit the second version. > Will you review any of the other patches from the series? I reviewed all x86 patches. The ARM patches seem okay at first glance. I also noticed a couple serial port issues not related to migration, but we can handle them separately. Paolo
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 6e59f38..cc2f705 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -3246,10 +3246,17 @@ static uint32_t rtl8139_mmio_readl(void *opaque, hwaddr addr) return val; } +static int rtl8139_pre_load(void *opaque) +{ + RTL8139State *s = opaque; + s->TimerExpire = 0; + timer_del(s->timer); + return 0; +} + static int rtl8139_post_load(void *opaque, int version_id) { RTL8139State* s = opaque; - rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); if (version_id < 4) { s->cplus_enabled = s->CpCmd != 0; } @@ -3275,6 +3282,38 @@ static const VMStateDescription vmstate_rtl8139_hotplug_ready ={ } }; +static bool rtl8139_TimerExpire_needed(void *opaque) +{ + RTL8139State *s = (RTL8139State *)opaque; + return s->TimerExpire != 0; +} + +static const VMStateDescription vmstate_rtl8139_TimerExpire = { + .name = "rtl8139/TimerExpire", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_INT64(TimerExpire, RTL8139State), + VMSTATE_END_OF_LIST() + } +}; + +static bool rtl8139_timer_needed(void *opaque) +{ + RTL8139State *s = (RTL8139State *)opaque; + return timer_pending(s->timer); +} + +static const VMStateDescription vmstate_rtl8139_timer = { + .name = "rtl8139/timer", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_TIMER(timer, RTL8139State), + VMSTATE_END_OF_LIST() + } +}; + static void rtl8139_pre_save(void *opaque) { RTL8139State* s = opaque; @@ -3289,8 +3328,9 @@ static void rtl8139_pre_save(void *opaque) static const VMStateDescription vmstate_rtl8139 = { .name = "rtl8139", - .version_id = 4, + .version_id = 5, .minimum_version_id = 3, + .pre_load = rtl8139_pre_load, .post_load = rtl8139_post_load, .pre_save = rtl8139_pre_save, .fields = (VMStateField[]) { @@ -3371,6 +3411,12 @@ static const VMStateDescription vmstate_rtl8139 = { .vmsd = &vmstate_rtl8139_hotplug_ready, .needed = rtl8139_hotplug_ready_needed, }, { + .vmsd = &vmstate_rtl8139_TimerExpire, + .needed = rtl8139_TimerExpire_needed, + }, { + .vmsd = &vmstate_rtl8139_timer, + .needed = rtl8139_timer_needed, + }, { /* empty */ } }
This patch adds virtual clock-dependent timers to VMState to allow correct saving and restoring the state of RTL8139 network controller. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- hw/net/rtl8139.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 48 insertions(+), 2 deletions(-)