[PATCH] hvm/hpet: Correctly gate the virtual HPET on HVM_PARAM_HPET_ENABLE

[PATCH] hvm/hpet: Correctly gate the virtual HPET on HVM_PARAM_HPET_ENABLE

[Andrew Cooper]

c/s 3f8e22de7 "x86 hvm: Allow HPET to be configured as a per-domain config
option" introduced the parameter to conditionally enable the HPET.

However, having the check in hpet_range() does not have the intended effect.
As currently implemented, when the HPET is disabled, the range is not claimed
and an ioreq is forwarded to qemu, which implements an HPET itself.

Properly disable the HPET by always claiming the range, dropping writes and
reading ~0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---

This patch has been in XenServer for a couple of releases now, although
disabling the HPET is not a common action to take and is only really useful
for debugging purposes.
---
 xen/arch/x86/hvm/hpet.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index bdfc6fc..d898169 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -173,6 +173,12 @@ static int hpet_read(
     unsigned long result;
     uint64_t val;
 
+    if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_HPET_ENABLED] )
+    {
+        result = ~0ul;
+        goto out;
+    }
+
     addr &= HPET_MMAP_SIZE-1;
 
     if ( hpet_check_access_length(addr, length) != 0 )
@@ -309,6 +315,9 @@ static int hpet_write(
 #define set_start_timer(n)   (__set_bit((n), &start_timers))
 #define set_restart_timer(n) (set_stop_timer(n),set_start_timer(n))
 
+    if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_HPET_ENABLED] )
+        goto out;
+
     addr &= HPET_MMAP_SIZE-1;
 
     if ( hpet_check_access_length(addr, length) != 0 )
@@ -491,9 +500,8 @@ static int hpet_write(
 
 static int hpet_range(struct vcpu *v, unsigned long addr)
 {
-    return (v->domain->arch.hvm_domain.params[HVM_PARAM_HPET_ENABLED] &&
-            (addr >= HPET_BASE_ADDRESS) &&
-            (addr < (HPET_BASE_ADDRESS + HPET_MMAP_SIZE)));
+    return ( (addr >= HPET_BASE_ADDRESS) &&
+             (addr < (HPET_BASE_ADDRESS + HPET_MMAP_SIZE)) );
 }
 
 const struct hvm_mmio_handler hpet_mmio_handler = {
-- 
1.7.10.4

Re: [PATCH] hvm/hpet: Correctly gate the virtual HPET on HVM_PARAM_HPET_ENABLE

[Jan Beulich]

>>> On 15.01.15 at 15:40, <andrew.cooper3@citrix.com> wrote:
> c/s 3f8e22de7 "x86 hvm: Allow HPET to be configured as a per-domain config
> option" introduced the parameter to conditionally enable the HPET.
> 
> However, having the check in hpet_range() does not have the intended effect.
> As currently implemented, when the HPET is disabled, the range is not 
> claimed
> and an ioreq is forwarded to qemu, which implements an HPET itself.
> 
> Properly disable the HPET by always claiming the range, dropping writes and
> reading ~0.

Hmm, while the patch certainly does what you describe above, is that
really correct? There could be something else at that address when
the HPET is disabled. Therefore I would rather think that qemu should
be told to also not make a HPET available when the option is off.

Jan

Re: [PATCH] hvm/hpet: Correctly gate the virtual HPET on HVM_PARAM_HPET_ENABLE

[Andrew Cooper]

On 15/01/15 15:34, Jan Beulich wrote:
>>>> On 15.01.15 at 15:40, <andrew.cooper3@citrix.com> wrote:
>> c/s 3f8e22de7 "x86 hvm: Allow HPET to be configured as a per-domain config
>> option" introduced the parameter to conditionally enable the HPET.
>>
>> However, having the check in hpet_range() does not have the intended effect.
>> As currently implemented, when the HPET is disabled, the range is not 
>> claimed
>> and an ioreq is forwarded to qemu, which implements an HPET itself.
>>
>> Properly disable the HPET by always claiming the range, dropping writes and
>> reading ~0.
> Hmm, while the patch certainly does what you describe above, is that
> really correct? There could be something else at that address when
> the HPET is disabled. Therefore I would rather think that qemu should
> be told to also not make a HPET available when the option is off.

Without out wiring up northbridge chipset reigsters from Qemu to Xen to
control where components such as the HPET appear, I think we can
reasonably assume that there will be nothing there.

Whether the HPET is present or not, the range ought to reserved in the
E820 and not free for arbitrary reuse by the OS.

~Andrew

Re: [PATCH] hvm/hpet: Correctly gate the virtual HPET on HVM_PARAM_HPET_ENABLE

[Jan Beulich]

>>> On 15.01.15 at 16:49, <andrew.cooper3@citrix.com> wrote:
> On 15/01/15 15:34, Jan Beulich wrote:
>>>>> On 15.01.15 at 15:40, <andrew.cooper3@citrix.com> wrote:
>>> c/s 3f8e22de7 "x86 hvm: Allow HPET to be configured as a per-domain config
>>> option" introduced the parameter to conditionally enable the HPET.
>>>
>>> However, having the check in hpet_range() does not have the intended effect.
>>> As currently implemented, when the HPET is disabled, the range is not 
>>> claimed
>>> and an ioreq is forwarded to qemu, which implements an HPET itself.
>>>
>>> Properly disable the HPET by always claiming the range, dropping writes and
>>> reading ~0.
>> Hmm, while the patch certainly does what you describe above, is that
>> really correct? There could be something else at that address when
>> the HPET is disabled. Therefore I would rather think that qemu should
>> be told to also not make a HPET available when the option is off.
> 
> Without out wiring up northbridge chipset reigsters from Qemu to Xen to
> control where components such as the HPET appear, I think we can
> reasonably assume that there will be nothing there.
> 
> Whether the HPET is present or not, the range ought to reserved in the
> E820 and not free for arbitrary reuse by the OS.

"assume" and "should" are kind of weak. Just having checked two
arbitrary physical machines - one reserves the HPET area in E820,
the other doesn't. But since hvmloader correctly reserves the
necessary (or actually a larger) area, I think I'm fine with the
change.

Jan