Re: [Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[John Snow <jsnow@redhat.com>]

PING for this refactor series, which will allow me to check in more AHCI 
tests. To assist review, I'll annotate this cover letter a little bit 
with some additional information.

The hardest/ugliest patch to review is the first one, but it is 
primarily just variable name/location changes and nothing substantial.

The subsequent patches are all fairly small in scope and mostly 
mechanical refactors, but do include some enhanced functionality beyond 
the scope of what the identify test requires, but will be useful later 
for additional tests.

The primary purpose of this series is to break up the monolithic 
identify test into smaller parts that allow it to be easily recycled for 
testing other ATA commands in the future.

(And by "in the future" I mean "I have a lot of test cases on my hard 
drive that I'd like to submit, but they use these re-factored helper 
commands.")

On 09/18/2014 07:43 PM, John Snow wrote:
> The original version of the AHCI test base
> which is now staged for being merged, processes
> the ahci_identify test in a monolithic fashion.
>
> In authoring new tests, it became necessary and
> obvious as to how the operation of this device
> should be factored out to ease the writing of
> new AHCI tests.
>
> This patch set issues the necessary refactorings
> to support future test development for AHCI.
>
> This patch set DOES NOT account for any new fixes
> and requires no fixes from my "AHCI fixes" RFC
> in order to run successfully on 2014-09-18's
> origin/master.
>
> This patch set does not alter the operation of the
> existing test, or add new tests. It only offers
> refactorings for future patch submissions which
> depend on them, but are still under consideration.
>
> John Snow (15):
>    qtest/ahci: Add AHCIState structure

This patch adds a state structure that the test uses to track various 
fields and information within the AHCI device during the run of the 
test, so we don't have to keep re-querying and re-verifying the sanity 
of various pointer fields stored in guest memory.

Capabilities, pointers, and other metadata are now stored within this 
AHCIState object.

Many function declarations are changed from accepting a QPCIDevice 
and/or a pointer to the HBA configuration space to just accepting a 
pointer to the new state object. Many macros are changed to operate on 
this object.

It's mostly just a lot of churn without much functional difference, but 
it keeps the code in future tests a little nicer, and the object is 
managed in a functional way in order to prevent state differences from 
impacting the results of subsequent test runs.

>    qtest/ahci: Add port_select helper

Factor out the loop that finds the first used port.

>    qtest/ahci: Add port_clear helper

Factor out the code that resets a port's state back to a known value.

>    qtest/ahci: Add command header helpers

-Compress 32bit upper/lower fields into single 64bit address fields.
-Add get_command_header, set_command_header and destroy_command as 
helpers to help retrieve the command information stored within the 
command list buffer (CLB)
-Add pick_cmd which picks a command slot (0-31) to use for a new command 
being built. pick_cmd attempts to cycle through all of the command slots 
instead of re-using a previous slot, to mimic real AHCI behavior.

>    qtest/ahci: Add build cmd table helper

build_cmd_table is added as a helper that builds the actual command that 
is pointed to by the command header structure -- the CTBA (Command Table 
Base Address.) Much of the logic of building the command table is ripped 
out of the identify test case and isolated within this helper.

>    qtest/ahci: Add link_cmd_slot helper

Generate a Command Header for use in the CLB from a given Table pointer, 
then commit the new header to the CLB. This command "links" the command 
list and the command table. The chosen command slot is returned.

>    qtest/ahci: Add port_check_error helper

Common error flags in the registers and FIS are checked in a separate 
function that can now be recycled.

>    qtest/ahci: Add issue_command helper

The code responsible for actually issuing a command once it is in-place 
is factored out into its own function.

>    qtest/ahci: Add port_check_interrupts helper

This helper is a shorthand for confirming that the interrupt status of a 
specific port is what we expect it to be; then clears the interrupt.

>    qtest/ahci: Add port_check_nonbusy helper

Simply asserts that the state of the port is consistent with what we 
expect a port who is ready to receive another command would be. This is 
an addition instead of a refactor.

>    qtest/ahci: Add cmd response sanity check helpers

- Fields in the FIS structure are renamed to be a little clearer.
- A PIO Setup FIS structure is added.
- Port_Check_D2H_Sanity inspects a D2H Register FIS from the device and 
makes sure it looks appropriate.
- Port_Check_PIO_Sanity does the same for a PIO Setup FIS.
- Port_Check_CMD_Sanity primarily checks the byte count response that is 
stored within the command header to ensure it matches what we think it 
should be (the full size of the transfer.)


All of the patches below this point are even smaller "niceness" fixes 
that don't really refactor anything at all:


>    qtest/ahci: Enforce zero-leaks for guest mem usage

Utilize extra flags to the PC Alloc layer in libqos to add an assertion 
if we leak any memory. This is mostly for debugging niceness to prove 
that we are not dropping any guest memory in the various linked AHCI 
structures. It also served as a POC for the new linked-list malloc 
implementation in libqos.

>    qtest/ahci: Add a macro bootup routine

For future use: Now that we have tests that establish the subcomponents 
of an AHCI bootup are sane, add a command that will all-at-once bring 
the AHCI device up to functional status for us. This may be useful for 
other tests in the future, as well, that would like to run various tests 
while using the AHCI controller.

>    qtest/ahci: Add human-readable command names

Mostly for future use: add some ATA command mnemonic mappings instead of 
using the raw hex codes.

>    qtest/ahci: Don't use a magic constant for buffer size

Small niceness: Get rid of the magic constant '512' for the IDENTIFY 
buffer size test routine.

>
>   tests/ahci-test.c | 860 ++++++++++++++++++++++++++++++++++++------------------
>   1 file changed, 583 insertions(+), 277 deletions(-)
>