date:20231220


On 2023/12/21 16:35, Xenia Ragiadakou wrote:


On 21/12/23 07:45, Akihiko Odaki wrote:

On 2023/12/19 16:53, Huang Rui wrote:

From: Xenia Ragiadakou 

When the memory region has a different life-cycle from that of her 
parent,
could be automatically released, once has been unparent and once all 
of her

references have gone away, via the object's free callback.

However, currently, the address space subsystem keeps references to the
memory region without first incrementing its object's reference count.
As a result, the automatic deallocation of the object, not taking into
account those references, results in use-after-free memory corruption.

More specifically, reference to the memory region is kept in flatview
ranges. If the reference count of the memory region is not incremented,
flatview_destroy(), that is asynchronous, may be called after memory
region's destruction. If the reference count of the memory region is
incremented, memory region's destruction will take place after
flatview_destroy() has released its references.

This patch increases the reference count of an owned memory region 
object
on each memory_region_ref() and decreases it on each 
memory_region_unref().


Why not pass the memory region itself as the owner parameter of 
memory_region_init_ram_ptr()?


Hmm, in that case, how will it be guaranteed that the VirtIOGPU won't 
disappear while the memory region is still in use?


You can object_ref() when you do memory_region_init_ram_ptr() and 
object_unref() when the memory region is being destroyed.

Re: [PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions

2023-12-20 Thread Xenia Ragiadakou




On 21/12/23 07:45, Akihiko Odaki wrote:

On 2023/12/19 16:53, Huang Rui wrote:

From: Xenia Ragiadakou 

When the memory region has a different life-cycle from that of her 
parent,
could be automatically released, once has been unparent and once all 
of her

references have gone away, via the object's free callback.

However, currently, the address space subsystem keeps references to the
memory region without first incrementing its object's reference count.
As a result, the automatic deallocation of the object, not taking into
account those references, results in use-after-free memory corruption.

More specifically, reference to the memory region is kept in flatview
ranges. If the reference count of the memory region is not incremented,
flatview_destroy(), that is asynchronous, may be called after memory
region's destruction. If the reference count of the memory region is
incremented, memory region's destruction will take place after
flatview_destroy() has released its references.

This patch increases the reference count of an owned memory region object
on each memory_region_ref() and decreases it on each 
memory_region_unref().


Why not pass the memory region itself as the owner parameter of 
memory_region_init_ram_ptr()?


Hmm, in that case, how will it be guaranteed that the VirtIOGPU won't 
disappear while the memory region is still in use?

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

2023-12-20 Thread Xenia Ragiadakou




On 21/12/23 07:57, Akihiko Odaki wrote:

On 2023/12/19 16:53, Huang Rui wrote:

From: Antonio Caggiano 

Support BLOB resources creation, mapping and unmapping by calling the
new stable virglrenderer 0.10 interface. Only enabled when available and
via the blob config. E.g. -device virtio-vga-gl,blob=true

Signed-off-by: Antonio Caggiano 
Signed-off-by: Dmitry Osipenko 
Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Huang Rui 
---

Changes in v6:
- Use new struct virgl_gpu_resource.
- Unmap, unref and destroy the resource only after the memory region
   has been completely removed.
- In unref check whether the resource is still mapped.
- In unmap_blob check whether the resource has been already unmapped.
- Fix coding style

  hw/display/virtio-gpu-virgl.c | 274 +-
  hw/display/virtio-gpu.c   |   4 +-
  meson.build   |   4 +
  3 files changed, 276 insertions(+), 6 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c 
b/hw/display/virtio-gpu-virgl.c

index faab374336..5a3a292f79 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,7 @@
  #include "trace.h"
  #include "hw/virtio/virtio.h"
  #include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
  #include "ui/egl-helpers.h"
@@ -24,8 +25,62 @@
  struct virgl_gpu_resource {
  struct virtio_gpu_simple_resource res;
+    uint32_t ref;
+    VirtIOGPU *g;
+
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    /* only blob resource needs this region to be mapped as guest 
mmio */

+    MemoryRegion *region;


Why not just embed MemoryRegion into struct virgl_gpu_resource instead 
of having a pointer?


To decouple memory region from the resource. Since there are different 
commands for creating and mapping maybe there is the intention to map 
the same resource multiple times. If the lifecycle of the resource is 
tightly coupled to the lifecycle of the memory region then the memory 
region could be embedded into the struct. Ray could give more insight.





+#endif
  };
+static void vres_get_ref(struct virgl_gpu_resource *vres)
+{
+    uint32_t ref;
+
+    ref = qatomic_fetch_inc(>ref);
+    g_assert(ref < INT_MAX);
+}
+
+static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
+{
+    struct virtio_gpu_simple_resource *res;
+    VirtIOGPU *g;
+
+    if (!vres) {
+    return;
+    }
+
+    g = vres->g;
+    res = >res;
+    QTAILQ_REMOVE(>reslist, res, next);
+    virtio_gpu_cleanup_mapping(g, res);
+    g_free(vres);
+}
+
+static void virgl_resource_unref(struct virgl_gpu_resource *vres)
+{
+    struct virtio_gpu_simple_resource *res;
+
+    if (!vres) {
+    return;
+    }
+
+    res = >res;
+    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
+    virgl_renderer_resource_unref(res->resource_id);
+}
+
+static void vres_put_ref(struct virgl_gpu_resource *vres)
+{
+    g_assert(vres->ref > 0);
+
+    if (qatomic_fetch_dec(>ref) == 1) {
+    virgl_resource_unref(vres);
+    virgl_resource_destroy(vres);
+    }
+}
+
  static struct virgl_gpu_resource *
  virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
  {
@@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
 c2d.width, c2d.height);
  vres = g_new0(struct virgl_gpu_resource, 1);
+    vres_get_ref(vres);
+    vres->g = g;
  vres->res.width = c2d.width;
  vres->res.height = c2d.height;
  vres->res.format = c2d.format;
@@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
 c3d.width, c3d.height, 
c3d.depth);

  vres = g_new0(struct virgl_gpu_resource, 1);
+    vres_get_ref(vres);
+    vres->g = g;
  vres->res.width = c3d.width;
  vres->res.height = c3d.height;
  vres->res.format = c3d.format;
@@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
  return;
  }
-    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
-    virgl_renderer_resource_unref(unref.resource_id);
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    if (vres->region) {
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+    MemoryRegion *mr = vres->region;
+
+    warn_report("%s: blob resource %d not unmapped",
+    __func__, unref.resource_id);
+    vres->region = NULL;
+    memory_region_set_enabled(mr, false);
+    memory_region_del_subregion(>hostmem, mr);
+    object_unparent(OBJECT(mr));
+    }
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
-    QTAILQ_REMOVE(>reslist, >res, next);
-    virtio_gpu_cleanup_mapping(g, >res);
-    g_free(vres);
+    vres_put_ref(vres);


What will happen if the guest consecutively requests 
VIRTIO_GPU_CMD_RESOURCE_UNREF twice for a mapped resource?


You are right. I think the resource needs to be removed from the list 
synchronously on first unref.

Re: [PATCH 3/6] fixup scsi: only access SCSIDevice->requests from one thread


On 12/21/23 02:49, Stefan Hajnoczi wrote:

Signed-off-by: Stefan Hajnoczi 
---
  hw/scsi/scsi-bus.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index df68a44b6a..5b08cbf60a 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -127,7 +127,8 @@ static void scsi_device_for_each_req_async_bh(void *opaque)
   */
  ctx = blk_get_aio_context(s->conf.blk);
  if (ctx != qemu_get_current_aio_context()) {
-aio_bh_schedule_oneshot(ctx, scsi_device_for_each_req_async_bh, data);
+aio_bh_schedule_oneshot(ctx, scsi_device_for_each_req_async_bh,
+g_steal_pointer());
  return;
  }
  


Reviewed-by: Paolo Bonzini

Re: [PATCH] target/i386: Add new CPU model SierraForest

2023-12-20 Thread Xiaoyao Li


On 12/6/2023 9:19 PM, Tao Su wrote:

SierraForest is Intel's first generation E-core based Xeon server
processor, which will be released in the first half of 2024.

SierraForest mainly adds the following new features based on
GraniteRapids:

- CMPCCXADD CPUID.(EAX=7,ECX=1):EAX[bit 7]
- AVX-IFMA CPUID.(EAX=7,ECX=1):EAX[bit 23]
- AVX-VNNI-INT8 CPUID.(EAX=7,ECX=1):EDX[bit 4]
- AVX-NE-CONVERT CPUID.(EAX=7,ECX=1):EDX[bit 5]
- LAM CPUID.(EAX=7,ECX=1):EAX[bit 26]
- LASS CPUID.(EAX=7,ECX=1):EAX[bit 6]

and removes the following features based on GraniteRapids:

