From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54980) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlMZE-0005Sp-5o for qemu-devel@nongnu.org; Mon, 03 Nov 2014 13:41:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XlMZ7-0006jG-Oe for qemu-devel@nongnu.org; Mon, 03 Nov 2014 13:41:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37521) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlMZ7-0006j7-Gf for qemu-devel@nongnu.org; Mon, 03 Nov 2014 13:41:21 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sA3IfIXM009352 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 3 Nov 2014 13:41:19 -0500 Message-ID: <5457CC4D.2000101@redhat.com> Date: Mon, 03 Nov 2014 13:41:17 -0500 From: John Snow MIME-Version: 1.0 References: <1411083819-9284-1-git-send-email-jsnow@redhat.com> In-Reply-To: <1411083819-9284-1-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 00/15] AHCI test helper refactors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, armbru@redhat.com, stefanha@redhat.com, mst@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(-) >