Re: [PATCH] raw-format: Fix error message for invalid offset/size

2024-10-25 Thread Michael Tokarev 

29.08.2024 21:55, Kevin Wolf wrote:

s->offset and s->size are only set at the end of the function and still
contain the old values when formatting the error message. Print the
parameters with the new values that we actually checked instead.

Fixes: 500e2434207d ('raw-format: Split raw_read_options()')
Signed-off-by: Kevin Wolf 


I picked this up for stable.  It's not a big deal but the change is trivial too.

JFYI,

/mjt

[PATCH v4 10/15] hw/vmapple/aes: Introduce aes engine

2024-10-25 Thread Phil Dennis-Jordan 
From: Alexander Graf 

VMApple contains an "aes" engine device that it uses to encrypt and
decrypt its nvram. It has trivial hard coded keys it uses for that
purpose.

Add device emulation for this device model.

Signed-off-by: Alexander Graf 
Signed-off-by: Phil Dennis-Jordan 
---
v3:

 * Rebased on latest upstream and fixed minor breakages.
 * Replaced legacy device reset method with Resettable method

v4:

 * Improved logging of unimplemented functions and guest errors.
 * Better adherence to naming and coding conventions.
 * Cleaner error handling and recovery, including using g_autoptr

 hw/vmapple/Kconfig  |   2 +
 hw/vmapple/aes.c| 572 
 hw/vmapple/meson.build  |   1 +
 hw/vmapple/trace-events |  16 ++
 include/qemu/cutils.h   |  15 ++
 util/hexdump.c  |  14 +
 6 files changed, 620 insertions(+)
 create mode 100644 hw/vmapple/aes.c

diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
index 8b137891791..a73504d5999 100644
--- a/hw/vmapple/Kconfig
+++ b/hw/vmapple/Kconfig
@@ -1 +1,3 @@
+config VMAPPLE_AES
+bool
 
diff --git a/hw/vmapple/aes.c b/hw/vmapple/aes.c
new file mode 100644
index 000..59cdcd65f90
--- /dev/null
+++ b/hw/vmapple/aes.c
@@ -0,0 +1,572 @@
+/*
+ * QEMU Apple AES device emulation
+ *
+ * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "trace.h"
+#include "crypto/hash.h"
+#include "crypto/aes.h"
+#include "crypto/cipher.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "qemu/cutils.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "sysemu/dma.h"
+
+#define TYPE_AES  "apple-aes"
+OBJECT_DECLARE_SIMPLE_TYPE(AESState, AES)
+
+#define MAX_FIFO_SIZE 9
+
+#define CMD_KEY   0x1
+#define CMD_KEY_CONTEXT_SHIFT27
+#define CMD_KEY_CONTEXT_MASK (0x1 << CMD_KEY_CONTEXT_SHIFT)
+#define CMD_KEY_SELECT_MAX_IDX   0x7
+#define CMD_KEY_SELECT_SHIFT 24
+#define CMD_KEY_SELECT_MASK  (CMD_KEY_SELECT_MAX_IDX << 
CMD_KEY_SELECT_SHIFT)
+#define CMD_KEY_KEY_LEN_NUM  4u
+#define CMD_KEY_KEY_LEN_SHIFT22
+#define CMD_KEY_KEY_LEN_MASK ((CMD_KEY_KEY_LEN_NUM - 1u) << 
CMD_KEY_KEY_LEN_SHIFT)
+#define CMD_KEY_ENCRYPT_SHIFT20
+#define CMD_KEY_ENCRYPT_MASK (0x1 << CMD_KEY_ENCRYPT_SHIFT)
+#define CMD_KEY_BLOCK_MODE_SHIFT 16
+#define CMD_KEY_BLOCK_MODE_MASK  (0x3 << CMD_KEY_BLOCK_MODE_SHIFT)
+#define CMD_IV0x2
+#define CMD_IV_CONTEXT_SHIFT 26
+#define CMD_IV_CONTEXT_MASK  (0x3 << CMD_KEY_CONTEXT_SHIFT)
+#define CMD_DSB   0x3
+#define CMD_SKG   0x4
+#define CMD_DATA  0x5
+#define CMD_DATA_KEY_CTX_SHIFT   27
+#define CMD_DATA_KEY_CTX_MASK(0x1 << CMD_DATA_KEY_CTX_SHIFT)
+#define CMD_DATA_IV_CTX_SHIFT25
+#define CMD_DATA_IV_CTX_MASK (0x3 << CMD_DATA_IV_CTX_SHIFT)
+#define CMD_DATA_LEN_MASK0xff
+#define CMD_STORE_IV  0x6
+#define CMD_STORE_IV_ADDR_MASK   0xff
+#define CMD_WRITE_REG 0x7
+#define CMD_FLAG  0x8
+#define CMD_FLAG_STOP_MASK   BIT(26)
+#define CMD_FLAG_RAISE_IRQ_MASK  BIT(27)
+#define CMD_FLAG_INFO_MASK   0xff
+#define CMD_MAX   0x10
+
+#define CMD_SHIFT 28
+
+#define REG_STATUS0xc
+#define REG_STATUS_DMA_READ_RUNNING BIT(0)
+#define REG_STATUS_DMA_READ_PENDING BIT(1)
+#define REG_STATUS_DMA_WRITE_RUNNINGBIT(2)
+#define REG_STATUS_DMA_WRITE_PENDINGBIT(3)
+#define REG_STATUS_BUSY BIT(4)
+#define REG_STATUS_EXECUTINGBIT(5)
+#define REG_STATUS_READYBIT(6)
+#define REG_STATUS_TEXT_DPA_SEEDED  BIT(7)
+#define REG_STATUS_UNWRAP_DPA_SEEDEDBIT(8)
+
+#define REG_IRQ_STATUS0x18
+#define REG_IRQ_STATUS_INVALID_CMD  BIT(2)
+#define REG_IRQ_STATUS_FLAG BIT(5)
+#define REG_IRQ_ENABLE0x1c
+#define REG_WATERMARK 0x20
+#define REG_Q_STATUS  0x24
+#define REG_FLAG_INFO 0x30
+#define REG_FIFO  0x200
+
+static const uint32_t key_lens[CMD_KEY_KEY_LEN_NUM] = {
+[0] = 16,
+[1] = 24,
+[2] = 32,
+[3] = 64,
+};
+
+typedef struct Key {
+uint32_t key_len;
+uint8_t key[32];
+} Key;
+
+typedef struct IV {
+uint32_t iv[4];
+} IV;
+
+static Key builtin_keys[CMD_KEY_SELECT_MAX_IDX + 1] = {
+[1] = {
+.key_len = 32,
+.key = { 0x1 },
+},
+[2] = {
+.key_len = 32,
+.key = { 0x2 },
+},
+[3] = {
+.key_len = 32,
+.key = { 0x3 },
+}
+};
+
+struct AESState {
+SysBusDevice parent_obj;
+
+qemu_irq irq;
+MemoryRegion iomem1;
+MemoryRegion iomem2;
+AddressSpace *as;
+
+uint32_t status;
+uint32_t q_status;
+uint32_t irq_status;
+uint32_t irq_enable;
+uint32_t watermark;
+uint32_t flag_info;

[PATCH v4 06/15] hw: Add vmapple subdir

2024-10-25 Thread Phil Dennis-Jordan 
From: Alexander Graf 

We will introduce a number of devices that are specific to the vmapple
target machine. To keep them all tidily together, let's put them into
a single target directory.

Signed-off-by: Alexander Graf 
Signed-off-by: Phil Dennis-Jordan 
Reviewed-by: Akihiko Odaki 
---
 MAINTAINERS | 7 +++
 hw/Kconfig  | 1 +
 hw/meson.build  | 1 +
 hw/vmapple/Kconfig  | 1 +
 hw/vmapple/meson.build  | 0
 hw/vmapple/trace-events | 2 ++
 hw/vmapple/trace.h  | 1 +
 meson.build | 1 +
 8 files changed, 14 insertions(+)
 create mode 100644 hw/vmapple/Kconfig
 create mode 100644 hw/vmapple/meson.build
 create mode 100644 hw/vmapple/trace-events
 create mode 100644 hw/vmapple/trace.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 16ea47a5e6d..104813ed85f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2733,6 +2733,13 @@ F: hw/hyperv/hv-balloon*.h
 F: include/hw/hyperv/dynmem-proto.h
 F: include/hw/hyperv/hv-balloon.h
 
+VMapple
+M: Alexander Graf 
+R: Phil Dennis-Jordan 
+S: Maintained
+F: hw/vmapple/*
+F: include/hw/vmapple/*
+
 Subsystems
 --
 Overall Audio backends
diff --git a/hw/Kconfig b/hw/Kconfig
index 1b4e9bb07f7..2871784cfdc 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -41,6 +41,7 @@ source ufs/Kconfig
 source usb/Kconfig
 source virtio/Kconfig
 source vfio/Kconfig
+source vmapple/Kconfig
 source xen/Kconfig
 source watchdog/Kconfig
 
diff --git a/hw/meson.build b/hw/meson.build
index b827c82c5d7..9c4f6d0d636 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -39,6 +39,7 @@ subdir('ufs')
 subdir('usb')
 subdir('vfio')
 subdir('virtio')
+subdir('vmapple')
 subdir('watchdog')
 subdir('xen')
 subdir('xenpv')
diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
new file mode 100644
index 000..8b137891791
--- /dev/null
+++ b/hw/vmapple/Kconfig
@@ -0,0 +1 @@
+
diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build
new file mode 100644
index 000..e69de29bb2d
diff --git a/hw/vmapple/trace-events b/hw/vmapple/trace-events
new file mode 100644
index 000..9ccc5790487
--- /dev/null
+++ b/hw/vmapple/trace-events
@@ -0,0 +1,2 @@
+# See docs/devel/tracing.rst for syntax documentation.
+
diff --git a/hw/vmapple/trace.h b/hw/vmapple/trace.h
new file mode 100644
index 000..572adbefe04
--- /dev/null
+++ b/hw/vmapple/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-hw_vmapple.h"
diff --git a/meson.build b/meson.build
index 0e124eff13f..dd07a425f0e 100644
--- a/meson.build
+++ b/meson.build
@@ -3478,6 +3478,7 @@ if have_system
 'hw/usb',
 'hw/vfio',
 'hw/virtio',
+'hw/vmapple',
 'hw/watchdog',
 'hw/xen',
 'hw/gpio',
-- 
2.39.3 (Apple Git-145)

[PATCH v4 09/15] gpex: Allow more than 4 legacy IRQs

2024-10-25 Thread Phil Dennis-Jordan 
From: Alexander Graf 

Some boards such as vmapple don't do real legacy PCI IRQ swizzling.
Instead, they just keep allocating more board IRQ lines for each new
legacy IRQ. Let's support that mode by giving instantiators a new
"nr_irqs" property they can use to support more than 4 legacy IRQ lines.
In this mode, GPEX will export more IRQ lines, one for each device.

Signed-off-by: Alexander Graf 
Signed-off-by: Phil Dennis-Jordan 
---

v4:

 * Turned pair of IRQ arrays into array of structs.
 * Simplified swizzling logic selection.

 hw/arm/sbsa-ref.c  |  2 +-
 hw/arm/virt.c  |  2 +-
 hw/i386/microvm.c  |  2 +-
 hw/loongarch/virt.c|  2 +-
 hw/mips/loongson3_virt.c   |  2 +-
 hw/openrisc/virt.c | 12 +--
 hw/pci-host/gpex.c | 43 ++
 hw/riscv/virt.c| 12 +--
 hw/xtensa/virt.c   |  2 +-
 include/hw/pci-host/gpex.h |  7 +++
 10 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index e3195d54497..7e7322486c2 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -673,7 +673,7 @@ static void create_pcie(SBSAMachineState *sms)
 /* Map IO port space */
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_pio);
 
