mbox series

[RFC,0/2] hw/ide/ahci: Delay a bit before completing reset

Message ID 20250526180558.65613-1-philmd@linaro.org
Headers show
Series hw/ide/ahci: Delay a bit before completing reset | expand

Message

Philippe Mathieu-Daudé May 26, 2025, 6:05 p.m. UTC 
Intented to help SeaBIOS development; untested there
(except with QEMU test suite).

Jiaxun, is it helpful to you?

Philippe Mathieu-Daudé (2):
  hw/ide/ahci: Introduce ahci_reset_delayed() and ahci_reset_complete()
  hw/ide/ahci: Delay a bit before completing reset

 include/hw/ide/ahci.h   |  1 +
 hw/ide/ahci.c           | 70 ++++++++++++++++++++++++++++++++++++++---
 tests/qtest/ahci-test.c |  4 +++
 hw/ide/trace-events     |  1 +
 4 files changed, 71 insertions(+), 5 deletions(-)

Comments

Jiaxun Yang May 26, 2025, 6:25 p.m. UTC | #1 
在2025年5月26日周一 下午7:05，Philippe Mathieu-Daudé写道：
> Intented to help SeaBIOS development; untested there
> (except with QEMU test suite).
>
> Jiaxun, is it helpful to you?


Hi Philippe,

Thanks for the proposal!

The spec says:

```
HBA Reset (HR): When set by SW, this bit causes an internal reset of the HBA. All
state machines that relate to data transfers and queuing shall return to an idle
condition, and all ports shall be re-initialized via COMRESET (if staggered spin-up is
not supported). If staggered spin-up is supported, then it is the responsibility of
software to spin-up each port after the reset has completed.

When the HBA has performed the reset action, it shall reset this bit to ‘0’. A software
write of ‘0’ shall have no effect. For a description on which bits are reset when this bit is
set, see section 10.4.3.
```

I do believe QEMU's current implementation is also in conformance to the spec,
as the reset process itself is done instantly in QEMU.

I don't know if it's worth it to introduce extra complexity in QEMU to model
a very specific hardware behaviour. Even some hardware is working in QEMU's way.

[...]

Thanks
Gerd Hoffmann May 27, 2025, 10:45 a.m. UTC | #2 
Hi,

> I do believe QEMU's current implementation is also in conformance to the spec,
> as the reset process itself is done instantly in QEMU.

Yes, that is fine spec-wise.  The problem is the seabios driver which
doesn't wait until the hardware signals completion.

> I don't know if it's worth it to introduce extra complexity in QEMU to model
> a very specific hardware behaviour. Even some hardware is working in QEMU's way.

We have that kind of differences between virtual and physical hardware
in other places too.  Timing is notoriously difficult to emulate.  Often
qemu completes hardware actions faster than physical hardware, or it at
least looks that way to the guest because it does not get CPU time until
qemu is done.

One way to change that would be to have all mmio writes return instantly
and only kick a timer or BH which runs the actual action.  I'm not
convinced this is worth the effort.  If you think it is I'd suggest to
move the complete driver to that mode of operation instead of adding a
reset tweak only.

take care,
  Gerd
Philippe Mathieu-Daudé May 27, 2025, 2 p.m. UTC | #3 
On 27/5/25 12:45, Gerd Hoffmann wrote:
>    Hi,
> 
>> I do believe QEMU's current implementation is also in conformance to the spec,
>> as the reset process itself is done instantly in QEMU.
> 
> Yes, that is fine spec-wise.  The problem is the seabios driver which
> doesn't wait until the hardware signals completion.
> 
>> I don't know if it's worth it to introduce extra complexity in QEMU to model
>> a very specific hardware behaviour. Even some hardware is working in QEMU's way.
> 
> We have that kind of differences between virtual and physical hardware
> in other places too.  Timing is notoriously difficult to emulate.  Often
> qemu completes hardware actions faster than physical hardware, or it at
> least looks that way to the guest because it does not get CPU time until
> qemu is done.
> 
> One way to change that would be to have all mmio writes return instantly
> and only kick a timer or BH which runs the actual action.  I'm not
> convinced this is worth the effort.  If you think it is I'd suggest to
> move the complete driver to that mode of operation instead of adding a
> reset tweak only.

Nah I'm fine ;)