| Message ID | da785047-0bee-4d16-a6ae-d1727bd8317a@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/HVM: correct partial HPET_STATUS write emulation | expand |
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
--- 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;
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.