-for (i = 0; i < GPEX_NUM_IRQS; i++) {
+for (i = 0; i < PCI_NUM_PINS; i++) {
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
qdev_get_gpio_in(sms->gic, irq + i));
 gpex_set_irq_num(GPEX_HOST(dev), i, irq + i);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8b2b991d978..bd3b17be2ea 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1547,7 +1547,7 @@ static void create_pcie(VirtMachineState *vms)
 /* Map IO port space */
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_pio);
 
-for (i = 0; i < GPEX_NUM_IRQS; i++) {
+for (i = 0; i < PCI_NUM_PINS; i++) {
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
qdev_get_gpio_in(vms->gic, irq + i));
 gpex_set_irq_num(GPEX_HOST(dev), i, irq + i);
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 693099f2256..b3a348bee09 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -139,7 +139,7 @@ static void create_gpex(MicrovmMachineState *mms)
 mms->gpex.mmio64.base, mmio64_alias);
 }
 
-for (i = 0; i < GPEX_NUM_IRQS; i++) {
+for (i = 0; i < PCI_NUM_PINS; i++) {
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
x86ms->gsi[mms->gpex.irq + i]);
 }
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 9a635d1d3d3..50056384994 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -741,7 +741,7 @@ static void virt_devices_init(DeviceState *pch_pic,
 memory_region_add_subregion(get_system_memory(), VIRT_PCI_IO_BASE,
 pio_alias);
 
-for (i = 0; i < GPEX_NUM_IRQS; i++) {
+for (i = 0; i < PCI_NUM_PINS; i++) {
 sysbus_connect_irq(d, i,
qdev_get_gpio_in(pch_pic, 16 + i));
 gpex_set_irq_num(GPEX_HOST(gpex_dev), i, 16 + i);
diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index f3b6326cc59..884b5f23a99 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -458,7 +458,7 @@ static inline void loongson3_virt_devices_init(MachineState 
*machine,
 virt_memmap[VIRT_PCIE_PIO].base, s->pio_alias);
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, virt_memmap[VIRT_PCIE_PIO].base);
 
-for (i = 0; i < GPEX_NUM_IRQS; i++) {
+for (i = 0; i < PCI_NUM_PINS; i++) {
 irq = qdev_get_gpio_in(pic, PCIE_IRQ_BASE + i);
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, irq);
 gpex_set_irq_num(GPEX_HOST(dev), i, PCIE_IRQ_BASE + i);
diff --git a/hw/openrisc/virt.c b/hw/openrisc/virt.c
index 47d2c9bd3c7..6f053bf48e0 100644
--- a/hw/openrisc/virt.c
+++ b/hw/openrisc/virt.c
@@ -318,7 +318,7 @@ static void create_pcie_irq_map(void *fdt, char *nodename, 
int irq_base,
 {
 int pin, dev;
 uint32_t irq_map_stride = 0;
-uint32_t full_irq_map[GPEX_NUM_IRQS * GPEX_NUM_IRQS * 6] = {};
+uint32_t full_irq_map[PCI_NUM_PINS * PCI_NUM_PINS * 6] = {};
 uint32_t *irq_map = full_irq_map;
 
 /*
@@ -330,11 +330,11 @@ static void create_pcie_irq_map(void *fdt, char 
*nodename, int irq_base,
  * possible slot) seeing the interrupt-map-mask will allow the table
  * to wrap to any number of devices.
  */
-for (dev = 0; dev < GPEX_NUM_IRQS; dev++) {
+for (dev = 0; dev < PCI_NUM_PINS; dev++) {
 int devfn = dev << 3;
 
-for (pin = 0; pin < GPEX_NUM_IRQS; pin++) {
-int irq_nr = irq_base + ((pin + PCI_SLOT(devfn)) % GPEX_NUM_IRQS);
+for (pin = 0; pin < PCI_NUM_PINS; pin++) {
+int irq_nr = irq_base + ((pin + PCI_SLOT(devfn)) % PCI_NUM_PINS);
 int i = 0;
 
 /* Fill PCI address

[PATCH v4 14/15] hw/block/virtio-blk: Replaces request free function with g_free

2024-10-25 Thread Phil Dennis-Jordan 
The virtio_blk_free_request() function has been a 1-liner forwarding
to g_free() for a while now. We may as well call g_free on the request
pointer directly.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/block/virtio-blk.c  | 43 +++---
 hw/vmapple/virtio-blk.c|  2 +-
 include/hw/virtio/virtio-blk.h |  1 -
 3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9e8337bb639..40d2c9bc591 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -50,11 +50,6 @@ static void virtio_blk_init_request(VirtIOBlock *s, 
VirtQueue *vq,
 req->mr_next = NULL;
 }
 
-void virtio_blk_free_request(VirtIOBlockReq *req)
-{
-g_free(req);
-}
-
 void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
 {
 VirtIOBlock *s = req->dev;
@@ -93,7 +88,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, 
int error,
 if (acct_failed) {
 block_acct_failed(blk_get_stats(s->blk), &req->acct);
 }
-virtio_blk_free_request(req);
+g_free(req);
 }
 
 blk_error_action(s->blk, action, is_read, error);
@@ -136,7 +131,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 
 virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
 block_acct_done(blk_get_stats(s->blk), &req->acct);
-virtio_blk_free_request(req);
+g_free(req);
 }
 }
 
@@ -151,7 +146,7 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
 
 virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
 block_acct_done(blk_get_stats(s->blk), &req->acct);
-virtio_blk_free_request(req);
+g_free(req);
 }
 
 static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
@@ -169,7 +164,7 @@ static void virtio_blk_discard_write_zeroes_complete(void 
*opaque, int ret)
 if (is_write_zeroes) {
 block_acct_done(blk_get_stats(s->blk), &req->acct);
 }
-virtio_blk_free_request(req);
+g_free(req);
 }
 
 static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s, VirtQueue *vq)
@@ -214,7 +209,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
 fail:
 virtio_blk_req_complete(req, status);
-virtio_blk_free_request(req);
+g_free(req);
 }
 
 static inline void submit_requests(VirtIOBlock *s, MultiReqBuffer *mrb,
@@ -612,7 +607,7 @@ static void virtio_blk_zone_report_complete(void *opaque, 
int ret)
 
 out:
 virtio_blk_req_complete(req, err_status);
-virtio_blk_free_request(req);
+g_free(req);
 g_free(data->zone_report_data.zones);
 g_free(data);
 }
@@ -661,7 +656,7 @@ static void virtio_blk_handle_zone_report(VirtIOBlockReq 
*req,
 return;
 out:
 virtio_blk_req_complete(req, err_status);
-virtio_blk_free_request(req);
+g_free(req);
 }
 
 static void virtio_blk_zone_mgmt_complete(void *opaque, int ret)
@@ -677,7 +672,7 @@ static void virtio_blk_zone_mgmt_complete(void *opaque, int 
ret)
 }
 
 virtio_blk_req_complete(req, err_status);
-virtio_blk_free_request(req);
+g_free(req);
 }
 
 static int virtio_blk_handle_zone_mgmt(VirtIOBlockReq *req, BlockZoneOp op)
@@ -719,7 +714,7 @@ static int virtio_blk_handle_zone_mgmt(VirtIOBlockReq *req, 
BlockZoneOp op)
 return 0;
 out:
 virtio_blk_req_complete(req, err_status);
-virtio_blk_free_request(req);
+g_free(req);
 return err_status;
 }
 
@@ -750,7 +745,7 @@ static void virtio_blk_zone_append_complete(void *opaque, 
int ret)
 
 out:
 virtio_blk_req_complete(req, err_status);
-virtio_blk_free_request(req);
+g_free(req);
 g_free(data);
 }
 
@@ -788,7 +783,7 @@ static int virtio_blk_handle_zone_append(VirtIOBlockReq 
*req,
 
 out:
 virtio_blk_req_complete(req, err_status);
-virtio_blk_free_request(req);
+g_free(req);
 return err_status;
 }
 
@@ -855,7 +850,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, 
MultiReqBuffer *mrb)
 virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
 block_acct_invalid(blk_get_stats(s->blk),
is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ);
-virtio_blk_free_request(req);
+g_free(req);
 return 0;
 }
 
@@ -911,7 +906,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, 
MultiReqBuffer *mrb)
   VIRTIO_BLK_ID_BYTES));
 iov_from_buf(in_iov, in_num, 0, serial, size);
 virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
-virtio_blk_free_request(req);
+g_free(req);
 break;
 }
 case VIRTIO_BLK_T_ZONE_APPEND & ~VIRTIO_BLK_T_OUT:
@@ -943,7 +938,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, 
MultiReqBuffer *mrb)
 if (unlikely(!(type & VIRTIO_BLK_T_OUT) ||
  out_len > sizeof(dwz_hdr))) {
 virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-virtio_blk_free_request(req);
+g_fr

Re: [PATCH] hw/nvme: fix handling of over-committed queues

2024-10-25 Thread Keith Busch 
On Fri, Oct 25, 2024 at 12:50:45PM +0200, Klaus Jensen wrote:
> @@ -1520,9 +1520,16 @@ static void nvme_post_cqes(void *opaque)
>  nvme_inc_cq_tail(cq);
>  nvme_sg_unmap(&req->sg);
> +
> +if (QTAILQ_EMPTY(&sq->req_list) && !nvme_sq_empty(sq)) {
> +qemu_bh_schedule(sq->bh);
> +}
> +
>  QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
>  }

Shouldn't we schedule the bottom half after the req has been added to
the list? I think everything the callback needs to be written prior to
calling qemu_bh_schedule().

[PATCH] hw/nvme: fix handling of over-committed queues

2024-10-25 Thread Klaus Jensen 
From: Klaus Jensen 

If a host chooses to use the SQHD "hint" in the CQE to know if there is
room in the submission queue for additional commands, it may result in a
situation where there are not enough internal resources (struct
NvmeRequest) available to process the command. For a lack of a better
term, the host may "over-commit" the device (i.e., it may have more
inflight commands than the queue size).

For example, assume a queue with N entries. The host submits N commands
and all are picked up for processing, advancing the head and emptying
the queue. Regardless of which of these N commands complete first, the
SQHD field of that CQE will indicate to the host that the queue is
empty, which allows the host to issue N commands again. However, if the
device has not posted CQEs for all the previous commands yet, the device
will have less than N resources available to process the commands, so
queue processing is suspended.

And here lies an 11 year latent bug. In the absense of any additional
tail updates on the submission queue, we never schedule the processing
bottom-half again unless we observe a head update on an associated full
completion queue. This has been sufficient to handle N-to-1 SQ/CQ setups
(in the absense of over-commit of course). Incidentially, that "kick all
associated SQs" mechanism can now be killed since we now just schedule
queue processing when we return a processing resource to a non-empty
submission queue, which happens to cover both edge cases.

So, apparently, no previous driver tested with hw/nvme has ever used
SQHD (e.g., neither the Linux NVMe driver or SPDK uses it). But then OSv
shows up with the driver that actually does. I salute you.

Fixes: f3c507adcd7b ("NVMe: Initial commit for new storage interface")
Cc: qemu-sta...@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2388
Reported-by: Waldemar Kozaczuk 
Signed-off-by: Klaus Jensen 
---
 hw/nvme/ctrl.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 
f4e89203c1a6e3b051fd7185cbf01ec9bae9684a..b13585c4da911b9e8ae4a722761fd85dfa24be4d
 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1520,9 +1520,16 @@ static void nvme_post_cqes(void *opaque)
 stl_le_p(&n->bar.csts, NVME_CSTS_FAILED);
 break;
 }
+
 QTAILQ_REMOVE(&cq->req_list, req, entry);
+
 nvme_inc_cq_tail(cq);
 nvme_sg_unmap(&req->sg);
+
+if (QTAILQ_EMPTY(&sq->req_list) && !nvme_sq_empty(sq)) {
+qemu_bh_schedule(sq->bh);
+}
+
 QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
 }
 if (cq->tail != cq->head) {
@@ -7950,7 +7957,6 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
 /* Completion queue doorbell write */
 
 uint16_t new_head = val & 0x;
-int start_sqs;
 NvmeCQueue *cq;
 
 qid = (addr - (0x1000 + (1 << 2))) >> 3;
@@ -8001,18 +8007,10 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, 
int val)
 
 trace_pci_nvme_mmio_doorbell_cq(cq->cqid, new_head);
 
-start_sqs = nvme_cq_full(cq) ? 1 : 0;
 cq->head = new_head;
 if (!qid && n->dbbuf_enabled) {
 stl_le_pci_dma(pci, cq->db_addr, cq->head, MEMTXATTRS_UNSPECIFIED);
 }
-if (start_sqs) {
-NvmeSQueue *sq;
-QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
-qemu_bh_schedule(sq->bh);
-}
-qemu_bh_schedule(cq->bh);
-}
 
     if (cq->tail == cq->head) {
 if (cq->irq_enabled) {

---
base-commit: e67b7aef7c7f67ecd0282e903e0daff806d5d680
change-id: 20241025-issue-2388-bd047487f74c

Best regards,
-- 
Klaus Jensen

Re: [PATCH v4 13/15] hw/vmapple/virtio-blk: Add support for apple virtio-blk

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

From: Alexander Graf 

Apple has its own virtio-blk PCI device ID where it deviates from the
official virtio-pci spec slightly: It puts a new "apple type"
field at a static offset in config space and introduces a new barrier
command.

This patch first creates a mechanism for virtio-blk downstream classes to
handle unknown commands. It then creates such a downstream class and a new
vmapple-virtio-blk-pci class which support the additional apple type config
identifier as well as the barrier command.

It then exposes 2 subclasses from that that we can use to expose root and
aux virtio-blk devices: "vmapple-virtio-root" and "vmapple-virtio-aux".

Signed-off-by: Alexander Graf 
Signed-off-by: Phil Dennis-Jordan 
---
v4:

  * Use recommended object type declaration pattern.
  * Correctly log unimplemented code paths.
  * Most header code moved to .c, type name #defines moved to vmapple.h

  hw/block/virtio-blk.c  |  19 ++-
  hw/vmapple/Kconfig |   3 +
  hw/vmapple/meson.build |   1 +
  hw/vmapple/virtio-blk.c| 233 +
  include/hw/pci/pci_ids.h   |   1 +
  include/hw/virtio/virtio-blk.h |  12 +-
  include/hw/vmapple/vmapple.h   |   4 +
  7 files changed, 268 insertions(+), 5 deletions(-)
  create mode 100644 hw/vmapple/virtio-blk.c

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9166d7974d4..9e8337bb639 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -50,12 +50,12 @@ static void virtio_blk_init_request(VirtIOBlock *s, 
VirtQueue *vq,
  req->mr_next = NULL;
  }
  
-static void virtio_blk_free_request(VirtIOBlockReq *req)

+void virtio_blk_free_request(VirtIOBlockReq *req)
  {
  g_free(req);
  }
  
-static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)

+void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
  {
  VirtIOBlock *s = req->dev;
  VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -966,8 +966,18 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, 
MultiReqBuffer *mrb)
  break;
  }
  default:
-virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-virtio_blk_free_request(req);
+{
+/*
+ * Give subclasses a chance to handle unknown requests. This way the
+ * class lookup is not in the hot path.
+ */
+VirtIOBlkClass *vbk = VIRTIO_BLK_GET_CLASS(s);
+if (!vbk->handle_unknown_request ||
+!vbk->handle_unknown_request(req, mrb, type)) {
+virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
+virtio_blk_free_request(req);
+}
+}
  }
  return 0;
  }
@@ -2044,6 +2054,7 @@ static const TypeInfo virtio_blk_info = {
  .instance_size = sizeof(VirtIOBlock),
  .instance_init = virtio_blk_instance_init,
  .class_init = virtio_blk_class_init,
+.class_size = sizeof(VirtIOBlkClass),
  };
  
  static void virtio_register_types(void)

diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
index 8bbeb9a9237..bcd1be63e3c 100644
--- a/hw/vmapple/Kconfig
+++ b/hw/vmapple/Kconfig
@@ -7,3 +7,6 @@ config VMAPPLE_BDIF
  config VMAPPLE_CFG
  bool
  
+config VMAPPLE_VIRTIO_BLK

+bool
+
diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build
index 64b78693a31..bf17cf906c9 100644
--- a/hw/vmapple/meson.build
+++ b/hw/vmapple/meson.build
@@ -1,3 +1,4 @@
  system_ss.add(when: 'CONFIG_VMAPPLE_AES',  if_true: files('aes.c'))
  system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: files('bdif.c'))
  system_ss.add(when: 'CONFIG_VMAPPLE_CFG',  if_true: files('cfg.c'))
+system_ss.add(when: 'CONFIG_VMAPPLE_VIRTIO_BLK',  if_true: 
files('virtio-blk.c'))
diff --git a/hw/vmapple/virtio-blk.c b/hw/vmapple/virtio-blk.c
new file mode 100644
index 000..3a8b47bc55f
--- /dev/null
+++ b/hw/vmapple/virtio-blk.c
@@ -0,0 +1,233 @@
+/*
+ * VMApple specific VirtIO Block implementation
+ *
+ * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * VMApple uses almost standard VirtIO Block, but with a few key differences:
+ *
+ *  - Different PCI device/vendor ID
+ *  - An additional "type" identifier to differentiate AUX and Root volumes
+ *  - An additional BARRIER command
+ */
+
+#include "qemu/osdep.h"
+#include "hw/vmapple/vmapple.h"
+#include "hw/virtio/virtio-blk.h"
+#include "hw/virtio/virtio-pci.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+
+OBJECT_DECLARE_TYPE(VMAppleVirtIOBlk, VMAppleVirtIOBlkClass, 
VMAPPLE_VIRTIO_BLK)
+
+typedef struct VMAppleVirtIOBlkClass {
+/*< private >*/
+VirtIOBlkClass parent;
+/*< public >*/


No need for these comments. All members are private to this file.


+void (*get_config)(VirtIODevice *vdev, uint8_t *config);
+} VMAppleVirtIOBlkClass;
+
+ty

Re: [PATCH v4 14/15] hw/block/virtio-blk: Replaces request free function with g_free

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

The virtio_blk_free_request() function has been a 1-liner forwarding
to g_free() for a while now. We may as well call g_free on the request
pointer directly.

Signed-off-by: Phil Dennis-Jordan 


Reviewed-by: Akihiko Odaki

Re: [PATCH v4 02/15] hw/display/apple-gfx: Introduce ParavirtualizedGraphics.Framework support

2024-10-25 Thread Akihiko Odaki 

On 2024/10/26 4:43, Phil Dennis-Jordan wrote:



On Fri, 25 Oct 2024 at 08:03, Akihiko Odaki > wrote:


On 2024/10/24 19:28, Phil Dennis-Jordan wrote:
 > MacOS provides a framework (library) that allows any vmm to
implement a
 > paravirtualized 3d graphics passthrough to the host metal stack
called
 > ParavirtualizedGraphics.Framework (PVG). The library abstracts away
 > almost every aspect of the paravirtualized device model and only
provides
 > and receives callbacks on MMIO access as well as to share memory
address
 > space between the VM and PVG.
 >
 > This patch implements a QEMU device that drives PVG for the VMApple
 > variant of it.
 >
 > Signed-off-by: Alexander Graf mailto:g...@amazon.com>>
 > Co-authored-by: Alexander Graf mailto:g...@amazon.com>>
 >
 > Subsequent changes:
 >
 >   * Cherry-pick/rebase conflict fixes, API use updates.
 >   * Moved from hw/vmapple/ (useful outside that machine type)
 >   * Overhaul of threading model, many thread safety improvements.
 >   * Asynchronous rendering.
 >   * Memory and object lifetime fixes.
 >   * Refactoring to split generic and (vmapple) MMIO variant specific
 >     code.
 >
 > Signed-off-by: Phil Dennis-Jordan mailto:p...@philjordan.eu>>
 > ---
 >
 > v2:
 >
 >   * Cherry-pick/rebase conflict fixes
 >   * BQL function renaming
 >   * Moved from hw/vmapple/ (useful outside that machine type)
 >   * Code review comments: Switched to DEFINE_TYPES macro & little
endian
 >     MMIO.
 >   * Removed some dead/superfluous code
 >   * Mad set_mode thread & memory safe
 >   * Added migration blocker due to lack of (de-)serialisation.
 >   * Fixes to ObjC refcounting and autorelease pool usage.
 >   * Fixed ObjC new/init misuse
 >   * Switched to ObjC category extension for private property.
 >   * Simplified task memory mapping and made it thread safe.
 >   * Refactoring to split generic and vmapple MMIO variant specific
 >     code.
 >   * Switched to asynchronous MMIO writes on x86-64
 >   * Rendering and graphics update are now done asynchronously
 >   * Fixed cursor handling
 >   * Coding convention fixes
 >   * Removed software cursor compositing
 >
 > v3:
 >
 >   * Rebased on latest upstream, fixed breakages including
switching to Resettable methods.
 >   * Squashed patches dealing with dGPUs, MMIO area size, and GPU
picking.
 >   * Allow re-entrant MMIO; this simplifies the code and solves
the divergence
 >     between x86-64 and arm64 variants.
 >
 > v4:
 >
 >   * Renamed '-vmapple' device variant to '-mmio'
 >   * MMIO device type now requires aarch64 host and guest
 >   * Complete overhaul of the glue code for making Qemu's and
 >     ParavirtualizedGraphics.framework's threading and
synchronisation models
 >     work together. Calls into PVG are from dispatch queues while the
 >     BQL-holding initiating thread processes AIO context events;
callbacks from
 >     PVG are scheduled as BHs on the BQL/main AIO context,
awaiting completion
 >     where necessary.
 >   * Guest frame rendering state is covered by the BQL, with only
the PVG calls
 >     outside the lock, and serialised on the named render_queue.
 >   * Simplified logic for dropping frames in-flight during mode
changes, fixed
 >     bug in pending frames logic.
 >   * Addressed smaller code review notes such as: function naming,
object type
 >     declarations, type names/declarations/casts, code formatting,
#include
 >     order, over-cautious ObjC retain/release, what goes in init
vs realize,
 >     etc.
 >
 >
 >   hw/display/Kconfig          |   9 +
 >   hw/display/apple-gfx-mmio.m | 284 ++
 >   hw/display/apple-gfx.h      |  58 +++
 >   hw/display/apple-gfx.m      | 713 +
+++
 >   hw/display/meson.build      |   4 +
 >   hw/display/trace-events     |  26 ++
 >   meson.build                 |   4 +
 >   7 files changed, 1098 insertions(+)
 >   create mode 100644 hw/display/apple-gfx-mmio.m
 >   create mode 100644 hw/display/apple-gfx.h
 >   create mode 100644 hw/display/apple-gfx.m
 >
 > diff --git a/hw/display/Kconfig b/hw/display/Kconfig
 > index 2250c740078..6a9b7b19ada 100644
 > --- a/hw/display/Kconfig
 > +++ b/hw/display/Kconfig
 > @@ -140,3 +140,12 @@ config XLNX_DISPLAYPORT
 >
 >   config DM163
 >       bool
 > +
 > +config MAC_PVG
 > +    bool
 > +    default y
 > +
 > +config MAC_PVG_MMIO
 > +    bool
 > +    depends on MAC_PVG && AARCH64
 > +
 > diff --git a/hw/display/apple-gfx-mmio.m b/hw/display/apple-gfx-
mm

Re: [PATCH 0/2] arm: Add collie and sx functional tests

2024-10-25 Thread Guenter Roeck 

On 10/25/24 21:47, Philippe Mathieu-Daudé wrote:

On 25/10/24 12:25, Jan Lübbe wrote:

On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:

On 10/25/24 02:57, Jan Lübbe wrote:

On Fri, 2024-10-25 at 08:55 +0200, Cédric Le Goater wrote:

On 10/24/24 19:59, Philippe Mathieu-Daudé wrote:

Cc'ing Jan.

On 22/10/24 12:04, Guenter Roeck wrote:


I have no idea how this is supposed to work, and I don't think it works
as implemented. I'll revert commit e32ac563b83 in my local copy of qemu.
Jan, can you have a look at this bug report please? Otherwise I'll
have a look during soft freeze.


Guenter, just to make sure: In your case [1], you had "boot-emmc" (see
aspeed_machine_ast2600_class_emmc_init) enabled, right? Otherwise the


I tried both enabled and disabled.


boot partition size would be 0, meaning that the eMMC has no boot
partitions.


That is what I would have expected, but it does not reflect reality.


In that configuration, your backing file needs to have space for the
boot partitions at the start (2*1MiB) and the rootfs starts at the 2
MiB offset.



boot-emmc doesn't make a difference, because

  if (emmc) {
  qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
  qdev_prop_set_uint8(card, "boot-config",
  boot_emmc ? 0x1 << 3 : 0x0);
  }

in hw/arm/aspeed.c sets the boot partition size independently of the
boot-emmc flag.


Ah, yes. :/

So you can have SD, eMMC with boot from boot partition *disabled* or
eMMC with boot from boot partition *enabled*.


I suspect that the expectation is that I always provide
an emmc image with a 2 MB gap at the beginning, but in my opinion that is
really hyper-clumsy, and I simply won't do it, sorry.


I was surprised that the boot partitions' contents are stored in the
same backing file as the main area (instead of being separate files).
But I've not researched why it was designed this way.


Yeah I'd have preferred separate files too, but when it seemed
to be simpler for the single user case:
https://lore.kernel.org/qemu-devel/d48b6357-c839-4971-aa28-bdbd5b1ba...@kaod.org/



I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.

Guenter




I can work around
the problem by changing the above code to only set boot-partition-size if
boot_emmc is set, or I can revert commit e32ac563b83. For now I chose the
latter.


With e32ac563b83 reverted, your backing file layout will change *at
runtime* depending on whether booting from the boot partition is
enabled via EXT CSD registers. You should be able to try that using
'mmc bootpart enable  0 /dev/mmcblk0' (with boot_part=0 for
disabled and boot_part=1/2 for enabled).


I'm not sure what the best way forward is... perhaps make the boot-
partition-size zero if boot_emmc is false? e.g.

@@ -339,7 +339,12 @@ static void sdhci_attach_drive(SDHCIState *sdhci, 
DriveInfo *dinfo, bool emmc,
  }
  card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
  if (emmc) {
-    qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
+    /*
+ * Avoid the requirement for a padded disk image if booting from
+ * eMMC boot partitions is not needed.
+ */
+    qdev_prop_set_uint64(card, "boot-partition-size",
+    boot_emmc ? 1 * MiB : 0);
  qdev_prop_set_uint8(card, "boot-config",
  boot_emmc ? 0x1 << 3 : 0x0);
  }


Then you'd have the choice between:
- an eMMC, but without boot partitions (and simple backing file layout)
- an eMMC with boot partitions and need a backing file with
   the boot partitions at the beginning

That might be the lesser evil. :)

Jan

Re: [PATCH v4 03/15] hw/display/apple-gfx: Adds PCI implementation

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

This change wires up the PCI variant of the paravirtualised
graphics device, mainly useful for x86-64 macOS guests, implemented
by macOS's ParavirtualizedGraphics.framework. It builds on code
shared with the vmapple/mmio variant of the PVG device.

Signed-off-by: Phil Dennis-Jordan 
---

v4:

  * Threading improvements analogous to those in common apple-gfx code
and mmio device variant.
  * Smaller code review issues addressed.

  hw/display/Kconfig |   4 +
  hw/display/apple-gfx-pci.m | 152 +
  hw/display/meson.build |   1 +
  3 files changed, 157 insertions(+)
  create mode 100644 hw/display/apple-gfx-pci.m

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 6a9b7b19ada..2b53dfd7d26 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -149,3 +149,7 @@ config MAC_PVG_MMIO
  bool
  depends on MAC_PVG && AARCH64
  
+config MAC_PVG_PCI

+bool
+depends on MAC_PVG && PCI
+default y if PCI_DEVICES
diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
new file mode 100644
index 000..4ee26dde422
--- /dev/null
+++ b/hw/display/apple-gfx-pci.m
@@ -0,0 +1,152 @@
+/*
+ * QEMU Apple ParavirtualizedGraphics.framework device, PCI variant
+ *
+ * Copyright © 2023-2024 Phil Dennis-Jordan
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * ParavirtualizedGraphics.framework is a set of libraries that macOS provides
+ * which implements 3d graphics passthrough to the host as well as a
+ * proprietary guest communication channel to drive it. This device model
+ * implements support to drive that library from within QEMU as a PCI device
+ * aimed primarily at x86-64 macOS VMs.
+ */
+
+#include "apple-gfx.h"
+#include "hw/pci/pci_device.h"
+#include "hw/pci/msi.h"
+#include "qapi/error.h"
+#include "trace.h"
+#import 
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXPCIState, APPLE_GFX_PCI)
+
+struct AppleGFXPCIState {
+PCIDevice parent_obj;
+
+AppleGFXState common;
+};
+
+static const char* apple_gfx_pci_option_rom_path = NULL;
+
+static void apple_gfx_init_option_rom_path(void)
+{
+NSURL *option_rom_url = PGCopyOptionROMURL();
+const char *option_rom_path = option_rom_url.fileSystemRepresentation;
+apple_gfx_pci_option_rom_path = g_strdup(option_rom_path);
+[option_rom_url release];
+}
+
+static void apple_gfx_pci_init(Object *obj)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
+
+if (!apple_gfx_pci_option_rom_path) {
+/* The following is done on device not class init to avoid running
+ * ObjC code before fork() in -daemonize mode. */
+PCIDeviceClass *pci = PCI_DEVICE_CLASS(object_get_class(obj));
+apple_gfx_init_option_rom_path();
+pci->romfile = apple_gfx_pci_option_rom_path;
+}
+
+apple_gfx_common_init(obj, &s->common, TYPE_APPLE_GFX_PCI);
+}
+
+typedef struct AppleGFXPCIInterruptJob {
+PCIDevice *device;
+uint32_t vector;
+} AppleGFXPCIInterruptJob;
+
+static void apple_gfx_pci_raise_interrupt(void *opaque)
+{
+AppleGFXPCIInterruptJob *job = opaque;
+
+if (msi_enabled(job->device)) {
+msi_notify(job->device, job->vector);
+}
+g_free(job);
+}
+
+static void apple_gfx_pci_interrupt(PCIDevice *dev, AppleGFXPCIState *s,
+uint32_t vector)
+{
+AppleGFXPCIInterruptJob *job;
+
+trace_apple_gfx_raise_irq(vector);
+job = g_malloc0(sizeof(*job));
+job->device = dev;
+job->vector = vector;
+aio_bh_schedule_oneshot(qemu_get_aio_context(),
+apple_gfx_pci_raise_interrupt, job);
+}
+
+static void apple_gfx_pci_realize(PCIDevice *dev, Error **errp)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(dev);
+Error *err = NULL;
+int ret;
+
+pci_register_bar(dev, PG_PCI_BAR_MMIO,
+ PCI_BASE_ADDRESS_SPACE_MEMORY, &s->common.iomem_gfx);
+
+ret = msi_init(dev, 0x0 /* config offset; 0 = find space */,
+   PG_PCI_MAX_MSI_VECTORS, true /* msi64bit */,
+   false /*msi_per_vector_mask*/, &err);
+if (ret != 0) {
+error_propagate(errp, err);


Don't use error_propaget() but just pass errp to msi_init.


+return;
+}
+
+@autoreleasepool {
+PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+desc.raiseInterrupt = ^(uint32_t vector) {
+apple_gfx_pci_interrupt(dev, s, vector);
+};
+
+apple_gfx_common_realize(&s->common, desc, errp);
+[desc release];
+desc = nil;
+}
+}
+
+static void apple_gfx_pci_reset(Object *obj, ResetType type)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
+[s->common.pgdev reset];
+}
+
+static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+assert(rc->phases.hold

Re: [PATCH 0/2] arm: Add collie and sx functional tests

2024-10-25 Thread Philippe Mathieu-Daudé 

On 25/10/24 12:25, Jan Lübbe wrote:

On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:

On 10/25/24 02:57, Jan Lübbe wrote:

On Fri, 2024-10-25 at 08:55 +0200, Cédric Le Goater wrote:

On 10/24/24 19:59, Philippe Mathieu-Daudé wrote:

Cc'ing Jan.

On 22/10/24 12:04, Guenter Roeck wrote:


I have no idea how this is supposed to work, and I don't think it works
as implemented. I'll revert commit e32ac563b83 in my local copy of qemu.
Jan, can you have a look at this bug report please? Otherwise I'll
have a look during soft freeze.


Guenter, just to make sure: In your case [1], you had "boot-emmc" (see
aspeed_machine_ast2600_class_emmc_init) enabled, right? Otherwise the


I tried both enabled and disabled.


boot partition size would be 0, meaning that the eMMC has no boot
partitions.


That is what I would have expected, but it does not reflect reality.


In that configuration, your backing file needs to have space for the
boot partitions at the start (2*1MiB) and the rootfs starts at the 2
MiB offset.



boot-emmc doesn't make a difference, because

  if (emmc) {
  qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
  qdev_prop_set_uint8(card, "boot-config",
  boot_emmc ? 0x1 << 3 : 0x0);
  }

in hw/arm/aspeed.c sets the boot partition size independently of the
boot-emmc flag.


Ah, yes. :/

So you can have SD, eMMC with boot from boot partition *disabled* or
eMMC with boot from boot partition *enabled*.


I suspect that the expectation is that I always provide
an emmc image with a 2 MB gap at the beginning, but in my opinion that is
really hyper-clumsy, and I simply won't do it, sorry.


I was surprised that the boot partitions' contents are stored in the
same backing file as the main area (instead of being separate files).
But I've not researched why it was designed this way.


Yeah I'd have preferred separate files too, but when it seemed
to be simpler for the single user case:
https://lore.kernel.org/qemu-devel/d48b6357-c839-4971-aa28-bdbd5b1ba...@kaod.org/




I can work around
the problem by changing the above code to only set boot-partition-size if
boot_emmc is set, or I can revert commit e32ac563b83. For now I chose the
latter.


With e32ac563b83 reverted, your backing file layout will change *at
runtime* depending on whether booting from the boot partition is
enabled via EXT CSD registers. You should be able to try that using
'mmc bootpart enable  0 /dev/mmcblk0' (with boot_part=0 for
disabled and boot_part=1/2 for enabled).


I'm not sure what the best way forward is... perhaps make the boot-
partition-size zero if boot_emmc is false? e.g.

@@ -339,7 +339,12 @@ static void sdhci_attach_drive(SDHCIState *sdhci, 
DriveInfo *dinfo, bool emmc,
  }
  card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
  if (emmc) {
-qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
+/*
+ * Avoid the requirement for a padded disk image if booting from
+ * eMMC boot partitions is not needed.
+ */
+qdev_prop_set_uint64(card, "boot-partition-size",
+boot_emmc ? 1 * MiB : 0);
  qdev_prop_set_uint8(card, "boot-config",
  boot_emmc ? 0x1 << 3 : 0x0);
  }


Then you'd have the choice between:
- an eMMC, but without boot partitions (and simple backing file layout)
- an eMMC with boot partitions and need a backing file with
   the boot partitions at the beginning

That might be the lesser evil. :)

