For partial writes the non-written parts of registers are folded into
the full 64-bit value from what they're presently set to. That's wrong
to do though when the behavior is write-1-to-clear: Writes not
including to low 3 bits would unconditionally clear all ISR bits which
are presently set. Re-calculate the value to use.

Fixes: be07023be115 ("x86/vhpet: add support for level triggered interrupts")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Alternatively we could also suppress the folding in of read bits, but
that looked to me to end up in a more intrusive change.

--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -404,7 +404,8 @@ static int cf_check hpet_write(
         break;
 
     case HPET_STATUS:
-        /* write 1 to clear. */
+        /* Write 1 to clear. Therefore don't use new_val directly here. */
+        new_val = val << ((addr & 7) * 8);
         while ( new_val )
         {
             bool active;

On 28/08/2024 10:00 am, Jan Beulich wrote:
> For partial writes the non-written parts of registers are folded into
> the full 64-bit value from what they're presently set to. That's wrong
> to do though when the behavior is write-1-to-clear: Writes not
> including to low 3 bits would unconditionally clear all ISR bits which
> are presently set. Re-calculate the value to use.
>
> Fixes: be07023be115 ("x86/vhpet: add support for level triggered interrupts")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Alternatively we could also suppress the folding in of read bits, but
> that looked to me to end up in a more intrusive change.
>
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -404,7 +404,8 @@ static int cf_check hpet_write(
>          break;
>  
>      case HPET_STATUS:
> -        /* write 1 to clear. */
> +        /* Write 1 to clear. Therefore don't use new_val directly here. */
> +        new_val = val << ((addr & 7) * 8);
>          while ( new_val )
>          {
>              bool active;

It's sad that we allow any sub-word accesses.  The spec makes it pretty
clear that only aligned 32-bit and 64-bit accesses are acceptable, and
it would simplify the merging logic substantially.

Nevertheless, this is the simplest bugfix for now.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

On 28.08.2024 13:06, Andrew Cooper wrote:
> On 28/08/2024 10:00 am, Jan Beulich wrote:
>> For partial writes the non-written parts of registers are folded into
>> the full 64-bit value from what they're presently set to. That's wrong
>> to do though when the behavior is write-1-to-clear: Writes not
>> including to low 3 bits would unconditionally clear all ISR bits which
>> are presently set. Re-calculate the value to use.
>>
>> Fixes: be07023be115 ("x86/vhpet: add support for level triggered interrupts")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Alternatively we could also suppress the folding in of read bits, but
>> that looked to me to end up in a more intrusive change.
>>
>> --- a/xen/arch/x86/hvm/hpet.c
>> +++ b/xen/arch/x86/hvm/hpet.c
>> @@ -404,7 +404,8 @@ static int cf_check hpet_write(
>>          break;
>>  
>>      case HPET_STATUS:
>> -        /* write 1 to clear. */
>> +        /* Write 1 to clear. Therefore don't use new_val directly here. */
>> +        new_val = val << ((addr & 7) * 8);
>>          while ( new_val )
>>          {
>>              bool active;
> 
> It's sad that we allow any sub-word accesses.  The spec makes it pretty
> clear that only aligned 32-bit and 64-bit accesses are acceptable, and
> it would simplify the merging logic substantially.

While that's true, the bug here would still have been there if we limited
emulation to 32- and 64-bit accesses.

> Nevertheless, this is the simplest bugfix for now.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan