Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
On 09/04/2019 17.37, Stephen Checkoway wrote: > > > On Apr 9, 2019, at 02:13, Thomas Huth wrote: > >> We'd like to get rid of global_qtest in the long run (since it is >> causing trouble for tests that run multiple instances of QEMU in >> parallel, e.g. migration tests)... so if it is feasible, please don't >> use it in new code anymore. Try to use a local variable in the function >> that call qtest_initf() and pass the test state around via a parameter >> to the functions that need it. > > One of the patches introduces a FlashConfig structure which gets passed > around to all the functions. I could stuff the local qtest in there. The > earlier patches would still use global_qtest. Would that be acceptable or > would you prefer that global_qtest not appear in any of the patches? Using FlashConfig sounds fine to me. If you need to use global_qtest in some of the earlier patches, that's ok, too (especially if these go away again in the later patches ;-)). Thomas
Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
On Apr 9, 2019, at 02:13, Thomas Huth wrote: > We'd like to get rid of global_qtest in the long run (since it is > causing trouble for tests that run multiple instances of QEMU in > parallel, e.g. migration tests)... so if it is feasible, please don't > use it in new code anymore. Try to use a local variable in the function > that call qtest_initf() and pass the test state around via a parameter > to the functions that need it. One of the patches introduces a FlashConfig structure which gets passed around to all the functions. I could stuff the local qtest in there. The earlier patches would still use global_qtest. Would that be acceptable or would you prefer that global_qtest not appear in any of the patches? Or rather than putting it in the FlashConfig struct, I could just pass a separate parameter around. Whatever you prefer. -- Stephen Checkoway
Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
Thomas Huth writes: > On 09/04/2019 10.35, Markus Armbruster wrote: >> Thomas Huth writes: >> >>> On 09/04/2019 09.45, Markus Armbruster wrote: Thomas Huth writes: > We'd like to get rid of global_qtest in the long run (since it is > causing trouble for tests that run multiple instances of QEMU in > parallel, e.g. migration tests)... so if it is feasible, please don't > use it in new code anymore. Try to use a local variable in the function > that call qtest_initf() and pass the test state around via a parameter > to the functions that need it. Twenty tests still use @global_qtest Either we're serious about getting rid of @global_qtest. Then we should just do it. >>> >>> Ha ha, "just do it" ... that's quite a bit of work, actually. It's not >>> just about grep'ing for global_qtest, you also have to replace all the >>> writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc. >> >> And that's precisely why I'm reluctant to demand this work from >> contributors. Asking nicely is of course fair. > > That's what I did, didn't I? I said "... so if it is feasible, please > don't use it in new code anymore". I did not say "you must not use this > in new code anymore". So where's your problem here, Markus? You did, I don't have a problem, I just wanted to make quite sure your asking nicely wasn't misunderstood as a polite way to demand. [...]
Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
On 09/04/2019 10.35, Markus Armbruster wrote: > Thomas Huth writes: > >> On 09/04/2019 09.45, Markus Armbruster wrote: >>> Thomas Huth writes: >>> We'd like to get rid of global_qtest in the long run (since it is causing trouble for tests that run multiple instances of QEMU in parallel, e.g. migration tests)... so if it is feasible, please don't use it in new code anymore. Try to use a local variable in the function that call qtest_initf() and pass the test state around via a parameter to the functions that need it. >>> >>> Twenty tests still use @global_qtest >>> >>> Either we're serious about getting rid of @global_qtest. Then we should >>> just do it. >> >> Ha ha, "just do it" ... that's quite a bit of work, actually. It's not >> just about grep'ing for global_qtest, you also have to replace all the >> writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc. > > And that's precisely why I'm reluctant to demand this work from > contributors. Asking nicely is of course fair. That's what I did, didn't I? I said "... so if it is feasible, please don't use it in new code anymore". I did not say "you must not use this in new code anymore". So where's your problem here, Markus? Stephen, for the records, in case my mail was not clear: I'm fine if you keep the code as it currently is. But I'd be really happy if you could change it to avoid global_qtest. Thomas
Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
Thomas Huth writes: > On 09/04/2019 09.45, Markus Armbruster wrote: >> Thomas Huth writes: >> >>> We'd like to get rid of global_qtest in the long run (since it is >>> causing trouble for tests that run multiple instances of QEMU in >>> parallel, e.g. migration tests)... so if it is feasible, please don't >>> use it in new code anymore. Try to use a local variable in the function >>> that call qtest_initf() and pass the test state around via a parameter >>> to the functions that need it. >> >> Twenty tests still use @global_qtest >> >> Either we're serious about getting rid of @global_qtest. Then we should >> just do it. > > Ha ha, "just do it" ... that's quite a bit of work, actually. It's not > just about grep'ing for global_qtest, you also have to replace all the > writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc. And that's precisely why I'm reluctant to demand this work from contributors. Asking nicely is of course fair. > If you feel like this is a "just do it" quick task, you're welcome to help! > >> Or we're not. Then we shouldn't ask contributors to do extra work. > > We are: > > git log --oneline -- tests | grep global_qtest > > ... it just takes time. > > Thomas
Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
On 09/04/2019 09.45, Markus Armbruster wrote: > Thomas Huth writes: > >> We'd like to get rid of global_qtest in the long run (since it is >> causing trouble for tests that run multiple instances of QEMU in >> parallel, e.g. migration tests)... so if it is feasible, please don't >> use it in new code anymore. Try to use a local variable in the function >> that call qtest_initf() and pass the test state around via a parameter >> to the functions that need it. > > Twenty tests still use @global_qtest > > Either we're serious about getting rid of @global_qtest. Then we should > just do it. Ha ha, "just do it" ... that's quite a bit of work, actually. It's not just about grep'ing for global_qtest, you also have to replace all the writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc. If you feel like this is a "just do it" quick task, you're welcome to help! > Or we're not. Then we shouldn't ask contributors to do extra work. We are: git log --oneline -- tests | grep global_qtest ... it just takes time. Thomas
Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
Thomas Huth writes: > We'd like to get rid of global_qtest in the long run (since it is > causing trouble for tests that run multiple instances of QEMU in > parallel, e.g. migration tests)... so if it is feasible, please don't > use it in new code anymore. Try to use a local variable in the function > that call qtest_initf() and pass the test state around via a parameter > to the functions that need it. Twenty tests still use @global_qtest Either we're serious about getting rid of @global_qtest. Then we should just do it. Or we're not. Then we shouldn't ask contributors to do extra work.
Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
On 08/04/2019 22.55, Stephen Checkoway wrote:
> From: Stephen Checkoway
>
> Test the AMD command set for parallel flash chips. This test uses an
> ARM musicpal board with a pflash drive to test the following list of
> currently-supported commands.
> - Autoselect
> - CFI
> - Sector erase
> - Chip erase
> - Program
> - Unlock bypass
> - Reset
>
> Signed-off-by: Stephen Checkoway
> ---
> tests/Makefile.include| 2 +
> tests/pflash-cfi02-test.c | 224 ++
> 2 files changed, 226 insertions(+)
> create mode 100644 tests/pflash-cfi02-test.c
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 6b904d7430..0a26eacce0 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -263,6 +263,7 @@ check-qtest-arm-y += tests/m25p80-test$(EXESUF)
> check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
> check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
> check-qtest-arm-y += tests/hexloader-test$(EXESUF)
> +check-qtest-arm-$(CONFIG_PFLASH_CFI02) += tests/pflash-cfi02-test$(EXESUF)
>
> check-qtest-aarch64-y = tests/numa-test$(EXESUF)
> check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
> @@ -773,6 +774,7 @@ tests/device-introspect-test$(EXESUF):
> tests/device-introspect-test.o
> tests/rtc-test$(EXESUF): tests/rtc-test.o
> tests/m48t59-test$(EXESUF): tests/m48t59-test.o
> tests/hexloader-test$(EXESUF): tests/hexloader-test.o
> +tests/pflash-cfi02$(EXESUF): tests/pflash-cfi02-test.o
> tests/endianness-test$(EXESUF): tests/endianness-test.o
> tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
> tests/rtas-test$(EXESUF): tests/rtas-test.o $(libqos-spapr-obj-y)
> diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
> new file mode 100644
> index 00..d349b2cc22
> --- /dev/null
> +++ b/tests/pflash-cfi02-test.c
> @@ -0,0 +1,224 @@
> +/*
> + * QTest testcase for parallel flash with AMD command set
> + *
> + * Copyright (c) 2018 Stephen Checkoway
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include
> +#include
> +#include "libqtest.h"
> +
> +/*
> + * To test the pflash_cfi02 device, we run QEMU with the musicpal machine
> with
> + * a pflash drive. This enables us to test some flash configurations, but not
> + * all. In particular, we're limited to a 16-bit wide flash device.
> + */
> +
> +#define MP_FLASH_SIZE_MAX (32*1024*1024)
> +#define BASE_ADDR (0x1ULL-MP_FLASH_SIZE_MAX)
> +
> +#define FLASH_WIDTH 2
> +#define CFI_ADDR (FLASH_WIDTH*0x55)
> +#define UNLOCK0_ADDR (FLASH_WIDTH*0x)
> +#define UNLOCK1_ADDR (FLASH_WIDTH*0x2AAA)
> +
> +#define CFI_CMD 0x98
> +#define UNLOCK0_CMD 0xAA
> +#define UNLOCK1_CMD 0x55
> +#define AUTOSELECT_CMD 0x90
> +#define RESET_CMD 0xF0
> +#define PROGRAM_CMD 0xA0
> +#define SECTOR_ERASE_CMD 0x30
> +#define CHIP_ERASE_CMD 0x10
> +#define UNLOCK_BYPASS_CMD 0x20
> +#define UNLOCK_BYPASS_RESET_CMD 0x00
> +
> +static char image_path[] = "/tmp/qtest.XX";
> +
> +static inline void flash_write(uint64_t byte_addr, uint16_t data)
> +{
> +qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
> +}
> +
> +static inline uint16_t flash_read(uint64_t byte_addr)
> +{
> +return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
> +}
We'd like to get rid of global_qtest in the long run (since it is
causing trouble for tests that run multiple instances of QEMU in
parallel, e.g. migration tests)... so if it is feasible, please don't
use it in new code anymore. Try to use a local variable in the function
that call qtest_initf() and pass the test state around via a parameter
to the functions that need it.
Thanks,
Thomas