Jan

Re: [PATCH v4 04/15] hw/display/apple-gfx: Adds configurable mode list

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

This change adds a property 'display_modes' on the graphics device
which permits specifying a list of display modes. (screen resolution
and refresh rate)

The property is an array of a custom type to make the syntax slightly
less awkward to use, for example:

-device '{"driver":"apple-gfx-pci", "display-modes":["1920x1080@60", 
"3840x2160@60"]}'

Signed-off-by: Phil Dennis-Jordan 
---

v4:

  * Switched to the native array property type, which recently gained
 command line support.
  * The property has also been added to the -mmio variant.
  * Tidied up the code a little.

  hw/display/apple-gfx-mmio.m |   8 +++
  hw/display/apple-gfx-pci.m  |   9 ++-
  hw/display/apple-gfx.h  |  12 
  hw/display/apple-gfx.m  | 127 
  hw/display/trace-events |   2 +
  5 files changed, 145 insertions(+), 13 deletions(-)

diff --git a/hw/display/apple-gfx-mmio.m b/hw/display/apple-gfx-mmio.m
index 06131bc23f1..5d427c7005e 100644
--- a/hw/display/apple-gfx-mmio.m
+++ b/hw/display/apple-gfx-mmio.m
@@ -261,6 +261,12 @@ static void apple_gfx_mmio_reset(Object *obj, ResetType 
type)
  [s->common.pgdev reset];
  }
  
