* [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle
@ 2017-11-08 19:46 Eric Chanudet
2017-11-09 8:55 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Eric Chanudet @ 2017-11-08 19:46 UTC (permalink / raw
To: xen-devel@lists.xen.org
Cc: andrew.cooper3@citrix.com, julien.grall@linaro.org,
jbeulich@suse.com
Do it once at domain creation (hpet_init).
Sleep -> Resume cycles will end up crashing an HVM guest with hpet as
the sequence during resume takes the path:
-> hvm_s3_suspend
-> hpet_reset
-> hpet_deinit
-> hpet_init
-> register_mmio_handler
-> hvm_next_io_handler
register_mmio_handler will use a new io handler each time, until
eventually it reaches NR_IO_HANDLERS, then hvm_next_io_handler calls
domain_crash.
Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
---
v2:
* make hpet_reinit static inline (one call site in this file)
* remove single use local variable.
---
xen/arch/x86/hvm/hpet.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 3ea895a0fb..d8c61ca155 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -635,14 +635,10 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
HVM_REGISTER_SAVE_RESTORE(HPET, hpet_save, hpet_load, 1, HVMSR_PER_DOM);
-void hpet_init(struct domain *d)
+static void hpet_set(HPETState *h)
{
- HPETState *h = domain_vhpet(d);
int i;
- if ( !has_vhpet(d) )
- return;
-
memset(h, 0, sizeof(HPETState));
rwlock_init(&h->lock);
@@ -668,11 +664,26 @@ void hpet_init(struct domain *d)
h->hpet.comparator64[i] = ~0ULL;
h->pt[i].source = PTSRC_isa;
}
+}
+void hpet_init(struct domain *d)
+{
+ if ( !has_vhpet(d) )
+ return;
+
+ hpet_set(domain_vhpet(d));
register_mmio_handler(d, &hpet_mmio_ops);
d->arch.hvm_domain.params[HVM_PARAM_HPET_ENABLED] = 1;
}
+static inline void hpet_reinit(struct domain *d)
+{
+ if ( !has_vhpet(d) )
+ return;
+
+ hpet_set(domain_vhpet(d));
+}
+
void hpet_deinit(struct domain *d)
{
int i;
@@ -698,7 +709,7 @@ void hpet_deinit(struct domain *d)
void hpet_reset(struct domain *d)
{
hpet_deinit(d);
- hpet_init(d);
+ hpet_reinit(d);
}
/*
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle
2017-11-08 19:46 [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle Eric Chanudet
@ 2017-11-09 8:55 ` Jan Beulich
2017-11-09 14:42 ` Julien Grall
2017-11-09 23:09 ` Eric Chanudet
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2017-11-09 8:55 UTC (permalink / raw
To: Eric Chanudet
Cc: andrew.cooper3@citrix.com, julien.grall@linaro.org,
xen-devel@lists.xen.org
>>> On 08.11.17 at 20:46, <chanudete@ainfosec.com> wrote:
> Do it once at domain creation (hpet_init).
>
> Sleep -> Resume cycles will end up crashing an HVM guest with hpet as
> the sequence during resume takes the path:
> -> hvm_s3_suspend
> -> hpet_reset
> -> hpet_deinit
> -> hpet_init
> -> register_mmio_handler
> -> hvm_next_io_handler
>
> register_mmio_handler will use a new io handler each time, until
> eventually it reaches NR_IO_HANDLERS, then hvm_next_io_handler calls
> domain_crash.
>
> Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
>
> ---
> v2:
> * make hpet_reinit static inline (one call site in this file)
Perhaps my prior reply was ambiguous: By "inlining" I meant
literally inlining it (i.e. dropping the standalone function
altogether). Static functions outside of header files should not
normally be marked "inline" explicitly - it should be the compiler
to make that decision.
As doing the adjustment it relatively simple, I wouldn't mind
doing so while committing, saving another round trip. With
that adjustment (or at the very least with the "inline" dropped)
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle
2017-11-09 8:55 ` Jan Beulich
@ 2017-11-09 14:42 ` Julien Grall
2017-11-09 14:45 ` Jan Beulich
2017-11-09 23:09 ` Eric Chanudet
1 sibling, 1 reply; 6+ messages in thread
From: Julien Grall @ 2017-11-09 14:42 UTC (permalink / raw
To: Jan Beulich, Eric Chanudet
Cc: andrew.cooper3@citrix.com, xen-devel@lists.xen.org
Hi,
On 09/11/17 08:55, Jan Beulich wrote:
>>>> On 08.11.17 at 20:46, <chanudete@ainfosec.com> wrote:
>> Do it once at domain creation (hpet_init).
>>
>> Sleep -> Resume cycles will end up crashing an HVM guest with hpet as
>> the sequence during resume takes the path:
>> -> hvm_s3_suspend
>> -> hpet_reset
>> -> hpet_deinit
>> -> hpet_init
>> -> register_mmio_handler
>> -> hvm_next_io_handler
>>
>> register_mmio_handler will use a new io handler each time, until
>> eventually it reaches NR_IO_HANDLERS, then hvm_next_io_handler calls
>> domain_crash.
>>
>> Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
>>
>> ---
>> v2:
>> * make hpet_reinit static inline (one call site in this file)
>
> Perhaps my prior reply was ambiguous: By "inlining" I meant
> literally inlining it (i.e. dropping the standalone function
> altogether). Static functions outside of header files should not
> normally be marked "inline" explicitly - it should be the compiler
> to make that decision.
>
> As doing the adjustment it relatively simple, I wouldn't mind
> doing so while committing, saving another round trip. With
> that adjustment (or at the very least with the "inline" dropped)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
What would be the risk to get this patch in Xen 4.10?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle
2017-11-09 14:42 ` Julien Grall
@ 2017-11-09 14:45 ` Jan Beulich
2017-11-13 16:27 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-11-09 14:45 UTC (permalink / raw
To: Julien Grall
Cc: Eric Chanudet, andrew.cooper3@citrix.com, xen-devel@lists.xen.org
>>> On 09.11.17 at 15:42, <julien.grall@linaro.org> wrote:
> Hi,
>
> On 09/11/17 08:55, Jan Beulich wrote:
>>>>> On 08.11.17 at 20:46, <chanudete@ainfosec.com> wrote:
>>> Do it once at domain creation (hpet_init).
>>>
>>> Sleep -> Resume cycles will end up crashing an HVM guest with hpet as
>>> the sequence during resume takes the path:
>>> -> hvm_s3_suspend
>>> -> hpet_reset
>>> -> hpet_deinit
>>> -> hpet_init
>>> -> register_mmio_handler
>>> -> hvm_next_io_handler
>>>
>>> register_mmio_handler will use a new io handler each time, until
>>> eventually it reaches NR_IO_HANDLERS, then hvm_next_io_handler calls
>>> domain_crash.
>>>
>>> Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
>>>
>>> ---
>>> v2:
>>> * make hpet_reinit static inline (one call site in this file)
>>
>> Perhaps my prior reply was ambiguous: By "inlining" I meant
>> literally inlining it (i.e. dropping the standalone function
>> altogether). Static functions outside of header files should not
>> normally be marked "inline" explicitly - it should be the compiler
>> to make that decision.
>>
>> As doing the adjustment it relatively simple, I wouldn't mind
>> doing so while committing, saving another round trip. With
>> that adjustment (or at the very least with the "inline" dropped)
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> What would be the risk to get this patch in Xen 4.10?
Close to none, I would say. Of course, if there really was
something wrong with the code restructuring to fix the bug,
basically all HVM guests would be hosed HPET-wise.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle
2017-11-09 8:55 ` Jan Beulich
2017-11-09 14:42 ` Julien Grall
@ 2017-11-09 23:09 ` Eric Chanudet
1 sibling, 0 replies; 6+ messages in thread
From: Eric Chanudet @ 2017-11-09 23:09 UTC (permalink / raw
To: Jan Beulich
Cc: andrew.cooper3@citrix.com, julien.grall@linaro.org,
xen-devel@lists.xen.org
On 09/11/17 at 01:55am, Jan Beulich wrote:
>Perhaps my prior reply was ambiguous: By "inlining" I meant literally
>inlining it
My apologies for the clumsiness, I can resend a v3 right away.
Thanks,
--
Eric Chanudet
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle
2017-11-09 14:45 ` Jan Beulich
@ 2017-11-13 16:27 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2017-11-13 16:27 UTC (permalink / raw
To: Jan Beulich
Cc: Eric Chanudet, andrew.cooper3@citrix.com, xen-devel@lists.xen.org
Hi Jan,
On 11/09/2017 02:45 PM, Jan Beulich wrote:
>>>> On 09.11.17 at 15:42, <julien.grall@linaro.org> wrote:
>> Hi,
>>
>> On 09/11/17 08:55, Jan Beulich wrote:
>>>>>> On 08.11.17 at 20:46, <chanudete@ainfosec.com> wrote:
>>>> Do it once at domain creation (hpet_init).
>>>>
>>>> Sleep -> Resume cycles will end up crashing an HVM guest with hpet as
>>>> the sequence during resume takes the path:
>>>> -> hvm_s3_suspend
>>>> -> hpet_reset
>>>> -> hpet_deinit
>>>> -> hpet_init
>>>> -> register_mmio_handler
>>>> -> hvm_next_io_handler
>>>>
>>>> register_mmio_handler will use a new io handler each time, until
>>>> eventually it reaches NR_IO_HANDLERS, then hvm_next_io_handler calls
>>>> domain_crash.
>>>>
>>>> Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
>>>>
>>>> ---
>>>> v2:
>>>> * make hpet_reinit static inline (one call site in this file)
>>>
>>> Perhaps my prior reply was ambiguous: By "inlining" I meant
>>> literally inlining it (i.e. dropping the standalone function
>>> altogether). Static functions outside of header files should not
>>> normally be marked "inline" explicitly - it should be the compiler
>>> to make that decision.
>>>
>>> As doing the adjustment it relatively simple, I wouldn't mind
>>> doing so while committing, saving another round trip. With
>>> that adjustment (or at the very least with the "inline" dropped)
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> What would be the risk to get this patch in Xen 4.10?
>
> Close to none, I would say. Of course, if there really was
> something wrong with the code restructuring to fix the bug,
> basically all HVM guests would be hosed HPET-wise.
On that basis:
Release-acked-by: Julien Grall <julien.grall@linaro.org>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-13 16:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-08 19:46 [PATCH v2] x86/hvm: do not register hpet mmio during s3 cycle Eric Chanudet
2017-11-09 8:55 ` Jan Beulich
2017-11-09 14:42 ` Julien Grall
2017-11-09 14:45 ` Jan Beulich
2017-11-13 16:27 ` Julien Grall
2017-11-09 23:09 ` Eric Chanudet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.