- HLE CPUID.(EAX=7,ECX=0):EBX[bit 4]
- RTM CPUID.(EAX=7,ECX=0):EBX[bit 11]
- AVX512F CPUID.(EAX=7,ECX=0):EBX[bit 16]
- AVX512DQ CPUID.(EAX=7,ECX=0):EBX[bit 17]
- AVX512_IFMA CPUID.(EAX=7,ECX=0):EBX[bit 21]
- AVX512CD CPUID.(EAX=7,ECX=0):EBX[bit 28]
- AVX512BW CPUID.(EAX=7,ECX=0):EBX[bit 30]
- AVX512VL CPUID.(EAX=7,ECX=0):EBX[bit 31]
- AVX512_VBMI CPUID.(EAX=7,ECX=0):ECX[bit 1]
- AVX512_VBMI2 CPUID.(EAX=7,ECX=0):ECX[bit 6]
- AVX512_VNNI CPUID.(EAX=7,ECX=0):ECX[bit 11]
- AVX512_BITALG CPUID.(EAX=7,ECX=0):ECX[bit 12]
- AVX512_VPOPCNTDQ CPUID.(EAX=7,ECX=0):ECX[bit 14]
- LA57 CPUID.(EAX=7,ECX=0):ECX[bit 16]
- TSXLDTRK CPUID.(EAX=7,ECX=0):EDX[bit 16]
- AMX-BF16 CPUID.(EAX=7,ECX=0):EDX[bit 22]
- AVX512_FP16 CPUID.(EAX=7,ECX=0):EDX[bit 23]
- AMX-TILE CPUID.(EAX=7,ECX=0):EDX[bit 24]
- AMX-INT8 CPUID.(EAX=7,ECX=0):EDX[bit 25]
- AVX512_BF16 CPUID.(EAX=7,ECX=1):EAX[bit 5]
- fast zero-length MOVSB CPUID.(EAX=7,ECX=1):EAX[bit 10]
- fast short CMPSB, SCASB CPUID.(EAX=7,ECX=1):EAX[bit 12]
- AMX-FP16 CPUID.(EAX=7,ECX=1):EAX[bit 21]
- PREFETCHI CPUID.(EAX=7,ECX=1):EDX[bit 14]
- XFD CPUID.(EAX=0xD,ECX=1):EAX[bit 4]
- EPT_PAGE_WALK_LENGTH_5 VMX_EPT_VPID_CAP(0x48c)[bit 7]

SierraForest doesn't support TSX, so TSX Async Abort(TAA) vulnerabilities
don't exist on SierraForest. On KVM side, if host doesn't enumerate RTM
or RTM gets disabled, ARCH_CAP_TAA_NO is reported as unsupported. To
avoid the confusing warning:
warning: host doesn't support requested feature: MSR(10AH).taa-no
  [bit 8]

just don't include TAA_NO in SierraForest CPU model.

Currently LAM and LASS are not enabled in KVM mainline yet,  will add
them after merged.

Signed-off-by: Tao Su 


Reviewed-by: Xiaoyao Li 


---
The new features can be found in Intel ISE[1].
LAM has just been accepted by KVM[2].

Although we would like to include all SierraForest features in the first
version of the CPU model, SierraForest will be released in the first half
of 2024[3], we would want user can have a first usable SierraForest CPU
model in the QEMU when they have the hardware in their hand.

[1] https://cdrdv2.intel.com/v1/dl/getContent/671368
[2]
https://lore.kernel.org/all/169810442917.2499338.3440694989716170017.b4...@google.com/
[3]
https://www.intel.com/content/www/us/en/newsroom/news/tackling-throughput-computing-sierra-forest.html
---
  target/i386/cpu.c | 126 ++
  1 file changed, 126 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index cd16cb893d..2405c9e407 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4099,6 +4099,132 @@ static const X86CPUDefinition builtin_x86_defs[] = {
  { /* end of list */ },
  },
  },
+{
+.name = "SierraForest",
+.level = 0x23,
+.vendor = CPUID_VENDOR_INTEL,
+.family = 6,
+.model = 175,
+.stepping = 0,
+/*
+ * please keep the ascending order so that we can have a clear view of
+ * bit position of each feature.
+ */
+.features[FEAT_1_EDX] =
+CPUID_FP87 | CPUID_VME | CPUID_DE | CPUID_PSE | CPUID_TSC |
+CPUID_MSR | CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC |
+CPUID_SEP | CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV |
+CPUID_PAT | CPUID_PSE36 | CPUID_CLFLUSH | CPUID_MMX | CPUID_FXSR |
+CPUID_SSE | CPUID_SSE2,
+.features[FEAT_1_ECX] =
+CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSSE3 |
+CPUID_EXT_FMA | CPUID_EXT_CX16 | CPUID_EXT_PCID | CPUID_EXT_SSE41 |
+CPUID_EXT_SSE42 | CPUID_EXT_X2APIC | CPUID_EXT_MOVBE |
+CPUID_EXT_POPCNT | CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_AES |
+CPUID_EXT_XSAVE | CPUID_EXT_AVX | CPUID_EXT_F16C | 
CPUID_EXT_RDRAND,
+.features[FEAT_8000_0001_EDX] =
+CPUID_EXT2_SYSCALL | CPUID_EXT2_NX | CPUID_EXT2_PDPE1GB |
+CPUID_EXT2_RDTSCP | CPUID_EXT2_LM,
+.features[FEAT_8000_0001_ECX] =
+CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM | CPUID_EXT3_3DNOWPREFETCH,
+.features[FEAT_8000_0008_EBX] =
+CPUID_8000_0008_EBX_WBNOINVD,
+.features[FEAT_7_0_EBX] =
+CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
+CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS |
+CPUID_7_0_EBX_INVPCID | CPUID_7_0_EBX_RDSEED |

Re: [PATCH v2 09/17] hw/loongarch: Fix fdt memory node wrong 'reg'





On 2023/12/18 下午5:00, Song Gao wrote:

The right fdt memory node like [1], not [2]

   [1]
 memory@0 {
 device_type = "memory";
 reg = <0x00 0x00 0x00 0x1000>;
 };
   [2]
 memory@0 {
 device_type = "memory";
 reg = <0x02 0x00 0x02 0x1000>;
 };

Signed-off-by: Song Gao 
---
  hw/loongarch/virt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index c45e724961..eaa0824f73 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -290,7 +290,7 @@ static void fdt_add_memory_node(MachineState *ms,
  char *nodename = g_strdup_printf("/memory@%" PRIx64, base);
  
  qemu_fdt_add_subnode(ms->fdt, nodename);

-qemu_fdt_setprop_cells(ms->fdt, nodename, "reg", 2, base, 2, size);
+qemu_fdt_setprop_cells(ms->fdt, nodename, "reg", 0, base, 0, size);
  qemu_fdt_setprop_string(ms->fdt, nodename, "device_type", "memory");
  
  if (ms->numa_state && ms->numa_state->num_nodes) {




Reviewed-by: Bibo Mao

Re: [PATCH 6/6] nbd/server: introduce NBDClient->lock to protect fields


On 12/21/23 02:49, Stefan Hajnoczi wrote:

  nbd_client_receive_next_request(client);
+
+qemu_mutex_unlock(>lock);
+
  if (ret == -EIO) {
  goto disconnect;
  }


I think I slightly prefer if disconnect is reached with lock taken, for 
consistency with the "done" label.  It does not complicate the code,

because you can just move qio_channel_set_cork() and replace:


@@ -3024,8 +3072,10 @@ static coroutine_fn void nbd_trip(void *opaque)
  }
  
  qio_channel_set_cork(client->ioc, false);

+qemu_mutex_lock(>lock);


with:

+qio_channel_set_cork(client->ioc, false);
+qemu_mutex_lock(>lock);

 if (ret < 0) {
 error_prepend(_err, "Failed to send reply: ");
 goto disconnect;
 }

 /*
  * We must disconnect after NBD_CMD_WRITE or BLOCK_STATUS with
  * payload if we did not read the payload.
  */
 if (!req->complete) {
 error_setg(_err, "Request handling failed in 
intermediate state");

 goto disconnect;
 }
-qio_channel_set_cork(client->ioc, false);
 done:

Thanks,

Paolo


  done:
  nbd_request_put(req);
+qemu_mutex_unlock(>lock);

Re: [PATCH 4/6] nbd/server: avoid per-NBDRequest nbd_client_get/put()


On 12/21/23 02:49, Stefan Hajnoczi wrote:

nbd_trip() processes a single NBD request from start to finish and holds
an NBDClient reference throughout. NBDRequest does not outlive the scope
of nbd_trip(). Therefore it is unnecessary to ref/unref NBDClient for
each NBDRequest.

Removing these nbd_client_get()/nbd_client_put() calls will make
thread-safety easier in the commits that follow.

Signed-off-by: Stefan Hajnoczi 
---
  nbd/server.c | 3 ---
  1 file changed, 3 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 895cf0a752..0b09ccc8dc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1557,7 +1557,6 @@ static NBDRequestData *nbd_request_get(NBDClient *client)
  client->nb_requests++;
  
  req = g_new0(NBDRequestData, 1);

-nbd_client_get(client);
  req->client = client;
  return req;
  }
@@ -1578,8 +1577,6 @@ static void nbd_request_put(NBDRequestData *req)
  }
  
  nbd_client_receive_next_request(client);

-
-nbd_client_put(client);
  }
  
  static void blk_aio_attached(AioContext *ctx, void *opaque)


