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 <[email protected]>
>> ---
>> 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 <[email protected]>
Thanks.
Jan
