* [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag"
@ 2017-03-22 12:13 Paolo Bonzini
2017-03-23 7:26 ` Peter Xu
2017-03-23 7:34 ` Pavel Dovgalyuk
0 siblings, 2 replies; 4+ messages in thread
From: Paolo Bonzini @ 2017-03-22 12:13 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, Pavel Dovgalyuk
This reverts commit 07bfa354772f2de67008dc66c201b627acff0106.
The global variable is only read as part of a
apic_reset_irq_delivered();
qemu_irq_raise(s->irq);
if (!apic_get_irq_delivered()) {
sequence, so the value never matters at migration time.
Reported-by: Dr. David Alan Gilbert <dglibert@redhat.com>
Cc: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/intc/apic_common.c | 33 ---------------------------------
include/hw/i386/apic_internal.h | 2 --
2 files changed, 35 deletions(-)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 7a6e771..c3829e3 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -387,25 +387,6 @@ static bool apic_common_sipi_needed(void *opaque)
return s->wait_for_sipi != 0;
}
-static bool apic_irq_delivered_needed(void *opaque)
-{
- APICCommonState *s = APIC_COMMON(opaque);
- return s->cpu == X86_CPU(first_cpu) && apic_irq_delivered != 0;
-}
-
-static void apic_irq_delivered_pre_save(void *opaque)
-{
- APICCommonState *s = APIC_COMMON(opaque);
- s->apic_irq_delivered = apic_irq_delivered;
-}
-
-static int apic_irq_delivered_post_load(void *opaque, int version_id)
-{
- APICCommonState *s = APIC_COMMON(opaque);
- apic_irq_delivered = s->apic_irq_delivered;
- return 0;
-}
-
static const VMStateDescription vmstate_apic_common_sipi = {
.name = "apic_sipi",
.version_id = 1,
@@ -418,19 +399,6 @@ static const VMStateDescription vmstate_apic_common_sipi = {
}
};
-static const VMStateDescription vmstate_apic_irq_delivered = {
- .name = "apic_irq_delivered",
- .version_id = 1,
- .minimum_version_id = 1,
- .needed = apic_irq_delivered_needed,
- .pre_save = apic_irq_delivered_pre_save,
- .post_load = apic_irq_delivered_post_load,
- .fields = (VMStateField[]) {
- VMSTATE_INT32(apic_irq_delivered, APICCommonState),
- VMSTATE_END_OF_LIST()
- }
-};
-
static const VMStateDescription vmstate_apic_common = {
.name = "apic",
.version_id = 3,
@@ -465,7 +433,6 @@ static const VMStateDescription vmstate_apic_common = {
},
.subsections = (const VMStateDescription*[]) {
&vmstate_apic_common_sipi,
- &vmstate_apic_irq_delivered,
NULL
}
};
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 20ad28c..1209eb4 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -189,8 +189,6 @@ struct APICCommonState {
DeviceState *vapic;
hwaddr vapic_paddr; /* note: persistence via kvmvapic */
bool legacy_instance_id;
-
- int apic_irq_delivered; /* for saving static variable */
};
typedef struct VAPICState {
--
2.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag"
2017-03-22 12:13 [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag" Paolo Bonzini
@ 2017-03-23 7:26 ` Peter Xu
2017-03-23 7:34 ` Pavel Dovgalyuk
1 sibling, 0 replies; 4+ messages in thread
From: Peter Xu @ 2017-03-23 7:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, dgilbert, Pavel Dovgalyuk
On Wed, Mar 22, 2017 at 01:13:44PM +0100, Paolo Bonzini wrote:
> This reverts commit 07bfa354772f2de67008dc66c201b627acff0106.
> The global variable is only read as part of a
>
> apic_reset_irq_delivered();
> qemu_irq_raise(s->irq);
> if (!apic_get_irq_delivered()) {
>
> sequence, so the value never matters at migration time.
>
> Reported-by: Dr. David Alan Gilbert <dglibert@redhat.com>
> Cc: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
-- peterx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag"
2017-03-22 12:13 [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag" Paolo Bonzini
2017-03-23 7:26 ` Peter Xu
@ 2017-03-23 7:34 ` Pavel Dovgalyuk
2017-03-23 11:45 ` Paolo Bonzini
1 sibling, 1 reply; 4+ messages in thread
From: Pavel Dovgalyuk @ 2017-03-23 7:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, dgilbert
This value is used by mc146818rtc.
Therefore it affects the vitrual machine state.
I've encountered the cases when replay was broken without migrating of this variable.
Отправлено с помощью BlueMail
На 22 Мар 2017 г., 15:13, в 15:13, Paolo Bonzini <pbonzini@redhat.com> написал:>This reverts commit 07bfa354772f2de67008dc66c201b627acff0106.
>The global variable is only read as part of a
>
> apic_reset_irq_delivered();
> qemu_irq_raise(s->irq);
> if (!apic_get_irq_delivered()) {
>
>sequence, so the value never matters at migration time.
>
>Reported-by: Dr. David Alan Gilbert <dglibert@redhat.com>
>Cc: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>---
> hw/intc/apic_common.c | 33 ---------------------------------
> include/hw/i386/apic_internal.h | 2 --
> 2 files changed, 35 deletions(-)
>
>diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>index 7a6e771..c3829e3 100644
>--- a/hw/intc/apic_common.c
>+++ b/hw/intc/apic_common.c
>@@ -387,25 +387,6 @@ static bool apic_common_sipi_needed(void *opaque)
> return s->wait_for_sipi != 0;
> }
>
>-static bool apic_irq_delivered_needed(void *opaque)
>-{
>- APICCommonState *s = APIC_COMMON(opaque);
>- return s->cpu == X86_CPU(first_cpu) && apic_irq_delivered != 0;
>-}
>-
>-static void apic_irq_delivered_pre_save(void *opaque)
>-{
>- APICCommonState *s = APIC_COMMON(opaque);
>- s->apic_irq_delivered = apic_irq_delivered;
>-}
>-
>-static int apic_irq_delivered_post_load(void *opaque, int version_id)
>-{
>- APICCommonState *s = APIC_COMMON(opaque);
>- apic_irq_delivered = s->apic_irq_delivered;
>- return 0;
>-}
>-
> static const VMStateDescription vmstate_apic_common_sipi = {
> .name = "apic_sipi",
> .version_id = 1,
>@@ -418,19 +399,6 @@ static const VMStateDescription
>vmstate_apic_common_sipi = {
> }
> };
>
>-static const VMStateDescription vmstate_apic_irq_delivered = {
>- .name = "apic_irq_delivered",
>- .version_id = 1,
>- .minimum_version_id = 1,
>- .needed = apic_irq_delivered_needed,
>- .pre_save = apic_irq_delivered_pre_save,
>- .post_load = apic_irq_delivered_post_load,
>- .fields = (VMStateField[]) {
>- VMSTATE_INT32(apic_irq_delivered, APICCommonState),
>- VMSTATE_END_OF_LIST()
>- }
>-};
>-
> static const VMStateDescription vmstate_apic_common = {
> .name = "apic",
> .version_id = 3,
>@@ -465,7 +433,6 @@ static const VMStateDescription vmstate_apic_common
>= {
> },
> .subsections = (const VMStateDescription*[]) {
> &vmstate_apic_common_sipi,
>- &vmstate_apic_irq_delivered,
> NULL
> }
> };
>diff --git a/include/hw/i386/apic_internal.h
>b/include/hw/i386/apic_internal.h
>index 20ad28c..1209eb4 100644
>--- a/include/hw/i386/apic_internal.h
>+++ b/include/hw/i386/apic_internal.h
>@@ -189,8 +189,6 @@ struct APICCommonState {
> DeviceState *vapic;
> hwaddr vapic_paddr; /* note: persistence via kvmvapic */
> bool legacy_instance_id;
>-
>- int apic_irq_delivered; /* for saving static variable */
> };
>
> typedef struct VAPICState {
>--
>2.9.3
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag"
2017-03-23 7:34 ` Pavel Dovgalyuk
@ 2017-03-23 11:45 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2017-03-23 11:45 UTC (permalink / raw)
To: Pavel Dovgalyuk; +Cc: qemu-devel, dgilbert
On 23/03/2017 08:34, Pavel Dovgalyuk wrote:
> This value is used by mc146818rtc.
> Therefore it affects the vitrual machine state.
It is used indeed, but it should not be used across virtual machine
migration or savevm. The value doesn't matter as soon as
apic_get_irq_delivered is called, because there will be an
apic_reset_irq_delivered call before the next access; and when migrating
or saving a virtual machine, you cannot be between the calls to
apic_reset_irq_delivered and apic_get_irq_delivered.
It would be interesting to try something like this:
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 7a6e771..10a114d 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -32,6 +32,7 @@
#include "hw/sysbus.h"
static int apic_irq_delivered;
+static bool apic_irq_delivered_valid;
bool apic_report_tpr_access;
void cpu_set_apic_base(DeviceState *dev, uint64_t val)
@@ -136,13 +137,18 @@ void apic_reset_irq_delivered(void)
volatile int a_i_d = apic_irq_delivered;
trace_apic_reset_irq_delivered(a_i_d);
+ assert(!apic_irq_delivered_valid);
apic_irq_delivered = 0;
+ apic_irq_delivered_valid = true;
}
int apic_get_irq_delivered(void)
{
trace_apic_get_irq_delivered(apic_irq_delivered);
+ assert(apic_irq_delivered_valid);
+ apic_irq_delivered_valid = false;
+
return apic_irq_delivered;
}
Paolo
> I've encountered the cases when replay was broken without migrating of
> this variable.
> Отправлено с помощью BlueMail <http://www.bluemail.me/r?b=9226>
> На 22 Мар 2017 г., в 15:13, Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> написал:
>
> This reverts commit 07bfa354772f2de67008dc66c201b627acff0106.
> The global variable is only read as part of a
>
> apic_reset_irq_delivered();
> qemu_irq_raise(s->irq);
> if (!apic_get_irq_delivered()) {
>
> sequence, so the value never matters at migration time.
>
> Reported-by: Dr. David Alan Gilbert <dglibert@redhat.com>
> Cc: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/intc/apic_common.c | 33
> ------------------------------------------------------------------------
>
> include/hw/i386/apic_internal.h | 2 --
> 2 files changed, 35 deletions(-)
>
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 7a6e771..c3829e3 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -387,25 +387,6 @@ static bool apic_common_sipi_needed(void *opaque)
> return s->wait_for_sipi != 0;
> }
>
> -static bool apic_irq_delivered_needed(void *opaque)
> -{
> - APICCommonState *s = APIC_COMMON(opaque);
> - return s->cpu == X86_CPU(first_cpu) && apic_irq_delivered != 0;
> -}
> -
> -static void apic_irq_delivered_pre_save(void *opaque)
> -{
> - APICCommonState *s = APIC_COMMON(opaque);
> - s->apic_irq_delivered = apic_irq_delivered;
> -}
> -
> -static int apic_irq_delivered_post_load(void *opaque, int version_id)
> -{
> - APICCommonState *s = APIC_COMMON(opaque);
> - apic_irq_delivered = s->apic_irq_delivered;
> - return 0;
> -}
> -
> static const VMStateDescription vmstate_apic_common_sipi = {
> .name = "apic_sipi",
> .version_id = 1,
> @@ -418,19 +399,6 @@ static const VMStateDescription vmstate_apic_common_sipi = {
> }
> };
>
> -static const VMStateDescription vmstate_apic_irq_delivered = {
> - .name = "apic_irq_delivered",
> - .version_id = 1,
> - .minimum_version_id = 1,
> - .needed = apic_irq_delivered_needed,
> - .pre_save = apic_irq_delivered_pre_save,
> - .post_load = apic_irq_delivered_post_load,
> - .fields = (VMStateField[]) {
> - VMSTATE_INT32(apic_irq_delivered, APICCommonState),
> - VMSTATE_END_OF_LIST()
> - }
> -};
> -
> static const VMStateDescription vmstate_apic_common = {
> .name = "apic",
> .version_id = 3,
> @@ -465,7 +433,6 @@ static const VMStateDescription vmstate_apic_common = {
> },
> .subsections = (const VMStateDescription*[]) {
> &vmstate_apic_common_sipi,
> - &vmstate_apic_irq_delivered,
> NULL
> }
> };
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 20ad28c..1209eb4 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -189,8 +189,6 @@ struct APICCommonState {
> DeviceState *vapic;
> hwaddr vapic_paddr; /* note: persistence via kvmvapic */
> bool legacy_instance_id;
> -
> - int apic_irq_delivered; /* for saving static variable */
> };
>
> typedef struct VAPICState {
>
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-23 11:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-22 12:13 [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag" Paolo Bonzini
2017-03-23 7:26 ` Peter Xu
2017-03-23 7:34 ` Pavel Dovgalyuk
2017-03-23 11:45 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).