Reviewed-by: Paolo Bonzini

Re: [PATCH 5/6] nbd/server: only traverse NBDExport->clients from main loop thread


On 12/21/23 02:49, Stefan Hajnoczi wrote:

The NBD clients list is currently accessed from both the export
AioContext and the main loop thread. When the AioContext lock is removed
there will be nothing protecting the clients list.

Adding a lock around the clients list is tricky because NBDClient
structs are refcounted and may be freed from the export AioContext or
the main loop thread. nbd_export_request_shutdown() -> client_close() ->
nbd_client_put() is also tricky because the list lock would be held
while indirectly dropping references to NDBClients.

A simpler approach is to only allow nbd_client_put() and client_close()
calls from the main loop thread. Then the NBD clients list is only
accessed from the main loop thread and no fancy locking is needed.

nbd_trip() just needs to reschedule itself in the main loop AioContext
before calling nbd_client_put() and client_close(). This costs more CPU
cycles per NBD request but is needed for thread-safety when the
AioContext lock is removed.

Note that nbd_client_get() can still be called from either thread, so
make NBDClient->refcount atomic.

Signed-off-by: Stefan Hajnoczi 
---
  nbd/server.c | 23 ---
  1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 0b09ccc8dc..527fbdab4a 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -122,7 +122,7 @@ struct NBDMetaContexts {
  };
  
  struct NBDClient {

-int refcount;
+int refcount; /* atomic */
  void (*close_fn)(NBDClient *client, bool negotiated);
  
  NBDExport *exp;

@@ -1501,14 +1501,17 @@ static int coroutine_fn nbd_receive_request(NBDClient 
*client, NBDRequest *reque
  
  #define MAX_NBD_REQUESTS 16
  
+/* Runs in export AioContext and main loop thread */

  void nbd_client_get(NBDClient *client)
  {
-client->refcount++;
+qatomic_inc(>refcount);
  }
  
  void nbd_client_put(NBDClient *client)

  {
-if (--client->refcount == 0) {
+assert(qemu_in_main_thread());
+
+if (qatomic_fetch_dec(>refcount) == 1) {
  /* The last reference should be dropped by client->close,
   * which is called by client_close.
   */
@@ -1531,6 +1534,8 @@ void nbd_client_put(NBDClient *client)
  
  static void client_close(NBDClient *client, bool negotiated)

  {
+assert(qemu_in_main_thread());
+
  if (client->closing) {
  return;
  }
@@ -2938,8 +2943,15 @@ static coroutine_fn void nbd_trip(void *opaque)
  int ret;
  Error *local_err = NULL;
  
+/*

+ * Note that nbd_client_put() and client_close() must be called from the
+ * main loop thread. Use aio_co_reschedule_self() to switch AioContext
+ * before calling these functions.
+ */
+
  trace_nbd_trip();
  if (client->closing) {
+aio_co_reschedule_self(qemu_get_aio_context());
  nbd_client_put(client);
  return;
  }
@@ -2949,6 +2961,7 @@ static coroutine_fn void nbd_trip(void *opaque)
   * We're switching between AIO contexts. Don't attempt to receive a 
new
   * request and kick the main context which may be waiting for us.
   */
+aio_co_reschedule_self(qemu_get_aio_context());
  nbd_client_put(client);
  client->recv_coroutine = NULL;
  aio_wait_kick();
@@ -3013,6 +3026,8 @@ static coroutine_fn void nbd_trip(void *opaque)
  qio_channel_set_cork(client->ioc, false);
  done:
  nbd_request_put(req);
+
+aio_co_reschedule_self(qemu_get_aio_context());
  nbd_client_put(client);
  return;


This is very expensive to do on every NBD receive, considering that it really
can happen only when closing (see the assertion in nbd_client_put).

In Linux there is a common pattern of "if refcount could go to zero, take
a lock before doing the decrement".  We can do something similar with "if
refcount could go to zero, move to main iothread before doing the decrement":

diff --git a/nbd/server.c b/nbd/server.c
index 895cf0a7525..aec306923d8 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1529,6 +1529,21 @@ void nbd_client_put(NBDClient *client)
 }
 }
 
+static bool nbd_client_put_nonzero(NBDClient *client)

+{
+int old = qatomic_read(>refcount);
+do {
+if (old == 1) {
+return false;
+}
+
+int expected = old;
+old = qatomic_cmpxchg(>refcount, expected, expected - 1);
+} while (old != expected);
+
+return true;
+}
+
 static void client_close(NBDClient *client, bool negotiated)
 {
 if (client->closing) {
@@ -2936,15 +2951,14 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
 static coroutine_fn void nbd_trip(void *opaque)
 {
 NBDClient *client = opaque;
-NBDRequestData *req;
+NBDRequestData *req = NULL;
 NBDRequest request = { 0 };/* GCC thinks it can be used uninitialized 
*/
 int ret;
 Error *local_err = NULL;
 
 trace_nbd_trip();

 if (client->closing) {
-nbd_client_put(client);
-return;

Re: [PATCH v2 04/17] hw/loongarch: Add slave cpu boot_code





On 2023/12/18 下午5:00, Song Gao wrote:

Signed-off-by: Song Gao 
---
  hw/loongarch/boot.c | 65 -
  1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/hw/loongarch/boot.c b/hw/loongarch/boot.c
index 4bfe24274a..076e795714 100644
--- a/hw/loongarch/boot.c
+++ b/hw/loongarch/boot.c
@@ -14,6 +14,62 @@
  #include "qemu/error-report.h"
  #include "sysemu/reset.h"
  
+enum {

+SLAVE_BOOT,
+};
+
+static const MemMapEntry loader_rommap[] = {
+[SLAVE_BOOT] = {0xf10, 0x1},
+};

Address 0xf10 had better be defined before 0x10

Regards
Bibo Mao


+
+static unsigned int slave_boot_code[] = {
+  /* Configure reset ebase. */
+0x0400302c,   /* csrwr  $r12,0xc*/
+
+  /* Disable interrupt. */
+0x0380100c,   /* ori$r12,$r0,0x4*/
+0x04000180,   /* csrxchg$r0,$r12,0x0*/
+
+  /* Clear mailbox. */
+0x142d,   /* lu12i.w$r13,1(0x1) */
+0x038081ad,   /* ori$r13,$r13,0x20  */
+0x06481da0,   /* iocsrwr.d  $r0,$r13*/
+
+  /* Enable IPI interrupt.  */
+0x142c,   /* lu12i.w$r12,1(0x1) */
+0x0400118c,   /* csrxchg$r12,$r12,0x4   */
+0x02fffc0c,   /* addi.d $r12,$r0,-1(0xfff)  */
+0x142d,   /* lu12i.w$r13,1(0x1) */
+0x038011ad,   /* ori$r13,$r13,0x4   */
+0x064819ac,   /* iocsrwr.w  $r12,$r13   */
+0x142d,   /* lu12i.w$r13,1(0x1) */
+0x038081ad,   /* ori$r13,$r13,0x20  */
+
+  /* Wait for wakeup  <.L11>:   */
+0x06488000,   /* idle   0x0 */
+0x0340,   /* andi   $r0,$r0,0x0 */
+0x064809ac,   /* iocsrrd.w  $r12,$r13   */
+0x43fff59f,   /* beqz   $r12,-12(0x74) # 48 <.L11> */
+
+  /* Read and clear IPI interrupt.  */
+0x142d,   /* lu12i.w$r13,1(0x1) */
+0x064809ac,   /* iocsrrd.w  $r12,$r13   */
+0x142d,   /* lu12i.w$r13,1(0x1) */
+0x038031ad,   /* ori$r13,$r13,0xc   */
+0x064819ac,   /* iocsrwr.w  $r12,$r13   */
+
+  /* Disable  IPI interrupt.*/
+0x142c,   /* lu12i.w$r12,1(0x1) */
+0x04001180,   /* csrxchg$r0,$r12,0x4*/
+
+  /* Read mail buf and jump to specified entry */
+0x142d,   /* lu12i.w$r13,1(0x1) */
+0x038081ad,   /* ori$r13,$r13,0x20  */
+0x06480dac,   /* iocsrrd.d  $r12,$r13   */
+0x00150181,   /* move   $r1,$r12*/
+0x4c20,   /* jirl   $r0,$r1,0   */
+};
+
  static int init_cmdline(struct loongarch_boot_info *info)
  {
  hwaddr cmdline_addr;
@@ -145,10 +201,17 @@ static void 
loongarch_direct_kernel_boot(LoongArchMachineState *lams,
  exit(1);
  }
  
+rom_add_blob_fixed("slave_boot", slave_boot_code, sizeof(slave_boot_code),

+   loader_rommap[SLAVE_BOOT].base);
+
  for (i = 0; i < machine->smp.cpus; i++) {
  lacpu = LOONGARCH_CPU(qemu_get_cpu(i));
  lacpu->env.load_elf = true;
-lacpu->env.elf_address = kernel_addr;
+if (i == 0) {
+lacpu->env.elf_address = kernel_addr;
+} else {
+lacpu->env.elf_address = loader_rommap[SLAVE_BOOT].base;
+}
  lacpu->env.boot_info = info;
  }
  }

Re: [PATCH v2 03/17] hw/loongarch: Add init_cmdline





On 2023/12/18 下午5:00, Song Gao wrote:

Add init_cmline and set boot_info->a0, a1

Signed-off-by: Song Gao 
---
  hw/loongarch/boot.c | 21 +
  include/hw/loongarch/virt.h |  2 ++
  target/loongarch/cpu.h  |  2 ++
  3 files changed, 25 insertions(+)

diff --git a/hw/loongarch/boot.c b/hw/loongarch/boot.c
index 2be6dfb037..4bfe24274a 100644
--- a/hw/loongarch/boot.c
+++ b/hw/loongarch/boot.c
@@ -14,6 +14,20 @@
  #include "qemu/error-report.h"
  #include "sysemu/reset.h"
  
+static int init_cmdline(struct loongarch_boot_info *info)

+{
+hwaddr cmdline_addr;
+cmdline_addr = 0xff0ULL;
+
+pstrcpy_targphys("cmdline", 0xff0ULL,
+ COMMAND_LINE_SIZE, info->kernel_cmdline);
There are two places using 0xff0ULL here, it had better be defined 
as macro. Also can address for cmdline be before FDT base 
address(0x10) rather than strange value 0xff0 ? -:)



+
+info->a0 = 1;
+info->a1 = cmdline_addr;
+
+return 0;
+}
+
  static uint64_t cpu_loongarch_virt_to_phys(void *opaque, uint64_t addr)
  {
  return addr & MAKE_64BIT_MASK(0, TARGET_PHYS_ADDR_SPACE_BITS);
@@ -63,6 +77,8 @@ static int64_t load_kernel_info(struct loongarch_boot_info 
*info)
  exit(1);
  }
  
+init_cmdline(info);

+
  return kernel_entry;
  }
  
@@ -73,6 +89,10 @@ static void reset_load_elf(void *opaque)
  
  cpu_reset(CPU(cpu));

  if (env->load_elf) {
+   if (cpu == LOONGARCH_CPU(first_cpu)) {
+env->gpr[4] = env->boot_info->a0;
+env->gpr[5] = env->boot_info->a1;
+}
  cpu_set_pc(CPU(cpu), env->elf_address);
  }
  }
@@ -129,6 +149,7 @@ static void 
loongarch_direct_kernel_boot(LoongArchMachineState *lams,
  lacpu = LOONGARCH_CPU(qemu_get_cpu(i));
  lacpu->env.load_elf = true;
  lacpu->env.elf_address = kernel_addr;
+lacpu->env.boot_info = info;
  }
  }
  
diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h

index e4126dd0e7..d21de2cef4 100644
--- a/include/hw/loongarch/virt.h
+++ b/include/hw/loongarch/virt.h
@@ -31,6 +31,8 @@
  #define VIRT_GED_MEM_ADDR   (VIRT_GED_EVT_ADDR + ACPI_GED_EVT_SEL_LEN)
  #define VIRT_GED_REG_ADDR   (VIRT_GED_MEM_ADDR + MEMORY_HOTPLUG_IO_LEN)
  
+#define COMMAND_LINE_SIZE   512
The macro COMMAND_LINE_SIZE is already defined in Linux header file, 
maybe standard header file can be used.


/usr/include/asm-generic/setup.h
#define COMMAND_LINE_SIZE  512


Regards
Bibo Mao

+
  struct LoongArchMachineState {
  /*< private >*/
  MachineState parent_obj;
diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index 00d1fba597..c7c695138e 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -362,6 +362,8 @@ typedef struct CPUArchState {
  uint64_t elf_address;
  /* Store ipistate to access from this struct */
  DeviceState *ipistate;
+
+struct loongarch_boot_info *boot_info;
  #endif
  } CPULoongArchState;

Re: [PATCH v7] ui/cocoa: Use NSWindow's ability to resize


On 2023/12/20 5:06, Marek Glogowski wrote:

Hi

For me, the problem does not occur if you use the maximum screen 
resolution available.

For me it is 1680x1050 - everything works fine.
When I change the screen preference to a smaller screen resolution than 
my maximum (1440x900,1024x768 ...) the mouse starts to malfunction. The 
mouse pointer works strenuously and with a delay.


Checked on the current version with git qemu-system-ppc AOS4 with V8 and 
V7 patch and the older version qemu-system-aarch64 Linux/Fedora


Please tell me your whole command line. It is also nice if you test the 
combination of  latest QEMU with and without the patches and Fedora.

Re: [PATCH v2 02/17] hw/loongarch: Add load initrd





On 2023/12/18 下午5:00, Song Gao wrote:

we load initrd ramdisk after kernel_high address

Signed-off-by: Song Gao 
---
  hw/loongarch/boot.c | 29 -
  1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/loongarch/boot.c b/hw/loongarch/boot.c
index 9f25ea5847..2be6dfb037 100644
--- a/hw/loongarch/boot.c
+++ b/hw/loongarch/boot.c
@@ -21,7 +21,8 @@ static uint64_t cpu_loongarch_virt_to_phys(void *opaque, 
uint64_t addr)
  
  static int64_t load_kernel_info(struct loongarch_boot_info *info)

  {
-uint64_t kernel_entry, kernel_low, kernel_high;
+uint64_t kernel_entry, kernel_low, kernel_high, initrd_size;
+ram_addr_t initrd_offset;
  ssize_t kernel_size;
  
  kernel_size = load_elf(info->kernel_filename, NULL,

@@ -36,6 +37,32 @@ static int64_t load_kernel_info(struct loongarch_boot_info 
*info)
   load_elf_strerror(kernel_size));
  exit(1);
  }
+
+if (info->initrd_filename) {
+initrd_size = get_image_size(info->initrd_filename);
+if (initrd_size > 0) {
+initrd_offset = ROUND_UP(kernel_high, 64 * KiB);

Do you test self-compressed vmlinuz elf load?

I think that offset of initrd had better be 4 * kernel_size from 
kernel_high, else uncompressed kernel may overwrite INITRD image.

such as:
 initrd_offset = ROUND_UP(kernel_high + 4 * kernel_size, 64 * KiB);

Regards
Bibo Mao

+
+if (initrd_offset + initrd_size > info->ram_size) {
+error_report("memory too small for initial ram disk '%s'",
+ info->initrd_filename);
+exit(1);
+}
+
+initrd_size = load_image_targphys(info->initrd_filename, 
initrd_offset,
+  info->ram_size - initrd_offset);
+}
+
+if (initrd_size == (target_ulong)-1) {
+error_report("could not load initial ram disk '%s'",
+ info->initrd_filename);
+exit(1);
+}
+} else {
+error_report("Need initrd!");
+exit(1);
+}
+
  return kernel_entry;
  }

Re: [PATCH v2 01/17] hw/loongarch: Move boot fucntions to boot.c





On 2023/12/18 下午5:00, Song Gao wrote:

Move some boot functions to boot.c and struct
loongarch_boot_info into struct LoongArchMachineState.

Signed-off-by: Song Gao 
---
  hw/loongarch/boot.c | 127 
  hw/loongarch/meson.build|   1 +
  hw/loongarch/virt.c | 118 ++---
  include/hw/loongarch/boot.h |  21 ++
  include/hw/loongarch/virt.h |   2 +
  5 files changed, 155 insertions(+), 114 deletions(-)
  create mode 100644 hw/loongarch/boot.c
  create mode 100644 include/hw/loongarch/boot.h

diff --git a/hw/loongarch/boot.c b/hw/loongarch/boot.c
new file mode 100644
index 00..9f25ea5847
--- /dev/null
+++ b/hw/loongarch/boot.c
@@ -0,0 +1,127 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * LoongArch boot helper functions.
+ *
+ * Copyright (c) 2023 Loongson Technology Corporation Limited
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "target/loongarch/cpu.h"
+#include "hw/loongarch/virt.h"
+#include "hw/loader.h"
+#include "elf.h"
+#include "qemu/error-report.h"
+#include "sysemu/reset.h"
+
+static uint64_t cpu_loongarch_virt_to_phys(void *opaque, uint64_t addr)
+{
+return addr & MAKE_64BIT_MASK(0, TARGET_PHYS_ADDR_SPACE_BITS);
+}
+
+static int64_t load_kernel_info(struct loongarch_boot_info *info)
+{
+uint64_t kernel_entry, kernel_low, kernel_high;
+ssize_t kernel_size;
+
+kernel_size = load_elf(info->kernel_filename, NULL,
+   cpu_loongarch_virt_to_phys, NULL,
+   _entry, _low,
+   _high, NULL, 0,
+   EM_LOONGARCH, 1, 0);
+
+if (kernel_size < 0) {
+error_report("could not load kernel '%s': %s",
+ info->kernel_filename,
+ load_elf_strerror(kernel_size));
+exit(1);
+}
+return kernel_entry;
+}
+
+static void reset_load_elf(void *opaque)
+{
+LoongArchCPU *cpu = opaque;
+CPULoongArchState *env = >env;
+
+cpu_reset(CPU(cpu));
+if (env->load_elf) {
+cpu_set_pc(CPU(cpu), env->elf_address);
+}
+}
+
+static void fw_cfg_add_kernel_info(struct loongarch_boot_info *info,
+   FWCfgState *fw_cfg)
+{
+/*
+ * Expose the kernel, the command line, and the initrd in fw_cfg.
+ * We don't process them here at all, it's all left to the
+ * firmware.
+ */
+load_image_to_fw_cfg(fw_cfg,
+ FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
+ info->kernel_filename,
+ false);
+
+if (info->initrd_filename) {
+load_image_to_fw_cfg(fw_cfg,
+ FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
+ info->initrd_filename, false);
+}
+
+if (info->kernel_cmdline) {
+fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+   strlen(info->kernel_cmdline) + 1);
+fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
+  info->kernel_cmdline);
+}
+}
+
+static void loongarch_firmware_boot(LoongArchMachineState *lams,
+struct loongarch_boot_info *info)
+{
+fw_cfg_add_kernel_info(info, lams->fw_cfg);
+}
+
+static void loongarch_direct_kernel_boot(LoongArchMachineState *lams,
+ struct loongarch_boot_info *info)
+{
+MachineState *machine = MACHINE(lams);
+int64_t kernel_addr = 0;
+LoongArchCPU *lacpu;
+int i;
+
+if (info->kernel_filename) {
+kernel_addr = load_kernel_info(info);
+} else {
+error_report("Need kernel filename\n");
+exit(1);
+}
+
+for (i = 0; i < machine->smp.cpus; i++) {
+lacpu = LOONGARCH_CPU(qemu_get_cpu(i));
+lacpu->env.load_elf = true;
+lacpu->env.elf_address = kernel_addr;
+}
+}
+
+void loongarch_load_kernel(MachineState *ms, struct loongarch_boot_info *info)
+{
+LoongArchMachineState *lams = LOONGARCH_MACHINE(ms);
+int i;
+
+/* register reset function */
+for (i = 0; i < ms->smp.cpus; i++) {
+qemu_register_reset(reset_load_elf, LOONGARCH_CPU(qemu_get_cpu(i)));
+}
+
+info->kernel_filename = ms->kernel_filename;
+info->kernel_cmdline = ms->kernel_cmdline;
+info->initrd_filename = ms->initrd_filename;
+
+if (lams->bios_loaded) {
+loongarch_firmware_boot(lams, info);
+} else {
+loongarch_direct_kernel_boot(lams, info);
+}
+}
diff --git a/hw/loongarch/meson.build b/hw/loongarch/meson.build
index c0421502ab..d306d82c2e 100644
--- a/hw/loongarch/meson.build
+++ b/hw/loongarch/meson.build
@@ -1,6 +1,7 @@
  loongarch_ss = ss.source_set()
  loongarch_ss.add(files(
  'fw_cfg.c',
+'boot.c',
  ))
  loongarch_ss.add(when: 'CONFIG_LOONGARCH_VIRT', if_true: [files('virt.c'), 
fdt])
  loongarch_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-build.c'))
diff --git

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

On 2023/12/19 23:14, Peter Maydell wrote:

On Tue, 19 Dec 2023 at 13:49, Huang Rui wrote:

On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:

On 2023/12/19 16:53, Huang Rui wrote:

Sync up kernel headers to update venus macro till they are merged into
mainline.

Thanks for sorting things out with the kernel and spec.

Signed-off-by: Huang Rui
---

Changes in v6:
- Venus capset is applied in kernel, so update it in qemu for future use.

https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7...@collabora.com/
https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0

Please include the link to the upstream commit in the commit message.

So far, it's in drm maintainers' branch not in kernel mainline yet. Do I
need to wait it to be merged into kernel mainline?

For an RFC patchset, no. For patches to be merged into QEMU
the headers change must be in the kernel mainline, and the
QEMU commit that updates our copy of the headers must be a
full-sync done with scripts/update-linux-headers.sh, not a
manual edit.

Apparently the kernel change is unlikely to be merged to mainline before
QEMU 9.0 so we need to come up with some idea to deal with this.

The release of Linux 6.7 is near and the development of 6.8 will start
soon. So it was nice if the change made into 6.8, but unfortunately it
missed the *probably last* drm-misc tree pull request for 6.8:

https://lore.kernel.org/all/aqpn5miejmkks7pbcfex7b6u63uwsruywxsnr3x5ljs45qatin@nbkkej2elk46/

It will still get into linux-next so we may retrieve headers from
linux-next. Or we may add the definition to
hw/display/virtio-gpu-virgl.c; the duplicate definition warning will
tell when the definition becomes no longer necessary. I'm fine with
either option.

Regards,
Akihiko Odaki

Re: [PATCH v2 38/71] hw/loongarch: Constify VMState

2023-12-20 Thread gaosong


在 2023/12/21 上午11:16, Richard Henderson 写道:

Signed-off-by: Richard Henderson 
---
  hw/loongarch/acpi-build.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Song Gao 

Thanks.
Song Gao

diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
index ae292fc543..730bc4a748 100644
--- a/hw/loongarch/acpi-build.c
+++ b/hw/loongarch/acpi-build.c
@@ -564,7 +564,7 @@ static const VMStateDescription vmstate_acpi_build = {
  .name = "acpi_build",
  .version_id = 1,
  .minimum_version_id = 1,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_UINT8(patched, AcpiBuildState),
  VMSTATE_END_OF_LIST()
  },

Re: [PATCH v2 09/71] target/loongarch: Constify VMState in machine.c

2023-12-20 Thread gaosong


在 2023/12/21 上午11:15, Richard Henderson 写道:

Signed-off-by: Richard Henderson 
---
  target/loongarch/machine.c | 18 +-
  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Song Gao 

Thanks.
Song Gao

diff --git a/target/loongarch/machine.c b/target/loongarch/machine.c
index 1c4e01d076..c7029fb9b4 100644
--- a/target/loongarch/machine.c
+++ b/target/loongarch/machine.c
@@ -14,7 +14,7 @@ static const VMStateDescription vmstate_fpu_reg = {
  .name = "fpu_reg",
  .version_id = 1,
  .minimum_version_id = 1,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_UINT64(UD(0), VReg),
  VMSTATE_END_OF_LIST()
  }
@@ -36,7 +36,7 @@ static const VMStateDescription vmstate_fpu = {
  .version_id = 1,
  .minimum_version_id = 1,
  .needed = fpu_needed,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_FPU_REGS(env.fpr, LoongArchCPU, 0),
  VMSTATE_UINT32(env.fcsr0, LoongArchCPU),
  VMSTATE_BOOL_ARRAY(env.cf, LoongArchCPU, 8),
@@ -48,7 +48,7 @@ static const VMStateDescription vmstate_lsxh_reg = {
  .name = "lsxh_reg",
  .version_id = 1,
  .minimum_version_id = 1,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_UINT64(UD(1), VReg),
  VMSTATE_END_OF_LIST()
  }
@@ -70,7 +70,7 @@ static const VMStateDescription vmstate_lsx = {
  .version_id = 1,
  .minimum_version_id = 1,
  .needed = lsx_needed,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_LSXH_REGS(env.fpr, LoongArchCPU, 0),
  VMSTATE_END_OF_LIST()
  },
@@ -80,7 +80,7 @@ static const VMStateDescription vmstate_lasxh_reg = {
  .name = "lasxh_reg",
  .version_id = 1,
  .minimum_version_id = 1,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_UINT64(UD(2), VReg),
  VMSTATE_UINT64(UD(3), VReg),
  VMSTATE_END_OF_LIST()
@@ -103,7 +103,7 @@ static const VMStateDescription vmstate_lasx = {
  .version_id = 1,
  .minimum_version_id = 1,
  .needed = lasx_needed,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_LASXH_REGS(env.fpr, LoongArchCPU, 0),
  VMSTATE_END_OF_LIST()
  },
@@ -114,7 +114,7 @@ const VMStateDescription vmstate_tlb = {
  .name = "cpu/tlb",
  .version_id = 0,
  .minimum_version_id = 0,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_UINT64(tlb_misc, LoongArchTLB),
  VMSTATE_UINT64(tlb_entry0, LoongArchTLB),
  VMSTATE_UINT64(tlb_entry1, LoongArchTLB),
@@ -127,7 +127,7 @@ const VMStateDescription vmstate_loongarch_cpu = {
  .name = "cpu",
  .version_id = 1,
  .minimum_version_id = 1,
-.fields = (VMStateField[]) {
+.fields = (const VMStateField[]) {
  VMSTATE_UINTTL_ARRAY(env.gpr, LoongArchCPU, 32),
  VMSTATE_UINTTL(env.pc, LoongArchCPU),
  
@@ -193,7 +193,7 @@ const VMStateDescription vmstate_loongarch_cpu = {
  
  VMSTATE_END_OF_LIST()

  },
-.subsections = (const VMStateDescription*[]) {
+.subsections = (const VMStateDescription * const []) {
  _fpu,
  _lsx,
  _lasx,

Re: [PATCH v4 0/4] Support RISC-V IOPMP

2023-12-20 Thread Ethan Chen via

On Mon, Dec 18, 2023 at 02:18:58PM +1000, Alistair Francis wrote:
> On Wed, Nov 22, 2023 at 3:36 PM Ethan Chen via  wrote:
> >
> > This series implements IOPMP specification v1.0.0-draft4 rapid-k model.
> > The specification url:
> > https://github.com/riscv-non-isa/iopmp-spec/blob/main/riscv_iopmp_specification.pdf
> >
> > When IOPMP is enabled, a DMA device ATCDMAC300 is added to RISC-V virt
> > platform. This DMA device is connected to the IOPMP and has the 
> > functionalities
> 
> I don't think we want to add an Andes DMA device to the virt machine.
> 
> I can't even find the spec for the ATCDMAC300, which isn't great
> 
> Alistair

Since the IOPMP does not take effect when there is no other device connects to
IOPMP, I think it is necessary to have a DMA device for IOPMP demonstration.

Do you have any suggestions for supporting IOPMP on RISC-V virt machine?

Thanks,
Ethan Chen

> 
> > required by IOPMP, including:
> > - Support setup the connection to IOPMP
> > - Support asynchronous I/O to handle stall transactions
> > - Send transaction information
> >
> > IOPMP takes a transaction which partially match an entry as a partially hit
> > error. The transaction size is depending on source device, destination 
> > device
> > and bus.
> >
> > Source device can send a transaction_info to IOPMP. IOPMP will check 
> > partially
> > hit by transaction_info. If source device does not send a transaction_info,
> > IOPMP checks information in IOMMU and dose not check partially hit.
> >
> > Changes for v4:
> >
> >   - Add descriptions of IOPMP and ATCDMAC300
> >   - Refine coding style and comments
> >   - config XILINX_AXI does not include file stream.c but selects config 
> > STREAM
> > instead.
> >   - ATCDMAC300: INT_STATUS is write 1 clear per bit
> > Rename iopmp_address_sink to 
> > transcation_info_sink
> >   - IOPMP: Refine error message and remove unused variable
> >   - VIRT: Document new options
> > atcdmac300 is only added when iopmp is enabled
> >   serial setting should not be changed
> >
> > Ethan Chen (4):
> >   hw/core: Add config stream
> >   Add RISC-V IOPMP support
> >   hw/dma: Add Andes ATCDMAC300 support
> >   hw/riscv/virt: Add IOPMP support
> >
> >  docs/system/riscv/virt.rst|  11 +
> >  hw/Kconfig|   1 +
> >  hw/core/Kconfig   |   3 +
> >  hw/core/meson.build   |   2 +-
> >  hw/dma/Kconfig|   4 +
> >  hw/dma/atcdmac300.c   | 566 ++
> >  hw/dma/meson.build|   1 +
> >  hw/misc/Kconfig   |   4 +
> >  hw/misc/meson.build   |   1 +
> >  hw/misc/riscv_iopmp.c | 966 ++
> >  hw/riscv/Kconfig  |   2 +
> >  hw/riscv/virt.c   |  65 ++
> >  include/hw/dma/atcdmac300.h   | 180 
> >  include/hw/misc/riscv_iopmp.h | 341 +++
> >  .../hw/misc/riscv_iopmp_transaction_info.h|  28 +
> >  include/hw/riscv/virt.h   |  10 +-
> >  16 files changed, 2183 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/dma/atcdmac300.c
> >  create mode 100644 hw/misc/riscv_iopmp.c
> >  create mode 100644 include/hw/dma/atcdmac300.h
> >  create mode 100644 include/hw/misc/riscv_iopmp.h
> >  create mode 100644 include/hw/misc/riscv_iopmp_transaction_info.h
> >
> > --
> > 2.34.1
> >
> >

Re: [PATCH v2] qdev: Report an error for machine without HotplugHandler

On 2023/12/21 1:46, Zhao Liu wrote:

Hi Markus,

On Wed, Dec 20, 2023 at 08:53:21AM +0100, Markus Armbruster wrote:

Date: Wed, 20 Dec 2023 08:53:21 +0100
From: Markus Armbruster 
Subject: Re: [PATCH v2] qdev: Report an error for machine without
  HotplugHandler

Akihiko Odaki  writes:

On 2023/12/18 23:02, Markus Armbruster wrote:

Akihiko Odaki  writes:

On 2023/12/11 15:51, Markus Armbruster wrote:

Akihiko Odaki  writes:

The HotplugHandler of the machine will be used when the parent bus does
not exist, but the machine may not have one. Report an error in such a
case instead of aborting.

Fixes: 7716b8ca74 ("qdev: HotplugHandler: Add support for unplugging BUS-less 
devices")
Signed-off-by: Akihiko Odaki 

Do you have a reproducer for the crash?

---
Changes in v2:
- Fixed indention.
- Link to v1: 
https://lore.kernel.org/r/20231202-bus-v1-1-f7540e3a8...@daynix.com
---
system/qdev-monitor.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index a13db763e5..5fe5d49c20 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -927,9 +927,16 @@ void qdev_unplug(DeviceState *dev, Error **errp)

void qdev_unplug(DeviceState *dev, Error **errp)
{
DeviceClass *dc = DEVICE_GET_CLASS(dev);
HotplugHandler *hotplug_ctrl;
HotplugHandlerClass *hdc;
Error *local_err = NULL;
if (qdev_unplug_blocked(dev, errp)) {
return;
}
if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
return;
}
if (!dc->hotpluggable) {
error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
   object_get_typename(OBJECT(dev)));
return;
}
if (!migration_is_idle() && !dev->allow_unplug_during_migration) {
error_setg(errp, "device_del not allowed while migrating");
return;
}

   qdev_hot_removed = true;
  hotplug_ctrl = qdev_get_hotplug_handler(dev);
-/* hotpluggable device MUST have HotplugHandler, if it doesn't
- * then something is very wrong with it */
-g_assert(hotplug_ctrl);
+if (!hotplug_ctrl) {
+/*
+ * hotpluggable bus MUST have HotplugHandler, if it doesn't
+ * then something is very wrong with it
+ */
+assert(!dev->parent_bus);
+
+error_setg(errp, "The machine does not support hotplugging for a device 
without parent bus");
+return;
+}

Extended version of my question above: what are the devices where
qdev_get_hotplug_handler(dev) returns null here?

Start a VM: qemu-system-aarch64 -M virt -nographic
Run the following on its HMP: device_del /machine/unattached/device[0]

It tries to unplug cortex-a15-arm-cpu and crashes.

This device has no parent bus (dev->parent_bus is null), but is marked
hot-pluggable (dc->hotpluggable is true).  Question for somebody
familiar with the hot-plug machinery: is this sane?

Setting hotpluggable false for each device without bus_type gives the same 
effect, but is error-prone.

Having hotpluggable = true when the device cannot be hot-plugged is
*wrong*.  You might be able to paper over the wrongness so the code
works anyway, but nothing good can come out of lying to developers
trying to understand how the code works.

Three ideas to avoid the lying:

1. default hotpluggable to bus_type != NULL.

I don't have an idea to achieve that. Currently bus_type is set after 
hotpluggable.

2. assert(dc->bus_type || !dc->hotpluggable) in a suitable spot.

It results in abortion and doesn't improve the situation.

3. Change the meaning of hotpluggable, and rename it to reflect its new
meaning.  Requires a careful reading of its uses.  I wouldn't go there.

I don't have an idea for such a naming.

So I'm stuck with the current proposal. It suppresses abortion at least. 
Any alternative idea is welcome.

What about 4 (or maybe 3.1) - droping this hotpluggable flag and just use a
helper (like qbus) to check if device is hotpluggable?

This removes the confusion of that flag and also reduces the number of
configuration items for DeviceState that require developer attention.
A simple helper is as follows:

Some devices simply doesn't support hotplugging even if the bus 
supports. virtio-gpu-pci doesn't support hotplugging because the display 
infrastructure cannot handle hotplugging, for example.

Regards,
Akihiko Odaki

Re: [PATCH v2] qdev: Report an error for machine without HotplugHandler

2023-12-20 Thread Markus Armbruster

Zhao Liu  writes:

> Hi Markus,
>
> On Wed, Dec 20, 2023 at 08:53:21AM +0100, Markus Armbruster wrote:
>> Date: Wed, 20 Dec 2023 08:53:21 +0100
>> From: Markus Armbruster 
>> Subject: Re: [PATCH v2] qdev: Report an error for machine without
>>  HotplugHandler
>> 
>> Having hotpluggable = true when the device cannot be hot-plugged is
>> *wrong*.  You might be able to paper over the wrongness so the code
>> works anyway, but nothing good can come out of lying to developers
>> trying to understand how the code works.
>> 
>> Three ideas to avoid the lying:
>> 
>> 1. default hotpluggable to bus_type != NULL.
>> 
>> 2. assert(dc->bus_type || !dc->hotpluggable) in a suitable spot.
>> 
>> 3. Change the meaning of hotpluggable, and rename it to reflect its new
>> meaning.  Requires a careful reading of its uses.  I wouldn't go there.
>> 
>
> What about 4 (or maybe 3.1) - droping this hotpluggable flag and just use a
> helper (like qbus) to check if device is hotpluggable?
>
> This removes the confusion of that flag and also reduces the number of
> configuration items for DeviceState that require developer attention.
> A simple helper is as follows:
>
> static inline bool qdev_is_hotpluggable(DeviceState *dev)
> {
> /*
>  * Many Machines don't implement qdev_hotplug_allowed().
>  *
>  * TODO: Once all of them complete missing qdev_hotplug_allowed(),
>  *   use qdev_hotplug_allowed() here.
>  */
> bool hotpluggable = !!qdev_get_machine_hotplug_handler(dev);
>
> if (!hotpluggable && dev->parent_bus) {
> hotpluggable = qbus_is_hotpluggable(dev->parent_bus);
> }
>
> return hotpluggable;
> }

Worth exploring, I think.

Re: [PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd


On 2023/12/19 16:53, Huang Rui wrote:

From: Robert Beckett 

This relies on a virglrenderer change to include the dmabuf fd when
returning resource info.

Signed-off-by: Robert Beckett 
Signed-off-by: Huang Rui 
---

Changes in v6:
- Add scanout_blob function for virtio-gpu-virgl.
- Update for new virgl_gpu_resource.

  hw/display/virtio-gpu-virgl.c  | 104 +
  hw/display/virtio-gpu.c|   4 +-
  include/hw/virtio/virtio-gpu.h |   6 ++
  3 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index c523a6717a..c384225a98 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -18,6 +18,7 @@
  #include "hw/virtio/virtio.h"
  #include "hw/virtio/virtio-gpu.h"
  #include "hw/virtio/virtio-gpu-bswap.h"
+#include "hw/virtio/virtio-gpu-pixman.h"
  
  #include "ui/egl-helpers.h"
  
@@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,

  object_unparent(OBJECT(mr));
  }
  
+static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,

+   struct virtio_gpu_ctrl_command *cmd)
+{
+struct virgl_gpu_resource *vres;
+struct virtio_gpu_framebuffer fb = { 0 };
+struct virtio_gpu_set_scanout_blob ss;
+struct virgl_renderer_resource_info info;
+uint64_t fbend;
+
+VIRTIO_GPU_FILL_CMD(ss);
+virtio_gpu_scanout_blob_bswap();
+trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
+  ss.r.width, ss.r.height, ss.r.x,
+  ss.r.y);
+
+if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
+  __func__, ss.scanout_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
+return;
+}
+
+if (ss.resource_id == 0) {
+virtio_gpu_disable_scanout(g, ss.scanout_id);
+return;
+}
+
+if (ss.width < 16 ||
+ss.height < 16 ||
+ss.r.x + ss.r.width > ss.width ||
+ss.r.y + ss.r.height > ss.height) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
+  " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
+  __func__, ss.scanout_id, ss.resource_id,
+  ss.r.x, ss.r.y, ss.r.width, ss.r.height,
+  ss.width, ss.height);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
+
+if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {


Shouldn't OpenGL always be available for virgl?


+qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without 
GL!\n", __func__);
+return;
+}
+
+vres = virgl_gpu_find_resource(g, ss.resource_id);
+if (!vres) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: illegal resource specified %d\n",
+  __func__, ss.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+if (virgl_renderer_resource_get_info(ss.resource_id, )) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: illegal virgl resource specified %d\n",
+  __func__, ss.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+if (!vres->res.dmabuf_fd && info.fd)
+vres->res.dmabuf_fd = info.fd;
+
+fb.format = virtio_gpu_get_pixman_format(ss.format);
+if (!fb.format) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: host couldn't handle guest format %d\n",
+  __func__, ss.format);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
+
+fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
+fb.width = ss.width;
+fb.height = ss.height;
+fb.stride = ss.strides[0];
+fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
+
+fbend = fb.offset;
+fbend += fb.stride * (ss.r.height - 1);
+fbend += fb.bytes_pp * ss.r.width;
+if (fbend > vres->res.blob_size) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: fb end out of range\n",
+  __func__);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
+
+g->parent_obj.enable = 1;
+if (virtio_gpu_update_dmabuf(g, ss.scanout_id, >res,
+ , )) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: failed to update dmabuf\n", __func__);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
+virtio_gpu_update_scanout(g, ss.scanout_id, >res, );
+}
+
  #endif /* HAVE_VIRGL_RESOURCE_BLOB */
  
  void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,

@@ -807,6 +908,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,

Re: [PATCH v3 09/70] physmem: Introduce ram_block_convert_range() for page conversion

2023-12-20 Thread Xiaoyao Li


On 12/8/2023 7:52 PM, David Hildenbrand wrote:

On 08.12.23 08:59, Xiaoyao Li wrote:

On 11/18/2023 5:03 AM, Isaku Yamahata wrote:

On Wed, Nov 15, 2023 at 02:14:18AM -0500,
Xiaoyao Li  wrote:


It's used for discarding opposite memory after memory conversion, for
confidential guest.

When page is converted from shared to private, the original shared
memory can be discarded via ram_block_discard_range();

When page is converted from private to shared, the original private
memory is back'ed by guest_memfd. Introduce
ram_block_discard_guest_memfd_range() for discarding memory in
guest_memfd.

Originally-from: Isaku Yamahata 
Codeveloped-by: Xiaoyao Li 
Signed-off-by: Xiaoyao Li 
---
   include/exec/cpu-common.h |  2 ++
   system/physmem.c  | 50 
+++

   2 files changed, 52 insertions(+)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 41115d891940..de728a18eef2 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -175,6 +175,8 @@ typedef int (RAMBlockIterFunc)(RAMBlock *rb, 
void *opaque);

   int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
   int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t 
length);
+int ram_block_convert_range(RAMBlock *rb, uint64_t start, size_t 
length,

+    bool shared_to_private);
   #endif
diff --git a/system/physmem.c b/system/physmem.c
index ddfecddefcd6..cd6008fa09ad 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3641,6 +3641,29 @@ err:
   return ret;
   }
+static int ram_block_discard_guest_memfd_range(RAMBlock *rb, 
uint64_t start,

+   size_t length)
+{
+    int ret = -1;
+
+#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
+    ret = fallocate(rb->guest_memfd, FALLOC_FL_PUNCH_HOLE | 
FALLOC_FL_KEEP_SIZE,

+    start, length);
+
+    if (ret) {
+    ret = -errno;
+    error_report("%s: Failed to fallocate %s:%" PRIx64 " +%zx 
(%d)",

+ __func__, rb->idstr, start, length, ret);
+    }
+#else
+    ret = -ENOSYS;
+    error_report("%s: fallocate not available %s:%" PRIx64 " +%zx 
(%d)",

+ __func__, rb->idstr, start, length, ret);
+#endif
+
+    return ret;
+}
+
   bool ramblock_is_pmem(RAMBlock *rb)
   {
   return rb->flags & RAM_PMEM;
@@ -3828,3 +3851,30 @@ bool ram_block_discard_is_required(void)
   return qatomic_read(_block_discard_required_cnt) ||
  
qatomic_read(_block_coordinated_discard_required_cnt);

   }
+
+int ram_block_convert_range(RAMBlock *rb, uint64_t start, size_t 
length,

+    bool shared_to_private)
+{
+    if (!rb || rb->guest_memfd < 0) {
+    return -1;
+    }
+
+    if (!QEMU_PTR_IS_ALIGNED(start, qemu_host_page_size) ||
+    !QEMU_PTR_IS_ALIGNED(length, qemu_host_page_size)) {
+    return -1;
+    }
+
+    if (!length) {
+    return -1;
+    }
+
+    if (start + length > rb->max_length) {
+    return -1;
+    }
+
+    if (shared_to_private) {
+    return ram_block_discard_range(rb, start, length);
+    } else {
+    return ram_block_discard_guest_memfd_range(rb, start, length);
+    }
+}


Originally this function issued KVM_SET_MEMORY_ATTRIBUTES, the 
function name
mad sense. But now it doesn't, and it issues only punch hole. We 
should rename

it to represent what it actually does. discard_range?


ram_block_discard_range() already exists for non-guest-memfd memory 
discard.


I cannot come up with a proper name. e.g.,
ram_block_discard_opposite_range() while *opposite* seems unclear.

Do you have any better idea?


Having some indication that this is about "guest_memfd" back and forth 
switching/conversion will make sense. But I'm also not able to come up 
with a better name.


Maybe have two functions:

ram_block_activate_guest_memfd_range
ram_block_deactivate_guest_memfd_range



finally, I decide to drop this function and expose 
ram_block_discard_guest_memfd_range() instead. So caller can call the 
ram_block_discard_*() on its own.

Re: [PATCH v3 06/70] kvm: Introduce support for memory_attributes

2023-12-20 Thread Xiaoyao Li


On 12/12/2023 9:56 PM, Wang, Wei W wrote:

On Wednesday, November 15, 2023 3:14 PM, Xiaoyao Li wrote:

Introduce the helper functions to set the attributes of a range of memory to
private or shared.

This is necessary to notify KVM the private/shared attribute of each gpa range.
KVM needs the information to decide the GPA needs to be mapped at hva-
based shared memory or guest_memfd based private memory.

Signed-off-by: Xiaoyao Li 
---
  accel/kvm/kvm-all.c  | 42 ++
  include/sysemu/kvm.h |  3 +++
  2 files changed, 45 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
69afeb47c9c0..76e2404d54d2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -102,6 +102,7 @@ bool kvm_has_guest_debug;  static int kvm_sstep_flags;
static bool kvm_immediate_exit;  static bool kvm_guest_memfd_supported;
+static uint64_t kvm_supported_memory_attributes;
  static hwaddr kvm_max_slot_size = ~0;

  static const KVMCapabilityInfo kvm_required_capabilites[] = { @@ -1305,6
+1306,44 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size)
  kvm_max_slot_size = max_slot_size;
  }

+static int kvm_set_memory_attributes(hwaddr start, hwaddr size,
+uint64_t attr) {
+struct kvm_memory_attributes attrs;
+int r;
+
+attrs.attributes = attr;
+attrs.address = start;
+attrs.size = size;
+attrs.flags = 0;
+
+r = kvm_vm_ioctl(kvm_state, KVM_SET_MEMORY_ATTRIBUTES, );
+if (r) {
+warn_report("%s: failed to set memory (0x%lx+%#zx) with attr 0x%lx
error '%s'",
+ __func__, start, size, attr, strerror(errno));
+}
+return r;
+}
+
+int kvm_set_memory_attributes_private(hwaddr start, hwaddr size) {
+if (!(kvm_supported_memory_attributes &
KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
+error_report("KVM doesn't support PRIVATE memory attribute\n");
+return -EINVAL;
+}
+
+return kvm_set_memory_attributes(start, size,
+KVM_MEMORY_ATTRIBUTE_PRIVATE); }
+
+int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size) {
+if (!(kvm_supported_memory_attributes &
KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
+error_report("KVM doesn't support PRIVATE memory attribute\n");
+return -EINVAL;
+}


Duplicate code in kvm_set_memory_attributes_shared/private.
Why not move the check into kvm_set_memory_attributes?


Because it's not easy to put the check into there.

Both setting and clearing one bit require the capability check. If 
moving the check into kvm_set_memory_attributes(), the check of 
KVM_MEMORY_ATTRIBUTE_PRIVATE will have to become unconditionally, which 
is not aligned to the function name because the name is not restricted 
to shared/private attribute only.

Re: [PATCH v6 08/11] virtio-gpu: Resource UUID


On 2023/12/19 16:53, Huang Rui wrote:

From: Antonio Caggiano 

Enable resource UUID feature and implement command resource assign UUID.
This is done by introducing a hash table to map resource IDs to their
UUIDs.


What about putting QemuUUID in struct virtio_gpu_simple_resource?

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands


On 2023/12/19 16:53, Huang Rui wrote:

From: Antonio Caggiano 

Support BLOB resources creation, mapping and unmapping by calling the
new stable virglrenderer 0.10 interface. Only enabled when available and
via the blob config. E.g. -device virtio-vga-gl,blob=true

Signed-off-by: Antonio Caggiano 
Signed-off-by: Dmitry Osipenko 
Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Huang Rui 
---

Changes in v6:
- Use new struct virgl_gpu_resource.
- Unmap, unref and destroy the resource only after the memory region
   has been completely removed.
- In unref check whether the resource is still mapped.
- In unmap_blob check whether the resource has been already unmapped.
- Fix coding style

  hw/display/virtio-gpu-virgl.c | 274 +-
  hw/display/virtio-gpu.c   |   4 +-
  meson.build   |   4 +
  3 files changed, 276 insertions(+), 6 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index faab374336..5a3a292f79 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,7 @@
  #include "trace.h"
  #include "hw/virtio/virtio.h"
  #include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
  
  #include "ui/egl-helpers.h"
  
@@ -24,8 +25,62 @@
  
  struct virgl_gpu_resource {

  struct virtio_gpu_simple_resource res;
+uint32_t ref;
+VirtIOGPU *g;
+
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+/* only blob resource needs this region to be mapped as guest mmio */
+MemoryRegion *region;


Why not just embed MemoryRegion into struct virgl_gpu_resource instead 
of having a pointer?



+#endif
  };
  
+static void vres_get_ref(struct virgl_gpu_resource *vres)

+{
+uint32_t ref;
+
+ref = qatomic_fetch_inc(>ref);
+g_assert(ref < INT_MAX);
+}
+
+static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
+{
+struct virtio_gpu_simple_resource *res;
+VirtIOGPU *g;
+
+if (!vres) {
+return;
+}
+
+g = vres->g;
+res = >res;
+QTAILQ_REMOVE(>reslist, res, next);
+virtio_gpu_cleanup_mapping(g, res);
+g_free(vres);
+}
+
+static void virgl_resource_unref(struct virgl_gpu_resource *vres)
+{
+struct virtio_gpu_simple_resource *res;
+
+if (!vres) {
+return;
+}
+
+res = >res;
+virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
+virgl_renderer_resource_unref(res->resource_id);
+}
+
+static void vres_put_ref(struct virgl_gpu_resource *vres)
+{
+g_assert(vres->ref > 0);
+
+if (qatomic_fetch_dec(>ref) == 1) {
+virgl_resource_unref(vres);
+virgl_resource_destroy(vres);
+}
+}
+
  static struct virgl_gpu_resource *
  virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
  {
@@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
 c2d.width, c2d.height);
  
  vres = g_new0(struct virgl_gpu_resource, 1);

+vres_get_ref(vres);
+vres->g = g;
  vres->res.width = c2d.width;
  vres->res.height = c2d.height;
  vres->res.format = c2d.format;
@@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
 c3d.width, c3d.height, c3d.depth);
  
  vres = g_new0(struct virgl_gpu_resource, 1);

+vres_get_ref(vres);
+vres->g = g;
  vres->res.width = c3d.width;
  vres->res.height = c3d.height;
  vres->res.format = c3d.format;
@@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
  return;
  }
  
-virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);

-virgl_renderer_resource_unref(unref.resource_id);
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+if (vres->region) {
+VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+MemoryRegion *mr = vres->region;
+
+warn_report("%s: blob resource %d not unmapped",
+__func__, unref.resource_id);
+vres->region = NULL;
+memory_region_set_enabled(mr, false);
+memory_region_del_subregion(>hostmem, mr);
+object_unparent(OBJECT(mr));
+}
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
  
-QTAILQ_REMOVE(>reslist, >res, next);

-virtio_gpu_cleanup_mapping(g, >res);
-g_free(vres);
+vres_put_ref(vres);


What will happen if the guest consecutively requests 
VIRTIO_GPU_CMD_RESOURCE_UNREF twice for a mapped resource?

Re: [PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions


On 2023/12/19 16:53, Huang Rui wrote:

From: Xenia Ragiadakou 

When the memory region has a different life-cycle from that of her parent,
could be automatically released, once has been unparent and once all of her
references have gone away, via the object's free callback.

However, currently, the address space subsystem keeps references to the
memory region without first incrementing its object's reference count.
As a result, the automatic deallocation of the object, not taking into
account those references, results in use-after-free memory corruption.

More specifically, reference to the memory region is kept in flatview
ranges. If the reference count of the memory region is not incremented,
flatview_destroy(), that is asynchronous, may be called after memory
region's destruction. If the reference count of the memory region is
incremented, memory region's destruction will take place after
flatview_destroy() has released its references.

This patch increases the reference count of an owned memory region object
on each memory_region_ref() and decreases it on each memory_region_unref().


Why not pass the memory region itself as the owner parameter of 
memory_region_init_ram_ptr()?

Re: [PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure