PING: [PATCH v3 0/6] Misc fixes for throttle

2023-07-20 Thread zhenwei pi 

Hi Kevin, Hanna,

Patch 1 -> patch 5 of this series are already reviewed by Alberto(these 
affects throttle framework only), the patch 6 affects qemu block layer, 
would you please review this(in the further step, merge this series if 
this is acceptable)?


On 7/13/23 14:41, zhenwei pi wrote:

v2 -> v3:
- patch 1 -> patch 5 are already reviewed by Alberto
- append patch 6: throttle: use enum ThrottleType instead of bool is_write

v1 -> v2:
- rename 'ThrottleTimerType' to 'ThrottleType'
- add assertion to throttle_schedule_timer

v1:
- introduce enum ThrottleTimerType instead of timers[0], timer[1]...
- support read-only and write-only for throttle
- adapt related test codes
- cryptodev uses a write-only throttle timer

Zhenwei Pi (6):
   throttle: introduce enum ThrottleType
   test-throttle: use enum ThrottleType
   throttle: support read-only and write-only
   test-throttle: test read only and write only
   cryptodev: use NULL throttle timer cb for read direction
   throttle: use enum ThrottleType instead of bool is_write

  backends/cryptodev.c| 12 +++---
  block/throttle-groups.c |  6 ++-
  fsdev/qemu-fsdev-throttle.c |  8 ++--
  include/qemu/throttle.h | 15 +---
  tests/unit/test-throttle.c  | 76 ++---
  util/throttle.c | 64 +++
  6 files changed, 136 insertions(+), 45 deletions(-)



--
zhenwei pi

[PATCH v6 2/3] hw/ufs: Support for Query Transfer Requests

2023-07-20 Thread Jeuk Kim 
This commit makes the UFS device support query
and nop out transfer requests.

The next patch would be support for UFS logical
unit and scsi command transfer request.

Signed-off-by: Jeuk Kim 
---
 hw/ufs/trace-events |   1 +
 hw/ufs/ufs.c| 980 +++-
 hw/ufs/ufs.h|  46 +++
 3 files changed, 1025 insertions(+), 2 deletions(-)

diff --git a/hw/ufs/trace-events b/hw/ufs/trace-events
index d1badcad10..665e1a942b 100644
--- a/hw/ufs/trace-events
+++ b/hw/ufs/trace-events
@@ -18,6 +18,7 @@ ufs_err_dma_read_req_upiu(uint32_t slot, uint64_t addr) 
"failed to read req upiu
 ufs_err_dma_read_prdt(uint32_t slot, uint64_t addr) "failed to read prdt. 
UTRLDBR slot %"PRIu32", prdt addr %"PRIu64""
 ufs_err_dma_write_utrd(uint32_t slot, uint64_t addr) "failed to write utrd. 
UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64""
 ufs_err_dma_write_rsp_upiu(uint32_t slot, uint64_t addr) "failed to write rsp 
upiu. UTRLDBR slot %"PRIu32", response upiu addr %"PRIu64""
+ufs_err_utrl_slot_error(uint32_t slot) "UTRLDBR slot %"PRIu32" is in error"
 ufs_err_utrl_slot_busy(uint32_t slot) "UTRLDBR slot %"PRIu32" is busy"
 ufs_err_unsupport_register_offset(uint32_t offset) "Register offset 
0x%"PRIx32" is not yet supported"
 ufs_err_invalid_register_offset(uint32_t offset) "Register offset 0x%"PRIx32" 
is invalid"
diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c
index 101082a8a3..cd33af2cde 100644
--- a/hw/ufs/ufs.c
+++ b/hw/ufs/ufs.c
@@ -15,10 +15,221 @@
 #include "ufs.h"
 
 /* The QEMU-UFS device follows spec version 3.1 */
-#define UFS_SPEC_VER 0x0310
+#define UFS_SPEC_VER 0x0310
 #define UFS_MAX_NUTRS 32
 #define UFS_MAX_NUTMRS 8
 
