Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
Hi, On Thu, Feb 08, 2024 at 07:12:40PM +0100, Philippe Mathieu-Daudé wrote: > We should not wire IRQs on unrealized device. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Peter Maydell > Reviewed-by: Yoshinori Sato qemu 9.0 fails to boot Linux from ide/ata drives with the sh4 and sh4eb emulations. Error log is as follows. ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100 ata1.00: 16384 sectors, multi 16: LBA48 ata1.00: configured for PIO scsi 0:0:0:0: Direct-Access ATA QEMU HARDDISK2.5+ PQ: 0 ANSI: 5 sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB) sd 0:0:0:0: [sda] Write Protect is off sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA ata1: lost interrupt (Status 0x58) [ and more similar errors ] qemu command line: qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \ -snapshot -drive file=rootfs.ext2,format=raw,if=ide \ -append "root=/dev/sda console=ttySC1,115200 noiotrap" \ -serial null -serial stdio -monitor null -nographic -no-reboot Bisect points to this patch (see below). Reverting it fixes the problem. Guenter --- bisect log: # bad: [c25df57ae8f9fe1c72eee2dab37d76d904ac382e] Update version for 9.0.0 release # good: [1600b9f46b1bd08b00fe86c46ef6dbb48cbe10d6] Update version for v8.2.0 release git bisect start 'v9.0.0' 'v8.2.0' # good: [62357c047a5abc6ede992159ed7c0aaaeb50617a] Merge tag 'qemu-sparc-20240213' of https://github.com/mcayland/qemu into staging git bisect good 62357c047a5abc6ede992159ed7c0aaaeb50617a # bad: [d65f1ed7de1559534d0a1fabca5bdd81c594c7ca] docs/acpi/bits: add some clarity and details while also improving formating git bisect bad d65f1ed7de1559534d0a1fabca5bdd81c594c7ca # bad: [99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c] hw/i386/pc: Populate RTC attribute directly git bisect bad 99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c # bad: [760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0] Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging git bisect bad 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0 # good: [f2b4a98930c122648e9dc494e49cea5dffbcc2be] target/arm: Allow access to SPSR_hyp from hyp mode git bisect good f2b4a98930c122648e9dc494e49cea5dffbcc2be # bad: [1a8e2f58c5dd721086284f827326b370d19ad9eb] hw/i386/q35: Use DEVICE() cast macro with PCIDevice object git bisect bad 1a8e2f58c5dd721086284f827326b370d19ad9eb # good: [59ae6bcddc3651b55b96c2bf05a6cd4312e46d10] hw/ppc/prep: Realize ISA bridge before accessing it git bisect good 59ae6bcddc3651b55b96c2bf05a6cd4312e46d10 # bad: [7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef] hw/intc/grlib_irqmp: implements the multiprocessor status register git bisect bad 7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef # bad: [d08b7af3f7f27f6f3da8446756bf0b9352026b1d] target/sparc: Provide hint about CPUSPARCState::irq_manager member git bisect bad d08b7af3f7f27f6f3da8446756bf0b9352026b1d # bad: [5e37bc4997c32a1c9a6621a060462c84df9f1b8f] hw/dma: Pass parent object to i8257_dma_init() git bisect bad 5e37bc4997c32a1c9a6621a060462c84df9f1b8f # bad: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it git bisect bad 3c5f86a22686ef475a8259c0d8ee714f61c770c9 # good: [fc432ba0f58343c8912b80e9056315bb9bd8df92] hw/misc/macio: Realize IDE controller before accessing it git bisect good fc432ba0f58343c8912b80e9056315bb9bd8df92 # first bad commit: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
On 2/17/24 01:06, Michael Tokarev wrote:
28.02.2023 20:11, Guenter Roeck wrote:
Host drivers do not necessarily set cdb_len in megasas io commands.
With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
scsi_req_new()"), this results in failures to boot Linux from affected
SCSI drives because cdb_len is set to 0 by the host driver.
Set the cdb length to its actual size to solve the problem.
Has this been lost/forgotten?
Not sure. My understanding was that I could not prove that this is how
real hardware handles the situation, thus it wasn't applied. I carry it
locally in my builds of qemu, so it is not a problem for me. Note that
I didn't check if the problem has since been fixed differently. Maybe
that is the case and the problem no longer exists in the upstream version
of qemu.
Guenter
Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation
Hi,
On Tue, Aug 02, 2022 at 07:32:41PM -0700, Iris Chen wrote:
> From: Iris Chen
>
> Signed-off-by: Iris Chen
> ---
Are there any plans to submit a new version of this patch ?
Thanks,
Guenter
> configs/devices/arm-softmmu/default.mak | 1 +
> hw/arm/Kconfig | 5 +
> hw/tpm/Kconfig | 5 +
> hw/tpm/meson.build | 1 +
> hw/tpm/tpm_tis_spi.c| 311
> include/sysemu/tpm.h| 3 +
> 6 files changed, 326 insertions(+)
> create mode 100644 hw/tpm/tpm_tis_spi.c
>
> diff --git a/configs/devices/arm-softmmu/default.mak
> b/configs/devices/arm-softmmu/default.mak
> index 6985a25377..80d2841568 100644
> --- a/configs/devices/arm-softmmu/default.mak
> +++ b/configs/devices/arm-softmmu/default.mak
> @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
> CONFIG_SEMIHOSTING=y
> CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> CONFIG_ALLWINNER_H3=y
> +CONFIG_FBOBMC_AST=y
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 15fa79afd3..193decaec1 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -458,6 +458,11 @@ config ASPEED_SOC
> select PMBUS
> select MAX31785
>
> +config FBOBMC_AST
> +bool
> +select ASPEED_SOC
> +select TPM_TIS_SPI
> +
> config MPS2
> bool
> imply I2C_DEVICES
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index 29e82f3c92..370a43f045 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
> depends on TPM
> select TPM_TIS
>
> +config TPM_TIS_SPI
> +bool
> +depends on TPM
> +select TPM_TIS
> +
> config TPM_TIS
> bool
> depends on TPM
> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> index 1c68d81d6a..1a057f4e36 100644
> --- a/hw/tpm/meson.build
> +++ b/hw/tpm/meson.build
> @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true:
> files('tpm_tis_common.c'))
> softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
> softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true:
> files('tpm_tis_sysbus.c'))
> softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
>
> specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true:
> files('tpm_ppi.c'))
> specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true:
> files('tpm_ppi.c'))
> diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> new file mode 100644
> index 00..c98ddcfddb
> --- /dev/null
> +++ b/hw/tpm/tpm_tis_spi.c
> @@ -0,0 +1,311 @@
> +#include "qemu/osdep.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "hw/acpi/tpm.h"
> +#include "tpm_prop.h"
> +#include "tpm_tis.h"
> +#include "qom/object.h"
> +#include "hw/ssi/ssi.h"
> +#include "hw/ssi/spi_gpio.h"
> +
> +#define TPM_TIS_SPI_ADDR_BYTES 3
> +#define SPI_WRITE 0
> +
> +typedef enum {
> +TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> +TIS_SPI_PKT_STATE_START,
> +TIS_SPI_PKT_STATE_ADDRESS,
> +TIS_SPI_PKT_STATE_DATA_WR,
> +TIS_SPI_PKT_STATE_DATA_RD,
> +TIS_SPI_PKT_STATE_DONE,
> +} TpmTisSpiPktState;
> +
> +union TpmTisRWSizeByte {
> +uint8_t byte;
> +struct {
> +uint8_t data_expected_size:6;
> +uint8_t resv:1;
> +uint8_t rwflag:1;
> +};
> +};
> +
> +union TpmTisSpiHwAddr {
> +hwaddr addr;
> +uint8_t bytes[sizeof(hwaddr)];
> +};
> +
> +union TpmTisSpiData {
> +uint32_t data;
> +uint8_t bytes[64];
> +};
> +
> +struct TpmTisSpiState {
> +/*< private >*/
> +SSIPeripheral parent_obj;
> +
> +/*< public >*/
> +TPMState tpm_state; /* not a QOM object */
> +TpmTisSpiPktState tpm_tis_spi_state;
> +
> +union TpmTisRWSizeByte first_byte;
> +union TpmTisSpiHwAddr addr;
> +union TpmTisSpiData data;
> +
> +uint32_t data_size;
> +uint8_t data_idx;
> +uint8_t addr_idx;
> +};
> +
> +struct TpmTisSpiClass {
> +SSIPeripheralClass parent_class;
> +};
> +
> +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> +
> +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> +{
> +uint16_t offset = tts->addr.addr & 0xffc;
> +
> +switch (offset) {
> +case TPM_TIS_REG_DATA_FIFO:
> +for (uint8_t i = 0; i < tts->data_size; i++) {
> +tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> + >tpm_state,
> + tts->addr.addr,
> + 1);
> +}
> +break;
> +default:
> +tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> + >tpm_state,
> + tts->addr.addr,
> + tts->data_size);
> +}
> +}
> +
> +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> +{
> +uint16_t offset =
Re: [PATCH] hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers
On 7/24/23 00:18, Bernhard Beschow wrote:
Am 16. Juli 2023 19:53:37 UTC schrieb Bernhard Beschow :
Am 10. Juli 2023 16:01:46 UTC schrieb Bernhard Beschow :
Am 10. Juli 2023 10:16:35 UTC schrieb "Philippe Mathieu-Daudé"
:
On 9/7/23 10:09, Bernhard Beschow wrote:
Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller
interfaces" sdhci_common_realize() forces all SD card controllers to use either
sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property.
However, there are device models which use different MMIO ops: TYPE_IMX_USDHC
uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops.
Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for
example. Fix this by defaulting the io_ops to little endian and switch to big
endian in sdhci_common_realize() only if there is a matchig big endian variant
available.
Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller
interfaces")
Signed-off-by: Bernhard Beschow
---
hw/sd/sdhci.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6811f0f1a8..362c2c86aa 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s)
s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
sdhci_raise_insertion_irq, s);
s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
sdhci_data_transfer, s);
+
+s->io_ops = _mmio_le_ops;
}
void sdhci_uninitfn(SDHCIState *s)
@@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
What about simply keeping the same code guarded with 'if (!s->io_ops)'?
I chose below approach since it provides an error message when one attempts to
set one of the other device models to BE rather than just silently ignoring it.
Also, I didn't want to make the assumption that `s->io_ops == NULL` implied
that sdhci_mmio_*_ops is needed. That's similar material the bug fixed is made of,
so I wanted to prevent that in the first place by being more explicit.
In combination with the new error message the limitations of the current code
become hopefully very apparent now, and at the same time should provide enough
hints for adding BE support to the other device models if ever needed.
Best regards,
Bernhard
Ping
Ping^2
I would like to have the bug fixed in 8.1.
+1
Not that I care too much - I build qemu myself anyway and carry the patch
locally -
but this really should get fixed.
Guenter
Re: [PATCH] hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers
On Mon, Jul 10, 2023 at 12:16:35PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/7/23 10:09, Bernhard Beschow wrote:
> > Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host
> > controller
> > interfaces" sdhci_common_realize() forces all SD card controllers to use
> > either
> > sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness"
> > property.
> > However, there are device models which use different MMIO ops:
> > TYPE_IMX_USDHC
> > uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops.
> >
> > Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board,
> > for
> > example. Fix this by defaulting the io_ops to little endian and switch to
> > big
> > endian in sdhci_common_realize() only if there is a matchig big endian
> > variant
> > available.
> >
> > Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller
> > interfaces")
> >
> > Signed-off-by: Bernhard Beschow
> > ---
> > hw/sd/sdhci.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 6811f0f1a8..362c2c86aa 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s)
> > s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > sdhci_raise_insertion_irq, s);
> > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > sdhci_data_transfer, s);
> > +
> > +s->io_ops = _mmio_le_ops;
> > }
> > void sdhci_uninitfn(SDHCIState *s)
> > @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error
> > **errp)
>
> What about simply keeping the same code guarded with 'if (!s->io_ops)'?
>
That was my quick fix which I considered a hack, and I didn't submit it
because I thought it was a hack ;-).
On the other side, this solution would probably break on big endian systems
which have their own io ops, so I am not sure if it is any better.
Guenter
> > switch (s->endianness) {
> > case DEVICE_LITTLE_ENDIAN:
> > -s->io_ops = _mmio_le_ops;
> > +/* s->io_ops is little endian by default */
> > break;
> > case DEVICE_BIG_ENDIAN:
> > +if (s->io_ops != _mmio_le_ops) {
> > +error_setg(errp, "SD controller doesn't support big
> > endianness");
> > +return;
> > +}
> > s->io_ops = _mmio_be_ops;
> > break;
> > default:
>
Re: [PATCH] hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers
On 7/9/23 01:09, Bernhard Beschow wrote:
Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller
interfaces" sdhci_common_realize() forces all SD card controllers to use either
sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property.
However, there are device models which use different MMIO ops: TYPE_IMX_USDHC
uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops.
Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for
example. Fix this by defaulting the io_ops to little endian and switch to big
endian in sdhci_common_realize() only if there is a matchig big endian variant
available.
Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller
interfaces")
Signed-off-by: Bernhard Beschow
Tested-by: Guenter Roeck
---
hw/sd/sdhci.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6811f0f1a8..362c2c86aa 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s)
s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer,
s);
+
+s->io_ops = _mmio_le_ops;
}
void sdhci_uninitfn(SDHCIState *s)
@@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
switch (s->endianness) {
case DEVICE_LITTLE_ENDIAN:
-s->io_ops = _mmio_le_ops;
+/* s->io_ops is little endian by default */
break;
case DEVICE_BIG_ENDIAN:
+if (s->io_ops != _mmio_le_ops) {
+error_setg(errp, "SD controller doesn't support big endianness");
+return;
+}
s->io_ops = _mmio_be_ops;
break;
default:
Re: [PATCH v6 2/3] hw/sd/sdhci: Support big endian SD host controller interfaces
On Sat, Apr 29, 2023 at 01:46:26PM -0700, Guenter Roeck wrote:
> Hi,
>
> On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote:
> > Some SDHCI IP can be synthetized in various endianness:
> > https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc
> >
> > - CONFIG_SYS_FSL_ESDHC_BE
> >
> >ESDHC IP is in big-endian mode. Accessing ESDHC registers can be
> >determined by ESDHC IP's endian mode or processor's endian mode.
> >
> > Our current implementation is little-endian. In order to support
> > big endianness:
> >
> > - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le')
> > - Add an 'endianness' property to SDHCIState (default little endian)
> > - Set the 'io_ops' field in realize() after checking the property
> > - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
>
> With this patch in place (in qemu v8.0), I can no longer boot linux
> from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations.
> I get the following persistent errors.
>
> [ 12.210101] sdhci-esdhc-imx 2194000.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.213222] sdhci-esdhc-imx 2194000.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.215072] sdhci-esdhc-imx 2194000.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.218766] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.220441] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.221542] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.241544] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.242608] sdhci-esdhc-imx 219.mmc: card clock still not stable in
> 100us!.
>
> The emulations start to work again after reverting this patch.
>
Cause explained below.
> Guenter
>
> > ---
> > hw/sd/sdhci-internal.h | 1 +
> > hw/sd/sdhci.c | 32 +---
> > include/hw/sd/sdhci.h | 1 +
> > 3 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> > index 964570f8e8..5f3765f12d 100644
> > --- a/hw/sd/sdhci-internal.h
> > +++ b/hw/sd/sdhci-internal.h
> > @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate;
> > #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
> >
> > #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> > +DEFINE_PROP_UINT8("endianness", _state, endianness,
> > DEVICE_LITTLE_ENDIAN), \
> > DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
> > DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
> > DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 22c758ad91..289baa879e 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t
> > val, unsigned size)
> > value >> shift, value >> shift);
> > }
> >
> > -static const MemoryRegionOps sdhci_mmio_ops = {
> > +static const MemoryRegionOps sdhci_mmio_le_ops = {
> > .read = sdhci_read,
> > .write = sdhci_write,
> > .impl = {
> > @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = {
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > };
> >
> > +static const MemoryRegionOps sdhci_mmio_be_ops = {
> > +.read = sdhci_read,
> > +.write = sdhci_write,
> > +.impl = {
> > +.min_access_size = 4,
> > +.max_access_size = 4,
> > +},
> > +.valid = {
> > +.min_access_size = 1,
> > +.max_access_size = 4,
> > +.unaligned = false
> > +},
> > +.endianness = DEVICE_BIG_ENDIAN,
> > +};
> > +
> > static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
> > {
> > ERRP_GUARD();
> > @@ -1371,8 +1386,6 @@ void sdhci_initfn(SDHCIState *s)
> >
> > s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > sdhci_raise_insertion_irq, s);
> > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
Re: [PATCH v6 2/3] hw/sd/sdhci: Support big endian SD host controller interfaces
Hi,
On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote:
> Some SDHCI IP can be synthetized in various endianness:
> https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc
>
> - CONFIG_SYS_FSL_ESDHC_BE
>
>ESDHC IP is in big-endian mode. Accessing ESDHC registers can be
>determined by ESDHC IP's endian mode or processor's endian mode.
>
> Our current implementation is little-endian. In order to support
> big endianness:
>
> - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le')
> - Add an 'endianness' property to SDHCIState (default little endian)
> - Set the 'io_ops' field in realize() after checking the property
> - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps.
>
> Signed-off-by: Philippe Mathieu-Daudé
With this patch in place (in qemu v8.0), I can no longer boot linux
from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations.
I get the following persistent errors.
[ 12.210101] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.213222] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.215072] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.218766] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.220441] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.221542] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.241544] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.242608] sdhci-esdhc-imx 219.mmc: card clock still not stable in
100us!.
The emulations start to work again after reverting this patch.
Guenter
> ---
> hw/sd/sdhci-internal.h | 1 +
> hw/sd/sdhci.c | 32 +---
> include/hw/sd/sdhci.h | 1 +
> 3 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 964570f8e8..5f3765f12d 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate;
> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>
> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> +DEFINE_PROP_UINT8("endianness", _state, endianness,
> DEVICE_LITTLE_ENDIAN), \
> DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
> DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
> DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 22c758ad91..289baa879e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
> unsigned size)
> value >> shift, value >> shift);
> }
>
> -static const MemoryRegionOps sdhci_mmio_ops = {
> +static const MemoryRegionOps sdhci_mmio_le_ops = {
> .read = sdhci_read,
> .write = sdhci_write,
> .impl = {
> @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> +static const MemoryRegionOps sdhci_mmio_be_ops = {
> +.read = sdhci_read,
> +.write = sdhci_write,
> +.impl = {
> +.min_access_size = 4,
> +.max_access_size = 4,
> +},
> +.valid = {
> +.min_access_size = 1,
> +.max_access_size = 4,
> +.unaligned = false
> +},
> +.endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
> {
> ERRP_GUARD();
> @@ -1371,8 +1386,6 @@ void sdhci_initfn(SDHCIState *s)
>
> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> sdhci_raise_insertion_irq, s);
> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> sdhci_data_transfer, s);
> -
> -s->io_ops = _mmio_ops;
> }
>
> void sdhci_uninitfn(SDHCIState *s)
> @@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
> {
> ERRP_GUARD();
>
> +switch (s->endianness) {
> +case DEVICE_LITTLE_ENDIAN:
> +s->io_ops = _mmio_le_ops;
> +break;
> +case DEVICE_BIG_ENDIAN:
> +s->io_ops = _mmio_be_ops;
> +break;
> +default:
> +error_setg(errp, "Incorrect endianness");
> +return;
> +}
> +
> sdhci_init_readonly_registers(s, errp);
> if (*errp) {
> return;
> }
> +
> s->buf_maxsz = sdhci_get_fifolen(s);
> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 01a64c5442..a989fca3b2 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -96,6 +96,7 @@ struct
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
On 3/3/23 01:02, Fiona Ebner wrote:
Am 28.02.23 um 18:11 schrieb Guenter Roeck:
Host drivers do not necessarily set cdb_len in megasas io commands.
With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
scsi_req_new()"), this results in failures to boot Linux from affected
SCSI drives because cdb_len is set to 0 by the host driver.
Set the cdb length to its actual size to solve the problem.
Tested-by: Fiona Ebner
But I do have a question:
Signed-off-by: Guenter Roeck
---
hw/scsi/megasas.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 9cbbb16121..d624866bb6 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
uint8_t cdb[16];
int len;
struct SCSIDevice *sdev = NULL;
-int target_id, lun_id, cdb_len;
+int target_id, lun_id;
lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
@@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
target_id = cmd->frame->header.target_id;
lun_id = cmd->frame->header.lun_id;
-cdb_len = cmd->frame->header.cdb_len;
if (target_id < MFI_MAX_LD && lun_id == 0) {
sdev = scsi_device_find(>bus, 0, target_id, lun_id);
@@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
return MFI_STAT_DEVICE_NOT_FOUND;
}
-if (cdb_len > 16) {
-trace_megasas_scsi_invalid_cdb_len(
-mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
-megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
-cmd->frame->header.scsi_status = CHECK_CONDITION;
-s->event_count++;
-return MFI_STAT_SCSI_DONE_WITH_ERROR;
-}
Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
consequence of
Host drivers do not necessarily set cdb_len in megasas io commands.
that this can be uninitialized memory and we need to assume it was not
explicitly set?
I doubt that real hardware uses or checks the field for the affected commands
because that would be pointless, but it is really up to you to decide how
you want to handle it.
Guenter
Best Regards,
Fiona
-
cmd->iov_size = lba_count * sdev->blocksize;
if (megasas_map_sgl(s, cmd, >frame->io.sgl)) {
megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
@@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
megasas_encode_lba(cdb, lba_start, lba_count, is_write);
cmd->req = scsi_req_new(sdev, cmd->index,
-lun_id, cdb, cdb_len, cmd);
+lun_id, cdb, sizeof(cdb), cmd);
if (!cmd->req) {
trace_megasas_scsi_req_alloc_failed(
mfi_frame_desc(frame_cmd), target_id, lun_id);
[PATCH] scsi: megasas: Internal cdbs have 16-byte length
Host drivers do not necessarily set cdb_len in megasas io commands.
With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
scsi_req_new()"), this results in failures to boot Linux from affected
SCSI drives because cdb_len is set to 0 by the host driver.
Set the cdb length to its actual size to solve the problem.
Signed-off-by: Guenter Roeck
---
hw/scsi/megasas.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 9cbbb16121..d624866bb6 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
uint8_t cdb[16];
int len;
struct SCSIDevice *sdev = NULL;
-int target_id, lun_id, cdb_len;
+int target_id, lun_id;
lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
@@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
target_id = cmd->frame->header.target_id;
lun_id = cmd->frame->header.lun_id;
-cdb_len = cmd->frame->header.cdb_len;
if (target_id < MFI_MAX_LD && lun_id == 0) {
sdev = scsi_device_find(>bus, 0, target_id, lun_id);
@@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
return MFI_STAT_DEVICE_NOT_FOUND;
}
-if (cdb_len > 16) {
-trace_megasas_scsi_invalid_cdb_len(
-mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
-megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
-cmd->frame->header.scsi_status = CHECK_CONDITION;
-s->event_count++;
-return MFI_STAT_SCSI_DONE_WITH_ERROR;
-}
-
cmd->iov_size = lba_count * sdev->blocksize;
if (megasas_map_sgl(s, cmd, >frame->io.sgl)) {
megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
@@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
megasas_encode_lba(cdb, lba_start, lba_count, is_write);
cmd->req = scsi_req_new(sdev, cmd->index,
-lun_id, cdb, cdb_len, cmd);
+lun_id, cdb, sizeof(cdb), cmd);
if (!cmd->req) {
trace_megasas_scsi_req_alloc_failed(
mfi_frame_desc(frame_cmd), target_id, lun_id);
--
2.39.1
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/19/23 00:02, Klaus Jensen wrote: On Jan 19 08:28, Klaus Jensen wrote: On Jan 18 21:03, Keith Busch wrote: On Thu, Jan 19, 2023 at 01:10:57PM +1000, Alistair Francis wrote: On Thu, Jan 19, 2023 at 12:44 PM Keith Busch wrote: Further up, it says the "interrupt gateway" is responsible for forwarding new interrupt requests while the level remains asserted, but it doesn't look like anything is handling that, which essentially turns this into an edge interrupt. Am I missing something, or is this really not being handled? Yeah, that wouldn't be handled. In QEMU the PLIC relies on QEMUs internal GPIO lines to trigger an interrupt. So with the current setup we only support edge triggered interrupts. Thanks for confirming! Klaus, I think we can justify introducing a work-around in the emulated device now. My previous proposal with pci_irq_pulse() is no good since it does assert+deassert, but it needs to be the other way around, so please don't considert that one. Also, we ought to revisit the intms/intmc usage in the linux driver for threaded interrupts. +CC: qemu-riscv Keith, Thanks for digging into this! Yeah, you are probably right that we should only use the intms/intmc changes in the use_threaded_interrupts case, not in general. While my RFC patch does seem to "fix" this, it is just a workaround as your analysis indicate. > +CC: Philippe, I am observing these timeouts/aborts on mips as well, so I guess that emulation could suffer from the same issue? I suspect it does. In my case, I have an Ethernet controller on the same interrupt line as the NVME controller, and it looks like one of the interrupts gets lost if both controllers raise an interrupt at the same time. The suggested workarounds "fix" the problem, but that doesn't seem to be the correct solution. Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On Tue, Jan 17, 2023 at 04:18:14PM +, Peter Maydell wrote: > On Tue, 17 Jan 2023 at 16:10, Guenter Roeck wrote: > > > > On Mon, Jan 16, 2023 at 09:58:13PM -0700, Keith Busch wrote: > > > On Mon, Jan 16, 2023 at 10:14:07PM +0100, Klaus Jensen wrote: > > > > I noticed that the Linux driver does not use the INTMS/INTMC registers > > > > to mask interrupts on the controller while processing CQEs. While not > > > > required by the spec, it is *recommended* in setups not using MSI-X to > > > > reduce the risk of spurious and/or missed interrupts. > > > > > > That's assuming completions are deferred to a bottom half. We don't do > > > that by default in Linux nvme, though you can ask the driver to do that > > > if you want. > > > > > > > With the patch below, running 100 boot iterations, no timeouts were > > > > observed on QEMU emulated riscv64 or mips64. > > > > > > > > No changes are required in the QEMU hw/nvme interrupt logic. > > > > > > Yeah, I can see why: it forces the irq line to deassert then assert, > > > just like we had forced to happen within the device side patches. Still, > > > none of that is supposed to be necessary, but this idea of using these > > > registers is probably fine. > > > > There is still no answer why this would be necessary in the first place, > > on either side. In my opinion, unless someone can confirm that the problem > > is seen with real hardware, we should assume that it happens on the qemu > > side and address it there. > > Sure, but that means identifying what the divergence > between QEMU's implementation and the hardware is first. I don't > want a fudged fix in QEMU's code any more than you want one in > the kernel's driver code :-) > I actually prefer it in qemu because that means I can test nvme support on all active LTS releases of the Linux kernel, but that is POV and secondary. This has been broken ever since I started testing nvme support with qemu, so it doesn't make much of a difference if fixing the problem for good takes a bit longer. Plus, I run my own version of qemu anyway, so carrying the fix (hack) in qemu doesn't make much of a difference for me. Anyway - any idea what to do to help figuring out what is happening ? Add tracing support to pci interrupt handling, maybe ? Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On Mon, Jan 16, 2023 at 09:58:13PM -0700, Keith Busch wrote:
> On Mon, Jan 16, 2023 at 10:14:07PM +0100, Klaus Jensen wrote:
> > I noticed that the Linux driver does not use the INTMS/INTMC registers
> > to mask interrupts on the controller while processing CQEs. While not
> > required by the spec, it is *recommended* in setups not using MSI-X to
> > reduce the risk of spurious and/or missed interrupts.
>
> That's assuming completions are deferred to a bottom half. We don't do
> that by default in Linux nvme, though you can ask the driver to do that
> if you want.
>
> > With the patch below, running 100 boot iterations, no timeouts were
> > observed on QEMU emulated riscv64 or mips64.
> >
> > No changes are required in the QEMU hw/nvme interrupt logic.
>
> Yeah, I can see why: it forces the irq line to deassert then assert,
> just like we had forced to happen within the device side patches. Still,
> none of that is supposed to be necessary, but this idea of using these
> registers is probably fine.
There is still no answer why this would be necessary in the first place,
on either side. In my opinion, unless someone can confirm that the problem
is seen with real hardware, we should assume that it happens on the qemu
side and address it there.
Guenter
>
> > static irqreturn_t nvme_irq(int irq, void *data)
> > +{
> > + struct nvme_queue *nvmeq = data;
> > + struct nvme_dev *dev = nvmeq->dev;
> > + u32 mask = 1 << nvmeq->cq_vector;
> > + irqreturn_t ret = IRQ_NONE;
> > + DEFINE_IO_COMP_BATCH(iob);
> > +
> > + writel(mask, dev->bar + NVME_REG_INTMS);
> > +
> > + if (nvme_poll_cq(nvmeq, )) {
> > + if (!rq_list_empty(iob.req_list))
> > + nvme_pci_complete_batch();
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > + writel(mask, dev->bar + NVME_REG_INTMC);
> > +
> > + return ret;
> > +}
>
> If threaded interrupts are used, you'll want to do the masking in
> nvme_irq_check(), then clear it in the threaded handler instead of doing
> both in the same callback.
>
> > +static irqreturn_t nvme_irq_msix(int irq, void *data)
> > {
> > struct nvme_queue *nvmeq = data;
> > DEFINE_IO_COMP_BATCH(iob);
> > @@ -1602,12 +1623,13 @@ static int queue_request_irq(struct nvme_queue
> > *nvmeq)
> > {
> > struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
> > int nr = nvmeq->dev->ctrl.instance;
> > + irq_handler_t handler = pdev->msix_enabled ? nvme_irq_msix :
> > nvme_irq;
> >
> > if (use_threaded_interrupts) {
> > return pci_request_irq(pdev, nvmeq->cq_vector,
> > nvme_irq_check,
> > - nvme_irq, nvmeq, "nvme%dq%d", nr,
> > nvmeq->qid);
> > + handler, nvmeq, "nvme%dq%d", nr,
> > nvmeq->qid);
> > } else {
> > - return pci_request_irq(pdev, nvmeq->cq_vector, nvme_irq,
> > + return pci_request_irq(pdev, nvmeq->cq_vector, handler,
> > NULL, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> > }
> > }
> >
> >
>
>
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On Mon, Jan 16, 2023 at 10:14:07PM +0100, Klaus Jensen wrote:
[ ... ]
>
> I noticed that the Linux driver does not use the INTMS/INTMC registers
> to mask interrupts on the controller while processing CQEs. While not
> required by the spec, it is *recommended* in setups not using MSI-X to
> reduce the risk of spurious and/or missed interrupts.
>
> With the patch below, running 100 boot iterations, no timeouts were
> observed on QEMU emulated riscv64 or mips64.
>
> No changes are required in the QEMU hw/nvme interrupt logic.
>
Yes, but isn't that technically similar to dropping the
interrupt request and raising it again, or in other words
pulsing the interrupt ?
I still don't understand why the (level triggered) interrupt
pin would require pulsing in the first place.
Thanks,
Guenter
>
> diff --git i/drivers/nvme/host/pci.c w/drivers/nvme/host/pci.c
> index b13baccedb4a..75f6b87c4c3f 100644
> --- i/drivers/nvme/host/pci.c
> +++ w/drivers/nvme/host/pci.c
> @@ -1128,6 +1128,27 @@ static inline int nvme_poll_cq(struct nvme_queue
> *nvmeq,
> }
>
> static irqreturn_t nvme_irq(int irq, void *data)
> +{
> + struct nvme_queue *nvmeq = data;
> + struct nvme_dev *dev = nvmeq->dev;
> + u32 mask = 1 << nvmeq->cq_vector;
> + irqreturn_t ret = IRQ_NONE;
> + DEFINE_IO_COMP_BATCH(iob);
> +
> + writel(mask, dev->bar + NVME_REG_INTMS);
> +
> + if (nvme_poll_cq(nvmeq, )) {
> + if (!rq_list_empty(iob.req_list))
> + nvme_pci_complete_batch();
> + ret = IRQ_HANDLED;
> + }
> +
> + writel(mask, dev->bar + NVME_REG_INTMC);
> +
> + return ret;
> +}
> +
> +static irqreturn_t nvme_irq_msix(int irq, void *data)
> {
> struct nvme_queue *nvmeq = data;
> DEFINE_IO_COMP_BATCH(iob);
> @@ -1602,12 +1623,13 @@ static int queue_request_irq(struct nvme_queue *nvmeq)
> {
> struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
> int nr = nvmeq->dev->ctrl.instance;
> + irq_handler_t handler = pdev->msix_enabled ? nvme_irq_msix : nvme_irq;
>
> if (use_threaded_interrupts) {
> return pci_request_irq(pdev, nvmeq->cq_vector, nvme_irq_check,
> - nvme_irq, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> + handler, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> } else {
> - return pci_request_irq(pdev, nvmeq->cq_vector, nvme_irq,
> + return pci_request_irq(pdev, nvmeq->cq_vector, handler,
> NULL, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> }
> }
>
>
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 11:27, Keith Busch wrote:
On Thu, Jan 12, 2023 at 06:45:55PM +0100, Klaus Jensen wrote:
On Jan 12 09:34, Keith Busch wrote:
On Thu, Jan 12, 2023 at 02:10:51PM +0100, Klaus Jensen wrote:
The pin-based interrupt logic in hw/nvme seems sound enough to me, so I
am wondering if there is something going on with the kernel driver (but
I certainly do not rule out that hw/nvme is at fault here, since
pin-based interrupts has also been a source of several issues in the
past).
Does it work if you change the pci_irq_assert() back to pci_irq_pulse()?
While probably not the "correct" thing to do, it has better results in
my testing.
A simple s/pci_irq_assert/pci_irq_pulse broke the device. However,
diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 03760ddeae8c..0fc46dcb9ec4 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -477,6 +477,7 @@ static void nvme_irq_check(NvmeCtrl *n)
return;
}
if (~intms & n->irq_status) {
+pci_irq_deassert(>parent_obj);
pci_irq_assert(>parent_obj);
} else {
pci_irq_deassert(>parent_obj);
seems to do the trick (pulse is the other way around, assert, then
deassert).
Probably not the "correct" thing to do, but I'll take it since it seems
to fix it. On a simple boot loop I got the timeout about 1 out of 5. I'm
on ~20 runs now and have not encountered it.
I'll see if I can set up a mips rootfs and test that. Guenter, what MIPS
machine/board(s) are you testing?
Could you try the below?
This works as well: 50 iterations with no failures after applying the patch
below on top of qemu v7.2.0. Tested with qemu-system-mipsel.
Guenter
---
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2c85de4700..521c3c80c1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -558,6 +558,20 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue *cq)
}
}
+static void nvme_irq_pulse(NvmeCtrl *n, NvmeCQueue *cq)
+{
+if (!cq->irq_enabled) {
+return;
+}
+
+if (msix_enabled(&(n->parent_obj))) {
+msix_notify(&(n->parent_obj), cq->vector);
+return;
+}
+
+pci_irq_pulse(>parent_obj);
+}
+
static void nvme_req_clear(NvmeRequest *req)
{
req->ns = NULL;
@@ -6917,6 +6931,12 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr,
int val)
}
nvme_irq_deassert(n, cq);
+} else {
+/*
+ * Retrigger the irq just to make sure the host has no excuse for
+ * not knowing there's more work to complete on this CQ.
+ */
+nvme_irq_pulse(n, cq);
}
} else {
/* Submission queue doorbell write */
--
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 09:45, Klaus Jensen wrote:
On Jan 12 09:34, Keith Busch wrote:
On Thu, Jan 12, 2023 at 02:10:51PM +0100, Klaus Jensen wrote:
The pin-based interrupt logic in hw/nvme seems sound enough to me, so I
am wondering if there is something going on with the kernel driver (but
I certainly do not rule out that hw/nvme is at fault here, since
pin-based interrupts has also been a source of several issues in the
past).
Does it work if you change the pci_irq_assert() back to pci_irq_pulse()?
While probably not the "correct" thing to do, it has better results in
my testing.
A simple s/pci_irq_assert/pci_irq_pulse broke the device. However,
diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 03760ddeae8c..0fc46dcb9ec4 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -477,6 +477,7 @@ static void nvme_irq_check(NvmeCtrl *n)
return;
}
if (~intms & n->irq_status) {
+pci_irq_deassert(>parent_obj);
pci_irq_assert(>parent_obj);
} else {
pci_irq_deassert(>parent_obj);
seems to do the trick (pulse is the other way around, assert, then
deassert).
Probably not the "correct" thing to do, but I'll take it since it seems
to fix it. On a simple boot loop I got the timeout about 1 out of 5. I'm
on ~20 runs now and have not encountered it.
I'll see if I can set up a mips rootfs and test that. Guenter, what MIPS
machine/board(s) are you testing?
So, for mipsel, two sets of results for the above:
First, qemu v7.2 is already much better than qemu v7.1. With qemu v7.1,
the boot test fails roughly every other test. Failure rate with qemu v7.2
is much less.
Second, my nvme boot test with qemu 7.2 fails after ~5-10 iterations.
After the above change, I did not see a single failure in 50 boot tests.
I'll test the other suggested change next.
Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 09:45, Klaus Jensen wrote:
On Jan 12 09:34, Keith Busch wrote:
On Thu, Jan 12, 2023 at 02:10:51PM +0100, Klaus Jensen wrote:
The pin-based interrupt logic in hw/nvme seems sound enough to me, so I
am wondering if there is something going on with the kernel driver (but
I certainly do not rule out that hw/nvme is at fault here, since
pin-based interrupts has also been a source of several issues in the
past).
Does it work if you change the pci_irq_assert() back to pci_irq_pulse()?
While probably not the "correct" thing to do, it has better results in
my testing.
A simple s/pci_irq_assert/pci_irq_pulse broke the device. However,
diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 03760ddeae8c..0fc46dcb9ec4 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -477,6 +477,7 @@ static void nvme_irq_check(NvmeCtrl *n)
return;
}
if (~intms & n->irq_status) {
+pci_irq_deassert(>parent_obj);
pci_irq_assert(>parent_obj);
} else {
pci_irq_deassert(>parent_obj);
seems to do the trick (pulse is the other way around, assert, then
deassert).
Probably not the "correct" thing to do, but I'll take it since it seems
to fix it. On a simple boot loop I got the timeout about 1 out of 5. I'm
on ~20 runs now and have not encountered it.
I'll see if I can set up a mips rootfs and test that. Guenter, what MIPS
machine/board(s) are you testing?
See
https://github.com/groeck/linux-build-test/blob/master/rootfs/mips/run-qemu-mips.sh
and
https://github.com/groeck/linux-build-test/blob/master/rootfs/mipsel/run-qemu-mipsel.sh
I'll apply the change suggested above to my version of qemu (7.2.0)
and give it a try.
Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 05:10, Klaus Jensen wrote: Hi all (linux-nvme, qemu-devel, maintainers), On QEMU riscv64, which does not use MSI/MSI-X and thus relies on pin-based interrupts, I'm seeing occasional completion timeouts, i.e. nvme nvme0: I/O 333 QID 1 timeout, completion polled To rule out issues with shadow doorbells (which have been a source of frustration in the past), those are disabled. FWIW I'm also seeing the issue with shadow doorbells. diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index f25cc2c235e9..28d8e7f4b56c 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -7407,7 +7407,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev) id->mdts = n->params.mdts; id->ver = cpu_to_le32(NVME_SPEC_VER); id->oacs = -cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT | NVME_OACS_DBBUF); +cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT); id->cntrltype = 0x1; /* I captured a trace from QEMU when this happens: pci_nvme_mmio_write addr 0x1008 data 0x4e size 4 pci_nvme_mmio_doorbell_sq sqid 1 new_tail 78 pci_nvme_io_cmd cid 4428 nsid 0x1 sqid 1 opc 0x2 opname 'NVME_NVM_CMD_READ' pci_nvme_read cid 4428 nsid 1 nlb 32 count 16384 lba 0x1324 pci_nvme_map_prp trans_len 4096 len 16384 prp1 0x80aca000 prp2 0x82474100 num_prps 5 pci_nvme_map_addr addr 0x80aca000 len 4096 pci_nvme_map_addr addr 0x80ac9000 len 4096 pci_nvme_map_addr addr 0x80ac8000 len 4096 pci_nvme_map_addr addr 0x80ac7000 len 4096 pci_nvme_io_cmd cid 4429 nsid 0x1 sqid 1 opc 0x2 opname 'NVME_NVM_CMD_READ' pci_nvme_read cid 4429 nsid 1 nlb 224 count 114688 lba 0x1242 pci_nvme_map_prp trans_len 4096 len 114688 prp1 0x80ae6000 prp2 0x82474000 num_prps 29 pci_nvme_map_addr addr 0x80ae6000 len 4096 pci_nvme_map_addr addr 0x80ae5000 len 4096 pci_nvme_map_addr addr 0x80ae4000 len 4096 pci_nvme_map_addr addr 0x80ae3000 len 4096 pci_nvme_map_addr addr 0x80ae2000 len 4096 pci_nvme_map_addr addr 0x80ae1000 len 4096 pci_nvme_map_addr addr 0x80ae len 4096 pci_nvme_map_addr addr 0x80adf000 len 4096 pci_nvme_map_addr addr 0x80ade000 len 4096 pci_nvme_map_addr addr 0x80add000 len 4096 pci_nvme_map_addr addr 0x80adc000 len 4096 pci_nvme_map_addr addr 0x80adb000 len 4096 pci_nvme_map_addr addr 0x80ada000 len 4096 pci_nvme_map_addr addr 0x80ad9000 len 4096 pci_nvme_map_addr addr 0x80ad8000 len 4096 pci_nvme_map_addr addr 0x80ad7000 len 4096 pci_nvme_map_addr addr 0x80ad6000 len 4096 pci_nvme_map_addr addr 0x80ad5000 len 4096 pci_nvme_map_addr addr 0x80ad4000 len 4096 pci_nvme_map_addr addr 0x80ad3000 len 4096 pci_nvme_map_addr addr 0x80ad2000 len 4096 pci_nvme_map_addr addr 0x80ad1000 len 4096 pci_nvme_map_addr addr 0x80ad len 4096 pci_nvme_map_addr addr 0x80acf000 len 4096 pci_nvme_map_addr addr 0x80ace000 len 4096 pci_nvme_map_addr addr 0x80acd000 len 4096 pci_nvme_map_addr addr 0x80acc000 len 4096 pci_nvme_map_addr addr 0x80acb000 len 4096 pci_nvme_rw_cb cid 4428 blk 'd0' pci_nvme_rw_complete_cb cid 4428 blk 'd0' pci_nvme_enqueue_req_completion cid 4428 cqid 1 dw0 0x0 dw1 0x0 status 0x0 [1]: pci_nvme_irq_pin pulsing IRQ pin pci_nvme_rw_cb cid 4429 blk 'd0' pci_nvme_rw_complete_cb cid 4429 blk 'd0' pci_nvme_enqueue_req_completion cid 4429 cqid 1 dw0 0x0 dw1 0x0 status 0x0 [2]: pci_nvme_irq_pin pulsing IRQ pin [3]: pci_nvme_mmio_write addr 0x100c data 0x4d size 4 [4]: pci_nvme_mmio_doorbell_cq cqid 1 new_head 77 TIMEOUT HERE (30s) --- [5]: pci_nvme_mmio_read addr 0x1c size 4 [6]: pci_nvme_mmio_write addr 0x100c data 0x4e size 4 [7]: pci_nvme_mmio_doorbell_cq cqid 1 new_head 78 --- Interrupt deasserted (cq->tail == cq->head) [ 31.757821] nvme nvme0: I/O 333 QID 1 timeout, completion polled Following the timeout, everything returns to "normal" and device/driver happily continues. The pin-based interrupt logic in hw/nvme seems sound enough to me, so I am wondering if there is something going on with the kernel driver (but I certainly do not rule out that hw/nvme is at fault here, since pin-based interrupts has also been a source of several issues in the past). What I'm thinking is that following the interrupt in [1], the driver picks up completion for cid 4428 but does not find cid 4429 in the queue since it has not been posted yet. Before getting a cq head doorbell write (which would cause the pin to be deasserted), the device posts the completion for cid 4429 which just keeps the interrupt asserted in [2]. The trace then shows the cq head doorbell update in [3,4] for cid 4428 and then we hit the timeout since the driver is not aware that cid 4429 has been posted in between this (why is it not aware of this?) Timing out, the driver then polls the queue and notices cid 4429 and updates the cq head doorbell in [5-7], causing the device to deassert the interrupt and we are "back in shape". I'm observing this on 6.0 kernels and v6.2-rc3 (have not tested <6.0). Tested on QEMU v7.0.0 (to rule out all the
Re: [PATCH] m25p80: Add the is25wp256 SFPD table
On Mon, Jan 02, 2023 at 06:42:03PM +0100, Michael Walle wrote: > Am 2023-01-02 17:23, schrieb Guenter Roeck: > > On Mon, Jan 02, 2023 at 04:43:49PM +0100, Michael Walle wrote: > > > Am 2023-01-02 14:53, schrieb Cédric Le Goater: > > > > On 12/27/22 07:31, Tudor Ambarus wrote: > > > > > > > > > > > > > > > On 25.12.2022 14:18, Ben Dooks wrote: > > > > > > On Wed, Dec 21, 2022 at 06:36:02PM +0100, Cédric Le Goater wrote: > > > > > > > On 12/21/22 13:22, Guenter Roeck wrote: > > > > > > > > Generated from hardware using the following command and > > > > > > > > then padding > > > > > > > > with 0xff to fill out a power-of-2: > > > > > > > > xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp > > > > > > > > > > > > > > > > Cc: Michael Walle > > > > > > > > Cc: Tudor Ambarus > > > > > > > > Signed-off-by: Guenter Roeck > > > > > > > > > > > > > > Reviewed-by: Cédric Le Goater > > > > > > > > > > > > If SFDP is a standard, couldn't we have an function to generate > > > > > > it from > > > > > > the flash parameters? > > > > > > > > > > > > > > > > No, it's not practical as you have to specify all the flash parameters > > > > > at flash declaration. > > > > > > > > Indeed and the definition of flash models in QEMU is far to cover all > > > > the SFDP > > > > features. The known_devices array of m25p80 would be huge ! However, we > > > > could > > > > generate some of the SFDP tables if no raw data is provided. It could be > > > > useful > > > > for testing drivers. > > > > > > I don't think adding (incomplete) SFDP tables makes sense for any real > > > devices. E.g. sometimes our linux driver will look at specific bits in > > > SFDP to figure out what particular flash device is attached. For > > > example > > > when there are different flashes with the same jedec id. > > > > > > But since the last released kernel, we support a generic SFDP > > > driver, which > > > is used when there is no matching driver for the flash's jedec id. > > > Theoretically, you can build your own flash device (with a unique > > > id) and > > > generate the sfdp tables for that one. > > > > > How about older kernels versions ? Would those still support such > > (virtual ?) > > flash devices ? > > No with older kernels you'd be out of luck. They will just print "unknown > flash > id" and skip the device. That would mean that qemu versions including this change could no longer be used to test flash support on older kernel versions. That would be extremely undesirable. I'd rather live with the current code and still be able to test older kernels. Thanks, Guenter
Re: [PATCH] m25p80: Add the is25wp256 SFPD table
On Mon, Jan 02, 2023 at 04:43:49PM +0100, Michael Walle wrote: > Am 2023-01-02 14:53, schrieb Cédric Le Goater: > > On 12/27/22 07:31, Tudor Ambarus wrote: > > > > > > > > > On 25.12.2022 14:18, Ben Dooks wrote: > > > > On Wed, Dec 21, 2022 at 06:36:02PM +0100, Cédric Le Goater wrote: > > > > > On 12/21/22 13:22, Guenter Roeck wrote: > > > > > > Generated from hardware using the following command and > > > > > > then padding > > > > > > with 0xff to fill out a power-of-2: > > > > > > xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp > > > > > > > > > > > > Cc: Michael Walle > > > > > > Cc: Tudor Ambarus > > > > > > Signed-off-by: Guenter Roeck > > > > > > > > > > Reviewed-by: Cédric Le Goater > > > > > > > > If SFDP is a standard, couldn't we have an function to generate > > > > it from > > > > the flash parameters? > > > > > > > > > > No, it's not practical as you have to specify all the flash parameters > > > at flash declaration. > > > > Indeed and the definition of flash models in QEMU is far to cover all > > the SFDP > > features. The known_devices array of m25p80 would be huge ! However, we > > could > > generate some of the SFDP tables if no raw data is provided. It could be > > useful > > for testing drivers. > > I don't think adding (incomplete) SFDP tables makes sense for any real > devices. E.g. sometimes our linux driver will look at specific bits in > SFDP to figure out what particular flash device is attached. For example > when there are different flashes with the same jedec id. > > But since the last released kernel, we support a generic SFDP driver, which > is used when there is no matching driver for the flash's jedec id. > Theoretically, you can build your own flash device (with a unique id) and > generate the sfdp tables for that one. > How about older kernels versions ? Would those still support such (virtual ?) flash devices ? Thanks, Guenter
[PATCH] m25p80: Add the is25wp256 SFPD table
Generated from hardware using the following command and then padding
with 0xff to fill out a power-of-2:
xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
Cc: Michael Walle
Cc: Tudor Ambarus
Signed-off-by: Guenter Roeck
---
hw/block/m25p80.c | 3 ++-
hw/block/m25p80_sfdp.c | 40
hw/block/m25p80_sfdp.h | 2 ++
3 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 02adc87527..802d2eb021 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -221,7 +221,8 @@ static const FlashPartInfo known_devices[] = {
{ INFO("is25wp032", 0x9d7016, 0, 64 << 10, 64, ER_4K) },
{ INFO("is25wp064", 0x9d7017, 0, 64 << 10, 128, ER_4K) },
{ INFO("is25wp128", 0x9d7018, 0, 64 << 10, 256, ER_4K) },
-{ INFO("is25wp256", 0x9d7019, 0, 64 << 10, 512, ER_4K) },
+{ INFO("is25wp256", 0x9d7019, 0, 64 << 10, 512, ER_4K),
+ .sfdp_read = m25p80_sfdp_is25wp256 },
/* Macronix */
{ INFO("mx25l2005a", 0xc22012, 0, 64 << 10, 4, ER_4K) },
diff --git a/hw/block/m25p80_sfdp.c b/hw/block/m25p80_sfdp.c
index 77615fa29e..b33811a4f5 100644
--- a/hw/block/m25p80_sfdp.c
+++ b/hw/block/m25p80_sfdp.c
@@ -330,3 +330,43 @@ static const uint8_t sfdp_w25q01jvq[] = {
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
};
define_sfdp_read(w25q01jvq);
+
+/*
+ * Integrated Silicon Solution (ISSI)
+ */
+
+static const uint8_t sfdp_is25wp256[] = {
+0x53, 0x46, 0x44, 0x50, 0x06, 0x01, 0x01, 0xff,
+0x00, 0x06, 0x01, 0x10, 0x30, 0x00, 0x00, 0xff,
+0x9d, 0x05, 0x01, 0x03, 0x80, 0x00, 0x00, 0x02,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xe5, 0x20, 0xf9, 0xff, 0xff, 0xff, 0xff, 0x0f,
+0x44, 0xeb, 0x08, 0x6b, 0x08, 0x3b, 0x80, 0xbb,
+0xfe, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0xff,
+0xff, 0xff, 0x44, 0xeb, 0x0c, 0x20, 0x0f, 0x52,
+0x10, 0xd8, 0x00, 0xff, 0x23, 0x4a, 0xc9, 0x00,
+0x82, 0xd8, 0x11, 0xce, 0xcc, 0xcd, 0x68, 0x46,
+0x7a, 0x75, 0x7a, 0x75, 0xf7, 0xae, 0xd5, 0x5c,
+0x4a, 0x42, 0x2c, 0xff, 0xf0, 0x30, 0xfa, 0xa9,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0x50, 0x19, 0x50, 0x16, 0x9f, 0xf9, 0xc0, 0x64,
+0x8f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+};
+define_sfdp_read(is25wp256);
diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
index df7adfb5ce..011a880f66 100644
--- a/hw/block/m25p80_sfdp.h
+++ b/hw/block/m25p80_sfdp.h
@@ -26,4 +26,6 @@ uint8_t m25p80_sfdp_w25q512jv(uint32_t addr);
uint8_t m25p80_sfdp_w25q01jvq(uint32_t addr);
+uint8_t m25p80_sfdp_is25wp256(uint32_t addr);
+
#endif
--
2.36.2
Re: [PATCH v2 3/3] hw/nvme: fix missing cq eventidx update
On Thu, Dec 08, 2022 at 01:26:42PM +0100, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Prior to reading the shadow doorbell cq head, we have to update the
> eventidx. Otherwise, we risk that the driver will skip an mmio doorbell
> write. This happens on riscv64, as reported by Guenter.
>
> Adding the missing update to the cq eventidx fixes the issue.
>
> Fixes: 3f7fe8de3d49 ("hw/nvme: Implement shadow doorbell buffer support")
> Cc: qemu-sta...@nongnu.org
> Cc: qemu-ri...@nongnu.org
> Reported-by: Guenter Roeck
> Signed-off-by: Klaus Jensen
Tested-by: Guenter Roeck
> ---
> hw/nvme/ctrl.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index cfab21b3436e..f6cc766aba4a 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -1334,6 +1334,14 @@ static inline void nvme_blk_write(BlockBackend *blk,
> int64_t offset,
> }
> }
>
> +static void nvme_update_cq_eventidx(const NvmeCQueue *cq)
> +{
> +trace_pci_nvme_update_cq_eventidx(cq->cqid, cq->head);
> +
> +pci_dma_write(PCI_DEVICE(cq->ctrl), cq->ei_addr, >head,
> + sizeof(cq->head));
> +}
> +
> static void nvme_update_cq_head(NvmeCQueue *cq)
> {
> pci_dma_read(PCI_DEVICE(cq->ctrl), cq->db_addr, >head,
> @@ -1355,6 +1363,7 @@ static void nvme_post_cqes(void *opaque)
> hwaddr addr;
>
> if (n->dbbuf_enabled) {
> +nvme_update_cq_eventidx(cq);
> nvme_update_cq_head(cq);
> }
>
Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands
On 7/21/20 10:36 AM, Cédric Le Goater wrote:
> Hello,
>
> On 2/6/20 7:32 PM, Guenter Roeck wrote:
>> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
>> always requests 6 bytes. The current implementation only returns three
>> bytes, and interprets the remaining three bytes as new commands.
>> While this does not matter most of the time, it is at the very least
>> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
>> data. Fill remaining data with 0.
>>
>> Signed-off-by: Guenter Roeck
>> ---
>> v2: Split patch into two parts; improved decription
>>
>> hw/block/m25p80.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 5ff8d270c4..53bf63856f 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>> for (i = 0; i < s->pi->id_len; i++) {
>> s->data[i] = s->pi->id[i];
>> }
>> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
>> +s->data[i] = 0;
>> +}
>
> This is breaking an old firmware (Linux version 2.6.28.9) for a SuperMicro
> board :
>
> https://www.supermicro.com/en/products/motherboard/X11SSL-F
>
> which expects the flash ID to repeat. Erik solved the problem with the patch
> below and I think it makes sense to wrap around. Anyone knows what should be
> the expected behavior ?
>
No idea what the expected behavior is, but I am fine with the code below
if it works.
Thanks,
Guenter
> Thanks,
>
> C.
>
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8227088441..5000930800 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> s->data[i] = s->pi->id[i];
> }
> for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> -s->data[i] = 0;
> +s->data[i] = s->pi->id[i % s->pi->id_len];
> }
>
> s->len = SPI_NOR_MAX_ID_LEN;
>
[PATCH] pflash: cfi02: Convert debug log to tracing
When trying to track down problems such as failing unlock sequences
it is essential to have a complete trace log. Having part of it as debug
output and the rest as trace output is counter-productive. Convert all
debug logs to tracing.
Signed-off-by: Guenter Roeck
---
hw/block/pflash_cfi02.c | 62 +++--
hw/block/trace-events | 18
2 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index ac7e34ecbf..4c6e8e3b9b 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -47,14 +47,6 @@
#include "migration/vmstate.h"
#include "trace.h"
-#define PFLASH_DEBUG false
-#define DPRINTF(fmt, ...) \
-do { \
-if (PFLASH_DEBUG) {\
-fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__); \
-} \
-} while (0)
-
#define PFLASH_LAZY_ROMD_THRESHOLD 42
/*
@@ -232,11 +224,10 @@ static void pflash_timer(void *opaque)
uint64_t timeout = pflash_erase_time(pfl);
timer_mod(>timer,
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
-DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
-__func__, pfl->sectors_to_erase);
+trace_pflash_sector_erase_timeout(pfl->sectors_to_erase);
return;
}
-DPRINTF("%s: sector erase complete\n", __func__);
+trace_pflash_sector_erase_complete();
bitmap_zero(pfl->sector_erase_map, pfl->total_sectors);
pfl->sectors_to_erase = 0;
reset_dq3(pfl);
@@ -324,7 +315,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset,
unsigned int width)
switch (pfl->cmd) {
default:
/* This should never happen : reset state & treat it as a read*/
-DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
+trace_pflash_unknown_command_state(pfl->cmd);
pfl->wcycle = 0;
pfl->cmd = 0;
/* fall through to the read code */
@@ -337,7 +328,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset,
unsigned int width)
toggle_dq2(pfl);
/* Status register read */
ret = pfl->status;
-DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
+trace_pflash_status(ret);
break;
}
/* Flash area read */
@@ -362,7 +353,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset,
unsigned int width)
default:
ret = pflash_data_read(pfl, offset, width);
}
-DPRINTF("%s: ID " TARGET_FMT_plx " %" PRIx64 "\n", __func__, boff,
ret);
+trace_pflash_flash_id(boff, ret);
break;
case 0x10: /* Chip Erase */
case 0x30: /* Sector Erase */
@@ -374,7 +365,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset,
unsigned int width)
toggle_dq6(pfl);
/* Status register read */
ret = pfl->status;
-DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
+trace_pflash_status(ret);
break;
case 0x98:
/* CFI query mode */
@@ -414,9 +405,7 @@ static void pflash_sector_erase(PFlashCFI02 *pfl, hwaddr
offset)
SectorInfo sector_info = pflash_sector_info(pfl, offset);
uint64_t sector_len = sector_info.len;
offset &= ~(sector_len - 1);
-DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
-__func__, pfl->width * 2, offset,
-pfl->width * 2, offset + sector_len - 1);
+trace_pflash_sector_erase_start(offset, offset + sector_len - 1);
if (!pfl->ro) {
uint8_t *p = pfl->storage;
memset(p + offset, 0xff, sector_len);
@@ -495,27 +484,24 @@ static void pflash_write(void *opaque, hwaddr offset,
uint64_t value,
return;
}
if (boff != pfl->unlock_addr0 || cmd != 0xAA) {
-DPRINTF("%s: unlock0 failed " TARGET_FMT_plx " %02x %04x\n",
-__func__, boff, cmd, pfl->unlock_addr0);
+trace_pflash_unlock0_failed(boff, cmd, pfl->unlock_addr0);
goto reset_flash;
}
-DPRINTF("%s: unlock sequence started\n", __func__);
+trace_pflash_unlock_sequence_started();
break;
case 1:
/* We started an unlock sequence */
check_unlock1:
if (boff != pfl->unlock_addr1 || cmd != 0x55) {
-DPRINTF("%s: unlock1 failed " TARGET_FMT_plx " %02x\n", __func__,
-boff, cmd);
+trace_pflash_unlock1_failed(boff
Re: Question about (and problem with) pflash data access
On Thu, Feb 13, 2020 at 04:24:24PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/13/20 3:39 PM, Peter Maydell wrote:
> > On Thu, 13 Feb 2020 at 14:26, Guenter Roeck wrote:
> > > What really puzzles me is that there is no trace output for
> > > flash data accesses (trace_pflash_data_read and trace_pflash_data_write),
> > > meaning the actual flash data access must be handled elsewhere.
> > > Can someone give me a hint where that might be ?
>
> If you can share built kernel/dtb/rootfs for this machine I can have a look
> at it.
>
> > > Clearly I am missing something about inner workings of qemu.
>
> You can see all the pflash events using '-trace pflash*'.
>
Yes, I got that much ;-).
> >
> > Probably the device is in 'romd' mode. A QEMU MemoryRegion
> > can be:
> > * RAM (includes ROM for these purposes) -- backed by host
> > memory, reads and writes (if permitted) go straight to
> > the host memory via fastpath accesses
>
> No tracing here.
>
> > * MMIO -- backed by a read and write accessor function,
> > all accesses go to these functions
> > * "ROM device" -- a mix of the above where there is a
> > backing bit of host memory but also accessor functions.
> > When the device is in "romd" mode, reads go direct to
> > host memory, and writes still go to the accessor function.
> > When the device is not in "romd" mode, reads also go
> > to the accessor function.
> >
> > We use this in the pflash devices to make the common case
> > ("just read the flash") fast. When the guest makes a write
> > to flash that puts it into programming mode, we call
> > memory_region_rom_device_set_romd(..., false) so we can
> > intercept reads and make them do the right thing for
> > programming mode.
> >
Disabling the calls to memory_region_rom_device_set_romd(..., true)
got me the trace output I was looking for. Turns out that reads
which supposedly are from the beginning of the flash start at offset
0x18 instead of 0. This explains the "corruption", since that is
exactly the data in my test file at that offset. Adding debug output
to the Linux kernel confirms that this offset originates from the Linux
kernel.
Taking a closer look into the kernel source shows that the flash partitions
for SX1 indeed start at offset 0x18 in the flash, not at 0. Bummer.
Sorry for all the noise. I should have paid closer attention to the
kernel source. Oh well, at least I learned a lot about qemu.
Thanks,
Guenter
Re: Question about (and problem with) pflash data access
On 2/13/20 1:51 AM, Paolo Bonzini wrote: On 13/02/20 08:40, Alexey Kardashevskiy wrote: memory-region: system - (prio 0, i/o): system -01ff (prio 0, romd): omap_sx1.flash0-1 -01ff (prio 0, rom): omap_sx1.flash0-0 Eh two memory regions with same size and same priority... Is this legal? I'd say yes if used with memory_region_set_enabled() to make sure only one is enabled. Having both enabled is weird and we should print a warning. Yeah, it's undefined which one becomes visible. I have a patch fixing that, resulting in (qemu) info mtree -f FlatView #0 AS "I/O", root: io Root memory region: io - (prio 0, i/o): io FlatView #1 AS "memory", root: system AS "cpu-memory-0", root: system Root memory region: system -01ff (prio 0, romd): omap_sx1.flash0 0200-03ff (prio 0, i/o): sx1.cs0 0400-07ff (prio 0, i/o): sx1.cs1 0800-0bff (prio 0, i/o): sx1.cs2 0c00-0fff (prio 0, i/o): sx1.cs3 ... but unfortunately that doesn't fix my problem. The data in the omap_sx1.flash0 region is as wrong as before. What really puzzles me is that there is no trace output for flash data accesses (trace_pflash_data_read and trace_pflash_data_write), meaning the actual flash data access must be handled elsewhere. Can someone give me a hint where that might be ? Clearly I am missing something about inner workings of qemu. Thanks, Guenter
Re: Question about (and problem with) pflash data access
On Wed, Feb 12, 2020 at 10:39:30PM +0100, Philippe Mathieu-Daudé wrote:
> Cc'ing Jean-Christophe and Peter.
>
> On 2/12/20 7:46 PM, Guenter Roeck wrote:
> > Hi,
> >
> > I have been playing with pflash recently. For the most part it works,
> > but I do have an odd problem when trying to instantiate pflash on sx1.
> >
> > My data file looks as follows.
> >
> > 000 0001
> > 020
> > *
> > 0002000 0002
> > 0002020
> > *
> > 0004000 0003
> > 0004020
> > ...
> >
> > In the sx1 machine, this becomes:
> >
> > 000 6001
> > 020
> > *
> > 0002000 6002
> > 0002020
> > *
> > 0004000 6003
> > 0004020
> > *
> > ...
> >
> > pflash is instantiated with "-drive
> > file=flash.32M.test,format=raw,if=pflash".
> >
> > I don't have much success with pflash tracing - data accesses don't
> > show up there.
> >
> > I did find a number of problems with the sx1 emulation, but I have no clue
> > what is going on with pflash. As far as I can see pflash works fine on
> > other machines. Can someone give me a hint what to look out for ?
>
> This is specific to the SX1, introduced in commit 997641a84ff:
>
> 64 static uint64_t static_read(void *opaque, hwaddr offset,
> 65 unsigned size)
> 66 {
> 67 uint32_t *val = (uint32_t *) opaque;
> 68 uint32_t mask = (4 / size) - 1;
> 69
> 70 return *val >> ((offset & mask) << 3);
> 71 }
>
> Only guessing, this looks like some hw parity, and I imagine you need to
> write the parity bits in your flash.32M file before starting QEMU, then it
> would appear "normal" within the guest.
>
I thought this might be related, but that is not the case. I added log
messages, and even ran the code in gdb. static_read() and static_write()
are not executed.
Also,
memory_region_init_io([0], NULL, _ops, ,
"sx1.cs0", OMAP_CS0_SIZE - flash_size);
^^
memory_region_add_subregion(address_space,
OMAP_CS0_BASE + flash_size, [0]);
^^
suggests that the code is only executed for memory accesses _after_
the actual flash. The memory tree is:
memory-region: system
- (prio 0, i/o): system
-01ff (prio 0, romd): omap_sx1.flash0-1
-01ff (prio 0, rom): omap_sx1.flash0-0
0200-03ff (prio 0, i/o): sx1.cs0
I thought that the dual memory assignment (omap_sx1.flash0-1 and
omap_sx1.flash0-0) might play a role, but removing that didn't make
a difference either (not that I have any idea what it is supposed
to be used for).
Thanks,
Guenter
Question about (and problem with) pflash data access
Hi, I have been playing with pflash recently. For the most part it works, but I do have an odd problem when trying to instantiate pflash on sx1. My data file looks as follows. 000 0001 020 * 0002000 0002 0002020 * 0004000 0003 0004020 ... In the sx1 machine, this becomes: 000 6001 020 * 0002000 6002 0002020 * 0004000 6003 0004020 * ... pflash is instantiated with "-drive file=flash.32M.test,format=raw,if=pflash". I don't have much success with pflash tracing - data accesses don't show up there. I did find a number of problems with the sx1 emulation, but I have no clue what is going on with pflash. As far as I can see pflash works fine on other machines. Can someone give me a hint what to look out for ? Thanks, Guenter
[PATCH v2 4/4] aspeed/smc: Fix number of dummy cycles for FAST_READ_4 command
The Linux kernel recently started using FAST_READ_4 commands.
This results in flash read failures. At the same time, the m25p80
emulation is seen to read 8 more bytes than expected. Adjusting the
expected number of dummy cycles to match FAST_READ fixes the problem.
Fixes: f95c4bffdc4c ("aspeed/smc: snoop SPI transfers to fake dummy cycles")
Reviewed-by: Cédric Le Goater
Signed-off-by: Guenter Roeck
---
v2: No change
hw/ssi/aspeed_smc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 23c8d2f062..0444570750 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -787,11 +787,11 @@ static int aspeed_smc_num_dummies(uint8_t command)
case FAST_READ:
case DOR:
case QOR:
+case FAST_READ_4:
case DOR_4:
case QOR_4:
return 1;
case DIOR:
-case FAST_READ_4:
case DIOR_4:
return 2;
case QIOR:
--
2.17.1
[PATCH v2 2/4] m25p80: Improve command handling for Jedec commands
When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
always requests 6 bytes. The current implementation only returns three
bytes, and interprets the remaining three bytes as new commands.
While this does not matter most of the time, it is at the very least
confusing. To avoid the problem, always report up to 6 bytes of JEDEC
data. Fill remaining data with 0.
Signed-off-by: Guenter Roeck
---
v2: Split patch into two parts; improved decription
hw/block/m25p80.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 5ff8d270c4..53bf63856f 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
for (i = 0; i < s->pi->id_len; i++) {
s->data[i] = s->pi->id[i];
}
+for (; i < SPI_NOR_MAX_ID_LEN; i++) {
+s->data[i] = 0;
+}
-s->len = s->pi->id_len;
+s->len = SPI_NOR_MAX_ID_LEN;
s->pos = 0;
s->state = STATE_READING_DATA;
break;
--
2.17.1
[PATCH v2 1/4] m25p80: Convert to support tracing
While at it, add some trace messages to help debug problems
seen when running the latest Linux kernel.
Signed-off-by: Guenter Roeck
---
v2: Print pointer to Flash data structure as flash ID with each trace
message to support systems with more than one instantiated flash.
hw/block/m25p80.c | 48 ---
hw/block/trace-events | 16 +++
2 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 61f2fb8f8f..5ff8d270c4 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -32,17 +32,7 @@
#include "qemu/module.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
-
-#ifndef M25P80_ERR_DEBUG
-#define M25P80_ERR_DEBUG 0
-#endif
-
-#define DB_PRINT_L(level, ...) do { \
-if (M25P80_ERR_DEBUG > (level)) { \
-fprintf(stderr, ": %s: ", __func__); \
-fprintf(stderr, ## __VA_ARGS__); \
-} \
-} while (0)
+#include "trace.h"
/* Fields for FlashPartInfo->flags */
@@ -574,7 +564,8 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
abort();
}
-DB_PRINT_L(0, "offset = %#x, len = %d\n", offset, len);
+trace_m25p80_flash_erase(s, offset, len);
+
if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d erase size not supported by"
" device\n", len);
@@ -607,8 +598,7 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
}
if ((prev ^ data) & data) {
-DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 " %" PRIx8
- " -> %" PRIx8 "\n", addr, prev, data);
+trace_m25p80_programming_zero_to_one(s, addr, prev, data);
}
if (s->pi->flags & EEPROM) {
@@ -662,6 +652,9 @@ static void complete_collecting_data(Flash *s)
s->state = STATE_IDLE;
+trace_m25p80_complete_collecting(s, s->cmd_in_progress, n, s->ear,
+ s->cur_addr);
+
switch (s->cmd_in_progress) {
case DPP:
case QPP:
@@ -825,7 +818,7 @@ static void reset_memory(Flash *s)
break;
}
-DB_PRINT_L(0, "Reset done.\n");
+trace_m25p80_reset_done(s);
}
static void decode_fast_read_cmd(Flash *s)
@@ -941,9 +934,10 @@ static void decode_qio_read_cmd(Flash *s)
static void decode_new_cmd(Flash *s, uint32_t value)
{
-s->cmd_in_progress = value;
int i;
-DB_PRINT_L(0, "decoded new command:%x\n", value);
+
+s->cmd_in_progress = value;
+trace_m25p80_command_decoded(s, value);
if (value != RESET_MEMORY) {
s->reset_enable = false;
@@ -1042,7 +1036,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
break;
case JEDEC_READ:
-DB_PRINT_L(0, "populated jedec code\n");
+trace_m25p80_populated_jedec(s);
for (i = 0; i < s->pi->id_len; i++) {
s->data[i] = s->pi->id[i];
}
@@ -1063,7 +1057,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
case BULK_ERASE_60:
case BULK_ERASE:
if (s->write_enable) {
-DB_PRINT_L(0, "chip erase\n");
+trace_m25p80_chip_erase(s);
flash_erase(s, 0, BULK_ERASE);
} else {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: chip erase with write "
@@ -1184,7 +1178,7 @@ static int m25p80_cs(SSISlave *ss, bool select)
s->data_read_loop = false;
}
-DB_PRINT_L(0, "%sselect\n", select ? "de" : "");
+trace_m25p80_select(s, select ? "de" : "");
return 0;
}
@@ -1194,19 +1188,20 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t
tx)
Flash *s = M25P80(ss);
uint32_t r = 0;
+trace_m25p80_transfer(s, s->state, s->len, s->needed_bytes, s->pos,
+ s->cur_addr, (uint8_t)tx);
+
switch (s->state) {
case STATE_PAGE_PROGRAM:
-DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8 "\n",
- s->cur_addr, (uint8_t)tx);
+trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
flash_write8(s, s->cur_addr, (uint8_t)tx);
s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
case STATE_READ:
r = s->storage[s->cur_addr];
-DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
- (uint8_t)r);
+trace_m25p80_read_byte(s, s->cur_addr, (uint8_t)r);
s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
@@ -1244,6 +1239,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss,
[PATCH v2 3/4] m25p80: Improve command handling for unsupported commands
Whenever an unsupported command is encountered, the current code interprets each transferred byte as new command. Most of the time, those 'commands' are interpreted as new unknown commands. However, in rare cases, it may be that for example address or length information passed with the original command is by itself a valid command. If that happens, the state machine may get completely confused and, worst case, start writing data into the flash or even erase it. To avoid the problem, transition into STATE_READING_DATA and keep sending a value of 0 until the chip is deselected after encountering an unsupported command. Signed-off-by: Guenter Roeck --- v2: Split patch into two parts; improved description. hw/block/m25p80.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 53bf63856f..8227088441 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -1161,6 +1161,11 @@ static void decode_new_cmd(Flash *s, uint32_t value) s->quad_enable = false; break; default: +s->pos = 0; +s->len = 1; +s->state = STATE_READING_DATA; +s->data_read_loop = true; +s->data[0] = 0; qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value); break; } -- 2.17.1
Re: [PATCH 2/3] m25p80: Improve command handling for Jedec and unsupported commands
On 2/5/20 11:04 PM, Joel Stanley wrote: On Wed, 5 Feb 2020 at 17:43, Guenter Roeck wrote: On Wed, Feb 05, 2020 at 11:08:07AM +0100, Cédric Le Goater wrote: ok. we will need to model SFDP one day. Are you using the OpenBMC images or do you have your own ? I am running images built from upstream/stable kernel branches. I think Cédric was wondering which flash images and therefore filesystems you were using in your testing. Ah, ok. Sorry, misunderstood. I had a poke around here but I couldn't work out where 'mtd32' came from: https://github.com/groeck/linux-build-test/blob/master/rootfs/arm/run-qemu-arm.sh mtd32 tells the infrastructure that the boot device is mtd (flash) with 32MB capacity. The infrastructure uses that to create the actual flash image and to generate the qemu command line. The root file system is is rootfs-armv5.ext2 (located in the same directory as the run script). It was generated using buildroot. Guenter
Re: [PATCH 1/3] m25p80: Convert to support tracing
On Wed, Feb 05, 2020 at 06:10:44PM +0100, Cédric Le Goater wrote: > On 2/5/20 5:35 PM, Guenter Roeck wrote: > > On Wed, Feb 05, 2020 at 11:05:04AM +0100, Cédric Le Goater wrote: > >> On 2/3/20 7:09 PM, Guenter Roeck wrote: > >>> While at it, add some trace messages to help debug problems > >>> seen when running the latest Linux kernel. > >> > >> In case you resend, It would be nice to printout a flash id in the tracing > >> else I have a patch for it on top of yours. It helped me track a squashfs > >> corruption on the Aspeed witherspoon-bmc machine which we were after since > >> 2017 or so. It seems to be a kernel bug. > >> > > > > I'll send a new version to split patch 2. Not sure I understand what you > > mean > > with the above. If you send me your patch I'll be happy to merge it into > > mine, > > otherwise we can just keep it as follow-ip patch. > > Here is the idea : > > > https://github.com/legoater/qemu/commit/a07727e9cfc8447ea18249ff68a561f7e8883584 > Ah, that. I had thought about doing that, but I found displaying the pointer a bit clumsy. It looks like there isn't really anything else available that would provide a flash index, and I agree that it would be useful, so I'll add it for all traces. Thanks, Guenter
Re: [PATCH 2/3] m25p80: Improve command handling for Jedec and unsupported commands
On Wed, Feb 05, 2020 at 11:08:07AM +0100, Cédric Le Goater wrote:
> On 2/4/20 3:28 PM, Guenter Roeck wrote:
> > On 2/4/20 12:53 AM, Cédric Le Goater wrote:
> >> On 2/3/20 7:09 PM, Guenter Roeck wrote:
> >>> Always report 6 bytes of JEDEC data. Fill remaining data with 0.
> >>>
> >>> For unsupported commands, keep sending a value of 0 until the chip
> >>> is deselected.
> >>>
> >>> Both changes avoid attempts to decode random commands. Up to now this
> >>> happened if the reported Jedec data was shorter than 6 bytes but the
> >>> host read 6 bytes, and with all unsupported commands.
> >>
> >> Do you have a concrete example for that ? machine and flash model.
> >>
> >
> > I noticed it while tracking down the bug fixed in patch 3 of the series,
> > ie with AST2500 evb using w25q256 flash, but it happens with all machines
> > using SPI NOR flash (ie all aspeed bmc machines) when running the Linux
> > kernel. Most of the time it doesn't cause harm, unless the host sends
> > an "address" as part of an unsupported command which happens to include
> > a valid command code.
>
> ok. we will need to model SFDP one day.
>
> Are you using the OpenBMC images or do you have your own ?
>
I am running images built from upstream/stable kernel branches.
Guenter
> >
> >>> Signed-off-by: Guenter Roeck
> >>> ---
> >>> hw/block/m25p80.c | 10 +-
> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >>> index 63e050d7d3..aca75edcc1 100644
> >>> --- a/hw/block/m25p80.c
> >>> +++ b/hw/block/m25p80.c
> >>> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t
> >>> value)
> >>> for (i = 0; i < s->pi->id_len; i++) {
> >>> s->data[i] = s->pi->id[i];
> >>> }
> >>> + for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> >>> + s->data[i] = 0;
> >>> + }
> >>
> >> It seems that data should be reseted in m25p80_cs() also.
> >>
> > Are you sure ?
> >
> > The current implementation sets s->data[] as needed when command decode
> > is complete. That seems less costly to me.
>
> ok.
> Reviewed-by: Cédric Le Goater
>
>
> Thanks,
>
> C.
>
> > Thanks,
> > Guenter
> >
> >>> - s->len = s->pi->id_len;
> >>> + s->len = SPI_NOR_MAX_ID_LEN;
> >>> s->pos = 0;
> >>> s->state = STATE_READING_DATA;
> >>> break;
> >>> @@ -1158,6 +1161,11 @@ static void decode_new_cmd(Flash *s, uint32_t
> >>> value)
> >>> s->quad_enable = false;
> >>> break;
> >>> default:
> >>> + s->pos = 0;
> >>> + s->len = 1;
> >>> + s->state = STATE_READING_DATA;
> >>> + s->data_read_loop = true;
> >>> + s->data[0] = 0;
> >>> qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n",
> >>> value);
> >>> break;
> >>> }
> >>>
> >>
> >
>
Re: [PATCH 1/3] m25p80: Convert to support tracing
On Wed, Feb 05, 2020 at 11:05:04AM +0100, Cédric Le Goater wrote: > On 2/3/20 7:09 PM, Guenter Roeck wrote: > > While at it, add some trace messages to help debug problems > > seen when running the latest Linux kernel. > > In case you resend, It would be nice to printout a flash id in the tracing > else I have a patch for it on top of yours. It helped me track a squashfs > corruption on the Aspeed witherspoon-bmc machine which we were after since > 2017 or so. It seems to be a kernel bug. > I'll send a new version to split patch 2. Not sure I understand what you mean with the above. If you send me your patch I'll be happy to merge it into mine, otherwise we can just keep it as follow-ip patch. Thanks, Guenter
Re: [PATCH 2/3] m25p80: Improve command handling for Jedec and unsupported commands
On 2/4/20 12:53 AM, Cédric Le Goater wrote:
On 2/3/20 7:09 PM, Guenter Roeck wrote:
Always report 6 bytes of JEDEC data. Fill remaining data with 0.
For unsupported commands, keep sending a value of 0 until the chip
is deselected.
Both changes avoid attempts to decode random commands. Up to now this
happened if the reported Jedec data was shorter than 6 bytes but the
host read 6 bytes, and with all unsupported commands.
Do you have a concrete example for that ? machine and flash model.
I noticed it while tracking down the bug fixed in patch 3 of the series,
ie with AST2500 evb using w25q256 flash, but it happens with all machines
using SPI NOR flash (ie all aspeed bmc machines) when running the Linux
kernel. Most of the time it doesn't cause harm, unless the host sends
an "address" as part of an unsupported command which happens to include
a valid command code.
Signed-off-by: Guenter Roeck
---
hw/block/m25p80.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 63e050d7d3..aca75edcc1 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
for (i = 0; i < s->pi->id_len; i++) {
s->data[i] = s->pi->id[i];
}
+for (; i < SPI_NOR_MAX_ID_LEN; i++) {
+s->data[i] = 0;
+}
It seems that data should be reseted in m25p80_cs() also.
Are you sure ?
The current implementation sets s->data[] as needed when command decode
is complete. That seems less costly to me.
Thanks,
Guenter
-s->len = s->pi->id_len;
+s->len = SPI_NOR_MAX_ID_LEN;
s->pos = 0;
s->state = STATE_READING_DATA;
break;
@@ -1158,6 +1161,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
s->quad_enable = false;
break;
default:
+s->pos = 0;
+s->len = 1;
+s->state = STATE_READING_DATA;
+s->data_read_loop = true;
+s->data[0] = 0;
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
break;
}
Re: [PATCH 3/3] aspeed/smc: Fix number of dummy cycles for FAST_READ_4 command
On 2/3/20 11:45 PM, Cédric Le Goater wrote:
On 2/3/20 7:09 PM, Guenter Roeck wrote:
The Linux kernel recently started using FAST_READ_4 commands.
This results in flash read failures. At the same time, the m25p80
emulation is seen to read 8 more bytes than expected. Adjusting the
expected number of dummy cycles to match FAST_READ fixes the problem.
Which machine are you using for these tests ? the AST2500 evb using
the w25q256 flash model ?
Correct.
Guenter
Any how, it looks correct.
Reviewed-by: Cédric Le Goater
Fixes: f95c4bffdc4c ("aspeed/smc: snoop SPI transfers to fake dummy cycles")
I think commit ef06ca3946e2 ("xilinx_spips: Add support for RX discard
and RX drain") needs a similar fix. Adding Francisco.
Thanks,
C.
Signed-off-by: Guenter Roeck
---
hw/ssi/aspeed_smc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index f0c7bbbad3..61e8fa57d3 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -762,11 +762,11 @@ static int aspeed_smc_num_dummies(uint8_t command)
case FAST_READ:
case DOR:
case QOR:
+case FAST_READ_4:
case DOR_4:
case QOR_4:
return 1;
case DIOR:
-case FAST_READ_4:
case DIOR_4:
return 2;
case QIOR:
Re: [PATCH 1/3] m25p80: Convert to support tracing
On 2/3/20 11:16 PM, Cédric Le Goater wrote:
On 2/3/20 7:09 PM, Guenter Roeck wrote:
While at it, add some trace messages to help debug problems
seen when running the latest Linux kernel.
Signed-off-by: Guenter Roeck
Reviewed-by: Cédric Le Goater
We have been chasing a bug for years on the witherspoon-bmc machine
using UBIfs. It will be useful.
What kind of issue are you looking at ?
aspeed-ast2500-evb no longer boots from flash with the current mainline kernel.
The problem is fixed with the 3rd patch of the series.
Guenter
Thanks,
C.
---
hw/block/m25p80.c | 48 ---
hw/block/trace-events | 16 +++
2 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 11ff5b9ad7..63e050d7d3 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -32,17 +32,7 @@
#include "qemu/module.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
-
-#ifndef M25P80_ERR_DEBUG
-#define M25P80_ERR_DEBUG 0
-#endif
-
-#define DB_PRINT_L(level, ...) do { \
-if (M25P80_ERR_DEBUG > (level)) { \
-fprintf(stderr, ": %s: ", __func__); \
-fprintf(stderr, ## __VA_ARGS__); \
-} \
-} while (0)
+#include "trace.h"
/* Fields for FlashPartInfo->flags */
@@ -574,7 +564,8 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
abort();
}
-DB_PRINT_L(0, "offset = %#x, len = %d\n", offset, len);
+trace_m25p80_flash_erase(offset, len);
+
if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d erase size not supported
by"
" device\n", len);
@@ -607,8 +598,7 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
}
if ((prev ^ data) & data) {
-DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 " %" PRIx8
- " -> %" PRIx8 "\n", addr, prev, data);
+trace_m25p80_programming_zero_to_one(addr, prev, data);
}
if (s->pi->flags & EEPROM) {
@@ -662,6 +652,9 @@ static void complete_collecting_data(Flash *s)
s->state = STATE_IDLE;
+trace_m25p80_complete_collecting(s->cmd_in_progress, n, s->ear,
+ s->cur_addr);
+
switch (s->cmd_in_progress) {
case DPP:
case QPP:
@@ -825,7 +818,7 @@ static void reset_memory(Flash *s)
break;
}
-DB_PRINT_L(0, "Reset done.\n");
+trace_m25p80_reset_done();
}
static void decode_fast_read_cmd(Flash *s)
@@ -941,9 +934,10 @@ static void decode_qio_read_cmd(Flash *s)
static void decode_new_cmd(Flash *s, uint32_t value)
{
-s->cmd_in_progress = value;
int i;
-DB_PRINT_L(0, "decoded new command:%x\n", value);
+
+s->cmd_in_progress = value;
+trace_m25p80_command_decoded(value);
if (value != RESET_MEMORY) {
s->reset_enable = false;
@@ -1042,7 +1036,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
break;
case JEDEC_READ:
-DB_PRINT_L(0, "populated jedec code\n");
+trace_m25p80_populated_jedec();
for (i = 0; i < s->pi->id_len; i++) {
s->data[i] = s->pi->id[i];
}
@@ -1063,7 +1057,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
case BULK_ERASE_60:
case BULK_ERASE:
if (s->write_enable) {
-DB_PRINT_L(0, "chip erase\n");
+trace_m25p80_chip_erase();
flash_erase(s, 0, BULK_ERASE);
} else {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: chip erase with write "
@@ -1184,7 +1178,7 @@ static int m25p80_cs(SSISlave *ss, bool select)
s->data_read_loop = false;
}
-DB_PRINT_L(0, "%sselect\n", select ? "de" : "");
+trace_m25p80_select(select ? "de" : "");
return 0;
}
@@ -1194,19 +1188,20 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t
tx)
Flash *s = M25P80(ss);
uint32_t r = 0;
+trace_m25p80_transfer(s->state, s->len, s->needed_bytes, s->pos,
+ s->cur_addr, (uint8_t)tx);
+
switch (s->state) {
case STATE_PAGE_PROGRAM:
-DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8 "\n",
- s->cur_addr, (uint8_t)tx);
+trace_m25p80_page_program(s->cur_addr, (uint8_t)tx);
flash_write8(s, s->cur_addr, (uint8_t)tx);
s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
case STATE_READ:
r = s->storage[s->c
[PATCH 1/3] m25p80: Convert to support tracing
While at it, add some trace messages to help debug problems
seen when running the latest Linux kernel.
Signed-off-by: Guenter Roeck
---
hw/block/m25p80.c | 48 ---
hw/block/trace-events | 16 +++
2 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 11ff5b9ad7..63e050d7d3 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -32,17 +32,7 @@
#include "qemu/module.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
-
-#ifndef M25P80_ERR_DEBUG
-#define M25P80_ERR_DEBUG 0
-#endif
-
-#define DB_PRINT_L(level, ...) do { \
-if (M25P80_ERR_DEBUG > (level)) { \
-fprintf(stderr, ": %s: ", __func__); \
-fprintf(stderr, ## __VA_ARGS__); \
-} \
-} while (0)
+#include "trace.h"
/* Fields for FlashPartInfo->flags */
@@ -574,7 +564,8 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
abort();
}
-DB_PRINT_L(0, "offset = %#x, len = %d\n", offset, len);
+trace_m25p80_flash_erase(offset, len);
+
if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d erase size not supported by"
" device\n", len);
@@ -607,8 +598,7 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
}
if ((prev ^ data) & data) {
-DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 " %" PRIx8
- " -> %" PRIx8 "\n", addr, prev, data);
+trace_m25p80_programming_zero_to_one(addr, prev, data);
}
if (s->pi->flags & EEPROM) {
@@ -662,6 +652,9 @@ static void complete_collecting_data(Flash *s)
s->state = STATE_IDLE;
+trace_m25p80_complete_collecting(s->cmd_in_progress, n, s->ear,
+ s->cur_addr);
+
switch (s->cmd_in_progress) {
case DPP:
case QPP:
@@ -825,7 +818,7 @@ static void reset_memory(Flash *s)
break;
}
-DB_PRINT_L(0, "Reset done.\n");
+trace_m25p80_reset_done();
}
static void decode_fast_read_cmd(Flash *s)
@@ -941,9 +934,10 @@ static void decode_qio_read_cmd(Flash *s)
static void decode_new_cmd(Flash *s, uint32_t value)
{
-s->cmd_in_progress = value;
int i;
-DB_PRINT_L(0, "decoded new command:%x\n", value);
+
+s->cmd_in_progress = value;
+trace_m25p80_command_decoded(value);
if (value != RESET_MEMORY) {
s->reset_enable = false;
@@ -1042,7 +1036,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
break;
case JEDEC_READ:
-DB_PRINT_L(0, "populated jedec code\n");
+trace_m25p80_populated_jedec();
for (i = 0; i < s->pi->id_len; i++) {
s->data[i] = s->pi->id[i];
}
@@ -1063,7 +1057,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
case BULK_ERASE_60:
case BULK_ERASE:
if (s->write_enable) {
-DB_PRINT_L(0, "chip erase\n");
+trace_m25p80_chip_erase();
flash_erase(s, 0, BULK_ERASE);
} else {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: chip erase with write "
@@ -1184,7 +1178,7 @@ static int m25p80_cs(SSISlave *ss, bool select)
s->data_read_loop = false;
}
-DB_PRINT_L(0, "%sselect\n", select ? "de" : "");
+trace_m25p80_select(select ? "de" : "");
return 0;
}
@@ -1194,19 +1188,20 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t
tx)
Flash *s = M25P80(ss);
uint32_t r = 0;
+trace_m25p80_transfer(s->state, s->len, s->needed_bytes, s->pos,
+ s->cur_addr, (uint8_t)tx);
+
switch (s->state) {
case STATE_PAGE_PROGRAM:
-DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8 "\n",
- s->cur_addr, (uint8_t)tx);
+trace_m25p80_page_program(s->cur_addr, (uint8_t)tx);
flash_write8(s, s->cur_addr, (uint8_t)tx);
s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
case STATE_READ:
r = s->storage[s->cur_addr];
-DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
- (uint8_t)r);
+trace_m25p80_read_byte(s->cur_addr, (uint8_t)r);
s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
@@ -1244,6 +1239,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t
tx)
}
r = s->data[s->pos];
+trace_m25p80_read_data(s->pos, (uint8_t)r);
s->pos++;
if (s->
[PATCH 2/3] m25p80: Improve command handling for Jedec and unsupported commands
Always report 6 bytes of JEDEC data. Fill remaining data with 0.
For unsupported commands, keep sending a value of 0 until the chip
is deselected.
Both changes avoid attempts to decode random commands. Up to now this
happened if the reported Jedec data was shorter than 6 bytes but the
host read 6 bytes, and with all unsupported commands.
Signed-off-by: Guenter Roeck
---
hw/block/m25p80.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 63e050d7d3..aca75edcc1 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
for (i = 0; i < s->pi->id_len; i++) {
s->data[i] = s->pi->id[i];
}
+for (; i < SPI_NOR_MAX_ID_LEN; i++) {
+s->data[i] = 0;
+}
-s->len = s->pi->id_len;
+s->len = SPI_NOR_MAX_ID_LEN;
s->pos = 0;
s->state = STATE_READING_DATA;
break;
@@ -1158,6 +1161,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
s->quad_enable = false;
break;
default:
+s->pos = 0;
+s->len = 1;
+s->state = STATE_READING_DATA;
+s->data_read_loop = true;
+s->data[0] = 0;
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
break;
}
--
2.17.1
[PATCH 3/3] aspeed/smc: Fix number of dummy cycles for FAST_READ_4 command
The Linux kernel recently started using FAST_READ_4 commands. This results in flash read failures. At the same time, the m25p80 emulation is seen to read 8 more bytes than expected. Adjusting the expected number of dummy cycles to match FAST_READ fixes the problem. Signed-off-by: Guenter Roeck --- hw/ssi/aspeed_smc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index f0c7bbbad3..61e8fa57d3 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -762,11 +762,11 @@ static int aspeed_smc_num_dummies(uint8_t command) case FAST_READ: case DOR: case QOR: +case FAST_READ_4: case DOR_4: case QOR_4: return 1; case DIOR: -case FAST_READ_4: case DIOR_4: return 2; case QIOR: -- 2.17.1
Re: [Qemu-block] [PATCH] nvme: Fix spurious interrupts
On Mon, Nov 26, 2018 at 10:17:45AM -0700, Keith Busch wrote:
> The code had asserted an interrupt every time it was requested to check
> for new completion queue entries.This can result in spurious interrupts
> seen by the guest OS.
>
> Fix this by asserting an interrupt only if there are un-acknowledged
> completion queue entries available.
>
> Reported-by: Guenter Roeck
> Signed-off-by: Keith Busch
Tested-by: Guenter Roeck
> ---
> hw/block/nvme.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9fbe5673cb..7c8c63e8f5 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
> sizeof(req->cqe));
> QTAILQ_INSERT_TAIL(>req_list, req, entry);
> }
> -nvme_irq_assert(n, cq);
> +if (cq->tail != cq->head) {
> +nvme_irq_assert(n, cq);
> +}
> }
>
> static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> --
> 2.14.4
>
Re: [Qemu-block] [PATCH] nvme: Only generate interupt if warranted
Hi Keith,
On Mon, Nov 26, 2018 at 09:17:17AM -0700, Keith Busch wrote:
> On Sun, Nov 25, 2018 at 03:03:55PM -0800, Guenter Roeck wrote:
> > So far the code generates interrupts even if it doesn't pass any new
> > information to the user. This can result in spurious interrupts as
> > sometimes observed with Linux.
> >
> > Only generate interrupts if warranted, ie if anything new is passed
> > to the emulated code.
> >
> > Signed-off-by: Guenter Roeck
> > ---
> > hw/block/nvme.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 9fbe567..c543c01 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
> > NvmeCQueue *cq = opaque;
> > NvmeCtrl *n = cq->ctrl;
> > NvmeRequest *req, *next;
> > +bool assert_irq = false;
> >
> > QTAILQ_FOREACH_SAFE(req, >req_list, entry, next) {
> > NvmeSQueue *sq;
> > @@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
> > pci_dma_write(>parent_obj, addr, (void *)>cqe,
> > sizeof(req->cqe));
> > QTAILQ_INSERT_TAIL(>req_list, req, entry);
> > +assert_irq = true;
> > +}
> > +if (assert_irq) {
> > +nvme_irq_assert(n, cq);
> > }
> > -nvme_irq_assert(n, cq);
> > }
> >
> > static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> > --
>
> There is a corner case this may break. For example, a controller posts
> 2 completions. The host happens to only see one of those completions due
> to the inherent race when reading the phase bit. After the host updates
> the CQ DB, the controller ought to send another interrupt even if it
> didn't post any new CQEs to prevent command completion timeouts. This
> might get the right behavior:
>
> ---
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..486db6e561 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
> sizeof(req->cqe));
> QTAILQ_INSERT_TAIL(>req_list, req, entry);
> }
> -nvme_irq_assert(n, cq);
> +if (cq->tail != cq->head) {
> +nvme_irq_assert(n, cq);
> +}
> }
>
That works for me as well, and looks much cleaner. Should I resubmit,
or do you want to take it from here ?
Thanks,
Guenter
[Qemu-block] [PATCH] nvme: Only generate interupt if warranted
So far the code generates interrupts even if it doesn't pass any new
information to the user. This can result in spurious interrupts as
sometimes observed with Linux.
Only generate interrupts if warranted, ie if anything new is passed
to the emulated code.
Signed-off-by: Guenter Roeck
---
hw/block/nvme.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9fbe567..c543c01 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
NvmeCQueue *cq = opaque;
NvmeCtrl *n = cq->ctrl;
NvmeRequest *req, *next;
+bool assert_irq = false;
QTAILQ_FOREACH_SAFE(req, >req_list, entry, next) {
NvmeSQueue *sq;
@@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
pci_dma_write(>parent_obj, addr, (void *)>cqe,
sizeof(req->cqe));
QTAILQ_INSERT_TAIL(>req_list, req, entry);
+assert_irq = true;
+}
+if (assert_irq) {
+nvme_irq_assert(n, cq);
}
-nvme_irq_assert(n, cq);
}
static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
--
2.7.4