+static Property apple_gfx_mmio_properties[] = {

+DEFINE_PROP_ARRAY("display-modes", AppleGFXMMIOState,
+  common.num_display_modes, common.display_modes,
+  qdev_prop_display_mode, AppleGFXDisplayMode),
+DEFINE_PROP_END_OF_LIST(),
+};
  
  static void apple_gfx_mmio_class_init(ObjectClass *klass, void *data)

  {
@@ -270,6 +276,8 @@ static void apple_gfx_mmio_class_init(ObjectClass *klass, 
void *data)
  rc->phases.hold = apple_gfx_mmio_reset;
  dc->hotpluggable = false;
  dc->realize = apple_gfx_mmio_realize;
+
+device_class_set_props(dc, apple_gfx_mmio_properties);
  }
  
  static TypeInfo apple_gfx_mmio_types[] = {

diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
index 4ee26dde422..32e81bbef8b 100644
--- a/hw/display/apple-gfx-pci.m
+++ b/hw/display/apple-gfx-pci.m
@@ -115,6 +115,13 @@ static void apple_gfx_pci_reset(Object *obj, ResetType 
type)
  [s->common.pgdev reset];
  }
  
+static Property apple_gfx_pci_properties[] = {

+DEFINE_PROP_ARRAY("display-modes", AppleGFXPCIState,
+  common.num_display_modes, common.display_modes,
+  qdev_prop_display_mode, AppleGFXDisplayMode),
+DEFINE_PROP_END_OF_LIST(),
+};
+
  static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
  {
  DeviceClass *dc = DEVICE_CLASS(klass);
@@ -132,7 +139,7 @@ static void apple_gfx_pci_class_init(ObjectClass *klass, 
void *data)
  pci->class_id = PCI_CLASS_DISPLAY_OTHER;
  pci->realize = apple_gfx_pci_realize;
  
-// TODO: Property for setting mode list

+device_class_set_props(dc, apple_gfx_pci_properties);
  }
  
  static TypeInfo apple_gfx_pci_types[] = {

diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index 39931fba65a..d2c6a14229a 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -9,6 +9,7 @@
  #import 
  #include "qemu/typedefs.h"
  #include "exec/memory.h"
+#include "hw/qdev-properties.h"
  #include "ui/surface.h"
  
  @class PGDeviceDescriptor;

@@ -20,6 +21,7 @@
  
  typedef QTAILQ_HEAD(, PGTask_s) PGTaskList;
  
+struct AppleGFXDisplayMode;

  struct AppleGFXMapMemoryJob;
  typedef struct AppleGFXState {
  MemoryRegion iomem_gfx;
@@ -31,6 +33,8 @@ typedef struct AppleGFXState {
  id mtl_queue;
  bool cursor_show;
  QEMUCursor *cursor;
+struct AppleGFXDisplayMode *display_modes;
+uint32_t num_display_modes;
  
  /* For running PVG memory-mapping requests in the AIO context */

  QemuCond job_cond;
@@ -47,6 +51,12 @@ typedef struct AppleGFXState {
  id texture;
  } AppleGFXState;
  
+typedef struct AppleGFXDisplayMode {

+uint16_t width_px;
+uint16_t height_px;
+uint16_t refresh_rate_hz;
+} AppleGFXDisplayMode;
+
  void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* 
obj_name);
  void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc,
Error **errp);
@@ -54,5 +64,7 @@ uintptr_t apple_gfx_host_address_for_gpa_range(uint64_t 
guest_physical,
 uint64_t length, bool 
read_only);
  void apple_gfx_await_bh_job(AppleGFXState *s, bool *job_done_flag);
  
+extern const PropertyInfo qdev_prop_display_mode;

+
  #endif
  
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m

index 46be9957f69..42b601329fb 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -28,9 +28,10 @@
  #include "qapi/error.h"
  #include "ui/console.h"
  
-static const PGDisplayCoord_t apple_gfx_modes[] = {

-{ .x = 1440, .y = 1080 },
-{ .x = 1280, .y = 1024 },
+static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
+{ 1920, 1080, 60 },
+{ 1440, 1080, 60 },

Re: [PATCH v4 09/15] gpex: Allow more than 4 legacy IRQs

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

From: Alexander Graf 

Some boards such as vmapple don't do real legacy PCI IRQ swizzling.
Instead, they just keep allocating more board IRQ lines for each new
legacy IRQ. Let's support that mode by giving instantiators a new
"nr_irqs" property they can use to support more than 4 legacy IRQ lines.
In this mode, GPEX will export more IRQ lines, one for each device.

Signed-off-by: Alexander Graf 
Signed-off-by: Phil Dennis-Jordan 


Reviewed-by: Akihiko Odaki

Re: [PATCH v4 15/15] hw/vmapple/vmapple: Add vmapple machine type

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

From: Alexander Graf 

Apple defines a new "vmapple" machine type as part of its proprietary
macOS Virtualization.Framework vmm. This machine type is similar to the
virt one, but with subtle differences in base devices, a few special
vmapple device additions and a vastly different boot chain.

This patch reimplements this machine type in QEMU. To use it, you
have to have a readily installed version of macOS for VMApple,
run on macOS with -accel hvf, pass the Virtualization.Framework
boot rom (AVPBooter) in via -bios, pass the aux and root volume as pflash
and pass aux and root volume as virtio drives. In addition, you also
need to find the machine UUID and pass that as -M vmapple,uuid= parameter:

$ qemu-system-aarch64 -accel hvf -M vmapple,uuid=0x1234 -m 4G \
 -bios 
/System/Library/Frameworks/Virtualization.framework/Versions/A/Resources/AVPBooter.vmapple2.bin
 -drive file=aux,if=pflash,format=raw \
 -drive file=root,if=pflash,format=raw \
 -drive file=aux,if=none,id=aux,format=raw \
 -device vmapple-virtio-aux,drive=aux \
 -drive file=root,if=none,id=root,format=raw \
 -device vmapple-virtio-root,drive=root

With all these in place, you should be able to see macOS booting
successfully.

Known issues:
  - Keyboard and mouse/tablet input is laggy. The reason for this is
either that macOS's XHCI driver is broken when the device/platform
does not support MSI/MSI-X, or there's some unfortunate interplay
with Qemu's XHCI implementation in this scenario.
  - Currently only macOS 12 guests are supported. The boot process for
13+ will need further investigation and adjustment.

Signed-off-by: Alexander Graf 
Co-authored-by: Phil Dennis-Jordan 
Signed-off-by: Phil Dennis-Jordan 
---
v3:
  * Rebased on latest upstream, updated affinity and NIC creation
API usage
  * Included Apple-variant virtio-blk in build dependency
  * Updated API usage for setting 'redist-region-count' array-typed property on 
GIC.
  * Switched from virtio HID devices (for which macOS 12 does not contain 
drivers) to an XHCI USB controller and USB HID devices.

v4:
  * Fixups for v4 changes to the other patches in the set.
  * Corrected the assert macro to use
  * Removed superfluous endian conversions corresponding to cfg's.
  * Init error handling improvement.
  * No need to select CPU type on TCG, as only HVF is supported.
  * Machine type version bumped to 9.2
  * #include order improved

  MAINTAINERS |   1 +
  docs/system/arm/vmapple.rst |  63 
  docs/system/target-arm.rst  |   1 +
  hw/vmapple/Kconfig  |  20 ++
  hw/vmapple/meson.build  |   1 +
  hw/vmapple/vmapple.c| 652 
  6 files changed, 738 insertions(+)
  create mode 100644 docs/system/arm/vmapple.rst
  create mode 100644 hw/vmapple/vmapple.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 104813ed85f..f44418b4a95 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2739,6 +2739,7 @@ R: Phil Dennis-Jordan 
  S: Maintained
  F: hw/vmapple/*
  F: include/hw/vmapple/*
+F: docs/system/arm/vmapple.rst
  
  Subsystems

  --
diff --git a/docs/system/arm/vmapple.rst b/docs/system/arm/vmapple.rst
new file mode 100644
index 000..acb921ffb35
--- /dev/null
+++ b/docs/system/arm/vmapple.rst
@@ -0,0 +1,63 @@
+VMApple machine emulation
+
+
+VMApple is the device model that the macOS built-in hypervisor called 
"Virtualization.framework"
+exposes to Apple Silicon macOS guests. The "vmapple" machine model in QEMU 
implements the same
+device model, but does not use any code from Virtualization.Framework.
+
+Prerequisites
+-
+
+To run the vmapple machine model, you need to
+
+ * Run on Apple Silicon
+ * Run on macOS 12.0 or above
+ * Have an already installed copy of a Virtualization.Framework macOS 12 
virtual machine. I will
+   assume that you installed it using the macosvm CLI.
+
+First, we need to extract the UUID from the virtual machine that you 
installed. You can do this
+by running the following shell script:
+
+.. code-block:: bash
+  :caption: uuid.sh script to extract the UUID from a macosvm.json file
+
+  #!/bin/bash
+
+  MID=$(cat "$1" | python3 -c 'import 
json,sys;obj=json.load(sys.stdin);print(obj["machineId"]);')
+  echo "$MID" | base64 -d | plutil -extract ECID raw -


I prefer it to be written entirely in Python instead of a mixture of 
Python and Bash.


Perhaps it is better to put this script in contrib to avoid requiring 
the user to create a file and copy and paste it.



+
+Now we also need to trim the aux partition. It contains metadata that we can 
just discard:
+
+.. code-block:: bash
+  :caption: Command to trim the aux file
+
+  $ dd if="aux.img" of="aux.img.trimmed" bs=$(( 0x4000 )) skip=1
+
+How to run
+--
+
+Then, we can launch QEMU with the Virtualization.Framework pre-boot 
environm

Re: [PATCH v4 10/15] hw/vmapple/aes: Introduce aes engine

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

From: Alexander Graf 

VMApple contains an "aes" engine device that it uses to encrypt and
decrypt its nvram. It has trivial hard coded keys it uses for that
purpose.

Add device emulation for this device model.

Signed-off-by: Alexander Graf 
Signed-off-by: Phil Dennis-Jordan 
---
v3:

  * Rebased on latest upstream and fixed minor breakages.
  * Replaced legacy device reset method with Resettable method

v4:

  * Improved logging of unimplemented functions and guest errors.
  * Better adherence to naming and coding conventions.
  * Cleaner error handling and recovery, including using g_autoptr

  hw/vmapple/Kconfig  |   2 +
  hw/vmapple/aes.c| 572 
  hw/vmapple/meson.build  |   1 +
  hw/vmapple/trace-events |  16 ++
  include/qemu/cutils.h   |  15 ++
  util/hexdump.c  |  14 +
  6 files changed, 620 insertions(+)
  create mode 100644 hw/vmapple/aes.c

diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
index 8b137891791..a73504d5999 100644
--- a/hw/vmapple/Kconfig
+++ b/hw/vmapple/Kconfig
@@ -1 +1,3 @@
+config VMAPPLE_AES
+bool
  
diff --git a/hw/vmapple/aes.c b/hw/vmapple/aes.c

new file mode 100644
index 000..59cdcd65f90
--- /dev/null
+++ b/hw/vmapple/aes.c
@@ -0,0 +1,572 @@
+/*
+ * QEMU Apple AES device emulation
+ *
+ * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "trace.h"
+#include "crypto/hash.h"
+#include "crypto/aes.h"
+#include "crypto/cipher.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "qemu/cutils.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "sysemu/dma.h"
+
+#define TYPE_AES  "apple-aes"
+OBJECT_DECLARE_SIMPLE_TYPE(AESState, AES)
+
+#define MAX_FIFO_SIZE 9
+
+#define CMD_KEY   0x1
+#define CMD_KEY_CONTEXT_SHIFT27
+#define CMD_KEY_CONTEXT_MASK (0x1 << CMD_KEY_CONTEXT_SHIFT)
+#define CMD_KEY_SELECT_MAX_IDX   0x7
+#define CMD_KEY_SELECT_SHIFT 24
+#define CMD_KEY_SELECT_MASK  (CMD_KEY_SELECT_MAX_IDX << 
CMD_KEY_SELECT_SHIFT)
+#define CMD_KEY_KEY_LEN_NUM  4u
+#define CMD_KEY_KEY_LEN_SHIFT22
+#define CMD_KEY_KEY_LEN_MASK ((CMD_KEY_KEY_LEN_NUM - 1u) << 
CMD_KEY_KEY_LEN_SHIFT)
+#define CMD_KEY_ENCRYPT_SHIFT20
+#define CMD_KEY_ENCRYPT_MASK (0x1 << CMD_KEY_ENCRYPT_SHIFT)
+#define CMD_KEY_BLOCK_MODE_SHIFT 16
+#define CMD_KEY_BLOCK_MODE_MASK  (0x3 << CMD_KEY_BLOCK_MODE_SHIFT)
+#define CMD_IV0x2
+#define CMD_IV_CONTEXT_SHIFT 26
+#define CMD_IV_CONTEXT_MASK  (0x3 << CMD_KEY_CONTEXT_SHIFT)
+#define CMD_DSB   0x3
+#define CMD_SKG   0x4
+#define CMD_DATA  0x5
+#define CMD_DATA_KEY_CTX_SHIFT   27
+#define CMD_DATA_KEY_CTX_MASK(0x1 << CMD_DATA_KEY_CTX_SHIFT)
+#define CMD_DATA_IV_CTX_SHIFT25
+#define CMD_DATA_IV_CTX_MASK (0x3 << CMD_DATA_IV_CTX_SHIFT)
+#define CMD_DATA_LEN_MASK0xff
+#define CMD_STORE_IV  0x6
+#define CMD_STORE_IV_ADDR_MASK   0xff
+#define CMD_WRITE_REG 0x7
+#define CMD_FLAG  0x8
+#define CMD_FLAG_STOP_MASK   BIT(26)
+#define CMD_FLAG_RAISE_IRQ_MASK  BIT(27)
+#define CMD_FLAG_INFO_MASK   0xff
+#define CMD_MAX   0x10
+
+#define CMD_SHIFT 28
+
+#define REG_STATUS0xc
+#define REG_STATUS_DMA_READ_RUNNING BIT(0)
+#define REG_STATUS_DMA_READ_PENDING BIT(1)
+#define REG_STATUS_DMA_WRITE_RUNNINGBIT(2)
+#define REG_STATUS_DMA_WRITE_PENDINGBIT(3)
+#define REG_STATUS_BUSY BIT(4)
+#define REG_STATUS_EXECUTINGBIT(5)
+#define REG_STATUS_READYBIT(6)
+#define REG_STATUS_TEXT_DPA_SEEDED  BIT(7)
+#define REG_STATUS_UNWRAP_DPA_SEEDEDBIT(8)
+
+#define REG_IRQ_STATUS0x18
+#define REG_IRQ_STATUS_INVALID_CMD  BIT(2)
+#define REG_IRQ_STATUS_FLAG BIT(5)
+#define REG_IRQ_ENABLE0x1c
+#define REG_WATERMARK 0x20
+#define REG_Q_STATUS  0x24
+#define REG_FLAG_INFO 0x30
+#define REG_FIFO  0x200
+
+static const uint32_t key_lens[CMD_KEY_KEY_LEN_NUM] = {
+[0] = 16,
+[1] = 24,
+[2] = 32,
+[3] = 64,
+};
+
+typedef struct Key {
+uint32_t key_len;
+uint8_t key[32];
+} Key;
+
+typedef struct IV {
+uint32_t iv[4];
+} IV;
+
+static Key builtin_keys[CMD_KEY_SELECT_MAX_IDX + 1] = {
+[1] = {
+.key_len = 32,
+.key = { 0x1 },
+},
+[2] = {
+.key_len = 32,
+.key = { 0x2 },
+},
+[3] = {
+.key_len = 32,
+.key = { 0x3 },
+}
+};
+
+struct AESState {
+SysBusDevice parent_obj;
+
+qemu_irq irq;
+MemoryRegion iomem1;
+MemoryRegion iomem2;
+AddressSpace *as;
+
+uint32_t status;
+uint32_t q_status;
+uint32_t irq_status;
+uint32

Re: [PATCH v4 12/15] hw/vmapple/cfg: Introduce vmapple cfg region

2024-10-25 Thread Akihiko Odaki 

On 2024/10/24 19:28, Phil Dennis-Jordan wrote:

From: Alexander Graf 

Instead of device tree or other more standardized means, VMApple passes
platform configuration to the first stage boot loader in a binary encoded
format that resides at a dedicated RAM region in physical address space.

This patch models this configuration space as a qdev device which we can
then map at the fixed location in the address space. That way, we can
influence and annotate all configuration fields easily.

Signed-off-by: Alexander Graf 
Signed-off-by: Phil Dennis-Jordan 
---
v3:

  * Replaced legacy device reset method with Resettable method

v4:

  * Fixed initialisation of default values for properties
  * Dropped superfluous endianness conversions
  * Moved most header code to .c, device name #define goes in vmapple.h

  hw/vmapple/Kconfig   |   3 +
  hw/vmapple/cfg.c | 197 +++
  hw/vmapple/meson.build   |   1 +
  include/hw/vmapple/vmapple.h |   2 +
  4 files changed, 203 insertions(+)
  create mode 100644 hw/vmapple/cfg.c

diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
index 68f88876eb9..8bbeb9a9237 100644
--- a/hw/vmapple/Kconfig
+++ b/hw/vmapple/Kconfig
@@ -4,3 +4,6 @@ config VMAPPLE_AES
  config VMAPPLE_BDIF
  bool
  
+config VMAPPLE_CFG

+bool
+
diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c
new file mode 100644
index 000..aeb76ba363c
--- /dev/null
+++ b/hw/vmapple/cfg.c
@@ -0,0 +1,197 @@
+/*
+ * VMApple Configuration Region
+ *
+ * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/vmapple/vmapple.h"
+#include "hw/sysbus.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+#include "net/net.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG)
+
+#define VMAPPLE_CFG_SIZE 0x0001
+
+typedef struct VMAppleCfg {
+uint32_t version; /* 0x000 */
+uint32_t nr_cpus; /* 0x004 */
+uint32_t unk1;/* 0x008 */
+uint32_t unk2;/* 0x00c */
+uint32_t unk3;/* 0x010 */
+uint32_t unk4;/* 0x014 */
+uint64_t ecid;/* 0x018 */
+uint64_t ram_size;/* 0x020 */
+uint32_t run_installer1;  /* 0x028 */
+uint32_t unk5;/* 0x02c */
+uint32_t unk6;/* 0x030 */
+uint32_t run_installer2;  /* 0x034 */
+uint32_t rnd; /* 0x038 */
+uint32_t unk7;/* 0x03c */
+MACAddr mac_en0;  /* 0x040 */
+uint8_t pad1[2];
+MACAddr mac_en1;  /* 0x048 */
+uint8_t pad2[2];
+MACAddr mac_wifi0;/* 0x050 */
+uint8_t pad3[2];
+MACAddr mac_bt0;  /* 0x058 */
+uint8_t pad4[2];
+uint8_t reserved[0xa0];   /* 0x060 */
+uint32_t cpu_ids[0x80];   /* 0x100 */
+uint8_t scratch[0x200];   /* 0x180 */
+char serial[32];  /* 0x380 */
+char unk8[32];/* 0x3a0 */
+char model[32];   /* 0x3c0 */
+uint8_t unk9[32]; /* 0x3e0 */
+uint32_t unk10;   /* 0x400 */
+char soc_name[32];/* 0x404 */
+} VMAppleCfg;
+
+struct VMAppleCfgState {
+SysBusDevice parent_obj;
+VMAppleCfg cfg;
+
+MemoryRegion mem;
+char *serial;
+char *model;
+char *soc_name;
+};
+
+static void vmapple_cfg_reset(Object *obj, ResetType type)
+{
+VMAppleCfgState *s = VMAPPLE_CFG(obj);
+VMAppleCfg *cfg;
+
+cfg = memory_region_get_ram_ptr(&s->mem);
+memset((void *)cfg, 0, VMAPPLE_CFG_SIZE);


This explicit cast is unnecessary.


+*cfg = s->cfg;
+}
+
+static bool strlcpy_set_error(char *restrict dst, const char *restrict src,
+  size_t dst_size, Error **errp,
+  const char *parent_func, const char *location,
+  const char *buffer_name)
+{
+size_t len;
+
+len = g_strlcpy(dst, src, dst_size);
+if (len < dst_size) { /* len does not count nul terminator */
+return true;
+}
+
+error_setg(errp,
+   "strlcpy_set_error: %s (%s): Destination buffer %s too small "
+   "(need %zu, have %zu)",
+   parent_func, location, buffer_name, len + 1, dst_size);


This error message is user-facing so please describe the property name 
the user specified instead of writing the function name.



+return false;
+}
+
+/*
+ * String copying wrapper that returns and reports a runtime error in
+ * case of truncation due to insufficient destination buffer space.
+ */
+#define strlcpy_array_return_error(dst_array, src, errp) \
+do { \
+if (!strlcpy_set_error((dst_array), (src), ARRAY_SIZE(dst_array), 
(errp),\
+   __func__, stringify(__LINE__), # dst_array)) { \
+return; \
+} \

Re: [PATCH v4 02/15] hw/display/apple-gfx: Introduce ParavirtualizedGraphics.Framework support

2024-10-25 Thread Phil Dennis-Jordan 
On Fri, 25 Oct 2024 at 08:03, Akihiko Odaki 
wrote:

> On 2024/10/24 19:28, Phil Dennis-Jordan wrote:
> > MacOS provides a framework (library) that allows any vmm to implement a
> > paravirtualized 3d graphics passthrough to the host metal stack called
> > ParavirtualizedGraphics.Framework (PVG). The library abstracts away
> > almost every aspect of the paravirtualized device model and only provides
> > and receives callbacks on MMIO access as well as to share memory address
> > space between the VM and PVG.
> >
> > This patch implements a QEMU device that drives PVG for the VMApple
> > variant of it.
> >
> > Signed-off-by: Alexander Graf 
> > Co-authored-by: Alexander Graf 
> >
> > Subsequent changes:
> >
> >   * Cherry-pick/rebase conflict fixes, API use updates.
> >   * Moved from hw/vmapple/ (useful outside that machine type)
> >   * Overhaul of threading model, many thread safety improvements.
> >   * Asynchronous rendering.
> >   * Memory and object lifetime fixes.
> >   * Refactoring to split generic and (vmapple) MMIO variant specific
> > code.
> >
> > Signed-off-by: Phil Dennis-Jordan 
> > ---
> >
> > v2:
> >
> >   * Cherry-pick/rebase conflict fixes
> >   * BQL function renaming
> >   * Moved from hw/vmapple/ (useful outside that machine type)
> >   * Code review comments: Switched to DEFINE_TYPES macro & little endian
> > MMIO.
> >   * Removed some dead/superfluous code
> >   * Mad set_mode thread & memory safe
> >   * Added migration blocker due to lack of (de-)serialisation.
> >   * Fixes to ObjC refcounting and autorelease pool usage.
> >   * Fixed ObjC new/init misuse
> >   * Switched to ObjC category extension for private property.
> >   * Simplified task memory mapping and made it thread safe.
> >   * Refactoring to split generic and vmapple MMIO variant specific
> > code.
> >   * Switched to asynchronous MMIO writes on x86-64
> >   * Rendering and graphics update are now done asynchronously
> >   * Fixed cursor handling
> >   * Coding convention fixes
> >   * Removed software cursor compositing
> >
> > v3:
> >
> >   * Rebased on latest upstream, fixed breakages including switching to
> Resettable methods.
> >   * Squashed patches dealing with dGPUs, MMIO area size, and GPU picking.
> >   * Allow re-entrant MMIO; this simplifies the code and solves the
> divergence
> > between x86-64 and arm64 variants.
> >
> > v4:
> >
> >   * Renamed '-vmapple' device variant to '-mmio'
> >   * MMIO device type now requires aarch64 host and guest
> >   * Complete overhaul of the glue code for making Qemu's and
> > ParavirtualizedGraphics.framework's threading and synchronisation
> models
> > work together. Calls into PVG are from dispatch queues while the
> > BQL-holding initiating thread processes AIO context events;
> callbacks from
> > PVG are scheduled as BHs on the BQL/main AIO context, awaiting
> completion
> > where necessary.
> >   * Guest frame rendering state is covered by the BQL, with only the PVG
> calls
> > outside the lock, and serialised on the named render_queue.
> >   * Simplified logic for dropping frames in-flight during mode changes,
> fixed
> > bug in pending frames logic.
> >   * Addressed smaller code review notes such as: function naming, object
> type
> > declarations, type names/declarations/casts, code formatting,
> #include
> > order, over-cautious ObjC retain/release, what goes in init vs
> realize,
> > etc.
> >
> >
> >   hw/display/Kconfig  |   9 +
> >   hw/display/apple-gfx-mmio.m | 284 ++
> >   hw/display/apple-gfx.h  |  58 +++
> >   hw/display/apple-gfx.m  | 713 
> >   hw/display/meson.build  |   4 +
> >   hw/display/trace-events |  26 ++
> >   meson.build |   4 +
> >   7 files changed, 1098 insertions(+)
> >   create mode 100644 hw/display/apple-gfx-mmio.m
> >   create mode 100644 hw/display/apple-gfx.h
> >   create mode 100644 hw/display/apple-gfx.m
> >
> > diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> > index 2250c740078..6a9b7b19ada 100644
> > --- a/hw/display/Kconfig
> > +++ b/hw/display/Kconfig
> > @@ -140,3 +140,12 @@ config XLNX_DISPLAYPORT
> >
> >   config DM163
> >   bool
> > +
> > +config MAC_PVG
> > +bool
> > +default y
> > +
> > +config MAC_PVG_MMIO
> > +bool
> > +depends on MAC_PVG && AARCH64
> > +
> > diff --git a/hw/display/apple-gfx-mmio.m b/hw/display/apple-gfx-mmio.m
> > new file mode 100644
> > index 000..06131bc23f1
> > --- /dev/null
> > +++ b/hw/display/apple-gfx-mmio.m
> > @@ -0,0 +1,284 @@
> > +/*
> > + * QEMU Apple ParavirtualizedGraphics.framework device, MMIO (arm64)
> variant
> > + *
> > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights
> Reserved.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> > + * See the COPYING file in the top-level directory.
>
> Use SPDX-License-Identifier. You can find some exam

Re: [PATCH] hw/nvme: fix handling of over-committed queues

2024-10-25 Thread Waldek Kozaczuk 
Hi,

I have applied this patch to the same QEMU source tree where I reproduced
this issue. I changed the queue size to 3 on the OSv side to trigger this
bug, but unfortunately, I still see the same behavior of the OSv guest
hanging.

Regards,
Waldemar Kozaczuk

On Fri, Oct 25, 2024 at 6:51 AM Klaus Jensen  wrote:

> From: Klaus Jensen 
>
> If a host chooses to use the SQHD "hint" in the CQE to know if there is
> room in the submission queue for additional commands, it may result in a
> situation where there are not enough internal resources (struct
> NvmeRequest) available to process the command. For a lack of a better
> term, the host may "over-commit" the device (i.e., it may have more
> inflight commands than the queue size).
>
> For example, assume a queue with N entries. The host submits N commands
> and all are picked up for processing, advancing the head and emptying
> the queue. Regardless of which of these N commands complete first, the
> SQHD field of that CQE will indicate to the host that the queue is
> empty, which allows the host to issue N commands again. However, if the
> device has not posted CQEs for all the previous commands yet, the device
> will have less than N resources available to process the commands, so
> queue processing is suspended.
>
> And here lies an 11 year latent bug. In the absense of any additional
> tail updates on the submission queue, we never schedule the processing
> bottom-half again unless we observe a head update on an associated full
> completion queue. This has been sufficient to handle N-to-1 SQ/CQ setups
> (in the absense of over-commit of course). Incidentially, that "kick all
> associated SQs" mechanism can now be killed since we now just schedule
> queue processing when we return a processing resource to a non-empty
> submission queue, which happens to cover both edge cases.
>
> So, apparently, no previous driver tested with hw/nvme has ever used
> SQHD (e.g., neither the Linux NVMe driver or SPDK uses it). But then OSv
> shows up with the driver that actually does. I salute you.
>
> Fixes: f3c507adcd7b ("NVMe: Initial commit for new storage interface")
> Cc: qemu-sta...@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2388
> Reported-by: Waldemar Kozaczuk 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/nvme/ctrl.c | 16 +++-
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index
> f4e89203c1a6e3b051fd7185cbf01ec9bae9684a..b13585c4da911b9e8ae4a722761fd85dfa24be4d
> 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -1520,9 +1520,16 @@ static void nvme_post_cqes(void *opaque)
>  stl_le_p(&n->bar.csts, NVME_CSTS_FAILED);
>  break;
>  }
> +
>  QTAILQ_REMOVE(&cq->req_list, req, entry);
> +
>  nvme_inc_cq_tail(cq);
>  nvme_sg_unmap(&req->sg);
> +
> +if (QTAILQ_EMPTY(&sq->req_list) && !nvme_sq_empty(sq)) {
> +qemu_bh_schedule(sq->bh);
> +}
> +
>  QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
>  }
>  if (cq->tail != cq->head) {
> @@ -7950,7 +7957,6 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr
> addr, int val)
>  /* Completion queue doorbell write */
>
>  uint16_t new_head = val & 0x;
> -int start_sqs;
>  NvmeCQueue *cq;
>
>  qid = (addr - (0x1000 + (1 << 2))) >> 3;
> @@ -8001,18 +8007,10 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr
> addr, int val)
>
>  trace_pci_nvme_mmio_doorbell_cq(cq->cqid, new_head);
>
> -start_sqs = nvme_cq_full(cq) ? 1 : 0;
>  cq->head = new_head;
>  if (!qid && n->dbbuf_enabled) {
>  stl_le_pci_dma(pci, cq->db_addr, cq->head,
> MEMTXATTRS_UNSPECIFIED);
>  }
> -    if (start_sqs) {
> -NvmeSQueue *sq;
> -QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
> -qemu_bh_schedule(sq->bh);
> -}
> -qemu_bh_schedule(cq->bh);
> -}
>
>  if (cq->tail == cq->head) {
>  if (cq->irq_enabled) {
>
> ---
> base-commit: e67b7aef7c7f67ecd0282e903e0daff806d5d680
> change-id: 20241025-issue-2388-bd047487f74c
>
> Best regards,
> --
> Klaus Jensen 
>
>