Re: [PATCH] x86/HVM: correct partial HPET_STATUS write emulation

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>

Thanks.

Jan