Re: [PULL v2 0/8] Ide patches

[Laszlo Ersek <lersek@redhat.com>]

On 10/07/19 19:55, Philippe Mathieu-Daudé wrote:
> On 10/7/19 7:35 PM, John Snow wrote:
>> On 10/7/19 8:33 AM, Peter Maydell wrote:
>>> On Thu, 3 Oct 2019 at 20:33, John Snow <jsnow@redhat.com> wrote:
>>>>
>>>> The following changes since commit
>>>> 7f21573c822805a8e6be379d9bcf3ad9effef3dc:
>>>>
>>>>    Merge remote-tracking branch
>>>> 'remotes/huth-gitlab/tags/pull-request-2019-10-01' into staging
>>>> (2019-10-01 13:13:38 +0100)
>>>>
>>>> are available in the Git repository at:
>>>>
>>>>    https://github.com/jnsnow/qemu.git tags/ide-pull-request
>>>>
>>>> for you to fetch changes up to
>>>> f6d61c9509c56eea3cdd2d23b40d285601b1c1ca:
>>>>
>>>>    hd-geo-test: Add tests for lchs override (2019-10-03 14:36:54 -0400)
>>>>
>>>> ----------------------------------------------------------------
>>>> Pull request V2
>>>>
>>>> - Added signoff into the mirrored commits themselves (vs just the
>>>> email)
>>>> - Kudos to `stg-foreach stg edit --sign`
>>>>
>>>> ----------------------------------------------------------------
>>>
>>> Hi; the new tests in hd-geo-test seem to hang on
>>> big-endian hosts (both s390x and ppc64 hung here):
>>>
>>> linux1@lxub05:~/qemu/build/all$ QTEST_QEMU_IMG=./qemu-img
>>> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
>>> ./tests/hd-geo-test
>>> /x86_64/hd-geo/ide/none: OK
>>> /x86_64/hd-geo/ide/drive/cd_0: OK
>>> /x86_64/hd-geo/ide/drive/mbr/blank: OK
>>> /x86_64/hd-geo/ide/drive/mbr/lba: OK
>>> /x86_64/hd-geo/ide/drive/mbr/chs: OK
>>> /x86_64/hd-geo/ide/device/mbr/blank: OK
>>> /x86_64/hd-geo/ide/device/mbr/lba: OK
>>> /x86_64/hd-geo/ide/device/mbr/chs: OK
>>> /x86_64/hd-geo/ide/device/user/chs: OK
>>> /x86_64/hd-geo/ide/device/user/chst: OK
>>> /x86_64/hd-geo/override/ide:
>>>
>>
>> :(
>>
>>>
>>> thanks
>>> -- PMM
>>>
>>
>> Sam, can you investigate this?
> 
> Not seeing my T-b tags makes me grumble because I don't remember which I
> reviewed and need to go check on the list.
> 
> If the error is a endianess bug related to fw_cfg, you can add the
> "-trace fw_cfg*" in hd-geo-test::create_args() and rerun the tests on a
> BE system, the bug should appear straightly on stdout.
> 
> Are FWLCHSEntry fields little-endian? Shouldn't
> get_boot_devices_lchs_list() use some le32_to_cpu() call for the LCHS
> values?
> 

*One* problem is most likely in the find_fw_cfg_file() function, in patch 8.

+static uint16_t find_fw_cfg_file(QFWCFG *fw_cfg,
+                                 const char *filename)
+{
+    struct QemuCfgFile qfile;
+    uint32_t count, e;
+    uint16_t select;
+
+    count = qfw_cfg_get_u32(fw_cfg, FW_CFG_FILE_DIR);
+    count = be32_to_cpu(count);
+    for (select = 0, e = 0; e < count; e++) {
+        qfw_cfg_read_data(fw_cfg, &qfile, sizeof(qfile));
+        if (!strcmp(filename, qfile.name)) {
+            select = be16_to_cpu(qfile.select);
+        }
+    }
+
+    return select;
+}

Note qfw_cfg_get_u32():

uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key)
{
    uint32_t value;
    qfw_cfg_get(fw_cfg, key, &value, sizeof(value));
    return le32_to_cpu(value);
}

This function assumes that the wire encoding of the value read is little
endian. So, calling this function is wrong; and calling be32_to_cpu()
afterwards does not help. Namely:

* On LE hosts, the find_fw_cfg_file() function happens to work, because:

- the le32_to_cpu() call in qfw_cfg_get_u32() does nothing (it's identity),
- the subsequent be32_to_cpu() call in find_fw_cfg_file() corresponds to
the *blob-specific* encoding of the "count" field, in the fw_cfg
directory blob. (Which is BE) Therefore we perform the one byte-swap
that we need.

* On BE hosts, stuff breaks, because:

- the le32_to_cpu() call in qfw_cfg_get_u32() swaps the byte-order,
- the subsequent be32_to_cpu() call in find_fw_cfg_file() does nothing,
- thus, ultimately we have byte-swapped the contents of the "count"
field of the directory blob, even though the blob-specific wire format
thereof is *already* BE (= host-endian). On a BE host, all in all, there
should be zero byte swaps for consuming "count".

Now, how to fix this: eliminate
- QemuCfgFile,
- find_fw_cfg_file(),
- and read_fw_cfg_file()

altogether, and call qfw_cfg_get_file(), from "tests/libqos/fw_cfg.c".

Some other tests look up fw_cfg directory entries with that function
already (see call sites in "tests/fw_cfg-test.c").

Thanks
Laszlo