+static MemTxResult ufs_addr_read(UfsHc *u, hwaddr addr, void *buf, int size)
+{
+hwaddr hi = addr + size - 1;
+
+if (hi < addr) {
+return MEMTX_DECODE_ERROR;
+}
+
+if (!FIELD_EX32(u->reg.cap, CAP, 64AS) && (hi >> 32)) {
+return MEMTX_DECODE_ERROR;
+}
+
+return pci_dma_read(PCI_DEVICE(u), addr, buf, size);
+}
+
+static MemTxResult ufs_addr_write(UfsHc *u, hwaddr addr, const void *buf,
+  int size)
+{
+hwaddr hi = addr + size - 1;
+if (hi < addr) {
+return MEMTX_DECODE_ERROR;
+}
+
+if (!FIELD_EX32(u->reg.cap, CAP, 64AS) && (hi >> 32)) {
+return MEMTX_DECODE_ERROR;
+}
+
+return pci_dma_write(PCI_DEVICE(u), addr, buf, size);
+}
+
+static void ufs_complete_req(UfsRequest *req, UfsReqResult req_result);
+
+static inline hwaddr ufs_get_utrd_addr(UfsHc *u, uint32_t slot)
+{
+hwaddr utrl_base_addr = (((hwaddr)u->reg.utrlbau) << 32) + u->reg.utrlba;
+hwaddr utrd_addr = utrl_base_addr + slot * sizeof(UtpTransferReqDesc);
+
+return utrd_addr;
+}
+
+static inline hwaddr ufs_get_req_upiu_base_addr(const UtpTransferReqDesc *utrd)
+{
+uint32_t cmd_desc_base_addr_lo =
+le32_to_cpu(utrd->command_desc_base_addr_lo);
+uint32_t cmd_desc_base_addr_hi =
+le32_to_cpu(utrd->command_desc_base_addr_hi);
+
+return (((hwaddr)cmd_desc_base_addr_hi) << 32) + cmd_desc_base_addr_lo;
+}
+
+static inline hwaddr ufs_get_rsp_upiu_base_addr(const UtpTransferReqDesc *utrd)
+{
+hwaddr req_upiu_base_addr = ufs_get_req_upiu_base_addr(utrd);
+uint32_t rsp_upiu_byte_off =
+le16_to_cpu(utrd->response_upiu_offset) * sizeof(uint32_t);
+return req_upiu_base_addr + rsp_upiu_byte_off;
+}
+
+static MemTxResult ufs_dma_read_utrd(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+hwaddr utrd_addr = ufs_get_utrd_addr(u, req->slot);
+MemTxResult ret;
+
+ret = ufs_addr_read(u, utrd_addr, &req->utrd, sizeof(req->utrd));
+if (ret) {
+trace_ufs_err_dma_read_utrd(req->slot, utrd_addr);
+}
+return ret;
+}
+
+static MemTxResult ufs_dma_read_req_upiu(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+hwaddr req_upiu_base_addr = ufs_get_req_upiu_base_addr(&req->utrd);
+UtpUpiuReq *req_upiu = &req->req_upiu;
+uint32_t copy_size;
+uint16_t data_segment_length;
+MemTxResult ret;
+
+/*
+ * To know the size of the req_upiu, we need to read the
+ * data_segment_length in the header first.
+ */
+ret = ufs_addr_read(u, req_upiu_base_addr, &req_upiu->header,
+sizeof(UtpUpiuHeader));
+if (ret) {
+trace_ufs_err_dma_read_req_upiu(req->slot, req_upiu_base_addr);
+return ret;
+}
+data_segment_length = be16_to_cpu(req_upiu->header.data_segment_length);
+
+copy_size = sizeof(UtpUpiuHeader) + UFS_TRANSACTION_SPECIFIC_FIELD_SIZE +
+data_segment_length;
+
+ret = ufs_addr_read(u, req_upiu_base_addr, &req->req_upiu, copy_size);
+if (ret) {
+trace_ufs_err_dma_read_req_upiu(req->slot, req_upiu_base_addr);
+}
+return ret;
+}
+
+static MemTxResult ufs_dma_read_prdt(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+uint16_t prdt_len = le16_to_cpu(req->utrd.prd_table_length);
+uint16_t prdt_byte_off =
+le16_t

[PATCH v6 0/3] hw/ufs: Add Universal Flash Storage (UFS) support

2023-07-20 Thread Jeuk Kim 
Since v5:
- Fix to print an error message instead of a segmentation fault
  when no drive property is specified for a ufs-lu device

Since v4:
Addressed review comment from Stefan Hajnoczi. The fixes are as
follows.
- Keep u->reg fields in host endian (Removed little-endian helper 
  functions from MemoryRegionOps)
- Remove unnecessary NULL checks for g_new and g_malloc0
- Replace DEFINE_PROP_DRIVE_IOTHREAD -> DEFINE_PROP_DRIVE

In case you were wondering, ufs and ufs-lu have been tested for the
following behaviours.
1. Checked ufs device recognition in Windows10 environment
2. Verified ufs device recognition in Ubuntu 22.04 environment
3. Verified io behaviour via fio in Ubuntu 22.04 environment
4. Verified query request via ufs-tools in Ubuntu 22.04 environment

Since v3:
- Replace softmmu_ss -> system_ss in meson

Since v2:
Addressed review comment from Stefan Hajnoczi. The main fixes are as
follows.
- Use of SPDX licence identifiers
- fixed endianness error
- removed memory leak
- fixed DMA error handling logic

Since v1:
- use macros of "hw/registerfields.h" (Addressed Philippe's review
  comments)

This patch series adds support for a new PCI-based UFS device.

The UFS pci device id (PCI_DEVICE_ID_REDHAT_UFS) is not registered
in the Linux kernel yet, so it does not work right away, but I confirmed
that it works with Linux when the UFS pci device id is registered.

I have also verified that it works with Windows 10.


Jeuk Kim (3):
  hw/ufs: Initial commit for emulated Universal-Flash-Storage
  hw/ufs: Support for Query Transfer Requests
  hw/ufs: Support for UFS logical unit

 MAINTAINERS  |6 +
 docs/specs/pci-ids.rst   |2 +
 hw/Kconfig   |1 +
 hw/meson.build   |1 +
 hw/ufs/Kconfig   |4 +
 hw/ufs/lu.c  | 1439 
 hw/ufs/meson.build   |1 +
 hw/ufs/trace-events  |   58 ++
 hw/ufs/trace.h   |1 +
 hw/ufs/ufs.c | 1494 ++
 hw/ufs/ufs.h |  131 
 include/block/ufs.h  | 1048 ++
 include/hw/pci/pci.h |1 +
 include/hw/pci/pci_ids.h |1 +
 include/scsi/constants.h |1 +
 meson.build  |1 +
 16 files changed, 4190 insertions(+)
 create mode 100644 hw/ufs/Kconfig
 create mode 100644 hw/ufs/lu.c
 create mode 100644 hw/ufs/meson.build
 create mode 100644 hw/ufs/trace-events
 create mode 100644 hw/ufs/trace.h
 create mode 100644 hw/ufs/ufs.c
 create mode 100644 hw/ufs/ufs.h
 create mode 100644 include/block/ufs.h

-- 
2.34.1

[PATCH v6 1/3] hw/ufs: Initial commit for emulated Universal-Flash-Storage

2023-07-20 Thread Jeuk Kim 
Universal Flash Storage (UFS) is a high-performance mass storage device
with a serial interface. It is primarily used as a high-performance
data storage device for embedded applications.

This commit contains code for UFS device to be recognized
as a UFS PCI device.
Patches to handle UFS logical unit and Transfer Request will follow.

Signed-off-by: Jeuk Kim 
---
 MAINTAINERS  |6 +
 docs/specs/pci-ids.rst   |2 +
 hw/Kconfig   |1 +
 hw/meson.build   |1 +
 hw/ufs/Kconfig   |4 +
 hw/ufs/meson.build   |1 +
 hw/ufs/trace-events  |   32 ++
 hw/ufs/trace.h   |1 +
 hw/ufs/ufs.c |  278 ++
 hw/ufs/ufs.h |   42 ++
 include/block/ufs.h  | 1048 ++
 include/hw/pci/pci.h |1 +
 include/hw/pci/pci_ids.h |1 +
 meson.build  |1 +
 14 files changed, 1419 insertions(+)
 create mode 100644 hw/ufs/Kconfig
 create mode 100644 hw/ufs/meson.build
 create mode 100644 hw/ufs/trace-events
 create mode 100644 hw/ufs/trace.h
 create mode 100644 hw/ufs/ufs.c
 create mode 100644 hw/ufs/ufs.h
 create mode 100644 include/block/ufs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 12e59b6b27..0c8a955b42 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2256,6 +2256,12 @@ F: tests/qtest/nvme-test.c
 F: docs/system/devices/nvme.rst
 T: git git://git.infradead.org/qemu-nvme.git nvme-next
 
+ufs
+M: Jeuk Kim 
+S: Supported
+F: hw/ufs/*
+F: include/block/ufs.h
+
 megasas
 M: Hannes Reinecke 
 L: qemu-block@nongnu.org
diff --git a/docs/specs/pci-ids.rst b/docs/specs/pci-ids.rst
index e302bea484..d6707fa069 100644
--- a/docs/specs/pci-ids.rst
+++ b/docs/specs/pci-ids.rst
@@ -92,6 +92,8 @@ PCI devices (other than virtio):
   PCI PVPanic device (``-device pvpanic-pci``)
 1b36:0012
   PCI ACPI ERST device (``-device acpi-erst``)
+1b36:0013
+  PCI UFS device (``-device ufs``)
 
 All these devices are documented in :doc:`index`.
 
diff --git a/hw/Kconfig b/hw/Kconfig
index ba62ff6417..9ca7b38c31 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -38,6 +38,7 @@ source smbios/Kconfig
 source ssi/Kconfig
 source timer/Kconfig
 source tpm/Kconfig
+source ufs/Kconfig
 source usb/Kconfig
 source virtio/Kconfig
 source vfio/Kconfig
diff --git a/hw/meson.build b/hw/meson.build
index c7ac7d3d75..f01fac4617 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -37,6 +37,7 @@ subdir('smbios')
 subdir('ssi')
 subdir('timer')
 subdir('tpm')
+subdir('ufs')
 subdir('usb')
 subdir('vfio')
 subdir('virtio')
diff --git a/hw/ufs/Kconfig b/hw/ufs/Kconfig
new file mode 100644
index 00..b7b3392e85
--- /dev/null
+++ b/hw/ufs/Kconfig
@@ -0,0 +1,4 @@
+config UFS_PCI
+bool
+default y if PCI_DEVICES
+depends on PCI
diff --git a/hw/ufs/meson.build b/hw/ufs/meson.build
new file mode 100644
index 00..eb5164bde9
--- /dev/null
+++ b/hw/ufs/meson.build
@@ -0,0 +1 @@
+system_ss.add(when: 'CONFIG_UFS_PCI', if_true: files('ufs.c'))
diff --git a/hw/ufs/trace-events b/hw/ufs/trace-events
new file mode 100644
index 00..d1badcad10
--- /dev/null
+++ b/hw/ufs/trace-events
@@ -0,0 +1,32 @@
+# ufs.c
+ufs_irq_raise(void) "INTx"
+ufs_irq_lower(void) "INTx"
+ufs_mmio_read(uint64_t addr, uint64_t data, unsigned size) "addr 0x%"PRIx64" 
data 0x%"PRIx64" size %d"
+ufs_mmio_write(uint64_t addr, uint64_t data, unsigned size) "addr 0x%"PRIx64" 
data 0x%"PRIx64" size %d"
+ufs_process_db(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_process_req(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_complete_req(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_sendback_req(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_exec_nop_cmd(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_exec_scsi_cmd(uint32_t slot, uint8_t lun, uint8_t opcode) "slot %"PRIu32", 
lun 0x%"PRIx8", opcode 0x%"PRIx8""
+ufs_exec_query_cmd(uint32_t slot, uint8_t opcode) "slot %"PRIu32", opcode 
0x%"PRIx8""
+ufs_process_uiccmd(uint32_t uiccmd, uint32_t ucmdarg1, uint32_t ucmdarg2, 
uint32_t ucmdarg3) "uiccmd 0x%"PRIx32", ucmdarg1 0x%"PRIx32", ucmdarg2 
0x%"PRIx32", ucmdarg3 0x%"PRIx32""
+
+# error condition
+ufs_err_dma_read_utrd(uint32_t slot, uint64_t addr) "failed to read utrd. 
UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64""
+ufs_err_dma_read_req_upiu(uint32_t slot, uint64_t addr) "failed to read req 
upiu. UTRLDBR slot %"PRIu32", request upiu addr %"PRIu64""
+ufs_err_dma_read_prdt(uint32_t slot, uint64_t addr) "failed to read prdt. 
UTRLDBR slot %"PRIu32", prdt addr %"PRIu64""
+ufs_err_dma_write_utrd(uint32_t slot, uint64_t addr) "failed to write utrd. 
UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64""
+ufs_err_dma_write_rsp_upiu(uint32_t slot, uint64_t addr) "failed to write rsp 
upiu. UTRLDBR slot %"PRIu32", response upiu addr %"PRIu64""
+ufs_err_utrl_slot_busy(uint32_t slot) "UTRLDBR slot %"PRIu32" is busy"
+ufs_err_unsupport_register_offset(uint32_t offset) "Register offset 
0x%"PRIx32" is not yet supported"
+ufs_err_invalid_register_offset(uint32_t o

[PATCH v6 3/3] hw/ufs: Support for UFS logical unit

2023-07-20 Thread Jeuk Kim 
This commit adds support for ufs logical unit.
The LU handles processing for the SCSI command,
unit descriptor query request.

This commit enables the UFS device to process
IO requests.

Signed-off-by: Jeuk Kim 
---
 hw/ufs/lu.c  | 1439 ++
 hw/ufs/meson.build   |2 +-
 hw/ufs/trace-events  |   25 +
 hw/ufs/ufs.c |  252 ++-
 hw/ufs/ufs.h |   43 ++
 include/scsi/constants.h |1 +
 6 files changed, 1755 insertions(+), 7 deletions(-)
 create mode 100644 hw/ufs/lu.c

diff --git a/hw/ufs/lu.c b/hw/ufs/lu.c
new file mode 100644
index 00..6f6d301bc7
--- /dev/null
+++ b/hw/ufs/lu.c
@@ -0,0 +1,1439 @@
+/*
+ * QEMU UFS Logical Unit
+ *
+ * Copyright (c) 2023 Samsung Electronics Co., Ltd. All rights reserved.
+ *
+ * Written by Jeuk Kim 
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "qemu/memalign.h"
+#include "hw/scsi/scsi.h"
+#include "scsi/constants.h"
+#include "sysemu/block-backend.h"
+#include "qemu/cutils.h"
+#include "trace.h"
+#include "ufs.h"
+
+/*
+ * The code below handling SCSI commands is copied from hw/scsi/scsi-disk.c,
+ * with minor adjustments to make it work for UFS.
+ */
+
+#define SCSI_DMA_BUF_SIZE (128 * KiB)
+#define SCSI_MAX_INQUIRY_LEN 256
+#define SCSI_INQUIRY_DATA_SIZE 36
+#define SCSI_MAX_MODE_LEN 256
+
+typedef struct UfsSCSIReq {
+SCSIRequest req;
+/* Both sector and sector_count are in terms of BDRV_SECTOR_SIZE bytes.  */
+uint64_t sector;
+uint32_t sector_count;
+uint32_t buflen;
+bool started;
+bool need_fua_emulation;
+struct iovec iov;
+QEMUIOVector qiov;
+BlockAcctCookie acct;
+} UfsSCSIReq;
+
+static void ufs_scsi_free_request(SCSIRequest *req)
+{
+UfsSCSIReq *r = DO_UPCAST(UfsSCSIReq, req, req);
+
+qemu_vfree(r->iov.iov_base);
+}
+
+static void scsi_check_condition(UfsSCSIReq *r, SCSISense sense)
+{
+trace_ufs_scsi_check_condition(r->req.tag, sense.key, sense.asc,
+   sense.ascq);
+scsi_req_build_sense(&r->req, sense);
+scsi_req_complete(&r->req, CHECK_CONDITION);
+}
+
+static int ufs_scsi_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf,
+ uint32_t outbuf_len)
+{
+UfsHc *u = UFS(req->bus->qbus.parent);
+UfsLu *lu = DO_UPCAST(UfsLu, qdev, req->dev);
+uint8_t page_code = req->cmd.buf[2];
+int start, buflen = 0;
+
+if (outbuf_len < SCSI_INQUIRY_DATA_SIZE) {
+return -1;
+}
+
+outbuf[buflen++] = lu->qdev.type & 0x1f;
+outbuf[buflen++] = page_code;
+outbuf[buflen++] = 0x00;
+outbuf[buflen++] = 0x00;
+start = buflen;
+
+switch (page_code) {
+case 0x00: /* Supported page codes, mandatory */
+{
+trace_ufs_scsi_emulate_vpd_page_00(req->cmd.xfer);
+outbuf[buflen++] = 0x00; /* list of supported pages (this page) */
+if (u->params.serial) {
+outbuf[buflen++] = 0x80; /* unit serial number */
+}
+outbuf[buflen++] = 0x87; /* mode page policy */
+break;
+}
+case 0x80: /* Device serial number, optional */
+{
+int l;
+
+if (!u->params.serial) {
+trace_ufs_scsi_emulate_vpd_page_80_not_supported();
+return -1;
+}
+
+l = strlen(u->params.serial);
+if (l > SCSI_INQUIRY_DATA_SIZE) {
+l = SCSI_INQUIRY_DATA_SIZE;
+}
+
+trace_ufs_scsi_emulate_vpd_page_80(req->cmd.xfer);
+memcpy(outbuf + buflen, u->params.serial, l);
+buflen += l;
+break;
+}
+case 0x87: /* Mode Page Policy, mandatory */
+{
+trace_ufs_scsi_emulate_vpd_page_87(req->cmd.xfer);
+outbuf[buflen++] = 0x3f; /* apply to all mode pages and subpages */
+outbuf[buflen++] = 0xff;
+outbuf[buflen++] = 0; /* shared */
+outbuf[buflen++] = 0;
+break;
+}
+default:
+return -1;
+}
+/* done with EVPD */
+assert(buflen - start <= 255);
+outbuf[start - 1] = buflen - start;
+return buflen;
+}
+
+static int ufs_scsi_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf,
+uint32_t outbuf_len)
+{
+int buflen = 0;
+
+if (outbuf_len < SCSI_INQUIRY_DATA_SIZE) {
+return -1;
+}
+
+if (req->cmd.buf[1] & 0x1) {
+/* Vital product data */
+return ufs_scsi_emulate_vpd_page(req, outbuf, outbuf_len);
+}
+
+/* Standard INQUIRY data */
+if (req->cmd.buf[2] != 0) {
+return -1;
+}
+
+/* PAGE CODE == 0 */
+buflen = req->cmd.xfer;
+if (buflen > SCSI_MAX_INQUIRY_LEN) {
+buflen = SCSI_MAX_INQUIRY_LEN;
+}
+
+if (is_wlun(req->lun)) {
+outbuf[0] = TYPE_WLUN;
+} else {
+outbuf[0] = 0;
+}
+outbuf[1] = 0;
+
+strpadcpy((char *)&outbuf[16], 16, "QEMU UFS", ' ');
+strpa

Re: [PATCH v5 0/3] hw/ufs: Add Universal Flash Storage (UFS) support

2023-07-20 Thread Jeuk Kim 

On 7/21/2023 3:49 AM, Stefan Hajnoczi wrote:

Hi,
I'm ready to merge this but encountered a bug when testing:

   $ qemu-system-x86_64 --device ufs --device ufs-lu
   Segmentation fault (core dumped)

Please ensure there is an error message like with SCSI disks:

   $ qemu-system-x86_64 --device virtio-scsi-pci --device scsi-hd
   qemu-system-x86_64: --device scsi-hd: drive property not set

Thanks,
Stefan


Thanks for letting me know.
I'll fix it right away and send the patch again.

Thanks,
Jeuk

Re: [PATCH v5 0/3] hw/ufs: Add Universal Flash Storage (UFS) support

2023-07-20 Thread Stefan Hajnoczi 
Hi,
I'm ready to merge this but encountered a bug when testing:

  $ qemu-system-x86_64 --device ufs --device ufs-lu
  Segmentation fault (core dumped)

Please ensure there is an error message like with SCSI disks:

  $ qemu-system-x86_64 --device virtio-scsi-pci --device scsi-hd
  qemu-system-x86_64: --device scsi-hd: drive property not set

Thanks,
Stefan


signature.asc
Description: PGP signature

Re: [PATCH] block: Be more verbose in create fallback

2023-07-20 Thread Eric Blake 
On Thu, Jul 20, 2023 at 04:00:24PM +0200, Hanna Czenczek wrote:
> For image creation code, we have central fallback code for protocols
> that do not support creating new images (like NBD or iscsi).  So for
> them, you can only specify existing paths/exports that are overwritten
> to make clean new images.  In such a case, if the given path cannot be
> opened (assuming a pre-existing image there), we print an error message
> that tries to describe what is going on: That with this protocol, you
> cannot create new images, but only overwrite existing ones; and the
> given path could not be opened as a pre-existing image.
> 
> However, the current message is confusing, because it does not say that
> the protocol in question does not support creating new images, but
> instead that "image creation" is unsupported.  This can be interpreted
> to mean that `qemu-img create` will not work in principle, which is not
> true.  Be more verbose for clarity.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2217204
> Signed-off-by: Hanna Czenczek 
> ---
>  block.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

Definitely more vebose, but I don't see that as a bad thing.

Reviewed-by: Eric Blake 

> 
> diff --git a/block.c b/block.c
> index a307c151a8..f530dd9c02 100644
> --- a/block.c
> +++ b/block.c
> @@ -661,8 +661,10 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver 
> *drv,
>  blk = blk_co_new_open(filename, NULL, options,
>BDRV_O_RDWR | BDRV_O_RESIZE, errp);
>  if (!blk) {
> -error_prepend(errp, "Protocol driver '%s' does not support image "
> -  "creation, and opening the image failed: ",
> +error_prepend(errp, "Protocol driver '%s' does not support creating "
> +  "new images, so an existing image must be selected as "
> +  "the target; however, opening the given target as an "
> +  "existing image failed: ",
>drv->format_name);
>  return -EINVAL;
>  }
> -- 
> 2.41.0
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] hw/nvme: use stl/ldl pci dma api

2023-07-20 Thread Cédric Le Goater 

On 7/20/23 11:42, Klaus Jensen wrote:

From: Klaus Jensen 

Use the stl/ldl pci dma api for writing/reading doorbells. This removes
the explicit endian conversions.

Signed-off-by: Klaus Jensen 


Reviewed-by: Cédric Le Goater 
Tested-by: Cédric Le Goater 

on big and little endian hosts,

Thanks,

C.



---
  hw/nvme/ctrl.c | 42 +-
  1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index dadc2dc7da10..f2e5a2fa737b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1468,20 +1468,16 @@ static inline void nvme_blk_write(BlockBackend *blk, 
int64_t offset,
  
  static void nvme_update_cq_eventidx(const NvmeCQueue *cq)

  {
-uint32_t v = cpu_to_le32(cq->head);
-
  trace_pci_nvme_update_cq_eventidx(cq->cqid, cq->head);
  
-pci_dma_write(PCI_DEVICE(cq->ctrl), cq->ei_addr, &v, sizeof(v));

+stl_le_pci_dma(PCI_DEVICE(cq->ctrl), cq->ei_addr, cq->head,
+   MEMTXATTRS_UNSPECIFIED);
  }
  
  static void nvme_update_cq_head(NvmeCQueue *cq)

  {
-uint32_t v;
-
-pci_dma_read(PCI_DEVICE(cq->ctrl), cq->db_addr, &v, sizeof(v));
-
-cq->head = le32_to_cpu(v);
+ldl_le_pci_dma(PCI_DEVICE(cq->ctrl), cq->db_addr, &cq->head,
+   MEMTXATTRS_UNSPECIFIED);
  
  trace_pci_nvme_update_cq_head(cq->cqid, cq->head);

  }
@@ -6801,7 +6797,6 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
  PCIDevice *pci = PCI_DEVICE(n);
  uint64_t dbs_addr = le64_to_cpu(req->cmd.dptr.prp1);
  uint64_t eis_addr = le64_to_cpu(req->cmd.dptr.prp2);
-uint32_t v;
  int i;
  
  /* Address should be page aligned */

@@ -6819,8 +6814,6 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
  NvmeCQueue *cq = n->cq[i];
  
  if (sq) {

-v = cpu_to_le32(sq->tail);
-
  /*
   * CAP.DSTRD is 0, so offset of ith sq db_addr is (i<<3)
   * nvme_process_db() uses this hard-coded way to calculate
@@ -6828,7 +6821,7 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
   */
  sq->db_addr = dbs_addr + (i << 3);
  sq->ei_addr = eis_addr + (i << 3);
-pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
+stl_le_pci_dma(pci, sq->db_addr, sq->tail, MEMTXATTRS_UNSPECIFIED);
  
  if (n->params.ioeventfd && sq->sqid != 0) {

  if (!nvme_init_sq_ioeventfd(sq)) {
@@ -6838,12 +6831,10 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
  }
  
  if (cq) {

-v = cpu_to_le32(cq->head);
-
  /* CAP.DSTRD is 0, so offset of ith cq db_addr is (i<<3)+(1<<2) */
  cq->db_addr = dbs_addr + (i << 3) + (1 << 2);
  cq->ei_addr = eis_addr + (i << 3) + (1 << 2);
-pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+stl_le_pci_dma(pci, cq->db_addr, cq->head, MEMTXATTRS_UNSPECIFIED);
  
  if (n->params.ioeventfd && cq->cqid != 0) {

  if (!nvme_init_cq_ioeventfd(cq)) {
@@ -6974,20 +6965,16 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest 
*req)
  
  static void nvme_update_sq_eventidx(const NvmeSQueue *sq)

  {
-uint32_t v = cpu_to_le32(sq->tail);
-
  trace_pci_nvme_update_sq_eventidx(sq->sqid, sq->tail);
  
-pci_dma_write(PCI_DEVICE(sq->ctrl), sq->ei_addr, &v, sizeof(v));

+stl_le_pci_dma(PCI_DEVICE(sq->ctrl), sq->ei_addr, sq->tail,
+   MEMTXATTRS_UNSPECIFIED);
  }
  
  static void nvme_update_sq_tail(NvmeSQueue *sq)

  {
-uint32_t v;
-
-pci_dma_read(PCI_DEVICE(sq->ctrl), sq->db_addr, &v, sizeof(v));
-
-sq->tail = le32_to_cpu(v);
+ldl_le_pci_dma(PCI_DEVICE(sq->ctrl), sq->db_addr, &sq->tail,
+   MEMTXATTRS_UNSPECIFIED);
  
  trace_pci_nvme_update_sq_tail(sq->sqid, sq->tail);

  }
@@ -7592,7 +7579,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, 
unsigned size)
  static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
  {
  PCIDevice *pci = PCI_DEVICE(n);
-uint32_t qid, v;
+uint32_t qid;
  
  if (unlikely(addr & ((1 << 2) - 1))) {

  NVME_GUEST_ERR(pci_nvme_ub_db_wr_misaligned,
@@ -7659,8 +7646,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
  start_sqs = nvme_cq_full(cq) ? 1 : 0;
  cq->head = new_head;
  if (!qid && n->dbbuf_enabled) {
-v = cpu_to_le32(cq->head);
-pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+stl_le_pci_dma(pci, cq->db_addr, cq->head, MEMTXATTRS_UNSPECIFIED);
  }
  if (start_sqs) {
  NvmeSQueue *sq;
@@ -7720,8 +7706,6 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
  
  sq->tail = new_tail;

  if (!qid && n->dbbuf_enabled) {
-v = cpu_to_le32(sq->tail);
-

Re: [PATCH 2/4] python/console_socket: accept existing FD in initializer

2023-07-20 Thread John Snow 
On Thu, Jul 20, 2023 at 10:01 AM Daniel P. Berrangé  wrote:
>
> On Thu, Jul 20, 2023 at 09:04:46AM -0400, John Snow wrote:
> > Useful if we want to use ConsoleSocket() for a socket created by
> > socketpair().
> >
> > Signed-off-by: John Snow 
> > ---
> >  python/qemu/machine/console_socket.py | 11 +++
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/python/qemu/machine/console_socket.py 
> > b/python/qemu/machine/console_socket.py
> > index 4e28ba9bb2..42bfa12411 100644
> > --- a/python/qemu/machine/console_socket.py
> > +++ b/python/qemu/machine/console_socket.py
> > @@ -17,7 +17,7 @@
> >  import socket
> >  import threading
> >  import time
> > -from typing import Deque, Optional
> > +from typing import Deque, Optional, Union
> >
> >
> >  class ConsoleSocket(socket.socket):
> > @@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
> >  Optionally a file path can be passed in and we will also
> >  dump the characters to this file for debugging purposes.
> >  """
> > -def __init__(self, address: str, file: Optional[str] = None,
> > +def __init__(self, address: Union[str, int], file: Optional[str] = 
> > None,
> >   drain: bool = False):
>
> IMHO calling the pre-opened FD an "address" is pushing the
> interpretation a bit.
>

You're right.

> It also makes the behaviour non-obvious from a caller. Seeing a
> caller, you have to work backwards to find out what type it has,
> to figure out the semantics of the method call.
>
> IOW, I'd prefer to see
>
>address: Optional[str], sock_fd: Optional[int]
>
> and then just do a check
>
>if (address is not None and sock_fd is not None) or
>   (address is None and sock_fd is None):
>   raise Exception("either 'address' or 'sock_fd' is required")
>
> thus when you see
>
>ConsoleSocket(sock_fd=xxx)
>
> it is now obvious it has different behaviour to
>
>ConsoleSocket(address=yyy)
>

Yeah, that's just a little harder to type - in the sense that it
appears as though you could omit either argument by just observing the
signature. One thing I like about "one mandatory argument that takes
many forms" is that it's obvious you need to give it *something* from
the signature.

You're right that the name is now very silly, though.

The "obvious it has different behavior" is a good argument, I'll change it.

--js

>
> >  self._recv_timeout_sec = 300.0
> >  self._sleep_time = 0.5
> >  self._buffer: Deque[int] = deque()
> > -socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> > -self.connect(address)
> > +if isinstance(address, str):
> > +socket.socket.__init__(self, socket.AF_UNIX, 
> > socket.SOCK_STREAM)
> > +self.connect(address)
> > +else:
> > +socket.socket.__init__(self, fileno=address)
> >  self._logfile = None
> >  if file:
> >  # pylint: disable=consider-using-with
> > --
> > 2.41.0
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>

Re: [PATCH 4/4] python/machine: remove unused console socket configuration arguments

2023-07-20 Thread John Snow 
On Thu, Jul 20, 2023 at 10:05 AM Daniel P. Berrangé  wrote:
>
> On Thu, Jul 20, 2023 at 09:04:48AM -0400, John Snow wrote:
> > By using a socketpair for the console, we don't need the sock_dir
> > argument for the base class anymore, remove it.
> >
> > The qtest subclass still uses the argument for the qtest socket for now.
> >
> > Signed-off-by: John Snow 
> > ---
> >  python/qemu/machine/machine.py | 18 --
> >  python/qemu/machine/qtest.py   |  6 +++---
> >  tests/qemu-iotests/tests/copy-before-write |  3 +--
> >  3 files changed, 4 insertions(+), 23 deletions(-)
>
> A couple of callers to be updated to remove 'sock_dir=':
>
> $ git grep 'sock_dir=' tests
> tests/avocado/acpi-bits.py: sock_dir=sock_dir, 
> qmp_timer=qmp_timer)
> tests/avocado/avocado_qemu/__init__.py: 
> sock_dir=self._sd.name, log_dir=self.logdir)
> tests/qemu-iotests/iotests.py: sock_dir=sock_dir, 
> qmp_timer=timer)
> tests/qemu-iotests/tests/copy-before-write:  
> sock_dir=iotests.sock_dir)
>
> presume the avocado_qemu/__init__.py one is what caused the
> failure Peter reported.
>

Yep, missed a spot, sorry :( I tested avocado after patch 3 but not here.

While I'm at it, though, I am testing the same treatment for the qtest
extension and just removing sock_dir from *everything*, since we don't
need that workaround anymore if we aren't creating named sockets.

...And if I get rid of *that*, I can get rid of some other temp
directory management stuff too.

> >
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index ef9b2dc02e..350aa8bb26 100644
> > --- a/python/qemu/machine/machine.py
> > +++ b/python/qemu/machine/machine.py
> > @@ -127,7 +127,6 @@ def __init__(self,
> >   name: Optional[str] = None,
> >   base_temp_dir: str = "/var/tmp",
> >   monitor_address: Optional[SocketAddrT] = None,
> > - sock_dir: Optional[str] = None,
> >   drain_console: bool = False,
> >   console_log: Optional[str] = None,
> >   log_dir: Optional[str] = None,
> > @@ -141,7 +140,6 @@ def __init__(self,
> >  @param name: prefix for socket and log file names (default: 
> > qemu-PID)
> >  @param base_temp_dir: default location where temp files are created
> >  @param monitor_address: address for QMP monitor
> > -@param sock_dir: where to create socket (defaults to base_temp_dir)
> >  @param drain_console: (optional) True to drain console socket to 
> > buffer
> >  @param console_log: (optional) path to console log file
> >  @param log_dir: where to create and keep log files
> > @@ -163,7 +161,6 @@ def __init__(self,
> >  Tuple[socket.socket, socket.socket]] = None
> >  self._temp_dir: Optional[str] = None
> >  self._base_temp_dir = base_temp_dir
> > -self._sock_dir = sock_dir
> >  self._log_dir = log_dir
> >
> >  self._monitor_address = monitor_address
> > @@ -189,9 +186,6 @@ def __init__(self,
> >  self._console_index = 0
> >  self._console_set = False
> >  self._console_device_type: Optional[str] = None
> > -self._console_address = os.path.join(
> > -self.sock_dir, f"{self._name}.con"
> > -)
> >  self._console_socket: Optional[socket.socket] = None
> >  self._remove_files: List[str] = []
> >  self._user_killed = False
> > @@ -334,9 +328,6 @@ def args(self) -> List[str]:
> >  return self._args
> >
> >  def _pre_launch(self) -> None:
> > -if self._console_set:
> > -self._remove_files.append(self._console_address)
> > -
> >  if self._qmp_set:
> >  if self._monitor_address is None:
> >  self._sock_pair = socket.socketpair()
> > @@ -900,15 +891,6 @@ def temp_dir(self) -> str:
> >dir=self._base_temp_dir)
> >  return self._temp_dir
> >
> > -@property
> > -def sock_dir(self) -> str:
> > -"""
> > -Returns the directory used for sockfiles by this machine.
> > -"""
> > -if self._sock_dir:
> > -return self._sock_dir
> > -return self.temp_dir
> > -
> >  @property
> >  def log_dir(self) -> str:
> >  """
> > diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
> > index 1c46138bd0..22f8045ef6 100644
> > --- a/python/qemu/machine/qtest.py
> > +++ b/python/qemu/machine/qtest.py
> > @@ -115,8 +115,8 @@ def __init__(self,
> >   wrapper: Sequence[str] = (),
> >   name: Optional[str] = None,
> >   base_temp_dir: str = "/var/tmp",
> > - sock_dir: Optional[str] = None,
> > - qmp_timer: Optional[float] = None):
> > +

Re: [PATCH 4/4] python/machine: remove unused console socket configuration arguments

2023-07-20 Thread Daniel P . Berrangé 
On Thu, Jul 20, 2023 at 09:04:48AM -0400, John Snow wrote:
> By using a socketpair for the console, we don't need the sock_dir
> argument for the base class anymore, remove it.
> 
> The qtest subclass still uses the argument for the qtest socket for now.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/machine.py | 18 --
>  python/qemu/machine/qtest.py   |  6 +++---
>  tests/qemu-iotests/tests/copy-before-write |  3 +--
>  3 files changed, 4 insertions(+), 23 deletions(-)

A couple of callers to be updated to remove 'sock_dir=':

$ git grep 'sock_dir=' tests
tests/avocado/acpi-bits.py: sock_dir=sock_dir, 
qmp_timer=qmp_timer)
tests/avocado/avocado_qemu/__init__.py: 
sock_dir=self._sd.name, log_dir=self.logdir)
tests/qemu-iotests/iotests.py: sock_dir=sock_dir, 
qmp_timer=timer)
tests/qemu-iotests/tests/copy-before-write:  
sock_dir=iotests.sock_dir)

presume the avocado_qemu/__init__.py one is what caused the
failure Peter reported.

> 
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index ef9b2dc02e..350aa8bb26 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -127,7 +127,6 @@ def __init__(self,
>   name: Optional[str] = None,
>   base_temp_dir: str = "/var/tmp",
>   monitor_address: Optional[SocketAddrT] = None,
> - sock_dir: Optional[str] = None,
>   drain_console: bool = False,
>   console_log: Optional[str] = None,
>   log_dir: Optional[str] = None,
> @@ -141,7 +140,6 @@ def __init__(self,
>  @param name: prefix for socket and log file names (default: qemu-PID)
>  @param base_temp_dir: default location where temp files are created
>  @param monitor_address: address for QMP monitor
> -@param sock_dir: where to create socket (defaults to base_temp_dir)
>  @param drain_console: (optional) True to drain console socket to 
> buffer
>  @param console_log: (optional) path to console log file
>  @param log_dir: where to create and keep log files
> @@ -163,7 +161,6 @@ def __init__(self,
>  Tuple[socket.socket, socket.socket]] = None
>  self._temp_dir: Optional[str] = None
>  self._base_temp_dir = base_temp_dir
> -self._sock_dir = sock_dir
>  self._log_dir = log_dir
>  
>  self._monitor_address = monitor_address
> @@ -189,9 +186,6 @@ def __init__(self,
>  self._console_index = 0
>  self._console_set = False
>  self._console_device_type: Optional[str] = None
> -self._console_address = os.path.join(
> -self.sock_dir, f"{self._name}.con"
> -)
>  self._console_socket: Optional[socket.socket] = None
>  self._remove_files: List[str] = []
>  self._user_killed = False
> @@ -334,9 +328,6 @@ def args(self) -> List[str]:
>  return self._args
>  
>  def _pre_launch(self) -> None:
> -if self._console_set:
> -self._remove_files.append(self._console_address)
> -
>  if self._qmp_set:
>  if self._monitor_address is None:
>  self._sock_pair = socket.socketpair()
> @@ -900,15 +891,6 @@ def temp_dir(self) -> str:
>dir=self._base_temp_dir)
>  return self._temp_dir
>  
> -@property
> -def sock_dir(self) -> str:
> -"""
> -Returns the directory used for sockfiles by this machine.
> -"""
> -if self._sock_dir:
> -return self._sock_dir
> -return self.temp_dir
> -
>  @property
>  def log_dir(self) -> str:
>  """
> diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
> index 1c46138bd0..22f8045ef6 100644
> --- a/python/qemu/machine/qtest.py
> +++ b/python/qemu/machine/qtest.py
> @@ -115,8 +115,8 @@ def __init__(self,
>   wrapper: Sequence[str] = (),
>   name: Optional[str] = None,
>   base_temp_dir: str = "/var/tmp",
> - sock_dir: Optional[str] = None,
> - qmp_timer: Optional[float] = None):
> + qmp_timer: Optional[float] = None,
> + sock_dir: Optional[str] = None):
>  # pylint: disable=too-many-arguments
>  
>  if name is None:
> @@ -125,7 +125,7 @@ def __init__(self,
>  sock_dir = base_temp_dir
>  super().__init__(binary, args, wrapper=wrapper, name=name,
>   base_temp_dir=base_temp_dir,
> - sock_dir=sock_dir, qmp_timer=qmp_timer)
> + qmp_timer=qmp_timer)
>  self._qtest: Optional[QEMUQtestProtocol] = None
>  self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
>  
> diff

Re: [PATCH 3/4] python/machine: use socketpair() for console connections

2023-07-20 Thread Daniel P . Berrangé 
On Thu, Jul 20, 2023 at 09:04:47AM -0400, John Snow wrote:
> Create a socketpair for the console output. This should help eliminate
> race conditions around console text early in the boot process that might
> otherwise have been dropped on the floor before being able to connect to
> QEMU under "server,nowait".
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/machine.py | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 2/4] python/console_socket: accept existing FD in initializer

2023-07-20 Thread Daniel P . Berrangé 
On Thu, Jul 20, 2023 at 09:04:46AM -0400, John Snow wrote:
> Useful if we want to use ConsoleSocket() for a socket created by
> socketpair().
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/console_socket.py | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/python/qemu/machine/console_socket.py 
> b/python/qemu/machine/console_socket.py
> index 4e28ba9bb2..42bfa12411 100644
> --- a/python/qemu/machine/console_socket.py
> +++ b/python/qemu/machine/console_socket.py
> @@ -17,7 +17,7 @@
>  import socket
>  import threading
>  import time
> -from typing import Deque, Optional
> +from typing import Deque, Optional, Union
>  
>  
>  class ConsoleSocket(socket.socket):
> @@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
>  Optionally a file path can be passed in and we will also
>  dump the characters to this file for debugging purposes.
>  """
> -def __init__(self, address: str, file: Optional[str] = None,
> +def __init__(self, address: Union[str, int], file: Optional[str] = None,
>   drain: bool = False):

IMHO calling the pre-opened FD an "address" is pushing the
interpretation a bit.

It also makes the behaviour non-obvious from a caller. Seeing a
caller, you have to work backwards to find out what type it has,
to figure out the semantics of the method call.

IOW, I'd prefer to see

   address: Optional[str], sock_fd: Optional[int]

and then just do a check

   if (address is not None and sock_fd is not None) or
  (address is None and sock_fd is None):
  raise Exception("either 'address' or 'sock_fd' is required")

thus when you see

   ConsoleSocket(sock_fd=xxx)

it is now obvious it has different behaviour to

   ConsoleSocket(address=yyy)


>  self._recv_timeout_sec = 300.0
>  self._sleep_time = 0.5
>  self._buffer: Deque[int] = deque()
> -socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> -self.connect(address)
> +if isinstance(address, str):
> +socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> +self.connect(address)
> +else:
> +socket.socket.__init__(self, fileno=address)
>  self._logfile = None
>  if file:
>  # pylint: disable=consider-using-with
> -- 
> 2.41.0
> 

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[PATCH] block: Be more verbose in create fallback

2023-07-20 Thread Hanna Czenczek 
For image creation code, we have central fallback code for protocols
that do not support creating new images (like NBD or iscsi).  So for
them, you can only specify existing paths/exports that are overwritten
to make clean new images.  In such a case, if the given path cannot be
opened (assuming a pre-existing image there), we print an error message
that tries to describe what is going on: That with this protocol, you
cannot create new images, but only overwrite existing ones; and the
given path could not be opened as a pre-existing image.

However, the current message is confusing, because it does not say that
the protocol in question does not support creating new images, but
instead that "image creation" is unsupported.  This can be interpreted
to mean that `qemu-img create` will not work in principle, which is not
true.  Be more verbose for clarity.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2217204
Signed-off-by: Hanna Czenczek 
---
 block.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index a307c151a8..f530dd9c02 100644
--- a/block.c
+++ b/block.c
@@ -661,8 +661,10 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver 
*drv,
 blk = blk_co_new_open(filename, NULL, options,
   BDRV_O_RDWR | BDRV_O_RESIZE, errp);
 if (!blk) {
-error_prepend(errp, "Protocol driver '%s' does not support image "
-  "creation, and opening the image failed: ",
+error_prepend(errp, "Protocol driver '%s' does not support creating "
+  "new images, so an existing image must be selected as "
+  "the target; however, opening the given target as an "
+  "existing image failed: ",
   drv->format_name);
 return -EINVAL;
 }
-- 
2.41.0

Re: [PATCH 1/4] python/machine: move socket setup out of _base_args property

2023-07-20 Thread Daniel P . Berrangé 
On Thu, Jul 20, 2023 at 09:04:45AM -0400, John Snow wrote:
> This property isn't meant to do much else besides return a list of
> strings, so move this setup back out into _pre_launch().
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/machine.py | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] python/machine: use socketpair() for console socket

2023-07-20 Thread Peter Maydell 
On Thu, 20 Jul 2023 at 14:04, John Snow  wrote:
>
> Like we did for the QMP socket, use socketpair() for the console socket
> so that hopefully there isn't a race condition during early boot where
> data might get dropped on the floor.
>
> "lightly tested"; passes local tests and gitlab CI. Doesn't seem to make
> anything worse.

I tried this on the s390 linux box and the test failed because
of a python exception:

__init__() got an unexpected keyword argument 'sock_dir'

  $ QEMU_TEST_FLAKY_TESTS=1 ./build/aarch64/tests/venv/bin/avocado run
./build/aarch64/tests/avocado/machine_aarch64_sbsaref.py:Aarch64SbsarefMachine.test_sbsaref_edk2_firmware
JOB ID : 8392ba37b5a825ed75278f85f686364d181c01d3
JOB LOG: 
/home/linux1/avocado/job-results/job-2023-07-20T13.41-8392ba3/job.log
 (1/1) 
./build/aarch64/tests/avocado/machine_aarch64_sbsaref.py:Aarch64SbsarefMachine.test_sbsaref_edk2_firmware:
ERROR: __init__() got an unexpected keyword argument 'sock_dir' (3.64
s)
RESULTS: PASS 0 | ERROR 1 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0
| CANCEL 0
JOB TIME   : 6.92 s

Backtrace etc from in the job.log:

2023-07-20 13:41:49,125 stacktrace   L0041 ERROR| Reproduced
traceback from:
/home/linux1/qemu/build/aarch64/tests/venv/lib/python3.8/site-package
s/avocado/core/test.py:770
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR| Traceback (most
recent call last):
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR|   File
"/home/linux1/qemu/build/aarch64/tests/venv/lib/python3.8/site-packages/avocado/core/decorators.py",
line 90, in wrapper
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR| return
function(obj, *args, **kwargs)
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR|   File
"/home/linux1/qemu/build/aarch64/tests/avocado/machine_aarch64_sbsaref.py",
line 84, in test_sbsaref_edk2_firmware
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR| self.fetch_firmware()
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR|   File
"/home/linux1/qemu/build/aarch64/tests/avocado/machine_aarch64_sbsaref.py",
line 66, in fetch_firmware
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR| self.vm.set_console()
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR|   File
"/home/linux1/qemu/build/aarch64/tests/avocado/avocado_qemu/__init__.py",
line 348, in vm
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR| return
self.get_vm(name='default')
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR|   File
"/home/linux1/qemu/build/aarch64/tests/avocado/avocado_qemu/__init__.py",
line 354, in get_vm
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR|
self._vms[name] = self._new_vm(name, *args)
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR|   File
"/home/linux1/qemu/build/aarch64/tests/avocado/avocado_qemu/__init__.py",
line 324, in _new_vm
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR| vm =
QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
2023-07-20 13:41:49,147 stacktrace   L0045 ERROR| TypeError:
__init__() got an unexpected keyword argument 'sock_dir'
2023-07-20 13:41:49,147 stacktrace   L0046 ERROR|
2023-07-20 13:41:49,147 test L0775 DEBUG| Local variables:
2023-07-20 13:41:49,160 test L0778 DEBUG|  -> obj :
1-./build/aarch64/tests/avocado/machine_aarch64_sbsaref.py:Aarch64SbsarefMachine.test_sbsaref_edk2_firmware
2023-07-20 13:41:49,160 test L0778 DEBUG|  -> args : ()
2023-07-20 13:41:49,160 test L0778 DEBUG|  -> kwargs
: {}
2023-07-20 13:41:49,160 test L0778 DEBUG|  -> condition
: 1
2023-07-20 13:41:49,160 test L0778 DEBUG|  -> function
: 
2023-07-20 13:41:49,160 test L0778 DEBUG|  -> message
: Test is not reliable
2023-07-20 13:41:49,160 test L0778 DEBUG|  -> negate
: True

thanks
-- PMM

Re: [PATCH] hw/nvme: use stl/ldl pci dma api

2023-07-20 Thread Philippe Mathieu-Daudé 

On 20/7/23 11:42, Klaus Jensen wrote:

From: Klaus Jensen 

Use the stl/ldl pci dma api for writing/reading doorbells. This removes
the explicit endian conversions.

Signed-off-by: Klaus Jensen 
---
  hw/nvme/ctrl.c | 42 +-
  1 file changed, 13 insertions(+), 29 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

[PATCH 1/4] python/machine: move socket setup out of _base_args property

2023-07-20 Thread John Snow 
This property isn't meant to do much else besides return a list of
strings, so move this setup back out into _pre_launch().

Signed-off-by: John Snow 
---
 python/qemu/machine/machine.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index c16a0b6fed..8be0f684fe 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -300,9 +300,7 @@ def _base_args(self) -> List[str]:
 
 if self._qmp_set:
 if self._sock_pair:
-fd = self._sock_pair[0].fileno()
-os.set_inheritable(fd, True)
-moncdev = f"socket,id=mon,fd={fd}"
+moncdev = f"socket,id=mon,fd={self._sock_pair[0].fileno()}"
 elif isinstance(self._monitor_address, tuple):
 moncdev = "socket,id=mon,host={},port={}".format(
 *self._monitor_address
@@ -339,6 +337,7 @@ def _pre_launch(self) -> None:
 if self._qmp_set:
 if self._monitor_address is None:
 self._sock_pair = socket.socketpair()
+os.set_inheritable(self._sock_pair[0].fileno(), True)
 sock = self._sock_pair[1]
 if isinstance(self._monitor_address, str):
 self._remove_files.append(self._monitor_address)
-- 
2.41.0

[PATCH 3/4] python/machine: use socketpair() for console connections

2023-07-20 Thread John Snow 
Create a socketpair for the console output. This should help eliminate
race conditions around console text early in the boot process that might
otherwise have been dropped on the floor before being able to connect to
QEMU under "server,nowait".

Signed-off-by: John Snow 
---
 python/qemu/machine/machine.py | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 8be0f684fe..ef9b2dc02e 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -159,6 +159,8 @@ def __init__(self,
 
 self._name = name or f"{id(self):x}"
 self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
+self._cons_sock_pair: Optional[
+Tuple[socket.socket, socket.socket]] = None
 self._temp_dir: Optional[str] = None
 self._base_temp_dir = base_temp_dir
 self._sock_dir = sock_dir
@@ -315,8 +317,9 @@ def _base_args(self) -> List[str]:
 for _ in range(self._console_index):
 args.extend(['-serial', 'null'])
 if self._console_set:
-chardev = ('socket,id=console,path=%s,server=on,wait=off' %
-   self._console_address)
+assert self._cons_sock_pair is not None
+fd = self._cons_sock_pair[0].fileno()
+chardev = f"socket,id=console,fd={fd}"
 args.extend(['-chardev', chardev])
 if self._console_device_type is None:
 args.extend(['-serial', 'chardev:console'])
@@ -351,6 +354,10 @@ def _pre_launch(self) -> None:
 nickname=self._name
 )
 
+if self._console_set:
+self._cons_sock_pair = socket.socketpair()
+os.set_inheritable(self._cons_sock_pair[0].fileno(), True)
+
 # NOTE: Make sure any opened resources are *definitely* freed in
 # _post_shutdown()!
 # pylint: disable=consider-using-with
@@ -873,8 +880,12 @@ def console_socket(self) -> socket.socket:
 Returns a socket connected to the console
 """
 if self._console_socket is None:
+if not self._console_set:
+raise QEMUMachineError(
+"Attempt to access console socket with no connection")
+assert self._cons_sock_pair is not None
 self._console_socket = console_socket.ConsoleSocket(
-self._console_address,
+self._cons_sock_pair[1].fileno(),
 file=self._console_log_path,
 drain=self._drain_console)
 return self._console_socket
-- 
2.41.0

[PATCH 2/4] python/console_socket: accept existing FD in initializer

2023-07-20 Thread John Snow 
Useful if we want to use ConsoleSocket() for a socket created by
socketpair().

Signed-off-by: John Snow 
---
 python/qemu/machine/console_socket.py | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/python/qemu/machine/console_socket.py 
b/python/qemu/machine/console_socket.py
index 4e28ba9bb2..42bfa12411 100644
--- a/python/qemu/machine/console_socket.py
+++ b/python/qemu/machine/console_socket.py
@@ -17,7 +17,7 @@
 import socket
 import threading
 import time
-from typing import Deque, Optional
+from typing import Deque, Optional, Union
 
 
 class ConsoleSocket(socket.socket):
@@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
 Optionally a file path can be passed in and we will also
 dump the characters to this file for debugging purposes.
 """
-def __init__(self, address: str, file: Optional[str] = None,
+def __init__(self, address: Union[str, int], file: Optional[str] = None,
  drain: bool = False):
 self._recv_timeout_sec = 300.0
 self._sleep_time = 0.5
 self._buffer: Deque[int] = deque()
-socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
-self.connect(address)
+if isinstance(address, str):
+socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
+self.connect(address)
+else:
+socket.socket.__init__(self, fileno=address)
 self._logfile = None
 if file:
 # pylint: disable=consider-using-with
-- 
2.41.0

[PATCH 4/4] python/machine: remove unused console socket configuration arguments

2023-07-20 Thread John Snow 
By using a socketpair for the console, we don't need the sock_dir
argument for the base class anymore, remove it.

The qtest subclass still uses the argument for the qtest socket for now.

Signed-off-by: John Snow 
---
 python/qemu/machine/machine.py | 18 --
 python/qemu/machine/qtest.py   |  6 +++---
 tests/qemu-iotests/tests/copy-before-write |  3 +--
 3 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ef9b2dc02e..350aa8bb26 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -127,7 +127,6 @@ def __init__(self,
  name: Optional[str] = None,
  base_temp_dir: str = "/var/tmp",
  monitor_address: Optional[SocketAddrT] = None,
- sock_dir: Optional[str] = None,
  drain_console: bool = False,
  console_log: Optional[str] = None,
  log_dir: Optional[str] = None,
@@ -141,7 +140,6 @@ def __init__(self,
 @param name: prefix for socket and log file names (default: qemu-PID)
 @param base_temp_dir: default location where temp files are created
 @param monitor_address: address for QMP monitor
-@param sock_dir: where to create socket (defaults to base_temp_dir)
 @param drain_console: (optional) True to drain console socket to buffer
 @param console_log: (optional) path to console log file
 @param log_dir: where to create and keep log files
@@ -163,7 +161,6 @@ def __init__(self,
 Tuple[socket.socket, socket.socket]] = None
 self._temp_dir: Optional[str] = None
 self._base_temp_dir = base_temp_dir
-self._sock_dir = sock_dir
 self._log_dir = log_dir
 
 self._monitor_address = monitor_address
@@ -189,9 +186,6 @@ def __init__(self,
 self._console_index = 0
 self._console_set = False
 self._console_device_type: Optional[str] = None
-self._console_address = os.path.join(
-self.sock_dir, f"{self._name}.con"
-)
 self._console_socket: Optional[socket.socket] = None
 self._remove_files: List[str] = []
 self._user_killed = False
@@ -334,9 +328,6 @@ def args(self) -> List[str]:
 return self._args
 
 def _pre_launch(self) -> None:
-if self._console_set:
-self._remove_files.append(self._console_address)
-
 if self._qmp_set:
 if self._monitor_address is None:
 self._sock_pair = socket.socketpair()
@@ -900,15 +891,6 @@ def temp_dir(self) -> str:
   dir=self._base_temp_dir)
 return self._temp_dir
 
-@property
-def sock_dir(self) -> str:
-"""
-Returns the directory used for sockfiles by this machine.
-"""
-if self._sock_dir:
-return self._sock_dir
-return self.temp_dir
-
 @property
 def log_dir(self) -> str:
 """
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 1c46138bd0..22f8045ef6 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -115,8 +115,8 @@ def __init__(self,
  wrapper: Sequence[str] = (),
  name: Optional[str] = None,
  base_temp_dir: str = "/var/tmp",
- sock_dir: Optional[str] = None,
- qmp_timer: Optional[float] = None):
+ qmp_timer: Optional[float] = None,
+ sock_dir: Optional[str] = None):
 # pylint: disable=too-many-arguments
 
 if name is None:
@@ -125,7 +125,7 @@ def __init__(self,
 sock_dir = base_temp_dir
 super().__init__(binary, args, wrapper=wrapper, name=name,
  base_temp_dir=base_temp_dir,
- sock_dir=sock_dir, qmp_timer=qmp_timer)
+ qmp_timer=qmp_timer)
 self._qtest: Optional[QEMUQtestProtocol] = None
 self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
diff --git a/tests/qemu-iotests/tests/copy-before-write 
b/tests/qemu-iotests/tests/copy-before-write
index 2ffe092b31..d3987db942 100755
--- a/tests/qemu-iotests/tests/copy-before-write
+++ b/tests/qemu-iotests/tests/copy-before-write
@@ -44,8 +44,7 @@ class TestCbwError(iotests.QMPTestCase):
 
 opts = ['-nodefaults', '-display', 'none', '-machine', 'none']
 self.vm = QEMUMachine(iotests.qemu_prog, opts,
-  base_temp_dir=iotests.test_dir,
-  sock_dir=iotests.sock_dir)
+  base_temp_dir=iotests.test_dir)
 self.vm.launch()
 
 def do_cbw_error(self, on_cbw_error):
-- 
2.41.0

[PATCH 0/4] python/machine: use socketpair() for console socket

2023-07-20 Thread John Snow 
Like we did for the QMP socket, use socketpair() for the console socket
so that hopefully there isn't a race condition during early boot where
data might get dropped on the floor.

"lightly tested"; passes local tests and gitlab CI. Doesn't seem to make
anything worse.

John Snow (4):
  python/machine: move socket setup out of _base_args property
  python/console_socket: accept existing FD in initializer
  python/machine: use socketpair() for console connections
  python/machine: remove unused console socket configuration arguments

 python/qemu/machine/console_socket.py  | 11 +++---
 python/qemu/machine/machine.py | 40 +-
 python/qemu/machine/qtest.py   |  6 ++--
 tests/qemu-iotests/tests/copy-before-write |  3 +-
 4 files changed, 27 insertions(+), 33 deletions(-)

-- 
2.41.0

Re: [PATCH] hw/nvme: use stl/ldl pci dma api

2023-07-20 Thread Thomas Huth 

On 20/07/2023 11.42, Klaus Jensen wrote:

From: Klaus Jensen 

Use the stl/ldl pci dma api for writing/reading doorbells. This removes
the explicit endian conversions.

Signed-off-by: Klaus Jensen 
---
  hw/nvme/ctrl.c | 42 +-
  1 file changed, 13 insertions(+), 29 deletions(-)



Reviewed-by: Thomas Huth

[PATCH] hw/nvme: use stl/ldl pci dma api

2023-07-20 Thread Klaus Jensen 
From: Klaus Jensen 

Use the stl/ldl pci dma api for writing/reading doorbells. This removes
the explicit endian conversions.

Signed-off-by: Klaus Jensen 
---
 hw/nvme/ctrl.c | 42 +-
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index dadc2dc7da10..f2e5a2fa737b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1468,20 +1468,16 @@ static inline void nvme_blk_write(BlockBackend *blk, 
int64_t offset,
 
 static void nvme_update_cq_eventidx(const NvmeCQueue *cq)
 {
-uint32_t v = cpu_to_le32(cq->head);
-
 trace_pci_nvme_update_cq_eventidx(cq->cqid, cq->head);
 
-pci_dma_write(PCI_DEVICE(cq->ctrl), cq->ei_addr, &v, sizeof(v));
+stl_le_pci_dma(PCI_DEVICE(cq->ctrl), cq->ei_addr, cq->head,
+   MEMTXATTRS_UNSPECIFIED);
 }
 
 static void nvme_update_cq_head(NvmeCQueue *cq)
 {
-uint32_t v;
-
-pci_dma_read(PCI_DEVICE(cq->ctrl), cq->db_addr, &v, sizeof(v));
-
-cq->head = le32_to_cpu(v);
+ldl_le_pci_dma(PCI_DEVICE(cq->ctrl), cq->db_addr, &cq->head,
+   MEMTXATTRS_UNSPECIFIED);
 
 trace_pci_nvme_update_cq_head(cq->cqid, cq->head);
 }
@@ -6801,7 +6797,6 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
 PCIDevice *pci = PCI_DEVICE(n);
 uint64_t dbs_addr = le64_to_cpu(req->cmd.dptr.prp1);
 uint64_t eis_addr = le64_to_cpu(req->cmd.dptr.prp2);
-uint32_t v;
 int i;
 
 /* Address should be page aligned */
@@ -6819,8 +6814,6 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
 NvmeCQueue *cq = n->cq[i];
 
 if (sq) {
-v = cpu_to_le32(sq->tail);
-
 /*
  * CAP.DSTRD is 0, so offset of ith sq db_addr is (i<<3)
  * nvme_process_db() uses this hard-coded way to calculate
@@ -6828,7 +6821,7 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
  */
 sq->db_addr = dbs_addr + (i << 3);
 sq->ei_addr = eis_addr + (i << 3);
-pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
+stl_le_pci_dma(pci, sq->db_addr, sq->tail, MEMTXATTRS_UNSPECIFIED);
 
 if (n->params.ioeventfd && sq->sqid != 0) {
 if (!nvme_init_sq_ioeventfd(sq)) {
@@ -6838,12 +6831,10 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
 }
 
 if (cq) {
-v = cpu_to_le32(cq->head);
-
 /* CAP.DSTRD is 0, so offset of ith cq db_addr is (i<<3)+(1<<2) */
 cq->db_addr = dbs_addr + (i << 3) + (1 << 2);
 cq->ei_addr = eis_addr + (i << 3) + (1 << 2);
-pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+stl_le_pci_dma(pci, cq->db_addr, cq->head, MEMTXATTRS_UNSPECIFIED);
 
 if (n->params.ioeventfd && cq->cqid != 0) {
 if (!nvme_init_cq_ioeventfd(cq)) {
@@ -6974,20 +6965,16 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest 
*req)
 
 static void nvme_update_sq_eventidx(const NvmeSQueue *sq)
 {
-uint32_t v = cpu_to_le32(sq->tail);
-
 trace_pci_nvme_update_sq_eventidx(sq->sqid, sq->tail);
 
-pci_dma_write(PCI_DEVICE(sq->ctrl), sq->ei_addr, &v, sizeof(v));
+stl_le_pci_dma(PCI_DEVICE(sq->ctrl), sq->ei_addr, sq->tail,
+   MEMTXATTRS_UNSPECIFIED);
 }
 
 static void nvme_update_sq_tail(NvmeSQueue *sq)
 {
-uint32_t v;
-
-pci_dma_read(PCI_DEVICE(sq->ctrl), sq->db_addr, &v, sizeof(v));
-
-sq->tail = le32_to_cpu(v);
+ldl_le_pci_dma(PCI_DEVICE(sq->ctrl), sq->db_addr, &sq->tail,
+   MEMTXATTRS_UNSPECIFIED);
 
 trace_pci_nvme_update_sq_tail(sq->sqid, sq->tail);
 }
@@ -7592,7 +7579,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, 
unsigned size)
 static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
 {
 PCIDevice *pci = PCI_DEVICE(n);
-uint32_t qid, v;
+uint32_t qid;
 
 if (unlikely(addr & ((1 << 2) - 1))) {
 NVME_GUEST_ERR(pci_nvme_ub_db_wr_misaligned,
@@ -7659,8 +7646,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
 start_sqs = nvme_cq_full(cq) ? 1 : 0;
 cq->head = new_head;
 if (!qid && n->dbbuf_enabled) {
-v = cpu_to_le32(cq->head);
-pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+stl_le_pci_dma(pci, cq->db_addr, cq->head, MEMTXATTRS_UNSPECIFIED);
 }
 if (start_sqs) {
 NvmeSQueue *sq;
@@ -7720,8 +7706,6 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
 
 sq->tail = new_tail;
 if (!qid && n->dbbuf_enabled) {
-v = cpu_to_le32(sq->tail);
-
 /*
  * The spec states "the host shall also update the controller's
  * corresponding doorbell property to match the value of that entry
@@ -7735,7 +7719,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr

Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells

2023-07-20 Thread Klaus Jensen 
On Jul 20 09:51, Peter Maydell wrote:
> On Thu, 20 Jul 2023 at 09:49, Klaus Jensen  wrote:
> >
> > On Jul 20 09:43, Peter Maydell wrote:
> > > On Wed, 19 Jul 2023 at 21:13, Michael Tokarev  wrote:
> > > >
> > > > 19.07.2023 10:36, Klaus Jensen wrote:
> > > > pu(req->cmd.dptr.prp2);
> > > > > +uint32_t v;
> > > >
> > > > >   if (sq) {
> > > > > +v = cpu_to_le32(sq->tail);
> > > >
> > > > > -pci_dma_write(pci, sq->db_addr, &sq->tail, 
> > > > > sizeof(sq->tail));
> > > > > +pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
> > > >
> > > > This and similar cases hurts my eyes.
> > > >
> > > > Why we pass address of v here, but use sizeof(sq->tail) ?
> > > >
> > > > Yes, I know both in theory should be of the same size, but heck,
> > > > this is puzzling at best, and confusing in a regular case.
> > > >
> > > > Dunno how it slipped in the review, it instantly catched my eye
> > > > in a row of applied patches..
> > > >
> > > > Also, why v is computed a few lines before it is used, with
> > > > some expressions between the assignment and usage?
> > > >
> > > > How about the following patch:
> > >
> > > If you're going to change this, better to take the approach
> > > Philippe suggested in review of using stl_le_pci_dma().
> > >
> > > https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/
> > >
> >
> > Yup, that was my plan for next. But the original patch was already
> > verified on hardware and mutiple testes, so wanted to go with that for
> > the "fix".
> >
> > But yes, I will refactor into the much nicer stl/ldl api.
> 
> FWIW, I don't think this bug fix was so urgent that we
> needed to go with a quick fix and a followup -- we're
> not yet that close to 8.1 release.
> 

Alright, noted ;) I will spin this into the other fix I have under
review.


signature.asc
Description: PGP signature

Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells

2023-07-20 Thread Peter Maydell 
On Thu, 20 Jul 2023 at 09:49, Klaus Jensen  wrote:
>
> On Jul 20 09:43, Peter Maydell wrote:
> > On Wed, 19 Jul 2023 at 21:13, Michael Tokarev  wrote:
> > >
> > > 19.07.2023 10:36, Klaus Jensen wrote:
> > > pu(req->cmd.dptr.prp2);
> > > > +uint32_t v;
> > >
> > > >   if (sq) {
> > > > +v = cpu_to_le32(sq->tail);
> > >
> > > > -pci_dma_write(pci, sq->db_addr, &sq->tail, 
> > > > sizeof(sq->tail));
> > > > +pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
> > >
> > > This and similar cases hurts my eyes.
> > >
> > > Why we pass address of v here, but use sizeof(sq->tail) ?
> > >
> > > Yes, I know both in theory should be of the same size, but heck,
> > > this is puzzling at best, and confusing in a regular case.
> > >
> > > Dunno how it slipped in the review, it instantly catched my eye
> > > in a row of applied patches..
> > >
> > > Also, why v is computed a few lines before it is used, with
> > > some expressions between the assignment and usage?
> > >
> > > How about the following patch:
> >
> > If you're going to change this, better to take the approach
> > Philippe suggested in review of using stl_le_pci_dma().
> >
> > https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/
> >
>
> Yup, that was my plan for next. But the original patch was already
> verified on hardware and mutiple testes, so wanted to go with that for
> the "fix".
>
> But yes, I will refactor into the much nicer stl/ldl api.

FWIW, I don't think this bug fix was so urgent that we
needed to go with a quick fix and a followup -- we're
not yet that close to 8.1 release.

thanks
-- PMM

Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells

2023-07-20 Thread Klaus Jensen 
On Jul 20 09:43, Peter Maydell wrote:
> On Wed, 19 Jul 2023 at 21:13, Michael Tokarev  wrote:
> >
> > 19.07.2023 10:36, Klaus Jensen wrote:
> > pu(req->cmd.dptr.prp2);
> > > +uint32_t v;
> >
> > >   if (sq) {
> > > +v = cpu_to_le32(sq->tail);
> >
> > > -pci_dma_write(pci, sq->db_addr, &sq->tail, sizeof(sq->tail));
> > > +pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
> >
> > This and similar cases hurts my eyes.
> >
> > Why we pass address of v here, but use sizeof(sq->tail) ?
> >
> > Yes, I know both in theory should be of the same size, but heck,
> > this is puzzling at best, and confusing in a regular case.
> >
> > Dunno how it slipped in the review, it instantly catched my eye
> > in a row of applied patches..
> >
> > Also, why v is computed a few lines before it is used, with
> > some expressions between the assignment and usage?
> >
> > How about the following patch:
> 
> If you're going to change this, better to take the approach
> Philippe suggested in review of using stl_le_pci_dma().
> 
> https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/
> 

Yup, that was my plan for next. But the original patch was already
verified on hardware and mutiple testes, so wanted to go with that for
the "fix".

But yes, I will refactor into the much nicer stl/ldl api.


signature.asc
Description: PGP signature

Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells

2023-07-20 Thread Peter Maydell 
On Wed, 19 Jul 2023 at 21:13, Michael Tokarev  wrote:
>
> 19.07.2023 10:36, Klaus Jensen wrote:
> pu(req->cmd.dptr.prp2);
> > +uint32_t v;
>
> >   if (sq) {
> > +v = cpu_to_le32(sq->tail);
>
> > -pci_dma_write(pci, sq->db_addr, &sq->tail, sizeof(sq->tail));
> > +pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
>
> This and similar cases hurts my eyes.
>
> Why we pass address of v here, but use sizeof(sq->tail) ?
>
> Yes, I know both in theory should be of the same size, but heck,
> this is puzzling at best, and confusing in a regular case.
>
> Dunno how it slipped in the review, it instantly catched my eye
> in a row of applied patches..
>
> Also, why v is computed a few lines before it is used, with
> some expressions between the assignment and usage?
>
> How about the following patch:

If you're going to change this, better to take the approach
Philippe suggested in review of using stl_le_pci_dma().

https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/

thanks
-- PMM