Re: [PATCH 1/2] target/s390x: Fix EXECUTE of relative long instructions
On 3/13/23 16:38, Ilya Leoshkevich wrote: The code uses the wrong base for relative addressing: it should use the target instruction address and not the EXECUTE's address. Fix by storing the target instruction address in the new CPUS390XState member and loading it from the code generated by in2_ri2(). Reported-by: Nina Schoetterl-Glausch Signed-off-by: Ilya Leoshkevich --- target/s390x/cpu.h| 1 + target/s390x/tcg/mem_helper.c | 1 + target/s390x/tcg/translate.c | 10 +- 3 files changed, 11 insertions(+), 1 deletion(-) Good solution, reading the value from env. Reviewed-by: Richard Henderson r~
[PULL 2/3] ps2: Don't send key release event for Lang1, Lang2 keys
From: Ross Lagerwall
The scancodes for the Lang1 and Lang2 keys (i.e. Hangeul, Hanja) are
special since they already have the 0x80 bit set which is commonly used
to indicate a key release in AT set 1. Reportedly, real hardware does
not send a key release scancode. So, skip sending a release for these
keys. This ensures that Windows behaves correctly and interprets it as a
single keypress rather than two consecutive keypresses.
Signed-off-by: Ross Lagerwall
Signed-off-by: Daniel P. Berrangé
---
hw/input/ps2.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 3253ab6a92..45af76a837 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -402,6 +402,9 @@ static void ps2_keyboard_event(DeviceState *dev,
QemuConsole *src,
ps2_put_keycode(s, 0xaa);
}
}
+} else if ((qcode == Q_KEY_CODE_LANG1 || qcode == Q_KEY_CODE_LANG2)
+ && !key->down) {
+/* Ignore release for these keys */
} else {
if (qcode < qemu_input_map_qcode_to_atset1_len) {
keycode = qemu_input_map_qcode_to_atset1[qcode];
@@ -497,6 +500,9 @@ static void ps2_keyboard_event(DeviceState *dev,
QemuConsole *src,
ps2_put_keycode(s, 0x12);
}
}
+} else if ((qcode == Q_KEY_CODE_LANG1 || qcode == Q_KEY_CODE_LANG2) &&
+ !key->down) {
+/* Ignore release for these keys */
} else {
if (qcode < qemu_input_map_qcode_to_atset2_len) {
keycode = qemu_input_map_qcode_to_atset2[qcode];
--
2.39.2
[PULL 1/3] Add qemu qcode support for keys F13 to F24
From: Willem van de Velde To be able to use the function keys F13 to F24 these should be defined in de keycodemapdb and added to the qapi. The keycodemapdb is updated in its own repository, this patch enables the use of those keys within qemu. Signed-off-by: Willem van de Velde Signed-off-by: Daniel P. Berrangé --- qapi/ui.json| 15 ++- ui/keycodemapdb | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/qapi/ui.json b/qapi/ui.json index 0abba3e930..98322342f7 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -886,6 +886,19 @@ # @lang1: since 6.1 # @lang2: since 6.1 # +# @f13: since 8.0 +# @f14: since 8.0 +# @f15: since 8.0 +# @f16: since 8.0 +# @f17: since 8.0 +# @f18: since 8.0 +# @f19: since 8.0 +# @f20: since 8.0 +# @f21: since 8.0 +# @f22: since 8.0 +# @f23: since 8.0 +# @f24: since 8.0 +# # 'sysrq' was mistakenly added to hack around the fact that # the ps2 driver was not generating correct scancodes sequences # when 'alt+print' was pressed. This flaw is now fixed and the @@ -918,7 +931,7 @@ 'volumeup', 'volumedown', 'mediaselect', 'mail', 'calculator', 'computer', 'ac_home', 'ac_back', 'ac_forward', 'ac_refresh', 'ac_bookmarks', -'lang1', 'lang2' ] } +'lang1', 'lang2','f13','f14','f15','f16','f17','f18','f19','f20','f21','f22','f23','f24' ] } ## # @KeyValueKind: diff --git a/ui/keycodemapdb b/ui/keycodemapdb index d21009b1c9..f5772a62ec 16 --- a/ui/keycodemapdb +++ b/ui/keycodemapdb @@ -1 +1 @@ -Subproject commit d21009b1c9f94b740ea66be8e48a1d8ad8124023 +Subproject commit f5772a62ec52591ff6870b7e8ef32482371f22c6 -- 2.39.2
[PULL 0/3] Misc next patches
The following changes since commit 5cfda4ce79dd455f1726874a555260a70f84b2ec: Merge tag 'pull-request-2023-03-13' of https://gitlab.com/thuth/qemu into staging (2023-03-13 17:09:33 +) are available in the Git repository at: https://gitlab.com/berrange/qemu tags/misc-next-pull-request for you to fetch changes up to c3a2c84ae3c1d5483ec30731321a674797dc5203: io/channel-tls: plug memory leakage on GSource (2023-03-14 13:41:21 +) Miscellaneous fixes * Avoid memory leak in TLS GSource usage * Avoid sending key releases for lang1/lang2 keys in ps2 keyboard * Add missing key name constants for F13-F24 keys Matheus Tavares Bernardino (1): io/channel-tls: plug memory leakage on GSource Ross Lagerwall (1): ps2: Don't send key release event for Lang1, Lang2 keys Willem van de Velde (1): Add qemu qcode support for keys F13 to F24 hw/input/ps2.c | 6 ++ io/channel-tls.c | 1 + qapi/ui.json | 15 ++- ui/keycodemapdb | 2 +- 4 files changed, 22 insertions(+), 2 deletions(-) -- 2.39.2
[PULL 3/3] io/channel-tls: plug memory leakage on GSource
From: Matheus Tavares Bernardino This leakage can be seen through test-io-channel-tls: $ ../configure --target-list=aarch64-softmmu --enable-sanitizers $ make ./tests/unit/test-io-channel-tls $ ./tests/unit/test-io-channel-tls Indirect leak of 104 byte(s) in 1 object(s) allocated from: #0 0x7f81d1725808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144 #1 0x7f81d135ae98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98) #2 0x55616c5d4c1b in object_new_with_propv ../qom/object.c:795 #3 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768 #4 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70 #5 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158 #6 0x7f81d137d58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d) Indirect leak of 32 byte(s) in 1 object(s) allocated from: #0 0x7f81d1725a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153 #1 0x7f81d1472a20 in gnutls_dh_params_init (/lib/x86_64-linux-gnu/libgnutls.so.30+0x46a20) #2 0x55616c6485ff in qcrypto_tls_creds_x509_load ../crypto/tlscredsx509.c:634 #3 0x55616c648ba2 in qcrypto_tls_creds_x509_complete ../crypto/tlscredsx509.c:694 #4 0x55616c5e1fea in user_creatable_complete ../qom/object_interfaces.c:28 #5 0x55616c5d4c8c in object_new_with_propv ../qom/object.c:807 #6 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768 #7 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70 #8 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158 #9 0x7f81d137d58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d) ... SUMMARY: AddressSanitizer: 49143 byte(s) leaked in 184 allocation(s). The docs for `g_source_add_child_source(source, child_source)` says "source will hold a reference on child_source while child_source is attached to it." Therefore, we should unreference the child source at `qio_channel_tls_read_watch()` after attaching it to `source`. With this change, ./tests/unit/test-io-channel-tls shows no leakages. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Matheus Tavares Bernardino Signed-off-by: Daniel P. Berrangé --- io/channel-tls.c | 1 + 1 file changed, 1 insertion(+) diff --git a/io/channel-tls.c b/io/channel-tls.c index 8052945ba0..5a7a3d48d6 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -446,6 +446,7 @@ qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source) object_ref(OBJECT(tioc)); g_source_add_child_source(source, child); +g_source_unref(child); } static GSource *qio_channel_tls_create_watch(QIOChannel *ioc, -- 2.39.2
Re: [PATCH v17 06/12] s390x/cpu topology: interception of PTF instruction
I am currently developing tests under avocado to help debugging.
And... it helps.
There is a bug here in s390_topology_set_cpus_entitlement for dedicated
CPUs.
On 3/9/23 13:15, Pierre Morel wrote:
[...]
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -87,6 +87,84 @@ static void s390_topology_init(MachineState *ms)
QTAILQ_INSERT_HEAD(_topology.list, entry, next);
}
+/**
+ * s390_topology_set_cpus_entitlement:
+ * @polarization: polarization requested by the caller
+ *
+ * On hotplug or when changing CPU attributes the shadow_entitlement
+ * is set to hold the entitlement used on a vertical polarization.
+ * When polarization is horizontal, the entitlement is horizontal too.
+ */
+static void s390_topology_set_cpus_entitlement(int polarization)
+{
+CPUState *cs;
+
+CPU_FOREACH(cs) {
+CPUS390XState *env = _CPU(cs)->env;
+
+if (polarization == S390_CPU_POLARIZATION_HORIZONTAL) {
+env->entitlement = S390_CPU_ENTITLEMENT_HORIZONTAL;
+} else {
+env->entitlement = env->shadow_entitlement;
+}
+}
+}
This should be something like:
static void s390_topology_set_cpus_entitlement(void)
{
CPUState *cs;
CPU_FOREACH(cs) {
CPUS390XState *env = _CPU(cs)->env;
if (s390_topology.polarization ==
S390_CPU_POLARIZATION_HORIZONTAL) {
env->entitlement = S390_CPU_ENTITLEMENT_HORIZONTAL;
} else if (env->entitlement == S390_CPU_ENTITLEMENT_HORIZONTAL) {
if (env->dedicated) {
env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
} else {
env->entitlement = env->shadow_entitlement;
}
}
}
}
Sorry.
I provide a new series including the avocado tests.
regards,
Pierre
Re: [PATCH v5 04/16] hw/9pfs: Implement Windows specific xxxdir() APIs
On Monday, February 20, 2023 11:08:03 AM CET Bin Meng wrote:
> From: Guohuai Shi
>
> This commit implements Windows specific xxxdir() APIs for safety
> directory access.
That comment is seriously too short for this patch.
1. You should describe the behaviour implementation that you have chosen and
why you have chosen it.
2. Like already said in the previous version of the patch, you should place a
link to the discussion we had on this issue.
> Signed-off-by: Guohuai Shi
> Signed-off-by: Bin Meng
> ---
>
> hw/9pfs/9p-util.h | 6 +
> hw/9pfs/9p-util-win32.c | 443
> 2 files changed, 449 insertions(+)
>
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 0f159fb4ce..c1c251fbd1 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -141,6 +141,12 @@ int unlinkat_win32(int dirfd, const char *pathname, int
> flags);
> int statfs_win32(const char *root_path, struct statfs *stbuf);
> int openat_dir(int dirfd, const char *name);
> int openat_file(int dirfd, const char *name, int flags, mode_t mode);
> +DIR *opendir_win32(const char *full_file_name);
> +int closedir_win32(DIR *pDir);
> +struct dirent *readdir_win32(DIR *pDir);
> +void rewinddir_win32(DIR *pDir);
> +void seekdir_win32(DIR *pDir, long pos);
> +long telldir_win32(DIR *pDir);
> #endif
>
> static inline void close_preserve_errno(int fd)
> diff --git a/hw/9pfs/9p-util-win32.c b/hw/9pfs/9p-util-win32.c
> index a99d579a06..e9408f3c45 100644
> --- a/hw/9pfs/9p-util-win32.c
> +++ b/hw/9pfs/9p-util-win32.c
> @@ -37,6 +37,16 @@
> *Windows does not support opendir, the directory fd is created by
> *CreateFile and convert to fd by _open_osfhandle(). Keep the fd open
> will
> *lock and protect the directory (can not be modified or replaced)
> + *
> + * 5. Neither Windows native APIs, nor MinGW provide a POSIX compatible API
> for
> + *acquiring directory entries in a safe way. Calling those APIs (native
> + *_findfirst() and _findnext() or MinGW's readdir(), seekdir() and
> + *telldir()) directly can lead to an inconsistent state if directory is
> + *modified in between, e.g. the same directory appearing more than once
> + *in output, or directories not appearing at all in output even though
> they
> + *were neither newly created nor deleted. POSIX does not define what
> happens
> + *with deleted or newly created directories in between, but it
> guarantees a
> + *consistent state.
> */
>
> #include "qemu/osdep.h"
> @@ -51,6 +61,25 @@
>
> #define V9FS_MAGIC 0x53465039 /* string "9PFS" */
>
> +/*
> + * MinGW and Windows does not provide a safe way to seek directory while
> other
> + * thread is modifying the same directory.
> + *
> + * This structure is used to store sorted file id and ensure directory seek
> + * consistency.
> + */
> +struct dir_win32 {
> +struct dirent dd_dir;
> +uint32_t offset;
> +uint32_t total_entries;
> +HANDLE hDir;
> +uint32_t dir_name_len;
> +uint64_t dot_id;
> +uint64_t dot_dot_id;
> +uint64_t *file_id_list;
> +char dd_name[1];
> +};
> +
> /*
> * win32_error_to_posix - convert Win32 error to POSIX error number
> *
> @@ -977,3 +1006,417 @@ int qemu_mknodat(int dirfd, const char *filename,
> mode_t mode, dev_t dev)
> errno = ENOTSUP;
> return -1;
> }
> +
> +static int file_id_compare(const void *id_ptr1, const void *id_ptr2)
> +{
> +uint64_t id[2];
> +
> +id[0] = *(uint64_t *)id_ptr1;
> +id[1] = *(uint64_t *)id_ptr2;
> +
> +if (id[0] > id[1]) {
> +return 1;
> +} else if (id[0] < id[1]) {
> +return -1;
> +} else {
> +return 0;
> +}
> +}
> +
> +static int get_next_entry(struct dir_win32 *stream)
> +{
> +HANDLE hDirEntry = INVALID_HANDLE_VALUE;
> +char *entry_name;
> +char *entry_start;
> +FILE_ID_DESCRIPTOR fid;
> +DWORD attribute;
> +
> +if (stream->file_id_list[stream->offset] == stream->dot_id) {
> +strcpy(stream->dd_dir.d_name, ".");
> +return 0;
> +}
> +
> +if (stream->file_id_list[stream->offset] == stream->dot_dot_id) {
> +strcpy(stream->dd_dir.d_name, "..");
> +return 0;
> +}
> +
> +fid.dwSize = sizeof(fid);
> +fid.Type = FileIdType;
> +
> +fid.FileId.QuadPart = stream->file_id_list[stream->offset];
> +
> +hDirEntry = OpenFileById(stream->hDir, , GENERIC_READ,
> + FILE_SHARE_READ | FILE_SHARE_WRITE
> + | FILE_SHARE_DELETE,
> + NULL,
> + FILE_FLAG_BACKUP_SEMANTICS
> + | FILE_FLAG_OPEN_REPARSE_POINT);
What's the purpose of FILE_FLAG_OPEN_REPARSE_POINT here? As it's apparently
not obvious, please add a comment.
> +
> +if (hDirEntry == INVALID_HANDLE_VALUE) {
> +/*
> + * Not open it successfully, it may be deleted.
Wrong English. "Open
Re: [PATCH v4 4/6] hw/cxl: QMP based poison injection support
The 03/03/2023 15:09, Jonathan Cameron wrote:
> Inject poison using qmp command cxl-inject-poison to add an entry to the
> poison list.
>
> For now, the poison is not returned CXL.mem reads, but only via the
> mailbox command Get Poison List.
>
> See CXL rev 3.0, sec 8.2.9.8.4.1 Get Poison list (Opcode 4300h)
>
> Kernel patches to use this interface here:
> https://lore.kernel.org/linux-cxl/cover.1665606782.git.alison.schofi...@intel.com/
>
> To inject poison using qmp (telnet to the qmp port)
> { "execute": "qmp_capabilities" }
>
> { "execute": "cxl-inject-poison",
> "arguments": {
> "path": "/machine/peripheral/cxl-pmem0",
> "start": 2048,
> "length": 256
> }
> }
>
> Adjusted to select a device on your machine.
>
> Note that the poison list supported is kept short enough to avoid the
> complexity of state machine that is needed to handle the MORE flag.
>
> Signed-off-by: Jonathan Cameron
>
Reviewed-by: Fan Ni
> ---
> v4:
> - Widen the mask on Poison source (lower bits of the address)
>to allow for Vendor Defined. Change will make it easier to potentially
>add a means to inject such poison in the future. Today it has no
>impact.
> ---
> hw/cxl/cxl-mailbox-utils.c | 90 +
> hw/mem/cxl_type3.c | 56 +++
> hw/mem/cxl_type3_stubs.c| 6 +++
> include/hw/cxl/cxl_device.h | 20 +
> qapi/cxl.json | 18
> 5 files changed, 190 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 702e16ca20..25933cf62c 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -62,6 +62,8 @@ enum {
> #define GET_PARTITION_INFO 0x0
> #define GET_LSA 0x2
> #define SET_LSA 0x3
> +MEDIA_AND_POISON = 0x43,
> +#define GET_POISON_LIST0x0
> };
>
> /* 8.2.8.4.5.1 Command Return Codes */
> @@ -295,6 +297,10 @@ static CXLRetCode cmd_identify_memory_device(struct
> cxl_cmd *cmd,
> stq_le_p(>persistent_capacity, cxl_dstate->pmem_size /
> CXL_CAPACITY_MULTIPLIER);
> stq_le_p(>volatile_capacity, cxl_dstate->vmem_size /
> CXL_CAPACITY_MULTIPLIER);
> stl_le_p(>lsa_size, cvc->get_lsa_size(ct3d));
> +/* 256 poison records */
> +st24_le_p(id->poison_list_max_mer, 256);
> +/* No limit - so limited by main poison record limit */
> +stw_le_p(>inject_poison_limit, 0);
>
> *len = sizeof(*id);
> return CXL_MBOX_SUCCESS;
> @@ -384,6 +390,88 @@ static CXLRetCode cmd_ccls_set_lsa(struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * This is very inefficient, but good enough for now!
> + * Also the payload will always fit, so no need to handle the MORE flag and
> + * make this stateful. We may want to allow longer poison lists to aid
> + * testing that kernel functionality.
> + */
> +static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd,
> +CXLDeviceState *cxl_dstate,
> +uint16_t *len)
> +{
> +struct get_poison_list_pl {
> +uint64_t pa;
> +uint64_t length;
> +} QEMU_PACKED;
> +
> +struct get_poison_list_out_pl {
> +uint8_t flags;
> +uint8_t rsvd1;
> +uint64_t overflow_timestamp;
> +uint16_t count;
> +uint8_t rsvd2[0x14];
> +struct {
> +uint64_t addr;
> +uint32_t length;
> +uint32_t resv;
> +} QEMU_PACKED records[];
> +} QEMU_PACKED;
> +
> +struct get_poison_list_pl *in = (void *)cmd->payload;
> +struct get_poison_list_out_pl *out = (void *)cmd->payload;
> +CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +uint16_t record_count = 0, i = 0;
> +uint64_t query_start, query_length;
> +CXLPoisonList *poison_list = >poison_list;
> +CXLPoison *ent;
> +uint16_t out_pl_len;
> +
> +query_start = ldq_le_p(>pa);
> +/* 64 byte alignemnt required */
> +if (query_start & 0x3f) {
> +return CXL_MBOX_INVALID_INPUT;
> +}
> +query_length = ldq_le_p(>length) * 64;
> +
> +QLIST_FOREACH(ent, poison_list, node) {
> +/* Check for no overlap */
> +if (ent->start >= query_start + query_length ||
> +ent->start + ent->length <= query_start) {
> +continue;
> +}
> +record_count++;
> +}
> +out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> +assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> +memset(out, 0, out_pl_len);
> +QLIST_FOREACH(ent, poison_list, node) {
> +uint64_t start, stop;
> +
> +/* Check for no overlap */
> +if (ent->start >= query_start + query_length ||
> +ent->start + ent->length <= query_start) {
> +continue;
> +}
> +
> +/* Deal with overlap */
> +
Re: [PATCH v4 5/6] hw/cxl: Add poison injection via the mailbox.
The 03/03/2023 15:09, Jonathan Cameron wrote:
> Very simple implementation to allow testing of corresponding
> kernel code. Note that for now we track each 64 byte section
> independently. Whilst a valid implementation choice, it may
> make sense to fuse entries so as to prove out more complex
> corners of the kernel code.
>
> Reviewed-by: Ira Weiny
> Signed-off-by: Jonathan Cameron
> ---
Reviewed-by: Fan Ni
> v4: No change
> ---
> hw/cxl/cxl-mailbox-utils.c | 41 ++
> 1 file changed, 41 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 25933cf62c..64a3f3c1bf 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -64,6 +64,7 @@ enum {
> #define SET_LSA 0x3
> MEDIA_AND_POISON = 0x43,
> #define GET_POISON_LIST0x0
> +#define INJECT_POISON 0x1
> };
>
> /* 8.2.8.4.5.1 Command Return Codes */
> @@ -472,6 +473,44 @@ static CXLRetCode cmd_media_get_poison_list(struct
> cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static CXLRetCode cmd_media_inject_poison(struct cxl_cmd *cmd,
> + CXLDeviceState *cxl_dstate,
> + uint16_t *len)
> +{
> +CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +CXLPoisonList *poison_list = >poison_list;
> +CXLPoison *ent;
> +struct inject_poison_pl {
> +uint64_t dpa;
> +};
> +struct inject_poison_pl *in = (void *)cmd->payload;
> +uint64_t dpa = ldq_le_p(>dpa);
> +CXLPoison *p;
> +
> +QLIST_FOREACH(ent, poison_list, node) {
> +if (dpa >= ent->start && dpa + 64 <= ent->start + ent->length) {
> +return CXL_MBOX_SUCCESS;
> +}
> +}
> +
> +if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
> +return CXL_MBOX_INJECT_POISON_LIMIT;
> +}
> +p = g_new0(CXLPoison, 1);
> +
> +p->length = 64;
> +p->start = dpa;
> +p->type = CXL_POISON_TYPE_INJECTED;
> +
> +/*
> + * Possible todo: Merge with existing entry if next to it and if same
> type
> + */
> +QLIST_INSERT_HEAD(poison_list, p, node);
> +ct3d->poison_list_cnt++;
> +
> +return CXL_MBOX_SUCCESS;
> +}
> +
> #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define IMMEDIATE_DATA_CHANGE (1 << 2)
> #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -501,6 +540,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> ~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE },
> [MEDIA_AND_POISON][GET_POISON_LIST] = {
> "MEDIA_AND_POISON_GET_POISON_LIST",
> cmd_media_get_poison_list, 16, 0 },
> +[MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON",
> +cmd_media_inject_poison, 8, 0 },
> };
>
> void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> --
> 2.37.2
>
--
John Smith
My name is not generic at all.
Re: [PATCH v4 6/6] hw/cxl: Add clear poison mailbox command support.
The 03/03/2023 15:09, Jonathan Cameron wrote:
> Current implementation is very simple so many of the corner
> cases do not exist (e.g. fragmenting larger poison list entries)
>
> Signed-off-by: Jonathan Cameron
> ---
Reviewed-by: Fan Ni
One minor thing as mentioned below.
> v4:
> - Fix off by one on check of edge of vmr (cut and paste from similar
> but long fixed in the volatile memory series)
> - Drop unnecessary overflow check.
> - Ensure that even in case of overflow we still delete the element
> replaced (in the hole punching case)
> ---
> hw/cxl/cxl-mailbox-utils.c | 77 +
> hw/mem/cxl_type3.c | 36 +
> include/hw/cxl/cxl_device.h | 1 +
> 3 files changed, 114 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 64a3f3c1bf..0b30307fa3 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -65,6 +65,7 @@ enum {
> MEDIA_AND_POISON = 0x43,
> #define GET_POISON_LIST0x0
> #define INJECT_POISON 0x1
> +#define CLEAR_POISON 0x2
> };
>
> /* 8.2.8.4.5.1 Command Return Codes */
> @@ -511,6 +512,80 @@ static CXLRetCode cmd_media_inject_poison(struct cxl_cmd
> *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static CXLRetCode cmd_media_clear_poison(struct cxl_cmd *cmd,
> + CXLDeviceState *cxl_dstate,
> + uint16_t *len)
Since 'len' is never used in the function, I am wondering whether it
would be better to rename it to makes that more obvious like "len_unused".
> +{
> +CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +CXLPoisonList *poison_list = >poison_list;
> +CXLType3Class *cvc = CXL_TYPE3_GET_CLASS(ct3d);
> +struct clear_poison_pl {
> +uint64_t dpa;
> +uint8_t data[64];
> +};
> +CXLPoison *ent;
> +uint64_t dpa;
> +
> +struct clear_poison_pl *in = (void *)cmd->payload;
> +
> +dpa = ldq_le_p(>dpa);
> +if (dpa + 64 > cxl_dstate->mem_size) {
> +return CXL_MBOX_INVALID_PA;
> +}
> +
> +/* Always exit loop on entry removal so no need for safe variant */
> +QLIST_FOREACH(ent, poison_list, node) {
> +/*
> + * Test for contained in entry. Simpler than general case
> + * as clearing 64 bytes and entries 64 byte aligned
> + */
> +if ((dpa < ent->start) || (dpa >= ent->start + ent->length)) {
> +continue;
> +}
> +/* Do accounting early as we know one will go away */
> +ct3d->poison_list_cnt--;
> +if (dpa > ent->start) {
> +CXLPoison *frag;
> +/* Cannot overflow as replacing existing entry */
> +
> +frag = g_new0(CXLPoison, 1);
> +
> +frag->start = ent->start;
> +frag->length = dpa - ent->start;
> +frag->type = ent->type;
> +
> +QLIST_INSERT_HEAD(poison_list, frag, node);
> +ct3d->poison_list_cnt++;
> +}
> +if (dpa + 64 < ent->start + ent->length) {
> +CXLPoison *frag;
> +
> +if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
> +cxl_set_poison_list_overflowed(ct3d);
> +} else {
> +frag = g_new0(CXLPoison, 1);
> +
> +frag->start = dpa + 64;
> +frag->length = ent->start + ent->length - frag->start;
> +frag->type = ent->type;
> +QLIST_INSERT_HEAD(poison_list, frag, node);
> +ct3d->poison_list_cnt++;
> +}
> +}
> +/* Any fragments have been added, free original entry */
> +QLIST_REMOVE(ent, node);
> +g_free(ent);
> +break;
> +}
> +/* Clearing a region with no poison is not an error so always do so */
> +if (cvc->set_cacheline)
> +if (!cvc->set_cacheline(ct3d, dpa, in->data)) {
> +return CXL_MBOX_INTERNAL_ERROR;
> +}
> +
> +return CXL_MBOX_SUCCESS;
> +}
> +
> #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define IMMEDIATE_DATA_CHANGE (1 << 2)
> #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -542,6 +617,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> cmd_media_get_poison_list, 16, 0 },
> [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON",
> cmd_media_inject_poison, 8, 0 },
> +[MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
> +cmd_media_clear_poison, 72, 0 },
> };
>
> void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 21e3a84785..0d9de0ee03 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -919,6 +919,41 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf,
> uint64_t size,
> */
> }
>
> +static bool set_cacheline(CXLType3Dev *ct3d,
Re: [PATCH] MAINTAINERS: Mark the Nios II CPU as orphan
On 3/13/23 14:46, Philippe Mathieu-Daudé wrote: +CodeSourcery folks On 13/3/23 19:33, Thomas Huth wrote: Marek and Chris haven't been active for Nios II since years (the last time seems to have been in 2017), and we've got unhandled severe Nios II bug tickets in the bug tracker since a long time, so to avoid wrong expectations of people who are looking at the MAINTAINERS file, it's maybe best to mark the Nios II entry as orphan nowadays. Thanks for the heads up. We're still maintaining Nios II support in other components like GCC for now, but I am not sure whether it makes any sense for us to take on QEMU too at this point. I'll raise the issue, anyway. -Sandra
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On 14/03/2023 14:21, Chuck Zmudzinski wrote:
On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
On 14/03/2023 13:26, Chuck Zmudzinski wrote:
On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
On Tue, Mar 14, 2023 at 12:43:12PM +, Mark Cave-Ayland wrote:
On 14/03/2023 06:33, Michael S. Tsirkin wrote:
On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
xenfv machine when the guest is configured for igd-passthru.
A desired extension to that commit is to allow use of the reserved slot
if the administrator manually configures a device to use the reserved
slot. Currently, slot_reserved_mask is enforced unconditionally. With
this patch, the pci bus can be configured so the slot is only reserved
if the pci device to be added to the bus is configured for automatic
slot assignment.
To enable the desired behavior of slot_reserved_mask machine, add a
boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
add a function pci_bus_ignore_slot_reserved_mask_manual which can be
called to change the default behavior of always enforcing
slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
when the pci device being added is configured for automatic slot
assignment.
Call the new pci_bus_ignore_slot_reserved_mask_manual function after
creating the pci bus for the pc/i440fx/xenfv machine type to implement
the desired behavior of causing slot_reserved_mask to only apply when
the pci device to be added to a pc/i440fx/xenfv machine is configured
for automatic slot assignment.
Link:
https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
Signed-off-by: Chuck Zmudzinski
I really dislike this.
It seems that xen should not have used slot_reserved_mask,
and instead needs something new like slot_manual_mask.
No?
My suggestion was to move the validation logic to a separate callback
function in PCIBus (see
https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
perhaps I wasn't clear enough in pointing out that I was thinking this could
*replace* the existing slot_reserved_mask mechanism, rather than providing a
hook to allow it to be manipulated.
Here's a very rough patch put together over lunch that attempts this for
pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn
implementation, and slot_reserved_mask gets removed completely i.e.:
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..30b856499a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
return host_bridge->bypass_iommu;
}
+static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
+ int devfn)
+{
+/* All slots accessible by default */
+return false;
+}
+
static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
@@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
DeviceState *parent,
{
assert(PCI_FUNC(devfn_min) == 0);
bus->devfn_min = devfn_min;
-bus->slot_reserved_mask = 0x0;
+bus->slot_reserved_fn = pci_bus_default_slot_reserved;
bus->address_space_mem = address_space_mem;
bus->address_space_io = address_space_io;
bus->flags |= PCI_BUS_IS_ROOT;
@@ -,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, int
devfn)
return !(bus->devices[devfn]);
}
-static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
+static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
+ PCISlotReservationType restype)
+{
+return bus->slot_reserved_fn(restype, devfn);
+}
+
+void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
{
-return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
+bus->slot_reserved_fn = fn;
}
/* -1 for devfn means auto assign */
@@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev,
for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
devfn += PCI_FUNC_MAX) {
if (pci_bus_devfn_available(bus, devfn) &&
- !pci_bus_devfn_reserved(bus, devfn)) {
+ !pci_bus_devfn_reserved(bus, devfn,
PCI_SLOT_RESERVATION_AUTO)) {
goto found;
}
}
@@ -1149,7 +1162,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev,
"or reserved", name);
return NULL;
found: ;
-} else if (pci_bus_devfn_reserved(bus, devfn)) {
+} else if (pci_bus_devfn_reserved(bus, devfn,
PCI_SLOT_RESERVATION_MANUAL))
Re: [PULL v3 00/91] tcg patch queue
On Mon, 13 Mar 2023 at 18:59, Richard Henderson wrote: > > Version 3 fixes a rebase error from v2 affecting ARM BFC insn. > > > r~ > > > The following changes since commit 29c8a9e31a982874ce4e2c15f2bf82d5f8dc3517: > > Merge tag 'linux-user-for-8.0-pull-request' of > https://gitlab.com/laurent_vivier/qemu into staging (2023-03-12 10:57:00 > +) > > are available in the Git repository at: > > https://gitlab.com/rth7680/qemu.git tags/pull-tcg-20230313 > > for you to fetch changes up to 0c8b6b9a6383e2e37ff3d1d12b40c58b7ed36c1c: > > tcg: Drop tcg_const_* (2023-03-13 07:03:39 -0700) > > > accel/tcg: Fix NB_MMU_MODES to 16 > Balance of the target/ patchset which eliminates tcg_temp_free > Balance of the target/ patchset which eliminates tcg_const Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.0 for any user-visible changes. -- PMM
Re: [PATCH] tests/qtest/migration-test: Disable postcopy/preempt tests
On 14/03/2023 15.08, Peter Maydell wrote:
On Tue, 14 Mar 2023 at 14:01, Thomas Huth wrote:
On 14/03/2023 14.33, Peter Maydell wrote:
The postcopy/preempt tests seem to have a race which makes them hang
on the s390x CI runner. Disable them for the moment, while we
investigate. As with the other disabled subtest, you can opt back in
by setting QEMU_TEST_FLAKY_TESTS=1 in your environment.
Suggested-by: Dr. David Alan Gilbert
Signed-off-by: Peter Maydell
---
tests/qtest/migration-test.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d4ab3934ed2..4643f7f49dc 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2464,6 +2464,11 @@ int main(int argc, char **argv)
const char *arch = qtest_get_arch();
g_autoptr(GError) err = NULL;
int ret;
+/*
+ * Race condition suspected in the postcopy/preempt tests: see
+ *
https://lore.kernel.org/qemu-devel/CAFEAcA-q1UwPePdHTzXNSX4i6Urh3j6h51kymy6=7szdafu...@mail.gmail.com/
+ */
+bool skip_postcopy_preempt = getenv("QEMU_TEST_FLAKY_TESTS");
You could maybe also call the variale skip_flaky_tests and use it in the
other spot where you recently added a getenv() already.
Thomas
Re: [PATCH] tests/qtest/migration-test: Disable postcopy/preempt tests
On Tue, 14 Mar 2023 at 14:14, Thomas Huth wrote:
>
> On 14/03/2023 15.08, Peter Maydell wrote:
> > On Tue, 14 Mar 2023 at 14:01, Thomas Huth wrote:
> >>
> >> On 14/03/2023 14.33, Peter Maydell wrote:
> >>> The postcopy/preempt tests seem to have a race which makes them hang
> >>> on the s390x CI runner. Disable them for the moment, while we
> >>> investigate. As with the other disabled subtest, you can opt back in
> >>> by setting QEMU_TEST_FLAKY_TESTS=1 in your environment.
> >>>
> >>> Suggested-by: Dr. David Alan Gilbert
> >>> Signed-off-by: Peter Maydell
> >>> ---
> >>>tests/qtest/migration-test.c | 23 ---
> >>>1 file changed, 16 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> >>> index d4ab3934ed2..4643f7f49dc 100644
> >>> --- a/tests/qtest/migration-test.c
> >>> +++ b/tests/qtest/migration-test.c
> >>> @@ -2464,6 +2464,11 @@ int main(int argc, char **argv)
> >>>const char *arch = qtest_get_arch();
> >>>g_autoptr(GError) err = NULL;
> >>>int ret;
> >>> +/*
> >>> + * Race condition suspected in the postcopy/preempt tests: see
> >>> + *
> >>> https://lore.kernel.org/qemu-devel/CAFEAcA-q1UwPePdHTzXNSX4i6Urh3j6h51kymy6=7szdafu...@mail.gmail.com/
> >>> + */
> >>> +bool skip_postcopy_preempt = getenv("QEMU_TEST_FLAKY_TESTS");
>
> You could maybe also call the variale skip_flaky_tests and use it in the
> other spot where you recently added a getenv() already.
That would make it a bit harder to do a simple revert of the
commits when we figure out the cause of the problem, though.
-- PMM
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
> On 14/03/2023 13:26, Chuck Zmudzinski wrote:
>
> > On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> >> On Tue, Mar 14, 2023 at 12:43:12PM +, Mark Cave-Ayland wrote:
> >>> On 14/03/2023 06:33, Michael S. Tsirkin wrote:
> >>>
> On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> > Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel
> > igd-passthru")
> > uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
> > xenfv machine when the guest is configured for igd-passthru.
> >
> > A desired extension to that commit is to allow use of the reserved slot
> > if the administrator manually configures a device to use the reserved
> > slot. Currently, slot_reserved_mask is enforced unconditionally. With
> > this patch, the pci bus can be configured so the slot is only reserved
> > if the pci device to be added to the bus is configured for automatic
> > slot assignment.
> >
> > To enable the desired behavior of slot_reserved_mask machine, add a
> > boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> > add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> > called to change the default behavior of always enforcing
> > slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> > when the pci device being added is configured for automatic slot
> > assignment.
> >
> > Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> > creating the pci bus for the pc/i440fx/xenfv machine type to implement
> > the desired behavior of causing slot_reserved_mask to only apply when
> > the pci device to be added to a pc/i440fx/xenfv machine is configured
> > for automatic slot assignment.
> >
> > Link:
> > https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
> > Signed-off-by: Chuck Zmudzinski
>
> I really dislike this.
> It seems that xen should not have used slot_reserved_mask,
> and instead needs something new like slot_manual_mask.
> No?
> >>>
> >>> My suggestion was to move the validation logic to a separate callback
> >>> function in PCIBus (see
> >>> https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
> >>> perhaps I wasn't clear enough in pointing out that I was thinking this
> >>> could
> >>> *replace* the existing slot_reserved_mask mechanism, rather than
> >>> providing a
> >>> hook to allow it to be manipulated.
> >>>
> >>> Here's a very rough patch put together over lunch that attempts this for
> >>> pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
> >>> pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn
> >>> implementation, and slot_reserved_mask gets removed completely i.e.:
> >>>
> >>>
> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>> index def5000e7b..30b856499a 100644
> >>> --- a/hw/pci/pci.c
> >>> +++ b/hw/pci/pci.c
> >>> @@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
> >>> return host_bridge->bypass_iommu;
> >>> }
> >>>
> >>> +static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
> >>> + int devfn)
> >>> +{
> >>> +/* All slots accessible by default */
> >>> +return false;
> >>> +}
> >>> +
> >>> static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
> >>> MemoryRegion *address_space_mem,
> >>> MemoryRegion *address_space_io,
> >>> @@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
> >>> DeviceState *parent,
> >>> {
> >>> assert(PCI_FUNC(devfn_min) == 0);
> >>> bus->devfn_min = devfn_min;
> >>> -bus->slot_reserved_mask = 0x0;
> >>> +bus->slot_reserved_fn = pci_bus_default_slot_reserved;
> >>> bus->address_space_mem = address_space_mem;
> >>> bus->address_space_io = address_space_io;
> >>> bus->flags |= PCI_BUS_IS_ROOT;
> >>> @@ -,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus,
> >>> int devfn)
> >>> return !(bus->devices[devfn]);
> >>> }
> >>>
> >>> -static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> >>> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
> >>> + PCISlotReservationType restype)
> >>> +{
> >>> +return bus->slot_reserved_fn(restype, devfn);
> >>> +}
> >>> +
> >>> +void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
> >>> {
> >>> -return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> >>> +bus->slot_reserved_fn = fn;
> >>> }
> >>>
> >>> /* -1 for devfn means auto assign */
> >>> @@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> >>> *pci_dev,
> >>> for(devfn = bus->devfn_min ; devfn <
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
> On 14/03/2023 13:26, Chuck Zmudzinski wrote:
>
> > On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> >> On Tue, Mar 14, 2023 at 12:43:12PM +, Mark Cave-Ayland wrote:
> >>> On 14/03/2023 06:33, Michael S. Tsirkin wrote:
> >>>
> On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> > Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel
> > igd-passthru")
> > uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
> > xenfv machine when the guest is configured for igd-passthru.
> >
> > A desired extension to that commit is to allow use of the reserved slot
> > if the administrator manually configures a device to use the reserved
> > slot. Currently, slot_reserved_mask is enforced unconditionally. With
> > this patch, the pci bus can be configured so the slot is only reserved
> > if the pci device to be added to the bus is configured for automatic
> > slot assignment.
> >
> > To enable the desired behavior of slot_reserved_mask machine, add a
> > boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> > add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> > called to change the default behavior of always enforcing
> > slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> > when the pci device being added is configured for automatic slot
> > assignment.
> >
> > Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> > creating the pci bus for the pc/i440fx/xenfv machine type to implement
> > the desired behavior of causing slot_reserved_mask to only apply when
> > the pci device to be added to a pc/i440fx/xenfv machine is configured
> > for automatic slot assignment.
> >
> > Link:
> > https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
> > Signed-off-by: Chuck Zmudzinski
>
> I really dislike this.
> It seems that xen should not have used slot_reserved_mask,
> and instead needs something new like slot_manual_mask.
> No?
> >>>
> >>> My suggestion was to move the validation logic to a separate callback
> >>> function in PCIBus (see
> >>> https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
> >>> perhaps I wasn't clear enough in pointing out that I was thinking this
> >>> could
> >>> *replace* the existing slot_reserved_mask mechanism, rather than
> >>> providing a
> >>> hook to allow it to be manipulated.
> >>>
> >>> Here's a very rough patch put together over lunch that attempts this for
> >>> pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
> >>> pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn
> >>> implementation, and slot_reserved_mask gets removed completely i.e.:
> >>>
> >>>
> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>> index def5000e7b..30b856499a 100644
> >>> --- a/hw/pci/pci.c
> >>> +++ b/hw/pci/pci.c
> >>> @@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
> >>> return host_bridge->bypass_iommu;
> >>> }
> >>>
> >>> +static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
> >>> + int devfn)
> >>> +{
> >>> +/* All slots accessible by default */
> >>> +return false;
> >>> +}
> >>> +
> >>> static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
> >>> MemoryRegion *address_space_mem,
> >>> MemoryRegion *address_space_io,
> >>> @@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
> >>> DeviceState *parent,
> >>> {
> >>> assert(PCI_FUNC(devfn_min) == 0);
> >>> bus->devfn_min = devfn_min;
> >>> -bus->slot_reserved_mask = 0x0;
> >>> +bus->slot_reserved_fn = pci_bus_default_slot_reserved;
> >>> bus->address_space_mem = address_space_mem;
> >>> bus->address_space_io = address_space_io;
> >>> bus->flags |= PCI_BUS_IS_ROOT;
> >>> @@ -,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus,
> >>> int devfn)
> >>> return !(bus->devices[devfn]);
> >>> }
> >>>
> >>> -static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> >>> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
> >>> + PCISlotReservationType restype)
> >>> +{
> >>> +return bus->slot_reserved_fn(restype, devfn);
> >>> +}
> >>> +
> >>> +void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
> >>> {
> >>> -return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> >>> +bus->slot_reserved_fn = fn;
> >>> }
> >>>
> >>> /* -1 for devfn means auto assign */
> >>> @@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> >>> *pci_dev,
> >>> for(devfn = bus->devfn_min ; devfn <
Re: [PATCH] tests/qtest/migration-test: Disable postcopy/preempt tests
On Tue, 14 Mar 2023 at 14:01, Thomas Huth wrote:
>
> On 14/03/2023 14.33, Peter Maydell wrote:
> > The postcopy/preempt tests seem to have a race which makes them hang
> > on the s390x CI runner. Disable them for the moment, while we
> > investigate. As with the other disabled subtest, you can opt back in
> > by setting QEMU_TEST_FLAKY_TESTS=1 in your environment.
> >
> > Suggested-by: Dr. David Alan Gilbert
> > Signed-off-by: Peter Maydell
> > ---
> > tests/qtest/migration-test.c | 23 ---
> > 1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index d4ab3934ed2..4643f7f49dc 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -2464,6 +2464,11 @@ int main(int argc, char **argv)
> > const char *arch = qtest_get_arch();
> > g_autoptr(GError) err = NULL;
> > int ret;
> > +/*
> > + * Race condition suspected in the postcopy/preempt tests: see
> > + *
> > https://lore.kernel.org/qemu-devel/CAFEAcA-q1UwPePdHTzXNSX4i6Urh3j6h51kymy6=7szdafu...@mail.gmail.com/
> > + */
> > +bool skip_postcopy_preempt = getenv("QEMU_TEST_FLAKY_TESTS");
>
> Shouldn't that be "!getenv(...)" ?
Doh!
thanks
-- PMM
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 14, 2023 at 12:43:12PM +, Mark Cave-Ayland wrote:
> > On 14/03/2023 06:33, Michael S. Tsirkin wrote:
> >
> > > On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> > > > Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel
> > > > igd-passthru")
> > > > uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
> > > > xenfv machine when the guest is configured for igd-passthru.
> > > >
> > > > A desired extension to that commit is to allow use of the reserved slot
> > > > if the administrator manually configures a device to use the reserved
> > > > slot. Currently, slot_reserved_mask is enforced unconditionally. With
> > > > this patch, the pci bus can be configured so the slot is only reserved
> > > > if the pci device to be added to the bus is configured for automatic
> > > > slot assignment.
> > > >
> > > > To enable the desired behavior of slot_reserved_mask machine, add a
> > > > boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> > > > add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> > > > called to change the default behavior of always enforcing
> > > > slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> > > > when the pci device being added is configured for automatic slot
> > > > assignment.
> > > >
> > > > Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> > > > creating the pci bus for the pc/i440fx/xenfv machine type to implement
> > > > the desired behavior of causing slot_reserved_mask to only apply when
> > > > the pci device to be added to a pc/i440fx/xenfv machine is configured
> > > > for automatic slot assignment.
> > > >
> > > > Link:
> > > > https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
> > > > Signed-off-by: Chuck Zmudzinski
> > >
> > > I really dislike this.
> > > It seems that xen should not have used slot_reserved_mask,
> > > and instead needs something new like slot_manual_mask.
> > > No?
> >
> > My suggestion was to move the validation logic to a separate callback
> > function in PCIBus (see
> > https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
> > perhaps I wasn't clear enough in pointing out that I was thinking this could
> > *replace* the existing slot_reserved_mask mechanism, rather than providing a
> > hook to allow it to be manipulated.
> >
> > Here's a very rough patch put together over lunch that attempts this for
> > pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
> > pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn
> > implementation, and slot_reserved_mask gets removed completely i.e.:
> >
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index def5000e7b..30b856499a 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
> > return host_bridge->bypass_iommu;
> > }
> >
> > +static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
> > + int devfn)
> > +{
> > +/* All slots accessible by default */
> > +return false;
> > +}
> > +
> > static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
> > MemoryRegion *address_space_mem,
> > MemoryRegion *address_space_io,
> > @@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
> > DeviceState *parent,
> > {
> > assert(PCI_FUNC(devfn_min) == 0);
> > bus->devfn_min = devfn_min;
> > -bus->slot_reserved_mask = 0x0;
> > +bus->slot_reserved_fn = pci_bus_default_slot_reserved;
> > bus->address_space_mem = address_space_mem;
> > bus->address_space_io = address_space_io;
> > bus->flags |= PCI_BUS_IS_ROOT;
> > @@ -,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, int
> > devfn)
> > return !(bus->devices[devfn]);
> > }
> >
> > -static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> > +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
> > + PCISlotReservationType restype)
> > +{
> > +return bus->slot_reserved_fn(restype, devfn);
> > +}
> > +
> > +void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
> > {
> > -return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> > +bus->slot_reserved_fn = fn;
> > }
> >
> > /* -1 for devfn means auto assign */
> > @@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> > *pci_dev,
> > for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> > devfn += PCI_FUNC_MAX) {
> > if (pci_bus_devfn_available(bus, devfn) &&
> > - !pci_bus_devfn_reserved(bus, devfn)) {
> > + !pci_bus_devfn_reserved(bus, devfn,
> >
Re: [PATCH] tests/qtest/migration-test: Disable postcopy/preempt tests
On 14/03/2023 14.33, Peter Maydell wrote:
The postcopy/preempt tests seem to have a race which makes them hang
on the s390x CI runner. Disable them for the moment, while we
investigate. As with the other disabled subtest, you can opt back in
by setting QEMU_TEST_FLAKY_TESTS=1 in your environment.
Suggested-by: Dr. David Alan Gilbert
Signed-off-by: Peter Maydell
---
tests/qtest/migration-test.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d4ab3934ed2..4643f7f49dc 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2464,6 +2464,11 @@ int main(int argc, char **argv)
const char *arch = qtest_get_arch();
g_autoptr(GError) err = NULL;
int ret;
+/*
+ * Race condition suspected in the postcopy/preempt tests: see
+ *
https://lore.kernel.org/qemu-devel/CAFEAcA-q1UwPePdHTzXNSX4i6Urh3j6h51kymy6=7szdafu...@mail.gmail.com/
+ */
+bool skip_postcopy_preempt = getenv("QEMU_TEST_FLAKY_TESTS");
Shouldn't that be "!getenv(...)" ?
Thomas
g_test_init(, , NULL);
@@ -2500,9 +2505,11 @@ int main(int argc, char **argv)
qtest_add_func("/migration/postcopy/plain", test_postcopy);
qtest_add_func("/migration/postcopy/recovery/plain",
test_postcopy_recovery);
-qtest_add_func("/migration/postcopy/preempt/plain",
test_postcopy_preempt);
-qtest_add_func("/migration/postcopy/preempt/recovery/plain",
- test_postcopy_preempt_recovery);
+if (!skip_postcopy_preempt) {
+qtest_add_func("/migration/postcopy/preempt/plain",
test_postcopy_preempt);
+qtest_add_func("/migration/postcopy/preempt/recovery/plain",
+ test_postcopy_preempt_recovery);
+}
}
qtest_add_func("/migration/bad_dest", test_baddest);
@@ -2521,10 +2528,12 @@ int main(int argc, char **argv)
qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
qtest_add_func("/migration/postcopy/recovery/tls/psk",
test_postcopy_recovery_tls_psk);
-qtest_add_func("/migration/postcopy/preempt/tls/psk",
- test_postcopy_preempt_tls_psk);
-qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
- test_postcopy_preempt_all);
+if (!skip_postcopy_preempt) {
+qtest_add_func("/migration/postcopy/preempt/tls/psk",
+ test_postcopy_preempt_tls_psk);
+qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
+ test_postcopy_preempt_all);
+}
}
#ifdef CONFIG_TASN1
qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
[PULL 3/3] MAINTAINERS: Remove CXL maintainer Ben Widawsky
From: Markus Armbruster Ben is no longer with intel. He told me he expected to get back to CXL, but it's not happening as quickly as he'd like, and that it's best to remove him as maintainer. So let's do that. Thank you for serving as maintainer, Ben! Signed-off-by: Markus Armbruster Acked-by: Jonathan Cameron Message-Id: <20230220212437.1462314-1-arm...@redhat.com> Signed-off-by: Laurent Vivier --- MAINTAINERS | 1 - 1 file changed, 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 883dc52063bf..7eab1a266fb8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2666,7 +2666,6 @@ T: git https://gitlab.com/jsnow/qemu.git jobs T: git https://gitlab.com/vsementsov/qemu.git block Compute Express Link -M: Ben Widawsky M: Jonathan Cameron R: Fan Ni S: Supported -- 2.39.2
Re: [PATCH v2 0/8] iotests: make meson aware of individual I/O tests
Daniel P. Berrangé writes: > To just repeat the patch 5 description... > Queued to for-8.0/tweaks-and-fixes, thanks. -- Alex Bennée Virtualisation Tech Lead @ Linaro
[PULL 2/3] MAINTAINERS: update my email address for the clock framework
From: Damien Hedde Also update mailmap Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Luc Michel Message-Id: <20230213105227.2357-1-damien.he...@dahe.fr> Signed-off-by: Laurent Vivier --- .mailmap| 1 + MAINTAINERS | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.mailmap b/.mailmap index fad2aff5aa55..767704795057 100644 --- a/.mailmap +++ b/.mailmap @@ -56,6 +56,7 @@ Aleksandar Rikalo Alexander Graf Anthony Liguori Anthony Liguori Christian Borntraeger +Damien Hedde Filip Bozuta Frederic Konrad Frederic Konrad diff --git a/MAINTAINERS b/MAINTAINERS index 95c957d587d9..883dc52063bf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3365,7 +3365,7 @@ F: .gitlab-ci.d/opensbi/ Clock framework M: Luc Michel -R: Damien Hedde +R: Damien Hedde S: Maintained F: include/hw/clock.h F: include/hw/qdev-clock.h -- 2.39.2
[PULL 0/3] Trivial branch for 8.0 patches
The following changes since commit 284c52eec2d0a1b9c47f06c3eee46762c5fc0915: Merge tag 'win-socket-pull-request' of https://gitlab.com/marcandre.lureau/qemu into staging (2023-03-13 13:44:17 +) are available in the Git repository at: https://gitlab.com/laurent_vivier/qemu.git tags/trivial-branch-for-8.0-pull-request for you to fetch changes up to fcc8f37ca3eca968932e5da716ec5e7fc05fdcf4: MAINTAINERS: Remove CXL maintainer Ben Widawsky (2023-03-14 14:46:38 +0100) trivial branch pull request 20230314 Update MAINTAINER file Fix typo in qemu-options.hx Damien Hedde (1): MAINTAINERS: update my email address for the clock framework John Snow (1): qemu-options.hx: remove stray quote Markus Armbruster (1): MAINTAINERS: Remove CXL maintainer Ben Widawsky .mailmap| 1 + MAINTAINERS | 3 +-- qemu-options.hx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) -- 2.39.2
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On 14/03/2023 13:26, Chuck Zmudzinski wrote:
On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
On Tue, Mar 14, 2023 at 12:43:12PM +, Mark Cave-Ayland wrote:
On 14/03/2023 06:33, Michael S. Tsirkin wrote:
On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
xenfv machine when the guest is configured for igd-passthru.
A desired extension to that commit is to allow use of the reserved slot
if the administrator manually configures a device to use the reserved
slot. Currently, slot_reserved_mask is enforced unconditionally. With
this patch, the pci bus can be configured so the slot is only reserved
if the pci device to be added to the bus is configured for automatic
slot assignment.
To enable the desired behavior of slot_reserved_mask machine, add a
boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
add a function pci_bus_ignore_slot_reserved_mask_manual which can be
called to change the default behavior of always enforcing
slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
when the pci device being added is configured for automatic slot
assignment.
Call the new pci_bus_ignore_slot_reserved_mask_manual function after
creating the pci bus for the pc/i440fx/xenfv machine type to implement
the desired behavior of causing slot_reserved_mask to only apply when
the pci device to be added to a pc/i440fx/xenfv machine is configured
for automatic slot assignment.
Link:
https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
Signed-off-by: Chuck Zmudzinski
I really dislike this.
It seems that xen should not have used slot_reserved_mask,
and instead needs something new like slot_manual_mask.
No?
My suggestion was to move the validation logic to a separate callback
function in PCIBus (see
https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
perhaps I wasn't clear enough in pointing out that I was thinking this could
*replace* the existing slot_reserved_mask mechanism, rather than providing a
hook to allow it to be manipulated.
Here's a very rough patch put together over lunch that attempts this for
pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn
implementation, and slot_reserved_mask gets removed completely i.e.:
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..30b856499a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
return host_bridge->bypass_iommu;
}
+static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
+ int devfn)
+{
+/* All slots accessible by default */
+return false;
+}
+
static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
@@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
DeviceState *parent,
{
assert(PCI_FUNC(devfn_min) == 0);
bus->devfn_min = devfn_min;
-bus->slot_reserved_mask = 0x0;
+bus->slot_reserved_fn = pci_bus_default_slot_reserved;
bus->address_space_mem = address_space_mem;
bus->address_space_io = address_space_io;
bus->flags |= PCI_BUS_IS_ROOT;
@@ -,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, int
devfn)
return !(bus->devices[devfn]);
}
-static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
+static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
+ PCISlotReservationType restype)
+{
+return bus->slot_reserved_fn(restype, devfn);
+}
+
+void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
{
-return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
+bus->slot_reserved_fn = fn;
}
/* -1 for devfn means auto assign */
@@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev,
for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
devfn += PCI_FUNC_MAX) {
if (pci_bus_devfn_available(bus, devfn) &&
- !pci_bus_devfn_reserved(bus, devfn)) {
+ !pci_bus_devfn_reserved(bus, devfn,
PCI_SLOT_RESERVATION_AUTO)) {
goto found;
}
}
@@ -1149,7 +1162,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev,
"or reserved", name);
return NULL;
found: ;
-} else if (pci_bus_devfn_reserved(bus, devfn)) {
+} else if (pci_bus_devfn_reserved(bus, devfn,
PCI_SLOT_RESERVATION_MANUAL)) {
error_setg(errp, "PCI: slot %d function %d not available for %s,"
[PULL 1/3] qemu-options.hx: remove stray quote
From: John Snow Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230202223121.252073-1-js...@redhat.com> Signed-off-by: Laurent Vivier --- qemu-options.hx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-options.hx b/qemu-options.hx index d42f60fb9178..59bdf67a2c51 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1606,7 +1606,7 @@ SRST .. parsed-literal:: -|qemu_system_x86| -drive file=a -drive file=b" +|qemu_system_x86| -drive file=a -drive file=b is interpreted like: -- 2.39.2
Re: [PATCH] tests/qtest/migration-test: Disable postcopy/preempt tests
* Peter Maydell (peter.mayd...@linaro.org) wrote:
> The postcopy/preempt tests seem to have a race which makes them hang
> on the s390x CI runner. Disable them for the moment, while we
> investigate. As with the other disabled subtest, you can opt back in
> by setting QEMU_TEST_FLAKY_TESTS=1 in your environment.
>
> Suggested-by: Dr. David Alan Gilbert
> Signed-off-by: Peter Maydell
Reviewed-by: Dr. David Alan Gilbert
> ---
> tests/qtest/migration-test.c | 23 ---
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index d4ab3934ed2..4643f7f49dc 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2464,6 +2464,11 @@ int main(int argc, char **argv)
> const char *arch = qtest_get_arch();
> g_autoptr(GError) err = NULL;
> int ret;
> +/*
> + * Race condition suspected in the postcopy/preempt tests: see
> + *
> https://lore.kernel.org/qemu-devel/CAFEAcA-q1UwPePdHTzXNSX4i6Urh3j6h51kymy6=7szdafu...@mail.gmail.com/
> + */
> +bool skip_postcopy_preempt = getenv("QEMU_TEST_FLAKY_TESTS");
>
> g_test_init(, , NULL);
>
> @@ -2500,9 +2505,11 @@ int main(int argc, char **argv)
> qtest_add_func("/migration/postcopy/plain", test_postcopy);
> qtest_add_func("/migration/postcopy/recovery/plain",
> test_postcopy_recovery);
> -qtest_add_func("/migration/postcopy/preempt/plain",
> test_postcopy_preempt);
> -qtest_add_func("/migration/postcopy/preempt/recovery/plain",
> - test_postcopy_preempt_recovery);
> +if (!skip_postcopy_preempt) {
> +qtest_add_func("/migration/postcopy/preempt/plain",
> test_postcopy_preempt);
> +qtest_add_func("/migration/postcopy/preempt/recovery/plain",
> + test_postcopy_preempt_recovery);
> +}
> }
>
> qtest_add_func("/migration/bad_dest", test_baddest);
> @@ -2521,10 +2528,12 @@ int main(int argc, char **argv)
> qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
> qtest_add_func("/migration/postcopy/recovery/tls/psk",
> test_postcopy_recovery_tls_psk);
> -qtest_add_func("/migration/postcopy/preempt/tls/psk",
> - test_postcopy_preempt_tls_psk);
> -qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
> - test_postcopy_preempt_all);
> +if (!skip_postcopy_preempt) {
> +qtest_add_func("/migration/postcopy/preempt/tls/psk",
> + test_postcopy_preempt_tls_psk);
> +qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
> + test_postcopy_preempt_all);
> +}
> }
> #ifdef CONFIG_TASN1
> qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
> --
> 2.34.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH] tests/qtest/migration-test: Disable postcopy/preempt tests
The postcopy/preempt tests seem to have a race which makes them hang
on the s390x CI runner. Disable them for the moment, while we
investigate. As with the other disabled subtest, you can opt back in
by setting QEMU_TEST_FLAKY_TESTS=1 in your environment.
Suggested-by: Dr. David Alan Gilbert
Signed-off-by: Peter Maydell
---
tests/qtest/migration-test.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d4ab3934ed2..4643f7f49dc 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2464,6 +2464,11 @@ int main(int argc, char **argv)
const char *arch = qtest_get_arch();
g_autoptr(GError) err = NULL;
int ret;
+/*
+ * Race condition suspected in the postcopy/preempt tests: see
+ *
https://lore.kernel.org/qemu-devel/CAFEAcA-q1UwPePdHTzXNSX4i6Urh3j6h51kymy6=7szdafu...@mail.gmail.com/
+ */
+bool skip_postcopy_preempt = getenv("QEMU_TEST_FLAKY_TESTS");
g_test_init(, , NULL);
@@ -2500,9 +2505,11 @@ int main(int argc, char **argv)
qtest_add_func("/migration/postcopy/plain", test_postcopy);
qtest_add_func("/migration/postcopy/recovery/plain",
test_postcopy_recovery);
-qtest_add_func("/migration/postcopy/preempt/plain",
test_postcopy_preempt);
-qtest_add_func("/migration/postcopy/preempt/recovery/plain",
- test_postcopy_preempt_recovery);
+if (!skip_postcopy_preempt) {
+qtest_add_func("/migration/postcopy/preempt/plain",
test_postcopy_preempt);
+qtest_add_func("/migration/postcopy/preempt/recovery/plain",
+ test_postcopy_preempt_recovery);
+}
}
qtest_add_func("/migration/bad_dest", test_baddest);
@@ -2521,10 +2528,12 @@ int main(int argc, char **argv)
qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
qtest_add_func("/migration/postcopy/recovery/tls/psk",
test_postcopy_recovery_tls_psk);
-qtest_add_func("/migration/postcopy/preempt/tls/psk",
- test_postcopy_preempt_tls_psk);
-qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
- test_postcopy_preempt_all);
+if (!skip_postcopy_preempt) {
+qtest_add_func("/migration/postcopy/preempt/tls/psk",
+ test_postcopy_preempt_tls_psk);
+qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
+ test_postcopy_preempt_all);
+}
}
#ifdef CONFIG_TASN1
qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
--
2.34.1
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On Tue, Mar 14, 2023 at 12:43:12PM +, Mark Cave-Ayland wrote:
> On 14/03/2023 06:33, Michael S. Tsirkin wrote:
>
> > On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> > > Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
> > > uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
> > > xenfv machine when the guest is configured for igd-passthru.
> > >
> > > A desired extension to that commit is to allow use of the reserved slot
> > > if the administrator manually configures a device to use the reserved
> > > slot. Currently, slot_reserved_mask is enforced unconditionally. With
> > > this patch, the pci bus can be configured so the slot is only reserved
> > > if the pci device to be added to the bus is configured for automatic
> > > slot assignment.
> > >
> > > To enable the desired behavior of slot_reserved_mask machine, add a
> > > boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> > > add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> > > called to change the default behavior of always enforcing
> > > slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> > > when the pci device being added is configured for automatic slot
> > > assignment.
> > >
> > > Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> > > creating the pci bus for the pc/i440fx/xenfv machine type to implement
> > > the desired behavior of causing slot_reserved_mask to only apply when
> > > the pci device to be added to a pc/i440fx/xenfv machine is configured
> > > for automatic slot assignment.
> > >
> > > Link:
> > > https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
> > > Signed-off-by: Chuck Zmudzinski
> >
> > I really dislike this.
> > It seems that xen should not have used slot_reserved_mask,
> > and instead needs something new like slot_manual_mask.
> > No?
>
> My suggestion was to move the validation logic to a separate callback
> function in PCIBus (see
> https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
> perhaps I wasn't clear enough in pointing out that I was thinking this could
> *replace* the existing slot_reserved_mask mechanism, rather than providing a
> hook to allow it to be manipulated.
>
> Here's a very rough patch put together over lunch that attempts this for
> pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
> pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn
> implementation, and slot_reserved_mask gets removed completely i.e.:
>
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index def5000e7b..30b856499a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
> return host_bridge->bypass_iommu;
> }
>
> +static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
> + int devfn)
> +{
> +/* All slots accessible by default */
> +return false;
> +}
> +
> static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
> MemoryRegion *address_space_mem,
> MemoryRegion *address_space_io,
> @@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
> DeviceState *parent,
> {
> assert(PCI_FUNC(devfn_min) == 0);
> bus->devfn_min = devfn_min;
> -bus->slot_reserved_mask = 0x0;
> +bus->slot_reserved_fn = pci_bus_default_slot_reserved;
> bus->address_space_mem = address_space_mem;
> bus->address_space_io = address_space_io;
> bus->flags |= PCI_BUS_IS_ROOT;
> @@ -,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, int
> devfn)
> return !(bus->devices[devfn]);
> }
>
> -static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
> + PCISlotReservationType restype)
> +{
> +return bus->slot_reserved_fn(restype, devfn);
> +}
> +
> +void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
> {
> -return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> +bus->slot_reserved_fn = fn;
> }
>
> /* -1 for devfn means auto assign */
> @@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev,
> for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> devfn += PCI_FUNC_MAX) {
> if (pci_bus_devfn_available(bus, devfn) &&
> - !pci_bus_devfn_reserved(bus, devfn)) {
> + !pci_bus_devfn_reserved(bus, devfn,
> PCI_SLOT_RESERVATION_AUTO)) {
> goto found;
> }
> }
> @@ -1149,7 +1162,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev,
> "or reserved", name);
> return NULL;
> found: ;
> -} else if
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On Tue, Mar 14, 2023 at 08:33:02AM -0400, Chuck Zmudzinski wrote:
> On 3/14/2023 2:33 AM, Michael S. Tsirkin wrote:
> > On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> > > Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
> > > uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
> > > xenfv machine when the guest is configured for igd-passthru.
> > >
> > > A desired extension to that commit is to allow use of the reserved slot
> > > if the administrator manually configures a device to use the reserved
> > > slot. Currently, slot_reserved_mask is enforced unconditionally. With
> > > this patch, the pci bus can be configured so the slot is only reserved
> > > if the pci device to be added to the bus is configured for automatic
> > > slot assignment.
> > >
> > > To enable the desired behavior of slot_reserved_mask machine, add a
> > > boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> > > add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> > > called to change the default behavior of always enforcing
> > > slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> > > when the pci device being added is configured for automatic slot
> > > assignment.
> > >
> > > Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> > > creating the pci bus for the pc/i440fx/xenfv machine type to implement
> > > the desired behavior of causing slot_reserved_mask to only apply when
> > > the pci device to be added to a pc/i440fx/xenfv machine is configured
> > > for automatic slot assignment.
> > >
> > > Link:
> > > https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
> > > Signed-off-by: Chuck Zmudzinski
> >
> > I really dislike this.
> > It seems that xen should not have used slot_reserved_mask,
> > and instead needs something new like slot_manual_mask.
> > No?
>
> Actually, xen would use something like slot_auto_mask, and
> sun4u would use both slot_auto_mask and slot_manual_mask.
>
> Is it just that this patch touches hw/pci-host/i440fx.c that you
> don't like or is it that you don't like adding slot_reserved_mask_manual
> and pci_bus_ignore_slot_reserved_mask_manual, or is it both
> that you don't like?
I don't like the enforce_slot_reserved_mask_manual flag -
I prefer straight forward logic with no branches in
the common code.
> If it's the former that you don't like, the call to
> pci_bus_ignore_slot_reserved_mask_manual can be moved to
> xen_igd_reserve_slot in hw/xen/xen_pt.c and this would
> avoid touching hw/pci-host/i440fx.c.
>
> If it's the latter that you don't like, both slot_reserved_mask_manual
> and pci_bus_ignore_slot_reserved_mask_manual can be removed
> and this can be implemented with two independent slot masks:
>
> rename slot_reserved_mask as slot_auto_mask - used by both xen and sun4u
> slot_manual_mask - new mask, used only by sun4u.
Sounds good to me, except let's add "reserved" in here.
slot_reserved_mask_auto, slot_reserved_mask_manual ?
> We would also need to have two sets of accessor functions in this case, one
> set to access slot_auto_mask, and the other to access slot_manual_mask.
> Since the sun4u machine does not need to either get the value of
> slot_manual_mask or clear the slot_manual_mask, slot_manual_mask
> would only need to have one accessor function to set the value of the
> mask. slot_auto_mask would have all three accessor functions that xen
> needs to use.
>
> Would that be OK?
Sounds good to me.
> >
> > > ---
> > > Changelog
> > >
> > > v2: Change Subject of patch from
> > > "pci: add enforce_slot_reserved_mask_manual property" To
> > > "pci: allow slot_reserved_mask to be ignored with manual slot
> > > assignment"
> > >
> > > Add pci_bus_ignore_slot_reserved_mask_manual function
> > >
> > > Call pci_bus_ignore_slot_reserved_mask_manual at appropriate place
> > > in hw/pci-host/i440fx.c
> > >
> > > hw/pci-host/i440fx.c | 1 +
> > > hw/pci/pci.c | 14 +-
> > > include/hw/pci/pci.h | 1 +
> > > include/hw/pci/pci_bus.h | 1 +
> > > 4 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> > > index 262f82c303..8e00b88926 100644
> > > --- a/hw/pci-host/i440fx.c
> > > +++ b/hw/pci-host/i440fx.c
> > > @@ -257,6 +257,7 @@ PCIBus *i440fx_init(const char *pci_type,
> > > s = PCI_HOST_BRIDGE(dev);
> > > b = pci_root_bus_new(dev, NULL, pci_address_space,
> > > address_space_io, 0, TYPE_PCI_BUS);
> > > +pci_bus_ignore_slot_reserved_mask_manual(b);
> > > s->bus = b;
> > > object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
> > > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 8a87ccc8b0..670ecc6986 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
>
Re: [PATCH] tests/qtest/migration-test: Disable migration/multifd/tcp/plain/cancel
* Peter Maydell (peter.mayd...@linaro.org) wrote: > On Tue, 14 Mar 2023 at 10:12, Dr. David Alan Gilbert > wrote: > > > > Copying Peter Xu on this one since it's poscopy, especially the newer > > postcopy preempt. > > > > * Peter Maydell (peter.mayd...@linaro.org) wrote: > > > On Sun, 12 Mar 2023 at 14:06, Peter Maydell > > > wrote: > > > > Here's another one, on the s390x CI runner: > > > > https://gitlab.com/qemu-project/qemu/-/jobs/3917587994 > > > > > > And here's a backtrace from a hung migration-test on the s390x > > > runner (looks like a deadlock, none of these processes were > > > using CPU): > > > > Ah a backtrace! > > I've attached another 2 -- migration-test seems to fairly > commonly hang on the s390 CI runner; there are several > stuck tests on it at the moment. These are the same as the last one I replied to; it's probably best if we disable /migration/postcopy/preempt/* until peterx has attacked it. Dave > -- PMM > Process tree: > migration-test(1841031)-+-qemu-system-x86(1841381) > `-qemu-system-x86(1841387) > === > PROCESS: 1841031 > gitlab-+ 1841031 1838397 0 Mar13 ?00:00:02 > /home/gitlab-runner/builds/FLaZkdt1/0/qemu-project/qemu/build/tests/qtest/migration-test > --tap -k > [New LWP 1841033] > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/s390x-linux-gnu/libthread_db.so.1". > __libc_recv (fd=fd@entry=5, buf=buf@entry=0x3ffdac792e7, len=len@entry=1, > flags=flags@entry=0) at ../sysdeps/unix/sysv/linux/recv.c:30 > > Thread 2 (Thread 0x3ffb01ff900 (LWP 1841033)): > #0 syscall () at ../sysdeps/unix/sysv/linux/s390/s390-64/syscall.S:37 > #1 0x02aa133dae34 in qemu_futex_wait (val=, f= out>) at > /home/gitlab-runner/builds/FLaZkdt1/0/qemu-project/qemu/include/qemu/futex.h:29 > #2 qemu_event_wait (ev=ev@entry=0x2aa135596b8 ) at > ../util/qemu-thread-posix.c:464 > #3 0x02aa134079ea in call_rcu_thread (opaque=opaque@entry=0x0) at > ../util/rcu.c:261 > #4 0x02aa133d9e9a in qemu_thread_start (args=) at > ../util/qemu-thread-posix.c:541 > #5 0x03ffb0b87e66 in start_thread (arg=0x3ffb01ff900) at > pthread_create.c:477 > #6 0x03ffb0a7cbe6 in thread_start () at > ../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:65 > > Thread 1 (Thread 0x3ffb126d990 (LWP 1841031)): > #0 __libc_recv (fd=fd@entry=5, buf=buf@entry=0x3ffdac792e7, len=len@entry=1, > flags=flags@entry=0) at ../sysdeps/unix/sysv/linux/recv.c:30 > #1 0x02aa133aba22 in recv (__flags=0, __n=1, __buf=0x3ffdac792e7, > __fd=5) at /usr/include/s390x-linux-gnu/bits/socket2.h:44 > #2 qmp_fd_receive (fd=) at ../tests/qtest/libqmp.c:73 > #3 0x02aa133a9b8e in qtest_qmp_receive_dict (s=0x2aa135fb800) at > ../tests/qtest/libqtest.c:837 > #4 qtest_qmp_eventwait_ref (event=, s=) at > ../tests/qtest/libqtest.c:837 > #5 qtest_qmp_eventwait_ref (s=0x2aa135fb800, event=) at > ../tests/qtest/libqtest.c:828 > #6 0x02aa133a9c1a in qtest_qmp_eventwait (s=, > event=) at ../tests/qtest/libqtest.c:850 > #7 0x02aa1339ec56 in test_postcopy_common (args=0x3ffdac795e8) at > ../tests/qtest/migration-test.c:1161 > #8 0x02aa1339edb4 in test_postcopy_preempt () at > ../tests/qtest/migration-test.c:1178 > #9 0x03ffb0d7e608 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #10 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #11 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #12 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #13 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #14 0x03ffb0d7eada in g_test_run_suite () from > /lib/s390x-linux-gnu/libglib-2.0.so.0 > #15 0x03ffb0d7eb10 in g_test_run () from > /lib/s390x-linux-gnu/libglib-2.0.so.0 > #16 0x02aa1339ab14 in main (argc=, argv=) > at ../tests/qtest/migration-test.c:2615 > [Inferior 1 (process 1841031) detached] > > === > PROCESS: 1841381 > gitlab-+ 1841381 1841031 0 Mar13 ?00:00:06 ./qemu-system-x86_64 > -qtest unix:/tmp/qtest-1841031.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-1841031.qmp,id=char0 -mon chardev=char0,mode=control > -display none -accel kvm -accel tcg -name source,debug-threads=on -m 150M > -serial file:/tmp/migration-test-RM5901/src_serial -drive > file=/tmp/migration-test-RM5901/bootsect,format=raw -accel qtest > [New LWP 1841383] > [New LWP 1841384] > [New LWP 1841385] > [New LWP 1841394] > [New LWP 1841395] > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/s390x-linux-gnu/libthread_db.so.1". > 0x03ffb51f1c8c in __ppoll (fds=0x2aa1c0ffbd0, nfds=5, timeout= out>, timeout@entry=0x3fffbb79bc8, sigmask=sigmask@entry=0x0) at > ../sysdeps/unix/sysv/linux/ppoll.c:44 > > Thread 6 (Thread 0x3ff469f7900 (LWP 1841395)): > #0 __libc_sendmsg (fd=,
Re: [PATCH] tests/qtest/migration-test: Disable migration/multifd/tcp/plain/cancel
On Tue, Mar 14, 2023 at 12:46:18PM +, Peter Maydell wrote: > On Tue, 14 Mar 2023 at 10:12, Dr. David Alan Gilbert > wrote: > > > > Copying Peter Xu on this one since it's poscopy, especially the newer > > postcopy preempt. > > > > * Peter Maydell (peter.mayd...@linaro.org) wrote: > > > On Sun, 12 Mar 2023 at 14:06, Peter Maydell > > > wrote: > > > > Here's another one, on the s390x CI runner: > > > > https://gitlab.com/qemu-project/qemu/-/jobs/3917587994 > > > > > > And here's a backtrace from a hung migration-test on the s390x > > > runner (looks like a deadlock, none of these processes were > > > using CPU): > > > > Ah a backtrace! > > I've attached another 2 -- migration-test seems to fairly > commonly hang on the s390 CI runner; there are several > stuck tests on it at the moment. > > -- PMM > Process tree: > migration-test(1841031)-+-qemu-system-x86(1841381) > `-qemu-system-x86(1841387) > === > Thread 1 (Thread 0x3ffb126d990 (LWP 1841031)): > #6 0x02aa133a9c1a in qtest_qmp_eventwait (s=, > event=) at ../tests/qtest/libqtest.c:850 > #7 0x02aa1339ec56 in test_postcopy_common (args=0x3ffdac795e8) at > ../tests/qtest/migration-test.c:1161 > #8 0x02aa1339edb4 in test_postcopy_preempt () at > ../tests/qtest/migration-test.c:1178 There's a stack frame optimized out between 6 & 7 which means we can't tell which event it is waiting for - could be a STOP or RESUME depending on where in migrate_postcopy_start it is. > === > PROCESS: 1841381 > gitlab-+ 1841381 1841031 0 Mar13 ?00:00:06 ./qemu-system-x86_64 > -qtest unix:/tmp/qtest-1841031.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-1841031.qmp,id=char0 -mon chardev=char0,mode=control > -display none -accel kvm -accel tcg -name source,debug-threads=on -m 150M > -serial file:/tmp/migration-test-RM5901/src_serial -drive > file=/tmp/migration-test-RM5901/bootsect,format=raw -accel qtest IIUC, the source QEMU > > Thread 6 (Thread 0x3ff469f7900 (LWP 1841395)): > #0 __libc_sendmsg (fd=, msg=msg@entry=0x3ff469f2810, > flags=flags@entry=0) at ../sysdeps/unix/sysv/linux/sendmsg.c:30 > #1 0x02aa1a34c52a in qio_channel_socket_writev (ioc=, > iov=, niov=, fds=, > nfds=, flags=0, errp=0x3ff469f2b80) at > ../io/channel-socket.c:605 > #2 0x02aa1a351b88 in qio_channel_writev_full > (ioc=ioc@entry=0x2aa1c0f8400, iov=0x2aa1c0e88e0, niov=64, fds=fds@entry=0x0, > nfds=nfds@entry=0, flags=0, errp=0x3ff469f2b80) at ../io/channel.c:108 > #3 0x02aa1a352a00 in qio_channel_writev_full_all (ioc=0x2aa1c0f8400, > iov=iov@entry=0x2aa1b2792a0, niov=, fds=fds@entry=0x0, > nfds=nfds@entry=0, flags=0, errp=0x3ff469f2b80) at ../io/channel.c:263 > #4 0x02aa1a352aae in qio_channel_writev_all (ioc=, > iov=iov@entry=0x2aa1b2792a0, niov=, > errp=errp@entry=0x3ff469f2b80) at ../io/channel.c:242 > #5 0x02aa1a10de94 in qemu_fflush (f=f@entry=0x2aa1b271260) at > ../migration/qemu-file.c:302 > #6 0x02aa1a10e122 in qemu_fflush (f=0x2aa1b271260) at > ../migration/qemu-file.c:297 > #7 add_to_iovec (f=f@entry=0x2aa1b271260, buf=, > size=size@entry=4096, may_free=) at > ../migration/qemu-file.c:510 > #8 0x02aa1a10e606 in qemu_put_buffer_async (f=f@entry=0x2aa1b271260, > buf=, > size=size@entry=4096, may_free=) at > ../migration/qemu-file.c:535 > #9 0x02aa1a2bd398 in save_normal_page (async=, > buf=, offset=1175552, block=0x2aa1b19f340, pss=0x3ff40003000) > at ../migration/ram.c:1400 > #10 ram_save_page (pss=0x3ff40003000, rs=0x3ff40003000) at > ../migration/ram.c:1449 > #11 ram_save_target_page_legacy (rs=0x3ff40003000, pss=0x3ff40003000) at > ../migration/ram.c:2381 > #12 0x02aa1a2ba330 in ram_save_host_page (pss=0x3ff40003000, > rs=0x3ff40003000) at ../migration/ram.c:2539 > #13 ram_find_and_save_block (rs=rs@entry=0x3ff40003000) at > ../migration/ram.c:2620 > #14 0x02aa1a2ba9e4 in ram_save_iterate (f=0x2aa1b271260, > opaque=) at ../migration/ram.c:3361 > #15 0x02aa1a12a524 in qemu_savevm_state_iterate (f=0x2aa1b271260, > postcopy=) at ../migration/savevm.c:1345 > #16 0x02aa1a11da14 in migration_iteration_run (s=0x2aa1af3a810) at > ../migration/migration.c:3896 > #17 migration_thread (opaque=opaque@entry=0x2aa1af3a810) at > ../migration/migration.c:4124 > #18 0x02aa1a4c42ca in qemu_thread_start (args=) at > ../util/qemu-thread-posix.c:541 > #19 0x03ffb5307e66 in start_thread (arg=0x3ff469f7900) at > pthread_create.c:477 > #20 0x03ffb51fcbe6 in thread_start () at > ../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:65 Blocked sending some RAM plages to the dst, presumably because the socket buffer is full. > Thread 5 (Thread 0x3ff471f8900 (LWP 1841394)): > #0 __libc_recvmsg (fd=, msg=msg@entry=0x3ff471f39f0, > flags=flags@entry=0) at ../sysdeps/unix/sysv/linux/recvmsg.c:30 > #1 0x02aa1a34c7e4 in
Re: [PATCH] tests/qtest/migration-test: Disable migration/multifd/tcp/plain/cancel
On Tue, 14 Mar 2023 at 10:12, Dr. David Alan Gilbert wrote: > > Copying Peter Xu on this one since it's poscopy, especially the newer > postcopy preempt. > > * Peter Maydell (peter.mayd...@linaro.org) wrote: > > On Sun, 12 Mar 2023 at 14:06, Peter Maydell > > wrote: > > > Here's another one, on the s390x CI runner: > > > https://gitlab.com/qemu-project/qemu/-/jobs/3917587994 > > > > And here's a backtrace from a hung migration-test on the s390x > > runner (looks like a deadlock, none of these processes were > > using CPU): > > Ah a backtrace! I've attached another 2 -- migration-test seems to fairly commonly hang on the s390 CI runner; there are several stuck tests on it at the moment. -- PMM Process tree: migration-test(1841031)-+-qemu-system-x86(1841381) `-qemu-system-x86(1841387) === PROCESS: 1841031 gitlab-+ 1841031 1838397 0 Mar13 ?00:00:02 /home/gitlab-runner/builds/FLaZkdt1/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k [New LWP 1841033] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/s390x-linux-gnu/libthread_db.so.1". __libc_recv (fd=fd@entry=5, buf=buf@entry=0x3ffdac792e7, len=len@entry=1, flags=flags@entry=0) at ../sysdeps/unix/sysv/linux/recv.c:30 Thread 2 (Thread 0x3ffb01ff900 (LWP 1841033)): #0 syscall () at ../sysdeps/unix/sysv/linux/s390/s390-64/syscall.S:37 #1 0x02aa133dae34 in qemu_futex_wait (val=, f=) at /home/gitlab-runner/builds/FLaZkdt1/0/qemu-project/qemu/include/qemu/futex.h:29 #2 qemu_event_wait (ev=ev@entry=0x2aa135596b8 ) at ../util/qemu-thread-posix.c:464 #3 0x02aa134079ea in call_rcu_thread (opaque=opaque@entry=0x0) at ../util/rcu.c:261 #4 0x02aa133d9e9a in qemu_thread_start (args=) at ../util/qemu-thread-posix.c:541 #5 0x03ffb0b87e66 in start_thread (arg=0x3ffb01ff900) at pthread_create.c:477 #6 0x03ffb0a7cbe6 in thread_start () at ../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:65 Thread 1 (Thread 0x3ffb126d990 (LWP 1841031)): #0 __libc_recv (fd=fd@entry=5, buf=buf@entry=0x3ffdac792e7, len=len@entry=1, flags=flags@entry=0) at ../sysdeps/unix/sysv/linux/recv.c:30 #1 0x02aa133aba22 in recv (__flags=0, __n=1, __buf=0x3ffdac792e7, __fd=5) at /usr/include/s390x-linux-gnu/bits/socket2.h:44 #2 qmp_fd_receive (fd=) at ../tests/qtest/libqmp.c:73 #3 0x02aa133a9b8e in qtest_qmp_receive_dict (s=0x2aa135fb800) at ../tests/qtest/libqtest.c:837 #4 qtest_qmp_eventwait_ref (event=, s=) at ../tests/qtest/libqtest.c:837 #5 qtest_qmp_eventwait_ref (s=0x2aa135fb800, event=) at ../tests/qtest/libqtest.c:828 #6 0x02aa133a9c1a in qtest_qmp_eventwait (s=, event=) at ../tests/qtest/libqtest.c:850 #7 0x02aa1339ec56 in test_postcopy_common (args=0x3ffdac795e8) at ../tests/qtest/migration-test.c:1161 #8 0x02aa1339edb4 in test_postcopy_preempt () at ../tests/qtest/migration-test.c:1178 #9 0x03ffb0d7e608 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 #10 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 #11 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 #12 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 #13 0x03ffb0d7e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 #14 0x03ffb0d7eada in g_test_run_suite () from /lib/s390x-linux-gnu/libglib-2.0.so.0 #15 0x03ffb0d7eb10 in g_test_run () from /lib/s390x-linux-gnu/libglib-2.0.so.0 #16 0x02aa1339ab14 in main (argc=, argv=) at ../tests/qtest/migration-test.c:2615 [Inferior 1 (process 1841031) detached] === PROCESS: 1841381 gitlab-+ 1841381 1841031 0 Mar13 ?00:00:06 ./qemu-system-x86_64 -qtest unix:/tmp/qtest-1841031.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1841031.qmp,id=char0 -mon chardev=char0,mode=control -display none -accel kvm -accel tcg -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-RM5901/src_serial -drive file=/tmp/migration-test-RM5901/bootsect,format=raw -accel qtest [New LWP 1841383] [New LWP 1841384] [New LWP 1841385] [New LWP 1841394] [New LWP 1841395] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/s390x-linux-gnu/libthread_db.so.1". 0x03ffb51f1c8c in __ppoll (fds=0x2aa1c0ffbd0, nfds=5, timeout=, timeout@entry=0x3fffbb79bc8, sigmask=sigmask@entry=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:44 Thread 6 (Thread 0x3ff469f7900 (LWP 1841395)): #0 __libc_sendmsg (fd=, msg=msg@entry=0x3ff469f2810, flags=flags@entry=0) at ../sysdeps/unix/sysv/linux/sendmsg.c:30 #1 0x02aa1a34c52a in qio_channel_socket_writev (ioc=, iov=, niov=, fds=, nfds=, flags=0, errp=0x3ff469f2b80) at ../io/channel-socket.c:605 #2 0x02aa1a351b88 in qio_channel_writev_full (ioc=ioc@entry=0x2aa1c0f8400, iov=0x2aa1c0e88e0, niov=64, fds=fds@entry=0x0, nfds=nfds@entry=0, flags=0, errp=0x3ff469f2b80) at
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On 14/03/2023 06:33, Michael S. Tsirkin wrote:
On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
xenfv machine when the guest is configured for igd-passthru.
A desired extension to that commit is to allow use of the reserved slot
if the administrator manually configures a device to use the reserved
slot. Currently, slot_reserved_mask is enforced unconditionally. With
this patch, the pci bus can be configured so the slot is only reserved
if the pci device to be added to the bus is configured for automatic
slot assignment.
To enable the desired behavior of slot_reserved_mask machine, add a
boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
add a function pci_bus_ignore_slot_reserved_mask_manual which can be
called to change the default behavior of always enforcing
slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
when the pci device being added is configured for automatic slot
assignment.
Call the new pci_bus_ignore_slot_reserved_mask_manual function after
creating the pci bus for the pc/i440fx/xenfv machine type to implement
the desired behavior of causing slot_reserved_mask to only apply when
the pci device to be added to a pc/i440fx/xenfv machine is configured
for automatic slot assignment.
Link:
https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
Signed-off-by: Chuck Zmudzinski
I really dislike this.
It seems that xen should not have used slot_reserved_mask,
and instead needs something new like slot_manual_mask.
No?
My suggestion was to move the validation logic to a separate callback function in
PCIBus (see https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
perhaps I wasn't clear enough in pointing out that I was thinking this could
*replace* the existing slot_reserved_mask mechanism, rather than providing a hook to
allow it to be manipulated.
Here's a very rough patch put together over lunch that attempts this for
pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn implementation,
and slot_reserved_mask gets removed completely i.e.:
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..30b856499a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
return host_bridge->bypass_iommu;
}
+static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
+ int devfn)
+{
+/* All slots accessible by default */
+return false;
+}
+
static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
@@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus, DeviceState
*parent,
{
assert(PCI_FUNC(devfn_min) == 0);
bus->devfn_min = devfn_min;
-bus->slot_reserved_mask = 0x0;
+bus->slot_reserved_fn = pci_bus_default_slot_reserved;
bus->address_space_mem = address_space_mem;
bus->address_space_io = address_space_io;
bus->flags |= PCI_BUS_IS_ROOT;
@@ -,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, int
devfn)
return !(bus->devices[devfn]);
}
-static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
+static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
+ PCISlotReservationType restype)
+{
+return bus->slot_reserved_fn(restype, devfn);
+}
+
+void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
{
-return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
+bus->slot_reserved_fn = fn;
}
/* -1 for devfn means auto assign */
@@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev,
for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
devfn += PCI_FUNC_MAX) {
if (pci_bus_devfn_available(bus, devfn) &&
- !pci_bus_devfn_reserved(bus, devfn)) {
+ !pci_bus_devfn_reserved(bus, devfn,
PCI_SLOT_RESERVATION_AUTO)) {
goto found;
}
}
@@ -1149,7 +1162,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev,
"or reserved", name);
return NULL;
found: ;
-} else if (pci_bus_devfn_reserved(bus, devfn)) {
+} else if (pci_bus_devfn_reserved(bus, devfn,
PCI_SLOT_RESERVATION_MANUAL)) {
error_setg(errp, "PCI: slot %d function %d not available for %s,"
MemoryRegion *address_space_io,
@@ -500,7 +507,7 @@ static void pci_root_bus_internal_init(PCIBus *bus, DeviceState
*parent,
{
assert(PCI_FUNC(devfn_min) == 0);
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On 3/14/2023 2:33 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> > Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
> > uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
> > xenfv machine when the guest is configured for igd-passthru.
> >
> > A desired extension to that commit is to allow use of the reserved slot
> > if the administrator manually configures a device to use the reserved
> > slot. Currently, slot_reserved_mask is enforced unconditionally. With
> > this patch, the pci bus can be configured so the slot is only reserved
> > if the pci device to be added to the bus is configured for automatic
> > slot assignment.
> >
> > To enable the desired behavior of slot_reserved_mask machine, add a
> > boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> > add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> > called to change the default behavior of always enforcing
> > slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> > when the pci device being added is configured for automatic slot
> > assignment.
> >
> > Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> > creating the pci bus for the pc/i440fx/xenfv machine type to implement
> > the desired behavior of causing slot_reserved_mask to only apply when
> > the pci device to be added to a pc/i440fx/xenfv machine is configured
> > for automatic slot assignment.
> >
> > Link:
> > https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
> > Signed-off-by: Chuck Zmudzinski
>
> I really dislike this.
> It seems that xen should not have used slot_reserved_mask,
> and instead needs something new like slot_manual_mask.
> No?
Actually, xen would use something like slot_auto_mask, and
sun4u would use both slot_auto_mask and slot_manual_mask.
Is it just that this patch touches hw/pci-host/i440fx.c that you
don't like or is it that you don't like adding slot_reserved_mask_manual
and pci_bus_ignore_slot_reserved_mask_manual, or is it both
that you don't like?
If it's the former that you don't like, the call to
pci_bus_ignore_slot_reserved_mask_manual can be moved to
xen_igd_reserve_slot in hw/xen/xen_pt.c and this would
avoid touching hw/pci-host/i440fx.c.
If it's the latter that you don't like, both slot_reserved_mask_manual
and pci_bus_ignore_slot_reserved_mask_manual can be removed
and this can be implemented with two independent slot masks:
rename slot_reserved_mask as slot_auto_mask - used by both xen and sun4u
slot_manual_mask - new mask, used only by sun4u.
We would also need to have two sets of accessor functions in this case, one
set to access slot_auto_mask, and the other to access slot_manual_mask.
Since the sun4u machine does not need to either get the value of
slot_manual_mask or clear the slot_manual_mask, slot_manual_mask
would only need to have one accessor function to set the value of the
mask. slot_auto_mask would have all three accessor functions that xen
needs to use.
Would that be OK?
>
> > ---
> > Changelog
> >
> > v2: Change Subject of patch from
> > "pci: add enforce_slot_reserved_mask_manual property" To
> > "pci: allow slot_reserved_mask to be ignored with manual slot
> > assignment"
> >
> > Add pci_bus_ignore_slot_reserved_mask_manual function
> >
> > Call pci_bus_ignore_slot_reserved_mask_manual at appropriate place
> > in hw/pci-host/i440fx.c
> >
> > hw/pci-host/i440fx.c | 1 +
> > hw/pci/pci.c | 14 +-
> > include/hw/pci/pci.h | 1 +
> > include/hw/pci/pci_bus.h | 1 +
> > 4 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> > index 262f82c303..8e00b88926 100644
> > --- a/hw/pci-host/i440fx.c
> > +++ b/hw/pci-host/i440fx.c
> > @@ -257,6 +257,7 @@ PCIBus *i440fx_init(const char *pci_type,
> > s = PCI_HOST_BRIDGE(dev);
> > b = pci_root_bus_new(dev, NULL, pci_address_space,
> > address_space_io, 0, TYPE_PCI_BUS);
> > +pci_bus_ignore_slot_reserved_mask_manual(b);
> > s->bus = b;
> > object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 8a87ccc8b0..670ecc6986 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -501,6 +501,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
> > DeviceState *parent,
> > assert(PCI_FUNC(devfn_min) == 0);
> > bus->devfn_min = devfn_min;
> > bus->slot_reserved_mask = 0x0;
> > +bus->enforce_slot_reserved_mask_manual = true;
> > bus->address_space_mem = address_space_mem;
> > bus->address_space_io = address_space_io;
> > bus->flags |= PCI_BUS_IS_ROOT;
> > @@ -1116,6 +1117,17 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int
> > devfn)
> >
Re: [PATCH nbd 2/4] nbd: Split out block device state from underlying NBD connections
On Thu, Mar 09, 2023 at 11:39:44AM +, Richard W.M. Jones wrote:
> To implement multi-conn, we will put multiple underlying NBD
> connections (ie. NBDClientConnection) inside the NBD block device
> handle (BDRVNBDState). This requires first breaking the one-to-one
> relationship between NBDClientConnection and BDRVNBDState.
>
> To do this a new structure (NBDConnState) is implemented.
> NBDConnState takes all the per-connection state out of the block
> driver struct. BDRVNBDState now contains a conns[] array of pointers
> to NBDConnState, one for each underlying multi-conn connection.
>
> After this change there is still a one-to-one relationship because we
> only ever use the zeroth slot in the conns[] array. Thus this does
> not implement multi-conn yet.
>
> Signed-off-by: Richard W.M. Jones
> ---
> block/coroutines.h | 5 +-
> block/nbd.c| 674 -
> 2 files changed, 358 insertions(+), 321 deletions(-)
>
> diff --git a/block/coroutines.h b/block/coroutines.h
> index dd9f3d449b..14b01d8591 100644
> --- a/block/coroutines.h
> +++ b/block/coroutines.h
> @@ -62,7 +62,7 @@ int coroutine_fn GRAPH_RDLOCK
> bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t
> pos);
>
> int coroutine_fn
> -nbd_co_do_establish_connection(BlockDriverState *bs, bool blocking,
> +nbd_co_do_establish_connection(BlockDriverState *bs, void *cs, bool blocking,
> Error **errp);
I guess the void* here is because you couldn't find a way to expose
the new type through the coroutine wrapper generator?
>
>
> @@ -86,6 +86,7 @@ bdrv_common_block_status_above(BlockDriverState *bs,
> BlockDriverState **file,
> int *depth);
> int co_wrapper_mixed
> -nbd_do_establish_connection(BlockDriverState *bs, bool blocking, Error
> **errp);
> +nbd_do_establish_connection(BlockDriverState *bs, void *cs, bool blocking,
> +Error **errp);
>
same here
> #endif /* BLOCK_COROUTINES_H */
> diff --git a/block/nbd.c b/block/nbd.c
> index 5ffae0b798..84e8a1add0 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -51,8 +51,8 @@
> #define MAX_NBD_REQUESTS16
> #define MAX_MULTI_CONN 16
>
> -#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
> -#define INDEX_TO_HANDLE(bs, index) ((index) ^ (uint64_t)(intptr_t)(bs))
> +#define HANDLE_TO_INDEX(cs, handle) ((handle) ^ (uint64_t)(intptr_t)(cs))
> +#define INDEX_TO_HANDLE(cs, index) ((index) ^ (uint64_t)(intptr_t)(cs))
Independently of your patches, these macros are odd. I think we could
just as easily define
#define HANDLE_TO_INDEX(XX, handle) ((handle) - 1)
#define INDEX_TO_HANDLE(XX, index) ((index) + 1)
and then refactor to drop the unused parameter, since we never really
depend on it (I audited the code when you first asked me about it on
IRC, and we do a bounds check that whatever the server returns lies
within our expected index of 0-15 before dereferencing any memory with
it, so we are NOT relying on the server passing us a pointer that we
depend on).
Overall, your patch is mostly mechanical and looks nice to me.
>
> /*
> * Update @bs with information learned during a completed negotiation
> process.
> * Return failure if the server's advertised options are incompatible with
> the
> * client's needs.
> + *
> + * Note that we are only called for the first connection (s->conns[0])
> + * because multi-conn assumes that all other connections are alike.
> + * We don't check that assumption but probably should (XXX).
> */
> -static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
> +static int nbd_handle_updated_info(BlockDriverState *bs,
> + NBDConnState *cs, Error **errp)
But as you pointed out in your cover letter, we'll probably want to
address the XXX comments like this one prior to actually committing
the series. We really should be making sure that the secondary
clients see the same server parameters as the first one, regardless of
whether they are open in parallel (once multi-conn is enabled) or in
series after a reopen (which is what we currently try to support).
> @@ -318,129 +332,132 @@ static int nbd_handle_updated_info(BlockDriverState
> *bs, Error **errp)
> }
>
> int coroutine_fn nbd_co_do_establish_connection(BlockDriverState *bs,
> +void *csvp,
> bool blocking, Error **errp)
> {
> BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> +NBDConnState *cs = csvp;
Is there a way to get the new type passed through the coroutine
generator without the use of void*?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [RESEND PATCH v6 8/8] hw/mem/cxl_type3: Add CXL RAS Error Injection Support.
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 7e5ad65c1d..d589f78202 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -232,6 +232,14 @@ REG64(CXL_MEM_DEV_STS, 0)
> > FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
> > FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
> >
> > +typedef struct CXLError {
> > +QTAILQ_ENTRY(CXLError) node;
> > +int type; /* Error code as per FE definition */
> > +uint32_t header[32];
> Instead of using 32 here, would it be better to use
> CXL_RAS_ERR_HEADER_NUM?
Yes, that would be better. Please send a patch.
> > +} CXLError;
Re: [PATCH] DO-NOT-MERGE: pipewire sample code
Hi Volker,
Thank you for the clarification. I see the problem now.
So is it safe to say that:
@@ -104,8 +104,9 @@ playback_on_process(void *data)
/* calculate the total no of bytes to read data from buffer */
req = b->requested * v->frame_size;
if (req == 0) {
-req = 4096 * v->frame_size;
+req = v->g->dev->timer_period * v->info.rate * v->frame_size * 1 /
2 / 100;
}
I used 50% of the timer-period and the frame_size which would give a close
margin to what the downstream audio device writes to the DAC.
Thanks,
Dorinda.
On Mon, Mar 13, 2023 at 9:06 PM Volker Rümelin wrote:
> Am 13.03.23 um 13:28 schrieb Dorinda Bassey:
> > Hi Volker,
> >
> > Thanks for the patch, I've tested the patch and it works. I don't hear
> > the choppy audio with this option "qemu-system-x86_64 -device
> > ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
> > pipewire,id=audio0,out.frequency=96000,in.frequency=96000 "
> >
> > I don't understand how the req == 0 case can work at all.
> >
> > how this works is that b->requested could be zero when no suggestion
> > is provided. For playback streams, this field contains the suggested
> > amount of data to provide. hence the reason for this check.
>
> Hi Dorinda,
>
> there has to be a control mechanism that ensures that our write rate on
> average is exactly the frame rate that the down stream audio device
> writes to the DAC. My question was how can this work if we always write
> 4096 frames.
>
> The answer is, that after a 4096 frames write, the callback is delayed
> by 4096 frames / 44100 frames/s = 93ms. This ensures that our write rate
> is exactly 44100 frames/s.
>
> This means a fixed 4096 frames write is wrong for the req == 0 case. We
> have to write 75% of timer-period frames.
>
> If you want to test this yourself, just ignore req and assume it's 0.
>
> With best regards,
> Volker
>
> >
> > I suggest to use the same option names as the pulseaudio backend.
> > out.latency is the effective Pipewire buffer size.
> >
> > Ack.
> >
> > Thanks,
> > Dorinda.
> >
> >
> > On Sat, Mar 11, 2023 at 5:19 PM Volker Rümelin
> > wrote:
> >
> > > Based-on:<20230306171020.381116-1-dbas...@redhat.com>
> > > ([PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU)
> > >
> > > This is sample code for the review of the pipewire backed. The
> > > code actually works.
> > >
> > > An email with explanations for the changes will follow.
> > >
> > > Signed-off-by: Volker Rümelin
> > > ---
> > > audio/pwaudio.c | 67
> > +
> > > qapi/audio.json | 10 +++-
> > > 2 files changed, 49 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> > > index d357761152..8e2a38938f 100644
> > > --- a/audio/pwaudio.c
> > > +++ b/audio/pwaudio.c
> > > @@ -23,7 +23,6 @@
> > > #define AUDIO_CAP "pipewire"
> > > #define RINGBUFFER_SIZE(1u << 22)
> > > #define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
> > > -#define BUFFER_SAMPLES512
> > >
> > > #include "audio_int.h"
> > >
> > > @@ -48,6 +47,7 @@ typedef struct PWVoice {
> > > struct pw_stream *stream;
> > > struct spa_hook stream_listener;
> > > struct spa_audio_info_raw info;
> > > +uint32_t highwater_mark;
> > > uint32_t frame_size;
> > > struct spa_ringbuffer ring;
> > > uint8_t buffer[RINGBUFFER_SIZE];
> > > @@ -82,7 +82,7 @@ playback_on_process(void *data)
> > > void *p;
> > > struct pw_buffer *b;
> > > struct spa_buffer *buf;
> > > -uint32_t n_frames, req, index, n_bytes;
> > > +uint32_t req, index, n_bytes;
> > > int32_t avail;
> > >
> > > if (!v->stream) {
> > > @@ -105,8 +105,7 @@ playback_on_process(void *data)
> > > if (req == 0) {
> > > req = 4096 * v->frame_size;
> > > }
> >
> > I don't understand how the req == 0 case can work at all. The
> > downstream
> > audio device is the thinnest point in the playback stream. We can't
> > write more audio frames than the audio device will consume.
> >
>
>
[PATCH] docs/sphinx/kerneldoc.py: Honour --enable-werror
Currently, the kerneldoc Sphinx plugin doesn't honour the
--enable-werror configure option, so its warnings are never fatal.
This is because although we do pass sphinx-build the -W switch, the
warnings from kerneldoc are produced by the scripts/kernel-doc script
directly and don't go through Sphinx's "emit a warning" function.
When --enable-werror is in effect, pass sphinx-build an extra
argument -Dkerneldoc_werror=1. The kerneldoc plugin can then use
this to determine whether it should be passing the kernel-doc script
-Werror.
We do this because there is no documented mechanism for
a Sphinx plugin to determine whether sphinx-build was
passed -W or not; if one is provided then we can switch to
that at a later date:
https://github.com/sphinx-doc/sphinx/issues/11239
Signed-off-by: Peter Maydell
---
NB: we need to land the fix for the current outstanding
warning before this one can go in...
https://lore.kernel.org/qemu-devel/20230310103123.2118519-11-alex.ben...@linaro.org/
---
docs/meson.build | 2 +-
docs/sphinx/kerneldoc.py | 5 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/docs/meson.build b/docs/meson.build
index bb72c10ea8c..f220800e3e5 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -7,7 +7,7 @@ if sphinx_build.found()
SPHINX_ARGS = ['env', 'CONFDIR=' + qemu_confdir, sphinx_build, '-q']
# If we're making warnings fatal, apply this to Sphinx runs as well
if get_option('werror')
-SPHINX_ARGS += [ '-W' ]
+SPHINX_ARGS += [ '-W', '-Dkerneldoc_werror=1' ]
endif
# This is a bit awkward but works: create a trivial document and
diff --git a/docs/sphinx/kerneldoc.py b/docs/sphinx/kerneldoc.py
index bf442150165..72c403a7379 100644
--- a/docs/sphinx/kerneldoc.py
+++ b/docs/sphinx/kerneldoc.py
@@ -74,6 +74,10 @@ def run(self):
# Sphinx versions
cmd += ['-sphinx-version', sphinx.__version__]
+# Pass through the warnings-as-errors flag
+if env.config.kerneldoc_werror:
+cmd += ['-Werror']
+
filename = env.config.kerneldoc_srctree + '/' + self.arguments[0]
export_file_patterns = []
@@ -167,6 +171,7 @@ def setup(app):
app.add_config_value('kerneldoc_bin', None, 'env')
app.add_config_value('kerneldoc_srctree', None, 'env')
app.add_config_value('kerneldoc_verbosity', 1, 'env')
+app.add_config_value('kerneldoc_werror', 0, 'env')
app.add_directive('kernel-doc', KernelDocDirective)
--
2.34.1
Re: [PATCH v7 1/6] memory: Reference as->current_map directly in memory commit
Chuang Xu writes:
> From: Peter Xu
>
> Calling RCU variance of address_space_get|to_flatview() during memory
"variants" rather than "variance", perhaps?
> commit (flatview updates, triggering memory listeners, or updating
> ioeventfds, etc.) is not 100% accurate, because commit() requires BQL
> rather than RCU read lock, so the context exclusively owns current_map and
> can be directly referenced.
>
> Neither does it need a refcount to current_map because it cannot be freed
> from under the caller.
>
> Add address_space_get_flatview_raw() for the case where the context holds
> BQL rather than RCU read lock and use it across the core memory updates,
> Drop the extra refcounts on FlatView*.
>
> Signed-off-by: Peter Xu
> ---
> softmmu/memory.c | 28
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 4699ba55ec..a992a365d9 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -61,6 +61,13 @@ struct AddrRange {
> Int128 size;
> };
>
> +/* Called with BQL held */
> +static inline FlatView *address_space_to_flatview_raw(AddressSpace *as)
> +{
> +assert(qemu_mutex_iothread_locked());
> +return as->current_map;
> +}
> +
> static AddrRange addrrange_make(Int128 start, Int128 size)
> {
> return (AddrRange) { start, size };
> @@ -155,7 +162,7 @@ enum ListenerDirection { Forward, Reverse };
> #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...) \
> do {\
> MemoryRegionSection mrs = section_from_flat_range(fr, \
> -address_space_to_flatview(as)); \
> +address_space_to_flatview_raw(as)); \
> MEMORY_LISTENER_CALL(as, callback, dir, , ##_args); \
> } while(0)
>
> @@ -753,6 +760,7 @@ static FlatView *generate_memory_topology(MemoryRegion
> *mr)
> }
>
> static void address_space_add_del_ioeventfds(AddressSpace *as,
> + FlatView *view,
> MemoryRegionIoeventfd *fds_new,
> unsigned fds_new_nb,
> MemoryRegionIoeventfd *fds_old,
> @@ -774,7 +782,7 @@ static void address_space_add_del_ioeventfds(AddressSpace
> *as,
>_new[inew]))) {
> fd = _old[iold];
> section = (MemoryRegionSection) {
> -.fv = address_space_to_flatview(as),
> +.fv = view,
> .offset_within_address_space = int128_get64(fd->addr.start),
> .size = fd->addr.size,
> };
> @@ -787,7 +795,7 @@ static void address_space_add_del_ioeventfds(AddressSpace
> *as,
> _old[iold]))) {
> fd = _new[inew];
> section = (MemoryRegionSection) {
> -.fv = address_space_to_flatview(as),
> +.fv = view,
> .offset_within_address_space = int128_get64(fd->addr.start),
> .size = fd->addr.size,
> };
> @@ -833,7 +841,7 @@ static void address_space_update_ioeventfds(AddressSpace
> *as)
> ioeventfd_max = QEMU_ALIGN_UP(as->ioeventfd_nb, 4);
> ioeventfds = g_new(MemoryRegionIoeventfd, ioeventfd_max);
>
> -view = address_space_get_flatview(as);
> +view = address_space_to_flatview_raw(as);
> FOR_EACH_FLAT_RANGE(fr, view) {
> for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
> tmp = addrrange_shift(fr->mr->ioeventfds[i].addr,
> @@ -852,13 +860,12 @@ static void
> address_space_update_ioeventfds(AddressSpace *as)
> }
> }
>
> -address_space_add_del_ioeventfds(as, ioeventfds, ioeventfd_nb,
> +address_space_add_del_ioeventfds(as, view, ioeventfds, ioeventfd_nb,
> as->ioeventfds, as->ioeventfd_nb);
>
> g_free(as->ioeventfds);
> as->ioeventfds = ioeventfds;
> as->ioeventfd_nb = ioeventfd_nb;
> -flatview_unref(view);
> }
>
> /*
> @@ -1026,7 +1033,7 @@ static void flatviews_reset(void)
>
> static void address_space_set_flatview(AddressSpace *as)
> {
> -FlatView *old_view = address_space_to_flatview(as);
> +FlatView *old_view = address_space_to_flatview_raw(as);
> MemoryRegion *physmr = memory_region_get_flatview_root(as->root);
> FlatView *new_view = g_hash_table_lookup(flat_views, physmr);
>
> @@ -2979,8 +2986,7 @@ static void listener_add_address_space(MemoryListener
> *listener,
> listener->log_global_start(listener);
> }
> }
> -
> -view = address_space_get_flatview(as);
> +view = address_space_to_flatview_raw(as);
> FOR_EACH_FLAT_RANGE(fr, view) {
> MemoryRegionSection section
[PATCH 2/4] hw/pci/msi: Ensure msi_init() is called before device is realized
A PCI device can't magically become MSI-capable at runtime. Guests aren't expecting that. Assert MSI is initialized _before_ a device instance is realized. Signed-off-by: Philippe Mathieu-Daudé --- hw/pci/msi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/pci/msi.c b/hw/pci/msi.c index 5de6df8154..dfa257cc22 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -182,6 +182,7 @@ bool msi_enabled(const PCIDevice *dev) * address. * If @msi_per_vector_mask, make the device support per-vector masking. * @errp is for returning errors. + * @dev must be unrealized. * Return 0 on success; set @errp and return -errno on error. * * -ENOTSUP means lacking msi support for a msi-capable platform. @@ -208,6 +209,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, " 64bit %d mask %d\n", offset, nr_vectors, msi64bit, msi_per_vector_mask); +assert(!DEVICE(dev)->realized); assert(!(nr_vectors & (nr_vectors - 1))); /* power of 2 */ assert(nr_vectors > 0); assert(nr_vectors <= PCI_MSI_VECTORS_MAX); -- 2.38.1
[PATCH 3/4] hw/pci: Add sanity check in pci_find_space()
This 'used' array is allocated via:
pci_qdev_realize() -> do_pci_register_device() -> pci_config_alloc()
In a perfect world where all device models are correctly QOM'ified
this can't happen. Still it occured to me while refactoring QDev and
it was not obvious to figure out. This assert helped, so keep it.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/pci/pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..ac41fcbf6a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2185,6 +2185,7 @@ static uint8_t pci_find_space(PCIDevice *pdev, uint8_t
size)
{
int offset = PCI_CONFIG_HEADER_SIZE;
int i;
+assert(pdev->used);
for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i) {
if (pdev->used[i])
offset = i + 1;
--
2.38.1
[PATCH 4/4] hw/pci: Ensure pci_add_capability() is called before device is realized
PCI capabilities can't appear magically at runtime.
Guests aren't expecting that. Assert all capabilities
are added _before_ a device instance is realized.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/pci/pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ac41fcbf6a..ed60b352e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2397,7 +2397,7 @@ static void pci_del_option_rom(PCIDevice *pdev)
* On success, pci_add_capability() returns a positive value
* that the offset of the pci capability.
* On failure, it sets an error and returns a negative error
- * code.
+ * code. @pdev must be unrealized.
*/
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
@@ -2406,6 +2406,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
uint8_t *config;
int i, overlapping_cap;
+assert(!DEVICE(pdev)->realized);
+
if (!offset) {
offset = pci_find_space(pdev, size);
/* out of PCI config space is programming error */
--
2.38.1
[PATCH 1/4] hw/pci/msi: Fix debug format string
Fix this format string warning when defining MSI_DEBUG:
hw/pci/msi.c:209:28: warning: format specifies type 'char' but the argument
has type 'unsigned int' [-Wformat]
offset, nr_vectors, msi64bit, msi_per_vector_mask);
^~
hw/pci/msi.c:83:61: note: expanded from macro 'MSI_DEV_PRINTF'
MSI_DPRINTF("%s:%x " fmt, (dev)->name, (dev)->devfn, ## __VA_ARGS__)
^~~~
hw/pci/msi.c:78:58: note: expanded from macro 'MSI_DPRINTF'
fprintf(stderr, "%s:%d " fmt, __func__, __LINE__, ## __VA_ARGS__)
~~~ ^~~
Signed-off-by: Philippe Mathieu-Daudé
---
hw/pci/msi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 041b0bdbec..5de6df8154 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -204,7 +204,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
}
MSI_DEV_PRINTF(dev,
- "init offset: 0x%"PRIx8" vector: %"PRId8
+ "init offset: 0x%"PRIx8" vector: %u"
" 64bit %d mask %d\n",
offset, nr_vectors, msi64bit, msi_per_vector_mask);
--
2.38.1
[PATCH 0/4] hw/pci: Ensure capabilities are added before calling pci_qdev_realize()
Per MST in [*]: "Calling pci_add_capability when VM is running is likely to confuse guests". Ensure this can't happen by asserting pci_add_capability() is never called after a PCI device is realized. [*] https://lore.kernel.org/qemu-devel/20230308071628-mutt-send-email-...@kernel.org/ Based-on: <20230313153031.86107-1-phi...@linaro.org> "hw/i386/amd_iommu: Orphanize & QDev cleanups" Philippe Mathieu-Daudé (4): hw/pci/msi: Fix debug format string hw/pci/msi: Ensure msi_init() is called before device is realized hw/pci: Add sanity check in pci_find_space() hw/pci: Ensure pci_add_capability() is called before device is realized hw/pci/msi.c | 4 +++- hw/pci/pci.c | 5 - 2 files changed, 7 insertions(+), 2 deletions(-) -- 2.38.1
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
On Monday, March 13, 2023 8:06:15 PM CET Dorinda Bassey wrote:
> >
> > Are you sure about sizeof(n_bytes) here? That's 4. ;-)
> >
> my bad!
>
> >
> > Volker's point was that "silence" is the center of the wave range. With
> > signed
> > range that's zero, yes, but with unsigned range that's 2^(bitdepth) / 2.
> >
> > So you need to memset() the correct value to generate "silence".
> >
> I understand now, Thanks. I guess it should work for signed range, so I
> would do:
>
> @@ -117,7 +117,9 @@ playback_on_process(void *data)
> }
>
> if (avail == 0) {
> -memset(p, 0, n_bytes);
> +memset(p, 0, (int32_t) n_bytes);
No, that would not fix anything. You are actually making several false
assumptions here.
Anyway, in audio/audio.c there is a function which does it correctly:
audio_pcm_info_clear_buf(struct audio_pcm_info *info, void *buf, int len)
So you probably just want to call this function instead to generate silence
correctly. Keep in mind though that it's `len` argument is in sample points,
not in bytes.
[PATCH 0/2] target/s390x: Implement Early Exception Recognition
Hi, Currently loading bad PSW flags does not lead to an exception, which is not correct. This series fixes this by implementing what PoP calls "Early Exception Recognition". Since it applies to both loading PSW with LPSW/LPSWE and to interrupt handling, s390_cpu_set_psw() looks like the right place for it to be in. Patch 1 fixes the issue, patch 2 adds a test. Best regards, Ilya Ilya Leoshkevich (2): target/s390x: Implement Early Exception Recognition tests/tcg/s390x: Add early-exception-recognition.S target/s390x/cpu.c| 26 + target/s390x/cpu.h| 1 + target/s390x/tcg/excp_helper.c| 3 +- tests/tcg/s390x/Makefile.softmmu-target | 1 + tests/tcg/s390x/early-exception-recognition.S | 38 +++ 5 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 tests/tcg/s390x/early-exception-recognition.S -- 2.39.2
[PATCH 1/2] target/s390x: Implement Early Exception Recognition
Generate specification exception if a reserved bit is set in the PSW
mask or if the PSW address is out of bounds dictated by the addresing
mode.
Reported-by: Nina Schoetterl-Glausch
Signed-off-by: Ilya Leoshkevich
---
target/s390x/cpu.c | 26 ++
target/s390x/cpu.h | 1 +
target/s390x/tcg/excp_helper.c | 3 ++-
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index b10a8541ff8..8e6e46aa3d8 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -41,6 +41,26 @@
#define CR0_RESET 0xE0UL
#define CR14_RESET 0xC200UL;
+#ifndef CONFIG_USER_ONLY
+static bool is_early_exception_recognized(uint64_t mask, uint64_t addr)
+{
+if (mask & PSW_MASK_RESERVED) {
+return true;
+}
+
+switch (mask & (PSW_MASK_32 | PSW_MASK_64)) {
+case 0:
+return addr & ~0xffULL;
+case PSW_MASK_32:
+return addr & ~0x7fffULL;
+case PSW_MASK_32 | PSW_MASK_64:
+return false;
+default: /* PSW_MASK_64 */
+return true;
+}
+}
+#endif
+
void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
{
#ifndef CONFIG_USER_ONLY
@@ -57,6 +77,12 @@ void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask,
uint64_t addr)
env->cc_op = (mask >> 44) & 3;
#ifndef CONFIG_USER_ONLY
+if (is_early_exception_recognized(mask, addr)) {
+env->int_pgm_ilen = 0;
+trigger_pgm_exception(env, PGM_SPECIFICATION);
+return;
+}
+
if ((old_mask ^ mask) & PSW_MASK_PER) {
s390_cpu_recompute_watchpoints(env_cpu(env));
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b2..b4de6945936 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -292,6 +292,7 @@ extern const VMStateDescription vmstate_s390_cpu;
#define PSW_MASK_32 0x8000ULL
#define PSW_MASK_SHORT_ADDR 0x7fffULL
#define PSW_MASK_SHORT_CTRL 0x8000ULL
+#define PSW_MASK_RESERVED 0xb87e7fffULL
#undef PSW_ASC_PRIMARY
#undef PSW_ASC_ACCREG
diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index bc767f04438..a7829b1e494 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -212,7 +212,8 @@ static void do_program_interrupt(CPUS390XState *env)
LowCore *lowcore;
int ilen = env->int_pgm_ilen;
-assert(ilen == 2 || ilen == 4 || ilen == 6);
+assert((env->int_pgm_code == PGM_SPECIFICATION && ilen == 0) ||
+ ilen == 2 || ilen == 4 || ilen == 6);
switch (env->int_pgm_code) {
case PGM_PER:
--
2.39.2
[PATCH 2/2] tests/tcg/s390x: Add early-exception-recognition.S
Add a small test that checks whether early exceptions are recognized and whether the correct ILC and old PSW are stored when they happen. Signed-off-by: Ilya Leoshkevich --- tests/tcg/s390x/Makefile.softmmu-target | 1 + tests/tcg/s390x/early-exception-recognition.S | 38 +++ 2 files changed, 39 insertions(+) create mode 100644 tests/tcg/s390x/early-exception-recognition.S diff --git a/tests/tcg/s390x/Makefile.softmmu-target b/tests/tcg/s390x/Makefile.softmmu-target index 725b6c598db..9d3180bbcba 100644 --- a/tests/tcg/s390x/Makefile.softmmu-target +++ b/tests/tcg/s390x/Makefile.softmmu-target @@ -9,3 +9,4 @@ QEMU_OPTS=-action panic=exit-failure -kernel TESTS += unaligned-lowcore TESTS += bal TESTS += sam +TESTS += early-exception-recognition diff --git a/tests/tcg/s390x/early-exception-recognition.S b/tests/tcg/s390x/early-exception-recognition.S new file mode 100644 index 000..4e9677fea36 --- /dev/null +++ b/tests/tcg/s390x/early-exception-recognition.S @@ -0,0 +1,38 @@ +/* + * Test early exception recognition. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ +.org 0x8d +ilc: +.org 0x8e +program_interruption_code: +.org 0x150 +program_old_psw: +.org 0x1D0 /* program new PSW */ +.quad 0,pgm +.org 0x200 /* lowcore padding */ + +.globl _start +_start: +lpswe bad_psw +j failure + +pgm: +chhsi program_interruption_code,0x6 /* specification exception? */ +jne failure +cli ilc,0/* ilc zero? */ +jne failure +clc program_old_psw(16),bad_psw /* correct old PSW? */ +jne failure +lpswe success_psw +failure: +lpswe failure_psw + +.align 8 +bad_psw: +.quad 0x8000,0xfedcba9876543210 /* bit 0 set */ +success_psw: +.quad 0x2,0xfff/* see is_special_wait_psw() */ +failure_psw: +.quad 0x2,0/* disabled wait */ -- 2.39.2
Re: [PATCH] accel/xen: Fix DM state change notification in dm_restrict mode
On Tue, Mar 14, 2023 at 4:35 AM David Woodhouse wrote:
>
> From: David Woodhouse
>
> When dm_restrict is set, QEMU isn't permitted to update the XenStore node
> to indicate its running status. Previously, the xs_write() call would fail
> but the failure was ignored.
>
> However, in refactoring to allow for emulated XenStore operations, a new
> call to xs_open() was added. That one didn't fail gracefully, causing a
> fatal error when running in dm_restrict mode.
>
> Partially revert the offending patch, removing the additional call to
> xs_open() because the global 'xenstore' variable is still available; it
> just needs to be used with qemu_xen_xs_write() now instead of directly
> with the xs_write() libxenstore function.
>
> Also make the whole thing conditional on !xen_domid_restrict. There's no
> point even registering the state change handler to attempt to update the
> XenStore node when we know it's destined to fail.
>
> Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to
> internal emulation")
> Reported-by: Jason Andryuk
> Co-developed-by: Jason Andryuk
> Not-Signed-off-by: Jason Andryuk
> Signed-off-by: David Woodhouse
> Will-be-Tested-by: Jason Andryuk
Signed-off-by: Jason Andryuk
Tested-by: Jason Andryuk
Thanks, David.
-Jason
Re: [PATCH] hw/net/can: Add mcp25625 model
On Tue, Jan 17, 2023 at 07:16:35PM +0100, Pavel Pisa wrote:
> Dear Ben,
>
> sorry for longer response times...
I think we've both dropped the ball on this one, just got reminded about
this set and found it got deleted from work email.
We've done review upates and will try and get some branches out today and
maybe a new patch out for inclusion.
> On Tuesday 17 of January 2023 14:32:29 Ben Dooks wrote:
> > On 04/01/2023 12:22, Ben Dooks wrote:
> > > From: Ben Dooks
> > >
> > > Add support for Microchip MCP25625 SPI based CAN controller which is
> > > very similar to the MCP2515 (and covered by the same Linux driver).
> ...
> > Has anyone had chance to review this, it would be great to get
> > this moving along.
>
> Generally, I am happy that you consider use and extend our work.
>
> I have looked at the code. But even that implementation of CAN
> subsystem in QEMU was my idea and I led studnets working on
> it and sometimes heavily rewritten code to be acceptable,
> I am not QEMU expert and I have not studied its SSI subsystem
> so for comment from QEMU code requiremts I would be happy
> for some other to step in.
>
> I would like to test the peripheral. Please, can you try to elaborate
> and prepare description how to config QEMU for RPi emulation with
> mcp2515 overlay?
>
>
> https://github.com/raspberrypi/linux/blob/rpi-6.1.y/arch/arm/boot/dts/overlays/mcp2515-can0-overlay.dts
>
> I have RPi images and I have experience with this hardware.
> Even that I consider mcp2515 as really unfortunate solution
> and we have spent lot of time to help colleagues to enhance a little
> latencies of this solution when they chose that HW for serious
> wok instead of some NXP, TI, Xilinx or other sane SoC with CAN.
I had a quick look at setting an pi3b emulation but it does not currently
have an SSI port available by default. I will try and post the branch I
used with the SiFive Unmatched board later.
>
> https://dspace.cvut.cz/bitstream/handle/10467/68605/F3-DP-2017-Prudek-Martin-Dp_2017_prudek_martin.pdf
>
> But yes, people are using this chip a lot so it would worth to have
> emulation in QEMU. If the SPI connection is required then mcp251xfd
> seems to have chance for lower SPI transfer count overhead nd performance.
> But real SoC bus connected controllers are much better for serious
> project, if you design chip there are more cores available M-CAN,
> GRCAN and even our own CTU CAN FD which has already emulation in QEMU.
>
> Back to MCP25x1x. I have gone through code and try to understand
> the function. There is lot of connected to the locations in the
> registers maps in the chip
>
> > s->ssi_addr = 0x31;
A lot of these where basically used in one place and we map the regs
into internal data structs. Nazar and I have been through and tried
to tidy some of this up and make it more explicit what is being done.
> I have took manual but I think that it would help to add there comments
> with registers names or even use defines for these. But may it be,
> that for the easy ampping in the table and increment logic numbers
> are reasonable option... But comments with register symbolic names
> would help.
>
> The code does define only single property ("canbus") to select CAN
> bus to connect controller to. Mapping to the SPI peripheral is
> provided by device tree on QEMU side or by some other machine specific
> glue code? Please, can you provide more information for intended
> target use and RPi option to use for testing?
Yes, at the moment it seems that command-line mapping of SSI is not
easy, or well documented? Currently, other than the CS line that is
generally done with the controller, the only ouput needed is the IRQ
line. We do have the RXB0 and RXB1 outputs but they're not really needed
and only there for people to try if they want.
> As for the code, I have read it the first time and for full check
> I would need to spent more time with it. But I expect that
> functionality check with the respect to mcp25x1x datasheet has been
> done mainly by you so the full check bit by bit is not necessary.
> If there is some omitted case it would be (hopefully) found during
> code use. As for generic code style and redability, I see no problem.
> I expect that you have checked for QEMU style and if there has been
> some problem you have propably received QEMU CI and static analysis
> feedback.
>
> > +static void mcp25625_rx_into_buf(MCP25625State *s,
> > + const qemu_can_frame *frame,
> > + unsigned buffnr, int filthit)
> > +{
> > +struct rxbuff *rxbuff = >rxbuffs[buffnr];
> > +qemu_canid_t e_id, sidl, id, q_id = frame->can_id;
> > +unsigned len = frame->can_dlc;
> > +
>
> I would suggest to check for can_dlc > 8 in this function or in
> its caller (mcp25625_can_receive), to ensure that QEMU cannot be
> attacked by some malformed CAN message from the host kernel
> or other VM...
Ok, added.
> > +static ssize_t
Re: [PATCH v2 1/3] qapi/machine-target: refactor machine-target
On 14/3/23 11:00, Dinah Baum wrote: Moved architecture agnostic data types to their own file to avoid "attempt to use poisoned TARGET_*" error that results when including qapi header with commands that aren't defined for all architectures. Required to implement enabling `query-cpu-model-expansion` on all architectures Signed-off-by: Dinah Baum --- MAINTAINERS | 1 + qapi/machine-target-common.json | 79 + qapi/machine-target.json| 73 +- qapi/meson.build| 1 + 4 files changed, 82 insertions(+), 72 deletions(-) create mode 100644 qapi/machine-target-common.json diff --git a/qapi/meson.build b/qapi/meson.build index 9fd480c4d8..48be47170f 100644 --- a/qapi/meson.build +++ b/qapi/meson.build @@ -38,6 +38,7 @@ qapi_all_modules = [ 'job', 'machine', 'machine-target', + 'machine-target-common', 'migration', 'misc', 'misc-target', Eh, this reminds me of https://lore.kernel.org/qemu-devel/20220204152924.6253-5-f4...@amsat.org/ I wonder between -common / -any / -all. Anyhow, Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v4 1/2] tests/qtest/cxl-test: whitespace, line ending cleanup
On 27/2/23 17:31, Jonathan Cameron wrote: From: Gregory Price Defines are starting to exceed line length limits, align them for cleanliness before making modifications. Signed-off-by: Gregory Price Signed-off-by: Jonathan Cameron --- tests/qtest/cxl-test.c | 84 +++--- 1 file changed, 46 insertions(+), 38 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH v2] s390x/gdb: Split s390-virt.xml
Both TCG and KVM emulate ckc, cputm, last_break and prefix, and it's
quite useful to have them during debugging. Right now they are grouped
together with KVM-only pp, pfault_token, pfault_select and
pfault_compare in s390-virt.xml, and are not available when debugging
TCG-emulated code.
Move KVM-only registers into the new s390-virt-kvm.xml file. Advertise
s390-virt.xml always, and the new s390-virt-kvm.xml only for KVM.
Signed-off-by: Ilya Leoshkevich
Message-Id: <20230313211614.98739-1-...@linux.ibm.com>
---
v1: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04307.html
v1 -> v2: Improve the naming and the commit message (Christian).
configs/targets/s390x-linux-user.mak | 2 +-
configs/targets/s390x-softmmu.mak| 2 +-
gdb-xml/s390-virt-kvm.xml| 14 ++
gdb-xml/s390-virt.xml| 4 --
target/s390x/gdbstub.c | 65 +++-
5 files changed, 61 insertions(+), 26 deletions(-)
create mode 100644 gdb-xml/s390-virt-kvm.xml
diff --git a/configs/targets/s390x-linux-user.mak
b/configs/targets/s390x-linux-user.mak
index e2978248ede..24c04c85894 100644
--- a/configs/targets/s390x-linux-user.mak
+++ b/configs/targets/s390x-linux-user.mak
@@ -2,4 +2,4 @@ TARGET_ARCH=s390x
TARGET_SYSTBL_ABI=common,64
TARGET_SYSTBL=syscall.tbl
TARGET_BIG_ENDIAN=y
-TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-gs.xml
+TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-virt-kvm.xml gdb-xml/s390-gs.xml
diff --git a/configs/targets/s390x-softmmu.mak
b/configs/targets/s390x-softmmu.mak
index 258b4cf3582..70d2f9f0ba0 100644
--- a/configs/targets/s390x-softmmu.mak
+++ b/configs/targets/s390x-softmmu.mak
@@ -1,4 +1,4 @@
TARGET_ARCH=s390x
TARGET_BIG_ENDIAN=y
TARGET_SUPPORTS_MTTCG=y
-TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-gs.xml
+TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-virt-kvm.xml gdb-xml/s390-gs.xml
diff --git a/gdb-xml/s390-virt-kvm.xml b/gdb-xml/s390-virt-kvm.xml
new file mode 100644
index 000..a256eddaf57
--- /dev/null
+++ b/gdb-xml/s390-virt-kvm.xml
@@ -0,0 +1,14 @@
+
+
+
+
+
+
+
+
+
+
diff --git a/gdb-xml/s390-virt.xml b/gdb-xml/s390-virt.xml
index e2e9a7ad3cc..438eb68aabe 100644
--- a/gdb-xml/s390-virt.xml
+++ b/gdb-xml/s390-virt.xml
@@ -11,8 +11,4 @@
-
-
-
-
diff --git a/target/s390x/gdbstub.c b/target/s390x/gdbstub.c
index a5d69d0e0bc..e1153f5dc4a 100644
--- a/target/s390x/gdbstub.c
+++ b/target/s390x/gdbstub.c
@@ -205,12 +205,8 @@ static int cpu_write_c_reg(CPUS390XState *env, uint8_t
*mem_buf, int n)
#define S390_VIRT_CPUTM_REGNUM 1
#define S390_VIRT_BEA_REGNUM2
#define S390_VIRT_PREFIX_REGNUM 3
-#define S390_VIRT_PP_REGNUM 4
-#define S390_VIRT_PFT_REGNUM5
-#define S390_VIRT_PFS_REGNUM6
-#define S390_VIRT_PFC_REGNUM7
/* total number of registers in s390-virt.xml */
-#define S390_NUM_VIRT_REGS 8
+#define S390_NUM_VIRT_REGS 4
static int cpu_read_virt_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
{
@@ -223,14 +219,6 @@ static int cpu_read_virt_reg(CPUS390XState *env,
GByteArray *mem_buf, int n)
return gdb_get_regl(mem_buf, env->gbea);
case S390_VIRT_PREFIX_REGNUM:
return gdb_get_regl(mem_buf, env->psa);
-case S390_VIRT_PP_REGNUM:
-return gdb_get_regl(mem_buf, env->pp);
-case S390_VIRT_PFT_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_token);
-case S390_VIRT_PFS_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_select);
-case S390_VIRT_PFC_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_compare);
default:
return 0;
}
@@ -255,19 +243,51 @@ static int cpu_write_virt_reg(CPUS390XState *env, uint8_t
*mem_buf, int n)
env->psa = ldtul_p(mem_buf);
cpu_synchronize_post_init(env_cpu(env));
return 8;
-case S390_VIRT_PP_REGNUM:
+default:
+return 0;
+}
+}
+
+/* the values represent the positions in s390-virt-kvm.xml */
+#define S390_VIRT_KVM_PP_REGNUM 0
+#define S390_VIRT_KVM_PFT_REGNUM1
+#define S390_VIRT_KVM_PFS_REGNUM2
+#define S390_VIRT_KVM_PFC_REGNUM3
+/* total number of registers in s390-virt-kvm.xml */
+#define S390_NUM_VIRT_KVM_REGS 4
+
+static int cpu_read_virt_kvm_reg(CPUS390XState *env, GByteArray *mem_buf, int
n)
+{
+switch (n) {
+case S390_VIRT_KVM_PP_REGNUM:
+return gdb_get_regl(mem_buf, env->pp);
+case S390_VIRT_KVM_PFT_REGNUM:
+return gdb_get_regl(mem_buf, env->pfault_token);
+case S390_VIRT_KVM_PFS_REGNUM:
+
Re: [PATCH] tests/qtest/migration-test: Disable migration/multifd/tcp/plain/cancel
Copying Peter Xu on this one since it's poscopy, especially the newer postcopy preempt. * Peter Maydell (peter.mayd...@linaro.org) wrote: > On Sun, 12 Mar 2023 at 14:06, Peter Maydell wrote: > > > > On Tue, 7 Mar 2023 at 09:53, Peter Maydell wrote: > > > > > > On Sat, 4 Mar 2023 at 15:39, Peter Maydell > > > wrote: > > > > > > > > On Thu, 2 Mar 2023 at 17:22, Peter Maydell > > > > wrote: > > > > > > > > > > migration-test has been flaky for a long time, both in CI and > > > > > otherwise: > > > > > > > > > > > > > > > > > > In the cases where I've looked at the underlying log, this seems to > > > > > be in the migration/multifd/tcp/plain/cancel subtest. Disable that > > > > > specific subtest by default until somebody can track down the > > > > > underlying cause. Enthusiasts can opt back in by setting > > > > > QEMU_TEST_FLAKY_TESTS=1 in their environment. > > > > > > > > So I'm going to apply this, because hopefully it will improve > > > > the reliability a bit, but it's clearly not all of the > > > > issues with migration-test > > > > Here's another one, on the s390x CI runner: > > https://gitlab.com/qemu-project/qemu/-/jobs/3917587994 > > And here's a backtrace from a hung migration-test on the s390x > runner (looks like a deadlock, none of these processes were > using CPU): Ah a backtrace! OK, I think I kind of see what's happening here, one for Peter Xu. If I'm right it's a race something like: a) The test harness tells the source it wants to enter postcopy b) The harness then waits for the source to stop c) ... and the dest to start It's blocked on one of b but can't tell which d) The main thread in the dest is waiting for the postcopy recovery fd to be opened e) But I think the source is still trying to send normal precopy RAM and perhaps hasn't got around yet to opening that socket yet f) But I think the dest isn't reading from the main channel at that point because of (d) Dave > Process tree: > migration-test(1464515)─┬─qemu-system-aar(1465117) > └─qemu-system-aar(1465125) > === > PROCESS: 1464515 > gitlab-+ 1464515 1463129 0 14:27 ?00:00:02 > /home/gitlab-runner/builds/FLaZkdt1/0/qemu-project/qemu/build/tests/qtest/migration-test > --tap -k > [New LWP 1464517] > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/s390x-linux-gnu/libthread_db.so.1". > __libc_recv (fd=fd@entry=5, buf=buf@entry=0x3ffc06f9987, > len=len@entry=1, flags=flags@entry=0) at > ../sysdeps/unix/sysv/linux/recv.c:30 > 30 ../sysdeps/unix/sysv/linux/recv.c: No such file or directory. > > Thread 2 (Thread 0x3ff9faff900 (LWP 1464517)): > #0 syscall () at ../sysdeps/unix/sysv/linux/s390/s390-64/syscall.S:37 > #1 0x02aa1175ae34 in qemu_futex_wait (val=, > f=) at > /home/gitlab-runner/builds/FLaZkdt1/0/qemu-project/qemu/include/qemu/futex.h:29 > #2 qemu_event_wait (ev=ev@entry=0x2aa118d96b8 ) > at ../util/qemu-thread-posix.c:464 > #3 0x02aa117879ea in call_rcu_thread (opaque=opaque@entry=0x0) at > ../util/rcu.c:261 > #4 0x02aa11759e9a in qemu_thread_start (args=) at > ../util/qemu-thread-posix.c:541 > #5 0x03ffa0487e66 in start_thread (arg=0x3ff9faff900) at > pthread_create.c:477 > #6 0x03ffa037cbe6 in thread_start () at > ../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:65 > > Thread 1 (Thread 0x3ffa0b6d990 (LWP 1464515)): > #0 __libc_recv (fd=fd@entry=5, buf=buf@entry=0x3ffc06f9987, > len=len@entry=1, flags=flags@entry=0) at > ../sysdeps/unix/sysv/linux/recv.c:30 > #1 0x02aa1172ba22 in recv (__flags=0, __n=1, __buf=0x3ffc06f9987, > __fd=5) at /usr/include/s390x-linux-gnu/bits/socket2.h:44 > #2 qmp_fd_receive (fd=) at ../tests/qtest/libqmp.c:73 > #3 0x02aa11729b8e in qtest_qmp_receive_dict (s=0x2aa129b97d0) at > ../tests/qtest/libqtest.c:837 > #4 qtest_qmp_eventwait_ref (event=, s=) > at ../tests/qtest/libqtest.c:837 > #5 qtest_qmp_eventwait_ref (s=0x2aa129b97d0, event=) > at ../tests/qtest/libqtest.c:828 > #6 0x02aa11729c1a in qtest_qmp_eventwait (s=, > event=) at ../tests/qtest/libqtest.c:850 > #7 0x02aa1171ec56 in test_postcopy_common (args=0x3ffc06f9c88) at > ../tests/qtest/migration-test.c:1161 so I think that's the call: migrate_postcopy_start(from, to); and migrate_postcopy_start has a pair of qtest_qmp_eventwait's in but we can't tell which. So it's waiting for an event after it's been told to enter postcopy mode. > #8 0x02aa1171edb4 in test_postcopy_preempt () at > ../tests/qtest/migration-test.c:1178 > #9 0x03ffa067e608 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #10 0x03ffa067e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #11 0x03ffa067e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #12 0x03ffa067e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #13 0x03ffa067e392 in ?? () from /lib/s390x-linux-gnu/libglib-2.0.so.0 > #14
Re: [PULL 0/5] s390x and test fixes for 8.0-rc0
On Mon, 13 Mar 2023 at 14:11, Thomas Huth wrote: > > Hi Peter! > > The following changes since commit 29c8a9e31a982874ce4e2c15f2bf82d5f8dc3517: > > Merge tag 'linux-user-for-8.0-pull-request' of > https://gitlab.com/laurent_vivier/qemu into staging (2023-03-12 10:57:00 > +) > > are available in the Git repository at: > > https://gitlab.com/thuth/qemu.git tags/pull-request-2023-03-13 > > for you to fetch changes up to 410791228c415c0e4f76e6cafae7c82fae7cb8cb: > > tests/tcg/s390x: Add C(G)HRL test (2023-03-13 09:23:42 +0100) > > > * One more fix for the migration qtest > * Remove the edk2 gitlab-CI job > * Improve the build-system-alpine CI job > * Fix emulation of the CHRL/CGHRL s390x instructions > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.0 for any user-visible changes. -- PMM
[PATCH v2 2/3] cpu, qapi, target/arm, i386, s390x: Generalize query-cpu-model-expansion
This patch enables 'query-cpu-model-expansion' on all
architectures. Only architectures that implement
the command will return results, others will return an
error message as before.
This patch lays the groundwork for parsing a
-cpu cpu,help option as specified in
https://gitlab.com/qemu-project/qemu/-/issues/1480
Signed-off-by: Dinah Baum
---
cpu.c| 20
include/exec/cpu-common.h| 8 +
qapi/machine-target-common.json | 51 +
qapi/machine-target.json | 56
target/arm/arm-qmp-cmds.c| 7 ++--
target/arm/cpu.h | 7 +++-
target/i386/cpu-sysemu.c | 7 ++--
target/i386/cpu.h| 6
target/s390x/cpu.h | 7
target/s390x/cpu_models_sysemu.c | 6 ++--
10 files changed, 108 insertions(+), 67 deletions(-)
diff --git a/cpu.c b/cpu.c
index 567b23af46..c09edc4556 100644
--- a/cpu.c
+++ b/cpu.c
@@ -291,6 +291,26 @@ void list_cpus(const char *optarg)
#endif
}
+CpuModelExpansionInfo *get_cpu_model_expansion_info(CpuModelExpansionType type,
+CpuModelInfo *model,
+Error **errp)
+{
+/* XXX: implement cpu_model_expansion for targets that still miss it */
+#if defined(cpu_model_expansion)
+return cpu_model_expansion(type, model, errp);
+#else
+error_setg(errp, "Could not query cpu model information");
+return NULL;
+#endif
+}
+
+CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType
type,
+ CpuModelInfo *model,
+ Error **errp)
+{
+return get_cpu_model_expansion_info(type, model, errp);
+}
+
#if defined(CONFIG_USER_ONLY)
void tb_invalidate_phys_addr(target_ulong addr)
{
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 6feaa40ca7..ec6024dfde 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -7,6 +7,8 @@
#include "exec/hwaddr.h"
#endif
+#include "qapi/qapi-commands-machine-target-common.h"
+
/**
* vaddr:
* Type wide enough to contain any #target_ulong virtual address.
@@ -166,5 +168,11 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
extern int singlestep;
void list_cpus(const char *optarg);
+typedef void (*cpu_model_expansion_func)(CpuModelExpansionType type,
+ CpuModelInfo *model,
+ Error **errp);
+CpuModelExpansionInfo *get_cpu_model_expansion_info(CpuModelExpansionType type,
+CpuModelInfo *model,
+Error **errp);
#endif /* CPU_COMMON_H */
diff --git a/qapi/machine-target-common.json b/qapi/machine-target-common.json
index 1e6da3177d..44713e9935 100644
--- a/qapi/machine-target-common.json
+++ b/qapi/machine-target-common.json
@@ -77,3 +77,54 @@
##
{ 'enum': 'CpuModelCompareResult',
'data': [ 'incompatible', 'identical', 'superset', 'subset' ] }
+
+##
+# @CpuModelExpansionInfo:
+#
+# The result of a cpu model expansion.
+#
+# @model: the expanded CpuModelInfo.
+#
+# Since: 2.8
+##
+{ 'struct': 'CpuModelExpansionInfo',
+ 'data': { 'model': 'CpuModelInfo' } }
+
+##
+# @query-cpu-model-expansion:
+#
+# Expands a given CPU model (or a combination of CPU model + additional
options)
+# to different granularities, allowing tooling to get an understanding what a
+# specific CPU model looks like in QEMU under a certain configuration.
+#
+# This interface can be used to query the "host" CPU model.
+#
+# The data returned by this command may be affected by:
+#
+# * QEMU version: CPU models may look different depending on the QEMU version.
+# (Except for CPU models reported as "static" in query-cpu-definitions.)
+# * machine-type: CPU model may look different depending on the machine-type.
+# (Except for CPU models reported as "static" in query-cpu-definitions.)
+# * machine options (including accelerator): in some architectures, CPU models
+# may look different depending on machine and accelerator options. (Except
for
+# CPU models reported as "static" in query-cpu-definitions.)
+# * "-cpu" arguments and global properties: arguments to the -cpu option and
+# global properties may affect expansion of CPU models. Using
+# query-cpu-model-expansion while using these is not advised.
+#
+# Some architectures may not support all expansion types. s390x supports
+# "full" and "static". Arm only supports "full".
+#
+# Returns: a CpuModelExpansionInfo. Returns an error if expanding CPU models is
+# not supported, if the model cannot be expanded, if the model
contains
+# an unknown CPU definition name, unknown properties or properties
+# with a wrong type. Also returns an error if an expansion type is
+#
[PATCH v2 3/3] cpu, qdict, vl: Enable printing options for CPU type
Change parsing of -cpu argument to allow -cpu cpu,help
to print options for the CPU type similar to
how the '-device' option works.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1480
Signed-off-by: Dinah Baum
---
cpu.c | 41 +++
include/exec/cpu-common.h | 2 ++
include/qapi/qmp/qdict.h | 2 ++
qemu-options.hx | 7 ---
qobject/qdict.c | 5 +
softmmu/vl.c | 36 --
6 files changed, 88 insertions(+), 5 deletions(-)
diff --git a/cpu.c b/cpu.c
index c09edc4556..fe33c51061 100644
--- a/cpu.c
+++ b/cpu.c
@@ -23,7 +23,9 @@
#include "exec/target_page.h"
#include "hw/qdev-core.h"
#include "hw/qdev-properties.h"
+#include "qemu/cutils.h"
#include "qemu/error-report.h"
+#include "qemu/qemu-print.h"
#include "migration/vmstate.h"
#ifdef CONFIG_USER_ONLY
#include "qemu.h"
@@ -42,6 +44,8 @@
#include "hw/core/accel-cpu.h"
#include "trace/trace-root.h"
#include "qemu/accel.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qobject.h"
uintptr_t qemu_host_page_size;
intptr_t qemu_host_page_mask;
@@ -311,6 +315,43 @@ CpuModelExpansionInfo
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
return get_cpu_model_expansion_info(type, model, errp);
}
+void list_cpu_model_expansion(CpuModelExpansionType type,
+ CpuModelInfo *model,
+ Error **errp)
+{
+CpuModelExpansionInfo *expansion_info;
+QDict *qdict;
+QDictEntry *qdict_entry;
+const char *key;
+QObject *obj;
+QType q_type;
+GPtrArray *array;
+int i;
+const char *type_name;
+
+expansion_info = get_cpu_model_expansion_info(type, model, errp);
+if (expansion_info) {
+qdict = qobject_to(QDict, expansion_info->model->props);
+if (qdict) {
+qemu_printf("%s features:\n", model->name);
+array = g_ptr_array_new();
+for (qdict_entry = (QDictEntry *)qdict_first(qdict); qdict_entry;
+ qdict_entry = (QDictEntry *)qdict_next(qdict, qdict_entry)) {
+g_ptr_array_add(array, qdict_entry);
+}
+g_ptr_array_sort(array, (GCompareFunc)dict_key_compare);
+for (i = 0; i < array->len; i++) {
+qdict_entry = array->pdata[i];
+key = qdict_entry_key(qdict_entry);
+obj = qdict_get(qdict, key);
+q_type = qobject_type(obj);
+type_name = QType_str(q_type);
+qemu_printf(" %s=<%s>\n", key, type_name);
+}
+}
+}
+}
+
#if defined(CONFIG_USER_ONLY)
void tb_invalidate_phys_addr(target_ulong addr)
{
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index ec6024dfde..8fc05307ad 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -174,5 +174,7 @@ typedef void
(*cpu_model_expansion_func)(CpuModelExpansionType type,
CpuModelExpansionInfo *get_cpu_model_expansion_info(CpuModelExpansionType type,
CpuModelInfo *model,
Error **errp);
+void list_cpu_model_expansion(CpuModelExpansionType type,
+ CpuModelInfo *model, Error **errp);
#endif /* CPU_COMMON_H */
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 82e90fc072..1ff9523a13 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -68,4 +68,6 @@ const char *qdict_get_try_str(const QDict *qdict, const char
*key);
QDict *qdict_clone_shallow(const QDict *src);
+int dict_key_compare(QDictEntry **entry1, QDictEntry **entry2);
+
#endif /* QDICT_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index d42f60fb91..d5284b9330 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -169,11 +169,12 @@ SRST
ERST
DEF("cpu", HAS_ARG, QEMU_OPTION_cpu,
-"-cpu cpuselect CPU ('-cpu help' for list)\n", QEMU_ARCH_ALL)
+"-cpu cpuselect CPU ('-cpu help' for list)\n"
+"use '-cpu cpu,help' to print possible properties\n",
QEMU_ARCH_ALL)
SRST
``-cpu model``
-Select CPU model (``-cpu help`` for list and additional feature
-selection)
+Select CPU model (``-cpu help`` and ``-cpu cpu,help``) for list and
additional feature
+selection
ERST
DEF("accel", HAS_ARG, QEMU_OPTION_accel,
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 8faff230d3..31407e62f6 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -447,3 +447,8 @@ void qdict_unref(QDict *q)
{
qobject_unref(q);
}
+
+int dict_key_compare(QDictEntry **entry1, QDictEntry **entry2)
+{
+return g_strcmp0(qdict_entry_key(*entry1), qdict_entry_key(*entry2));
+}
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3340f63c37..a9d70e559e 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -500,6 +500,15 @@ static QemuOptsList qemu_action_opts = {
[PATCH v2 1/3] qapi/machine-target: refactor machine-target
Moved architecture agnostic data types to their own
file to avoid "attempt to use poisoned TARGET_*"
error that results when including qapi header
with commands that aren't defined for all architectures.
Required to implement enabling `query-cpu-model-expansion`
on all architectures
Signed-off-by: Dinah Baum
---
MAINTAINERS | 1 +
qapi/machine-target-common.json | 79 +
qapi/machine-target.json| 73 +-
qapi/meson.build| 1 +
4 files changed, 82 insertions(+), 72 deletions(-)
create mode 100644 qapi/machine-target-common.json
diff --git a/MAINTAINERS b/MAINTAINERS
index 95c957d587..bc4a6ba399 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1750,6 +1750,7 @@ F: hw/core/numa.c
F: hw/cpu/cluster.c
F: qapi/machine.json
F: qapi/machine-target.json
+F: qapi/machine-target-common.json
F: include/hw/boards.h
F: include/hw/core/cpu.h
F: include/hw/cpu/cluster.h
diff --git a/qapi/machine-target-common.json b/qapi/machine-target-common.json
new file mode 100644
index 00..1e6da3177d
--- /dev/null
+++ b/qapi/machine-target-common.json
@@ -0,0 +1,79 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+
+##
+# = Common data types for machine target commands
+##
+
+##
+# @CpuModelInfo:
+#
+# Virtual CPU model.
+#
+# A CPU model consists of the name of a CPU definition, to which
+# delta changes are applied (e.g. features added/removed). Most magic values
+# that an architecture might require should be hidden behind the name.
+# However, if required, architectures can expose relevant properties.
+#
+# @name: the name of the CPU definition the model is based on
+# @props: a dictionary of QOM properties to be applied
+#
+# Since: 2.8
+##
+{ 'struct': 'CpuModelInfo',
+'data': { 'name': 'str',
+ '*props': 'any' } }
+
+##
+# @CpuModelExpansionType:
+#
+# An enumeration of CPU model expansion types.
+#
+# @static: Expand to a static CPU model, a combination of a static base
+# model name and property delta changes. As the static base model will
+# never change, the expanded CPU model will be the same, independent
of
+# QEMU version, machine type, machine options, and accelerator
options.
+# Therefore, the resulting model can be used by tooling without having
+# to specify a compatibility machine - e.g. when displaying the "host"
+# model. The @static CPU models are migration-safe.
+
+# @full: Expand all properties. The produced model is not guaranteed to be
+#migration-safe, but allows tooling to get an insight and work with
+#model details.
+#
+# Note: When a non-migration-safe CPU model is expanded in static mode, some
+# features enabled by the CPU model may be omitted, because they can't be
+# implemented by a static CPU model definition (e.g. cache info
passthrough and
+# PMU passthrough in x86). If you need an accurate representation of the
+# features enabled by a non-migration-safe CPU model, use @full. If you
need a
+# static representation that will keep ABI compatibility even when
changing QEMU
+# version or machine-type, use @static (but keep in mind that some
features may
+# be omitted).
+#
+# Since: 2.8
+##
+{ 'enum': 'CpuModelExpansionType',
+ 'data': [ 'static', 'full' ] }
+
+##
+# @CpuModelCompareResult:
+#
+# An enumeration of CPU model comparison results. The result is usually
+# calculated using e.g. CPU features or CPU generations.
+#
+# @incompatible: If model A is incompatible to model B, model A is not
+#guaranteed to run where model B runs and the other way around.
+#
+# @identical: If model A is identical to model B, model A is guaranteed to run
+# where model B runs and the other way around.
+#
+# @superset: If model A is a superset of model B, model B is guaranteed to run
+#where model A runs. There are no guarantees about the other way.
+#
+# @subset: If model A is a subset of model B, model A is guaranteed to run
+# where model B runs. There are no guarantees about the other way.
+#
+# Since: 2.8
+##
+{ 'enum': 'CpuModelCompareResult',
+ 'data': [ 'incompatible', 'identical', 'superset', 'subset' ] }
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 2e267fa458..1cacfde88e 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -4,78 +4,7 @@
# This work is licensed under the terms of the GNU GPL, version 2 or later.
# See the COPYING file in the top-level directory.
-##
-# @CpuModelInfo:
-#
-# Virtual CPU model.
-#
-# A CPU model consists of the name of a CPU definition, to which
-# delta changes are applied (e.g. features added/removed). Most magic values
-# that an architecture might require should be hidden behind the name.
-# However, if required, architectures can expose relevant properties.
-#
-# @name: the name of the CPU definition the model is based on
-#
[PATCH v2 0/3] Enable -cpu ,help
Part 1 is a refactor/code motion patch for qapi/machine target required for setup of Part 2 which enables query-cpu-model-expansion on all architectures Part 3 implements the ',help' feature Limitations: Currently only 'FULL' expansion queries are implemented since that's the only type enabled on the architectures that allow feature probing Unlike the 'device,help' command, default values aren't printed Dinah Baum (3): qapi/machine-target: refactor machine-target cpu, qapi, target/arm, i386, s390x: Generalize query-cpu-model-expansion cpu, qdict, vl: Enable printing options for CPU type MAINTAINERS | 1 + cpu.c| 61 +++ include/exec/cpu-common.h| 10 +++ include/qapi/qmp/qdict.h | 2 + qapi/machine-target-common.json | 130 +++ qapi/machine-target.json | 129 +- qapi/meson.build | 1 + qemu-options.hx | 7 +- qobject/qdict.c | 5 ++ softmmu/vl.c | 36 - target/arm/arm-qmp-cmds.c| 7 +- target/arm/cpu.h | 7 +- target/i386/cpu-sysemu.c | 7 +- target/i386/cpu.h| 6 ++ target/s390x/cpu.h | 7 ++ target/s390x/cpu_models_sysemu.c | 6 +- 16 files changed, 278 insertions(+), 144 deletions(-) create mode 100644 qapi/machine-target-common.json -- 2.30.2
Re: [PATCH] s390x/gdb: Split s390-virt.xml
On Tue, 2023-03-14 at 08:06 +0100, Christian Borntraeger wrote: > > > Am 13.03.23 um 22:16 schrieb Ilya Leoshkevich: > > TCG emulates ckc, cputm, last_break and prefix, and it's quite > > useful > > to have them during debugging. > > KVM provides those as well so I dont get what you are trying to do > here. (I would understand moving out the pfault things into a KVM > section) The end result here is that ckc, cputm, last_break and prefix are always available, and pp, pfault_token, pfault_select and pfault_compare are available only with KVM. I guess I chose the naming poorly, I will send a v2 with s390-virt.xml containing common registers, and s390-virt-kvm.xml containing KVM-only registers. >
Re: [PATCH for-8.0] ide: Fix manual in-flight count for TRIM BH
On Mon, Mar 13, 2023 at 5:32 PM Kevin Wolf wrote: > > So I still think that this bug is a symptom of a problem in the design > > of request queuing. > > > > In fact, shouldn't request queuing was enabled at the _end_ of > > bdrv_drained_begin (once the BlockBackend has reached a quiescent > > state on its own terms), rather than at the beginning (which leads to > > deadlocks like this one)? > > 1. I want to have exclusive access to the node. This one wants request >queuing from the start to avoid losing time unnecessarily until the >guest stops sending new requests. > > 2. I want to wait for my requests to complete. This one never wants >request queuing. Enabling it at the end of bdrv_drained_begin() >wouldn't hurt it (because it has already achieved its goal then), but >it's also not necessary for the same reason. Right, doing it at the end would be needed to avoid the deadlocks. On the other hand, case 1 can (and I think should) be handled by .drained_begin, or shortcut through aio_disable_external() for those devices that use ioeventfd. Paolo > So maybe what we could take from this is that request queuing should be > temporarily disabled while we're in blk_drain*() because these > interfaces are only meant for case 2. In all other cases, it should > continue to work as it does now.
Re: [PATCH 1/2] target/s390x: Fix EXECUTE of relative long instructions
On 14.03.23 00:38, Ilya Leoshkevich wrote:
The code uses the wrong base for relative addressing: it should use the
target instruction address and not the EXECUTE's address.
Fix by storing the target instruction address in the new CPUS390XState
member and loading it from the code generated by in2_ri2().
Reported-by: Nina Schoetterl-Glausch
Signed-off-by: Ilya Leoshkevich
---
target/s390x/cpu.h| 1 +
target/s390x/tcg/mem_helper.c | 1 +
target/s390x/tcg/translate.c | 10 +-
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b2..8aaf8dd5a3b 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -87,6 +87,7 @@ struct CPUArchState {
uint64_t cc_vr;
uint64_t ex_value;
+uint64_t ex_target;
uint64_t __excp_addr;
uint64_t psa;
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 6835c26dda4..00afae2b640 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2530,6 +2530,7 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen,
uint64_t r1, uint64_t addr)
that ex_value is non-zero, which flags that we are in a state
that requires such execution. */
env->ex_value = insn | ilen;
+env->ex_target = addr;
}
uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 811049ea281..fefff95b91c 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5888,7 +5888,15 @@ static void in2_a2(DisasContext *s, DisasOps *o)
static void in2_ri2(DisasContext *s, DisasOps *o)
{
-o->in2 = tcg_const_i64(s->base.pc_next + (int64_t)get_field(s, i2) * 2);
+int64_t delta = (int64_t)get_field(s, i2) * 2;
+
+if (unlikely(s->ex_value)) {
+o->in2 = tcg_temp_new_i64();
+tcg_gen_ld_i64(o->in2, cpu_env, offsetof(CPUS390XState, ex_target));
+tcg_gen_addi_i64(o->in2, o->in2, delta);
+} else {
+o->in2 = tcg_const_i64(s->base.pc_next + delta);
+}
}
#define SPEC_in2_ri2 0
LGTM, hopefully Richard can have another look. Thanks!
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH] accel/xen: Fix DM state change notification in dm_restrict mode
On 14/03/2023 08:35, David Woodhouse wrote:
From: David Woodhouse
When dm_restrict is set, QEMU isn't permitted to update the XenStore node
to indicate its running status. Previously, the xs_write() call would fail
but the failure was ignored.
However, in refactoring to allow for emulated XenStore operations, a new
call to xs_open() was added. That one didn't fail gracefully, causing a
fatal error when running in dm_restrict mode.
Partially revert the offending patch, removing the additional call to
xs_open() because the global 'xenstore' variable is still available; it
just needs to be used with qemu_xen_xs_write() now instead of directly
with the xs_write() libxenstore function.
Also make the whole thing conditional on !xen_domid_restrict. There's no
point even registering the state change handler to attempt to update the
XenStore node when we know it's destined to fail.
Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to
internal emulation")
Reported-by: Jason Andryuk
Co-developed-by: Jason Andryuk
Not-Signed-off-by: Jason Andryuk
Signed-off-by: David Woodhouse
Will-be-Tested-by: Jason Andryuk
---
accel/xen/xen-all.c | 27 ++-
1 file changed, 10 insertions(+), 17 deletions(-)
Reviewed-by: Paul Durrant
[PATCH] accel/xen: Fix DM state change notification in dm_restrict mode
From: David Woodhouse
When dm_restrict is set, QEMU isn't permitted to update the XenStore node
to indicate its running status. Previously, the xs_write() call would fail
but the failure was ignored.
However, in refactoring to allow for emulated XenStore operations, a new
call to xs_open() was added. That one didn't fail gracefully, causing a
fatal error when running in dm_restrict mode.
Partially revert the offending patch, removing the additional call to
xs_open() because the global 'xenstore' variable is still available; it
just needs to be used with qemu_xen_xs_write() now instead of directly
with the xs_write() libxenstore function.
Also make the whole thing conditional on !xen_domid_restrict. There's no
point even registering the state change handler to attempt to update the
XenStore node when we know it's destined to fail.
Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to
internal emulation")
Reported-by: Jason Andryuk
Co-developed-by: Jason Andryuk
Not-Signed-off-by: Jason Andryuk
Signed-off-by: David Woodhouse
Will-be-Tested-by: Jason Andryuk
---
accel/xen/xen-all.c | 27 ++-
1 file changed, 10 insertions(+), 17 deletions(-)
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 00221e23c5..5ff0cb8bd9 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -32,28 +32,13 @@ xendevicemodel_handle *xen_dmod;
static void xenstore_record_dm_state(const char *state)
{
-struct xs_handle *xs;
char path[50];
-/* We now have everything we need to set the xenstore entry. */
-xs = xs_open(0);
-if (xs == NULL) {
-fprintf(stderr, "Could not contact XenStore\n");
-exit(1);
-}
-
snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
-/*
- * This call may fail when running restricted so don't make it fatal in
- * that case. Toolstacks should instead use QMP to listen for state
changes.
- */
-if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) &&
-!xen_domid_restrict) {
+if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) {
error_report("error recording dm state");
exit(1);
}
-
-xs_close(xs);
}
@@ -111,7 +96,15 @@ static int xen_init(MachineState *ms)
xc_interface_close(xen_xc);
return -1;
}
-qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+
+/*
+ * The XenStore write would fail when running restricted so don't attempt
+ * it in that case. Toolstacks should instead use QMP to listen for state
+ * changes.
+ */
+if (!xen_domid_restrict) {
+qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+}
/*
* opt out of system RAM being allocated by generic code
*/
--
2.34.1
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Mon, 2023-03-13 at 19:17 -0400, Jason Andryuk wrote:
> This looks good, better than what I posted, and seems to work for both
> dm_restrict set and unset.
Thanks.
> For dm_restricted, xs_write() does fail. I verified that with a print
> statement. I think "shouldn't even try" makes sense. I'm thinking
> that xen_domid_restricted shouldn't even add the callback. Something
> like:
>
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -39,8 +39,7 @@ static void xenstore_record_dm_state(const char *state)
> * This call may fail when running restricted so don't make it fatal in
> * that case. Toolstacks should instead use QMP to listen for state
> changes.
> */
> - if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) &&
> - !xen_domid_restrict) {
> + if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) {
> error_report("error recording dm state");
> exit(1);
> }
> @@ -101,7 +100,10 @@ static int xen_init(MachineState *ms)
> xc_interface_close(xen_xc);
> return -1;
> }
> - qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
> +
> + if(!xen_domid_restrict)
> + qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
> +
> /*
> * opt out of system RAM being allocated by generic code
> */
>
> That works for both dm_restrict 0 & 1.
>
> I think you should submit your change and I can follow up with the
> above if it seems desirable.
Let's just do it in one. I'll move that comment about 'call may fail'
down to where you've made the qemu_add_vm_change_state_handler()
conditional. And QEMU style requires braces even for a one-line if().
I'll send it out and let you add your Signed-off-by: and Tested-by: to
it.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests
On 3/13/23 19:21, Richard Henderson wrote: On 3/13/23 04:44, Luis Machado wrote: Luis: I think that rather than doing (2) with a QEMU namespace, we should define a gdb namespace for this. That makes it clear that this is still a gdb-upstream-sanctioned way of exposing the pauth registers. That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash. We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2. What if we leave the original two registers, pauth_[cd]mask, in org.gnu.gdb.aarch64.pauth and move the new *_high registers into a different feature? That would maximize the set of gdb version for which the original user-only support is functional. From what I recall from the gdb bug, I don't think that will help. gdb will detect pauth support, will add the ra_sign_state pseudo-register internally with the incorrect numbering and will also see the additional system registers from QEMU, resulting in a crash. r~ IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Re: [PATCH] s390x/gdb: Split s390-virt.xml
Am 13.03.23 um 22:16 schrieb Ilya Leoshkevich:
TCG emulates ckc, cputm, last_break and prefix, and it's quite useful
to have them during debugging.
KVM provides those as well so I dont get what you are trying to do here. (I
would understand moving out the pfault things into a KVM section)
So move them into the new s390-virt-tcg.xml file.
pp, pfault_token, pfault_select and pfault_compare are not emulated,
so keep them in s390-virt.xml.
Signed-off-by: Ilya Leoshkevich
---
configs/targets/s390x-linux-user.mak | 2 +-
configs/targets/s390x-softmmu.mak| 2 +-
gdb-xml/s390-virt-tcg.xml| 14 +
gdb-xml/s390-virt.xml| 4 --
target/s390x/gdbstub.c | 82 ++--
5 files changed, 69 insertions(+), 35 deletions(-)
create mode 100644 gdb-xml/s390-virt-tcg.xml
diff --git a/configs/targets/s390x-linux-user.mak
b/configs/targets/s390x-linux-user.mak
index e2978248ede..fb3e2b73be7 100644
--- a/configs/targets/s390x-linux-user.mak
+++ b/configs/targets/s390x-linux-user.mak
@@ -2,4 +2,4 @@ TARGET_ARCH=s390x
TARGET_SYSTBL_ABI=common,64
TARGET_SYSTBL=syscall.tbl
TARGET_BIG_ENDIAN=y
-TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-gs.xml
+TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-virt-tcg.xml gdb-xml/s390-gs.xml
diff --git a/configs/targets/s390x-softmmu.mak
b/configs/targets/s390x-softmmu.mak
index 258b4cf3582..554330d7c85 100644
--- a/configs/targets/s390x-softmmu.mak
+++ b/configs/targets/s390x-softmmu.mak
@@ -1,4 +1,4 @@
TARGET_ARCH=s390x
TARGET_BIG_ENDIAN=y
TARGET_SUPPORTS_MTTCG=y
-TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-gs.xml
+TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-virt-tcg.xml gdb-xml/s390-gs.xml
diff --git a/gdb-xml/s390-virt-tcg.xml b/gdb-xml/s390-virt-tcg.xml
new file mode 100644
index 000..0f77c9b48c6
--- /dev/null
+++ b/gdb-xml/s390-virt-tcg.xml
@@ -0,0 +1,14 @@
+
+
+
+
+
+
+
+
+
+
diff --git a/gdb-xml/s390-virt.xml b/gdb-xml/s390-virt.xml
index e2e9a7ad3cc..a79c0307682 100644
--- a/gdb-xml/s390-virt.xml
+++ b/gdb-xml/s390-virt.xml
@@ -7,10 +7,6 @@
-
-
-
-
diff --git a/target/s390x/gdbstub.c b/target/s390x/gdbstub.c
index a5d69d0e0bc..111b695dc85 100644
--- a/target/s390x/gdbstub.c
+++ b/target/s390x/gdbstub.c
@@ -200,61 +200,81 @@ static int cpu_write_c_reg(CPUS390XState *env, uint8_t
*mem_buf, int n)
}
}
-/* the values represent the positions in s390-virt.xml */
-#define S390_VIRT_CKC_REGNUM0
-#define S390_VIRT_CPUTM_REGNUM 1
-#define S390_VIRT_BEA_REGNUM2
-#define S390_VIRT_PREFIX_REGNUM 3
-#define S390_VIRT_PP_REGNUM 4
-#define S390_VIRT_PFT_REGNUM5
-#define S390_VIRT_PFS_REGNUM6
-#define S390_VIRT_PFC_REGNUM7
-/* total number of registers in s390-virt.xml */
-#define S390_NUM_VIRT_REGS 8
+/* the values represent the positions in s390-virt-tcg.xml */
+#define S390_VIRT_TCG_CKC_REGNUM0
+#define S390_VIRT_TCG_CPUTM_REGNUM 1
+#define S390_VIRT_TCG_BEA_REGNUM2
+#define S390_VIRT_TCG_PREFIX_REGNUM 3
+/* total number of registers in s390-virt-tcg.xml */
+#define S390_NUM_VIRT_TCG_REGS 4
-static int cpu_read_virt_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
+static int cpu_read_virt_tcg_reg(CPUS390XState *env, GByteArray *mem_buf, int
n)
{
switch (n) {
-case S390_VIRT_CKC_REGNUM:
+case S390_VIRT_TCG_CKC_REGNUM:
return gdb_get_regl(mem_buf, env->ckc);
-case S390_VIRT_CPUTM_REGNUM:
+case S390_VIRT_TCG_CPUTM_REGNUM:
return gdb_get_regl(mem_buf, env->cputm);
-case S390_VIRT_BEA_REGNUM:
+case S390_VIRT_TCG_BEA_REGNUM:
return gdb_get_regl(mem_buf, env->gbea);
-case S390_VIRT_PREFIX_REGNUM:
+case S390_VIRT_TCG_PREFIX_REGNUM:
return gdb_get_regl(mem_buf, env->psa);
-case S390_VIRT_PP_REGNUM:
-return gdb_get_regl(mem_buf, env->pp);
-case S390_VIRT_PFT_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_token);
-case S390_VIRT_PFS_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_select);
-case S390_VIRT_PFC_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_compare);
default:
return 0;
}
}
-static int cpu_write_virt_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_virt_tcg_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
{
switch (n) {
-case S390_VIRT_CKC_REGNUM:
+case S390_VIRT_TCG_CKC_REGNUM:
env->ckc = ldtul_p(mem_buf);
Re: [RFC PATCH v2 1/2] util: Add thread-safe qemu_strerror() function
I'm sorry for sending ill-formed thread twice.
This problem was because the SMTP server overwrites Message-ID,
and git-sendemail does not reflect it to In-Reply-To: and Reply-To: in the
header.
I will test well before sending the next patch.
The original cover letter was
https://lore.kernel.org/qemu-devel/tyzpr06mb5418216269d0ed2eb37d6ff49d...@tyzpr06mb5418.apcprd06.prod.outlook.com/T/#u
Thank you.
On 2023/03/14 15:40, Yohei Kojima wrote:
> Add qemu_strerror() which follows the POSIX specification for
> strerror(). While strerror() is not guaranteed to be thread-safe, this
> function is thread-safe.
>
> This function is added to solve the following issue:
> https://gitlab.com/qemu-project/qemu/-/issues/416
>
> Signed-off-by: Yohei Kojima
> ---
> include/qemu/cutils.h | 20 +++
> util/cutils.c | 45 +++
> 2 files changed, 65 insertions(+)
>
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 92c436d8c7..0bcae0049a 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -117,6 +117,26 @@ int stristart(const char *str, const char *val, const
> char **ptr);
> * Returns: length of @s in bytes, or @max_len, whichever is smaller.
> */
> int qemu_strnlen(const char *s, int max_len);
> +/**
> + * qemu_strerror:
> + * @errnum: error number
> + *
> + * Return the pointer to a string that describes errnum, like
> + * strerror(). This function is thread-safe because the buffer for
> + * returned string is allocated per thread.
> + *
> + * This function is thread-safe, but not reentrant. In other words,
> + * if a thread is interrupted by a signal in this function, and the
> + * thread calls this function again in the signal handling, then
> + * the result might be corrupted.
> + *
> + * This function has the same behaviour as the POSIX strerror()
> + * function.
> + *
> + * Returns: the pointer to an error description, or an
> + * "Unknown error nnn" message if errnum is invalid.
> + */
> +char *qemu_strerror(int errnum);
> /**
> * qemu_strsep:
> * @input: pointer to string to parse
> diff --git a/util/cutils.c b/util/cutils.c
> index 5887e74414..3d14f50c75 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -131,6 +131,51 @@ int qemu_strnlen(const char *s, int max_len)
> return i;
> }
>
> +/**
> + * It assumes the length of error descriptions are at most 1024.
> + * The concern of write buffer overflow is cared by strerror_r().
> + */
> +static __thread char qemu_strerror_buf[1024];
> +
> +#if (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE
> +/**
> + * In POSIX.1-2001 and after, the return type of strerror_r is int, but
> + * glibc overrides the definition of strerror_r to the old strerror_r
> + * if _GNU_SOURCE is defined. This condition handles it.
> + */
> +
> +char *qemu_strerror(int errnum)
> +{
> +int is_error = strerror_r(errnum, qemu_strerror_buf, 1024);
> +
> +if (is_error == 0) return qemu_strerror_buf;
> +
> +/**
> + * handle the error occured in strerror_r
> + *
> + * If is_error is greater than 0, errno might not be updated by
> + * strerror_r. Otherwise, errno is updated.
> + */
> +if (is_error > 0) errno = is_error;
> +
> +strncpy(qemu_strerror_buf, "Error %d occured\n", errno);
> +return qemu_strerror_buf;
> +}
> +#else
> +/**
> + * In glibc, the return type of strerror_r is char* if _GNU_SOURCE
> + * is defined. In this case, strerror_r returns qemu_strerror_buf iff
> + * some error occured in strerror_r, and otherwise it returns a pointer
> + * to the pre-defined description for errnum.
> + *
> + * This is the same behaviour until POSIX.1-2001.
> + */
> +char *qemu_strerror(int errnum)
> +{
> +return strerror_r(errnum, qemu_strerror_buf, 1024);
> +}
> +#endif
> +
> char *qemu_strsep(char **input, const char *delim)
> {
> char *result = *input;
[RFC PATCH v2 0/2] util: Add thread-safe qemu_strerror() function
This patch series adds qemu_strerror() function, which is thread-safe version of the libc strerror(). The first patch introduces the qemu_strerror() function, and the second patch replaces strerror() function in linux-user/* with qemu_strerror() function. Because it involves thread safety, qemu_strerror() should be tested carefully. But before adding tests, I want to ask (1) will this patch be acceptable to QEMU project after adding tests, (2) where and how qemu_strerror() should be tested. (1) means that: is my approach too complicated to solve potential thread-unsafe implementation of strerror()? Although strerror() is not guaranteed to be thread-safe, glibc implements thread-safe strerror(). We have to consider the balance between maintenance costs and potential risks. (2) means that: is tests/unit/test-cutils.c a good place for tests? Because the behavior of qemu_strerror() is changed by the feature test macros, the tests should be run with different test macros, hopefully in different OSs. Note that strerror_r() function called by qemu_strerror() has different return types between architectures because of the historical reason. qemu_strerror() handles both the newer POSIX strerror() and the older POSIX strerror(). All tests except for skipped ones are passed in my environment (x86_64 linux). Yohei Kojima (2): util: Add thread-safe qemu_strerror() function linux-user: replace strerror() function to the thread safe qemu_strerror() include/qemu/cutils.h | 20 +++ linux-user/elfload.c | 4 ++-- linux-user/main.c | 4 ++-- linux-user/syscall.c | 2 +- util/cutils.c | 45 +++ 5 files changed, 70 insertions(+), 5 deletions(-) -- 2.39.2
[RFC PATCH v2 2/2] linux-user: replace strerror() function to the thread safe qemu_strerror()
strerror() is not guaranteed to be thread-safe as described in
(https://gitlab.com/qemu-project/qemu/-/issues/416).
This commit changes files under /linux-user that call strerror() to call
the safer qemu_strerror().
Signed-off-by: Yohei Kojima
---
linux-user/elfload.c | 4 ++--
linux-user/main.c| 4 ++--
linux-user/syscall.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 150d1d4503..6ed2141674 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2771,7 +2771,7 @@ static void pgb_reserved_va(const char *image_name,
abi_ulong guest_loaddr,
error_report("Unable to reserve 0x%lx bytes of virtual address "
"space at %p (%s) for use as guest address space (check
your "
"virtual memory ulimit setting, min_mmap_addr or reserve
less "
- "using -R option)", reserved_va, test, strerror(errno));
+ "using -R option)", reserved_va, test,
qemu_strerror(errno));
exit(EXIT_FAILURE);
}
@@ -3564,7 +3564,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct
image_info *info)
g_free(scratch);
if (!bprm->p) {
-fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG));
+fprintf(stderr, "%s: %s\n", bprm->filename, qemu_strerror(E2BIG));
exit(-1);
}
diff --git a/linux-user/main.c b/linux-user/main.c
index 4b18461969..e0e133d631 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -744,7 +744,7 @@ int main(int argc, char **argv, char **envp)
if (execfd == 0) {
execfd = open(exec_path, O_RDONLY);
if (execfd < 0) {
-printf("Error while loading %s: %s\n", exec_path, strerror(errno));
+printf("Error while loading %s: %s\n", exec_path,
qemu_strerror(errno));
_exit(EXIT_FAILURE);
}
}
@@ -887,7 +887,7 @@ int main(int argc, char **argv, char **envp)
ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
info, );
if (ret != 0) {
-printf("Error while loading %s: %s\n", exec_path, strerror(-ret));
+printf("Error while loading %s: %s\n", exec_path, qemu_strerror(-ret));
_exit(EXIT_FAILURE);
}
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 24cea6fb6a..50090feac5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -580,7 +580,7 @@ const char *target_strerror(int err)
return "Successful exit from sigreturn";
}
-return strerror(target_to_host_errno(err));
+return qemu_strerror(target_to_host_errno(err));
}
static int check_zeroed_user(abi_long addr, size_t ksize, size_t usize)
--
2.39.2
[RFC PATCH v2 1/2] util: Add thread-safe qemu_strerror() function
Add qemu_strerror() which follows the POSIX specification for
strerror(). While strerror() is not guaranteed to be thread-safe, this
function is thread-safe.
This function is added to solve the following issue:
https://gitlab.com/qemu-project/qemu/-/issues/416
Signed-off-by: Yohei Kojima
---
include/qemu/cutils.h | 20 +++
util/cutils.c | 45 +++
2 files changed, 65 insertions(+)
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 92c436d8c7..0bcae0049a 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -117,6 +117,26 @@ int stristart(const char *str, const char *val, const char
**ptr);
* Returns: length of @s in bytes, or @max_len, whichever is smaller.
*/
int qemu_strnlen(const char *s, int max_len);
+/**
+ * qemu_strerror:
+ * @errnum: error number
+ *
+ * Return the pointer to a string that describes errnum, like
+ * strerror(). This function is thread-safe because the buffer for
+ * returned string is allocated per thread.
+ *
+ * This function is thread-safe, but not reentrant. In other words,
+ * if a thread is interrupted by a signal in this function, and the
+ * thread calls this function again in the signal handling, then
+ * the result might be corrupted.
+ *
+ * This function has the same behaviour as the POSIX strerror()
+ * function.
+ *
+ * Returns: the pointer to an error description, or an
+ * "Unknown error nnn" message if errnum is invalid.
+ */
+char *qemu_strerror(int errnum);
/**
* qemu_strsep:
* @input: pointer to string to parse
diff --git a/util/cutils.c b/util/cutils.c
index 5887e74414..3d14f50c75 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -131,6 +131,51 @@ int qemu_strnlen(const char *s, int max_len)
return i;
}
+/**
+ * It assumes the length of error descriptions are at most 1024.
+ * The concern of write buffer overflow is cared by strerror_r().
+ */
+static __thread char qemu_strerror_buf[1024];
+
+#if (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE
+/**
+ * In POSIX.1-2001 and after, the return type of strerror_r is int, but
+ * glibc overrides the definition of strerror_r to the old strerror_r
+ * if _GNU_SOURCE is defined. This condition handles it.
+ */
+
+char *qemu_strerror(int errnum)
+{
+int is_error = strerror_r(errnum, qemu_strerror_buf, 1024);
+
+if (is_error == 0) return qemu_strerror_buf;
+
+/**
+ * handle the error occured in strerror_r
+ *
+ * If is_error is greater than 0, errno might not be updated by
+ * strerror_r. Otherwise, errno is updated.
+ */
+if (is_error > 0) errno = is_error;
+
+strncpy(qemu_strerror_buf, "Error %d occured\n", errno);
+return qemu_strerror_buf;
+}
+#else
+/**
+ * In glibc, the return type of strerror_r is char* if _GNU_SOURCE
+ * is defined. In this case, strerror_r returns qemu_strerror_buf iff
+ * some error occured in strerror_r, and otherwise it returns a pointer
+ * to the pre-defined description for errnum.
+ *
+ * This is the same behaviour until POSIX.1-2001.
+ */
+char *qemu_strerror(int errnum)
+{
+return strerror_r(errnum, qemu_strerror_buf, 1024);
+}
+#endif
+
char *qemu_strsep(char **input, const char *delim)
{
char *result = *input;
--
2.39.2
[PULL 1/2] disas/riscv: Fix slli_uw decoding
From: Ivan Klokov
The decoding of the slli_uw currently contains decoding
error: shamt part of opcode has six bits, not five.
Fixes 3de1fb71("target/riscv: update disas.c for xnor/orn/andn and slli.uw")
Signed-off-by: Ivan Klokov
Reviewed-by: Philipp Tomsich
Acked-by: Alistair Francis
Message-Id: <20230227090228.17117-1-ivan.klo...@syntacore.com>
Signed-off-by: Alistair Francis
---
disas/riscv.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/disas/riscv.c b/disas/riscv.c
index 544558..2aca11b90e 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -1647,7 +1647,7 @@ const rv_opcode_data opcode_data[] = {
{ "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
{ "ctzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
{ "cpopw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "slli.uw", rv_codec_i_sh5, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
+{ "slli.uw", rv_codec_i_sh6, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
{ "add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
{ "rolw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
{ "rorw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
@@ -2617,10 +2617,10 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa
isa)
switch (((inst >> 12) & 0b111)) {
case 0: op = rv_op_addiw; break;
case 1:
-switch (((inst >> 25) & 0b111)) {
+switch (((inst >> 26) & 0b11)) {
case 0: op = rv_op_slliw; break;
-case 4: op = rv_op_slli_uw; break;
-case 48:
+case 2: op = rv_op_slli_uw; break;
+case 24:
switch ((inst >> 20) & 0b1) {
case 0b0: op = rv_op_clzw; break;
case 0b1: op = rv_op_ctzw; break;
--
2.39.2
[PULL 2/2] Fix incorrect register name in disassembler for fmv, fabs, fneg instructions
From: Mikhail Tyutin
Fix incorrect register name in RISC-V disassembler for fmv,fabs,fneg
instructions
Signed-off-by: Mikhail Tyutin
Reviewed-by: Alistair Francis
Message-Id: <3454991f-7f64-24c3-9a36-f5fa2cc38...@yadro.com>
Signed-off-by: Alistair Francis
---
disas/riscv.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/disas/riscv.c b/disas/riscv.c
index 2aca11b90e..d6b0fbe5e8 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -1014,6 +1014,7 @@ static const char rv_vreg_name_sym[32][4] = {
#define rv_fmt_rd_offset "O\t0,o"
#define rv_fmt_rd_rs1_rs2 "O\t0,1,2"
#define rv_fmt_frd_rs1"O\t3,1"
+#define rv_fmt_frd_frs1 "O\t3,4"
#define rv_fmt_rd_frs1"O\t0,4"
#define rv_fmt_rd_frs1_frs2 "O\t0,4,5"
#define rv_fmt_frd_frs1_frs2 "O\t3,4,5"
@@ -1580,15 +1581,15 @@ const rv_opcode_data opcode_data[] = {
{ "snez", rv_codec_r, rv_fmt_rd_rs2, NULL, 0, 0, 0 },
{ "sltz", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
{ "sgtz", rv_codec_r, rv_fmt_rd_rs2, NULL, 0, 0, 0 },
-{ "fmv.s", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fabs.s", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fneg.s", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fmv.d", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fabs.d", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fneg.d", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fmv.q", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fabs.q", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
-{ "fneg.q", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
+{ "fmv.s", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fabs.s", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fneg.s", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fmv.d", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fabs.d", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fneg.d", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fmv.q", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fabs.q", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
+{ "fneg.q", rv_codec_r, rv_fmt_frd_frs1, NULL, 0, 0, 0 },
{ "beqz", rv_codec_sb, rv_fmt_rs1_offset, NULL, 0, 0, 0 },
{ "bnez", rv_codec_sb, rv_fmt_rs1_offset, NULL, 0, 0, 0 },
{ "blez", rv_codec_sb, rv_fmt_rs2_offset, NULL, 0, 0, 0 },
--
2.39.2
[PULL 0/2] riscv-to-apply queue
From: Alistair Francis The following changes since commit 284c52eec2d0a1b9c47f06c3eee46762c5fc0915: Merge tag 'win-socket-pull-request' of https://gitlab.com/marcandre.lureau/qemu into staging (2023-03-13 13:44:17 +) are available in the Git repository at: https://github.com/alistair23/qemu.git tags/pull-riscv-to-apply-20230314 for you to fetch changes up to 0d581506de803204c5a321100afa270573382932: Fix incorrect register name in disassembler for fmv,fabs,fneg instructions (2023-03-14 16:36:43 +1000) Seventh RISC-V PR for 8.0 * Fix slli_uw decoding * Fix incorrect register name in disassembler for fmv,fabs,fneg instructions Ivan Klokov (1): disas/riscv: Fix slli_uw decoding Mikhail Tyutin (1): Fix incorrect register name in disassembler for fmv,fabs,fneg instructions disas/riscv.c | 27 ++- 1 file changed, 14 insertions(+), 13 deletions(-)
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment
On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
> uses slot_reserved_mask to reserve slot 2 for the Intel IGD for the
> xenfv machine when the guest is configured for igd-passthru.
>
> A desired extension to that commit is to allow use of the reserved slot
> if the administrator manually configures a device to use the reserved
> slot. Currently, slot_reserved_mask is enforced unconditionally. With
> this patch, the pci bus can be configured so the slot is only reserved
> if the pci device to be added to the bus is configured for automatic
> slot assignment.
>
> To enable the desired behavior of slot_reserved_mask machine, add a
> boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> called to change the default behavior of always enforcing
> slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> when the pci device being added is configured for automatic slot
> assignment.
>
> Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> creating the pci bus for the pc/i440fx/xenfv machine type to implement
> the desired behavior of causing slot_reserved_mask to only apply when
> the pci device to be added to a pc/i440fx/xenfv machine is configured
> for automatic slot assignment.
>
> Link:
> https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-...@kernel.org/
> Signed-off-by: Chuck Zmudzinski
I really dislike this.
It seems that xen should not have used slot_reserved_mask,
and instead needs something new like slot_manual_mask.
No?
> ---
> Changelog
>
> v2: Change Subject of patch from
> "pci: add enforce_slot_reserved_mask_manual property" To
> "pci: allow slot_reserved_mask to be ignored with manual slot assignment"
>
> Add pci_bus_ignore_slot_reserved_mask_manual function
>
> Call pci_bus_ignore_slot_reserved_mask_manual at appropriate place
> in hw/pci-host/i440fx.c
>
> hw/pci-host/i440fx.c | 1 +
> hw/pci/pci.c | 14 +-
> include/hw/pci/pci.h | 1 +
> include/hw/pci/pci_bus.h | 1 +
> 4 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 262f82c303..8e00b88926 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -257,6 +257,7 @@ PCIBus *i440fx_init(const char *pci_type,
> s = PCI_HOST_BRIDGE(dev);
> b = pci_root_bus_new(dev, NULL, pci_address_space,
> address_space_io, 0, TYPE_PCI_BUS);
> +pci_bus_ignore_slot_reserved_mask_manual(b);
> s->bus = b;
> object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 8a87ccc8b0..670ecc6986 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -501,6 +501,7 @@ static void pci_root_bus_internal_init(PCIBus *bus,
> DeviceState *parent,
> assert(PCI_FUNC(devfn_min) == 0);
> bus->devfn_min = devfn_min;
> bus->slot_reserved_mask = 0x0;
> +bus->enforce_slot_reserved_mask_manual = true;
> bus->address_space_mem = address_space_mem;
> bus->address_space_io = address_space_io;
> bus->flags |= PCI_BUS_IS_ROOT;
> @@ -1116,6 +1117,17 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int
> devfn)
> return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> }
>
> +static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
> +{
> +return bus->enforce_slot_reserved_mask_manual &&
> +(bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
> +}
> +
> +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus)
> +{
> +bus->enforce_slot_reserved_mask_manual = false;
> +}
> +
> uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus)
> {
> return bus->slot_reserved_mask;
> @@ -1164,7 +1176,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev,
> "or reserved", name);
> return NULL;
> found: ;
> -} else if (pci_bus_devfn_reserved(bus, devfn)) {
> +} else if (pci_bus_devfn_reserved_manual(bus, devfn)) {
> error_setg(errp, "PCI: slot %d function %d not available for %s,"
> " reserved",
> PCI_SLOT(devfn), PCI_FUNC(devfn), name);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 935b4b91b4..48d29ec234 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -287,6 +287,7 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
> void pci_bus_map_irqs(PCIBus *bus, pci_map_irq_fn map_irq);
> void pci_bus_irqs_cleanup(PCIBus *bus);
> int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus);
> uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus);
> void
Re: [PATCH v4 0/6] hw/cxl: Poison get, inject, clear
Hi Jonathan, On 3/3/23 16:09, Jonathan Cameron wrote: Note there are several series ahead of this one and in particular the RAS error injection series needs some QAPI review. The QAPI stuff in this patch is similar but in essence very similar to what we have in that series. Whilst I'm an always an optimist, this may well end up as 8.1 material now. Ira Weiny (2): hw/cxl: Introduce cxl_device_get_timestamp() utility function bswap: Add the ability to store to an unaligned 24 bit field Jonathan Cameron (4): hw/cxl: rename mailbox return code type from ret_code to CXLRetCode hw/cxl: QMP based poison injection support hw/cxl: Add poison injection via the mailbox. hw/cxl: Add clear poison mailbox command support. hw/cxl/cxl-device-utils.c | 15 ++ hw/cxl/cxl-mailbox-utils.c | 283 ++-- hw/mem/cxl_type3.c | 92 hw/mem/cxl_type3_stubs.c| 6 + include/hw/cxl/cxl_device.h | 23 +++ There is a '64' magic number used in various places, I haven't tried to figure what is / where it comes from, but a CXL #definition for it could make sense.
Re: [PATCH v4 5/6] hw/cxl: Add poison injection via the mailbox.
On 3/3/23 16:09, Jonathan Cameron wrote:
Very simple implementation to allow testing of corresponding
kernel code. Note that for now we track each 64 byte section
independently. Whilst a valid implementation choice, it may
make sense to fuse entries so as to prove out more complex
corners of the kernel code.
Reviewed-by: Ira Weiny
Signed-off-by: Jonathan Cameron
---
v4: No change
---
hw/cxl/cxl-mailbox-utils.c | 41 ++
1 file changed, 41 insertions(+)
+static CXLRetCode cmd_media_inject_poison(struct cxl_cmd *cmd,
+ CXLDeviceState *cxl_dstate,
+ uint16_t *len)
+{
+CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
This makes me wonder why CXLDeviceState isn't QDev based.
(Also, why include/hw/cxl/cxl_device.h is under GPL-2.0-only license?)
Re: [PATCH v3 1/3] numa: Validate cluster and NUMA node boundary if required
On 3/13/23 7:40 PM, Philippe Mathieu-Daudé wrote:
On 25/2/23 07:35, Gavin Shan wrote:
For some architectures like ARM64, multiple CPUs in one cluster can be
associated with different NUMA nodes, which is irregular configuration
because we shouldn't have this in baremetal environment. The irregular
configuration causes Linux guest to misbehave, as the following warning
messages indicate.
-smp 6,maxcpus=6,sockets=2,clusters=1,cores=3,threads=1 \
-numa node,nodeid=0,cpus=0-1,memdev=ram0 \
-numa node,nodeid=1,cpus=2-3,memdev=ram1 \
-numa node,nodeid=2,cpus=4-5,memdev=ram2 \
[ cut here ]
WARNING: CPU: 0 PID: 1 at kernel/sched/topology.c:2271
build_sched_domains+0x284/0x910
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0-268.el9.aarch64 #1
pstate: 0045 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : build_sched_domains+0x284/0x910
lr : build_sched_domains+0x184/0x910
sp : 8804bd50
x29: 8804bd50 x28: 0002 x27:
x26: 89cf9a80 x25: x24: 89cbf840
x23: 80325000 x22: 005df800 x21: 8a4ce508
x20: x19: 80324440 x18: 0014
x17: 388925c0 x16: 5386a066 x15: 9c10cc2e
x14: 01c0 x13: 0001 x12: 7fffb1a0
x11: 7fffb180 x10: 8a4ce508 x9 : 0041
x8 : 8a4ce500 x7 : 8a4cf920 x6 : 0001
x5 : 0001 x4 : 0007 x3 : 0002
x2 : 1000 x1 : 8a4cf928 x0 : 0001
Call trace:
build_sched_domains+0x284/0x910
sched_init_domains+0xac/0xe0
sched_init_smp+0x48/0xc8
kernel_init_freeable+0x140/0x1ac
kernel_init+0x28/0x140
ret_from_fork+0x10/0x20
Improve the situation to warn when multiple CPUs in one cluster have
been associated with different NUMA nodes. However, one NUMA node is
allowed to be associated with different clusters.
Signed-off-by: Gavin Shan
---
hw/core/machine.c | 42 ++
include/hw/boards.h | 1 +
2 files changed, 43 insertions(+)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index f29e700ee4..3513df5a86 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1252,6 +1252,45 @@ static void machine_numa_finish_cpu_init(MachineState
*machine)
g_string_free(s, true);
}
+static void validate_cpu_cluster_to_numa_boundary(MachineState *ms)
+{
+ MachineClass *mc = MACHINE_GET_CLASS(ms);
+ NumaState *state = ms->numa_state;
+ const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(ms);
+ const CPUArchId *cpus = possible_cpus->cpus;
+ int len = possible_cpus->len, i, j;
(Nitpicking, 'len' variable is not very useful).
Yes, Lets drop it if I need to post a new revision :)
+
+ if (state->num_nodes <= 1 || len <= 1) {
+ return;
+ }
+
+ /*
+ * The Linux scheduling domain can't be parsed when the multiple CPUs
+ * in one cluster have been associated with different NUMA nodes. However,
+ * it's fine to associate one NUMA node with CPUs in different clusters.
+ */
+ for (i = 0; i < len; i++) {
+ for (j = i + 1; j < len; j++) {
+ if (cpus[i].props.has_socket_id &&
+ cpus[i].props.has_cluster_id &&
+ cpus[i].props.has_node_id &&
+ cpus[j].props.has_socket_id &&
+ cpus[j].props.has_cluster_id &&
+ cpus[j].props.has_node_id &&
+ cpus[i].props.socket_id == cpus[j].props.socket_id &&
+ cpus[i].props.cluster_id == cpus[j].props.cluster_id &&
+ cpus[i].props.node_id != cpus[j].props.node_id) {
+ warn_report("CPU-%d and CPU-%d in socket-%ld-cluster-%ld "
+ "have been associated with node-%ld and node-%ld "
+ "respectively. It can cause OSes like Linux to"
+ "misbehave", i, j, cpus[i].props.socket_id,
+ cpus[i].props.cluster_id, cpus[i].props.node_id,
+ cpus[j].props.node_id);
machine_run_board_init() takes an Error* argument, but is only called
once by qemu_init_board() with errp=_fatal. I suppose using
warn_report() here is OK.
Acked-by: Philippe Mathieu-Daudé
warn_report() here is correct because it's inappropriate to propogate the
warning message to @error_fatal through error_setg(). When the messages
included in @error_fatal is handled and printed in util/error.c::error_handle(),
the QEMU process will be terminated unexpectedly.
+ }
+ }
+ }
+}
+
MemoryRegion *machine_consume_memdev(MachineState *machine,
HostMemoryBackend *backend)
{
@@ -1337,6 +1376,9 @@ void
Re: [PATCH v4 4/6] hw/cxl: QMP based poison injection support
On 3/3/23 16:09, Jonathan Cameron wrote:
Inject poison using qmp command cxl-inject-poison to add an entry to the
poison list.
For now, the poison is not returned CXL.mem reads,
What do you mean?
but only via the
mailbox command Get Poison List.
See CXL rev 3.0, sec 8.2.9.8.4.1 Get Poison list (Opcode 4300h)
Kernel patches to use this interface here:
https://lore.kernel.org/linux-cxl/cover.1665606782.git.alison.schofi...@intel.com/
To inject poison using qmp (telnet to the qmp port)
{ "execute": "qmp_capabilities" }
{ "execute": "cxl-inject-poison",
"arguments": {
"path": "/machine/peripheral/cxl-pmem0",
"start": 2048,
"length": 256
}
}
Adjusted to select a device on your machine.
Note that the poison list supported is kept short enough to avoid the
complexity of state machine that is needed to handle the MORE flag.
Signed-off-by: Jonathan Cameron
---
v4:
- Widen the mask on Poison source (lower bits of the address)
to allow for Vendor Defined. Change will make it easier to potentially
add a means to inject such poison in the future. Today it has no
impact.
---
hw/cxl/cxl-mailbox-utils.c | 90 +
hw/mem/cxl_type3.c | 56 +++
hw/mem/cxl_type3_stubs.c| 6 +++
include/hw/cxl/cxl_device.h | 20 +
qapi/cxl.json | 18
5 files changed, 190 insertions(+)
+/*
+ * This is very inefficient, but good enough for now!
+ * Also the payload will always fit, so no need to handle the MORE flag and
+ * make this stateful. We may want to allow longer poison lists to aid
+ * testing that kernel functionality.
+ */
+static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd,
+CXLDeviceState *cxl_dstate,
+uint16_t *len)
+{
+struct get_poison_list_pl {
+uint64_t pa;
+uint64_t length;
+} QEMU_PACKED;
+
+struct get_poison_list_out_pl {
+uint8_t flags;
+uint8_t rsvd1;
+uint64_t overflow_timestamp;
+uint16_t count;
+uint8_t rsvd2[0x14];
+struct {
+uint64_t addr;
+uint32_t length;
+uint32_t resv;
+} QEMU_PACKED records[];
+} QEMU_PACKED;
+
+struct get_poison_list_pl *in = (void *)cmd->payload;
+struct get_poison_list_out_pl *out = (void *)cmd->payload;
+CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+uint16_t record_count = 0, i = 0;
+uint64_t query_start, query_length;
+CXLPoisonList *poison_list = >poison_list;
+CXLPoison *ent;
+uint16_t out_pl_len;
+
+query_start = ldq_le_p(>pa);
+/* 64 byte alignemnt required */
+if (query_start & 0x3f) {
+return CXL_MBOX_INVALID_INPUT;
+}
+query_length = ldq_le_p(>length) * 64;
+
+QLIST_FOREACH(ent, poison_list, node) {
+/* Check for no overlap */
+if (ent->start >= query_start + query_length ||
+ent->start + ent->length <= query_start) {
+continue;
+}
+record_count++;
+}
+out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+memset(out, 0, out_pl_len);
+QLIST_FOREACH(ent, poison_list, node) {
+uint64_t start, stop;
+
+/* Check for no overlap */
+if (ent->start >= query_start + query_length ||
+ent->start + ent->length <= query_start) {
+continue;
+}
+
+/* Deal with overlap */
+start = MAX(ent->start & 0xffc0, query_start);
+stop = MIN((ent->start & 0xffc0) + ent->length,
~63ull or ROUND_DOWN(, 64ull) could be easier to read.
+ query_start + query_length);
+stq_le_p(>records[i].addr, start | (ent->type & 0x7));
+stl_le_p(>records[i].length, (stop - start) / 64);
+i++;
+}
+if (ct3d->poison_list_overflowed) {
+out->flags = (1 << 1);
+stq_le_p(>overflow_timestamp, ct3d->poison_list_overflow_ts);
+}
+stw_le_p(>count, record_count);
+*len = out_pl_len;
+return CXL_MBOX_SUCCESS;
+}
+
diff --git a/qapi/cxl.json b/qapi/cxl.json
index 4be7d46041..9ebd680dfe 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -5,6 +5,24 @@
# = CXL devices
##
+##
+# @cxl-inject-poison:
+#
+# Poison records indicate that a CXL memory device knows that a particular
+# memory region may be corrupted. This may be because of locally detected
+# errors (e.g. ECC failure) or poisoned writes received from other components
+# in the system. This injection mechanism enables testing of the OS handling
+# of poison records which may be queried via the CXL mailbox.
+#
+# @path: CXL type 3 device canonical QOM path
+# @start: Start address - must be 64 byte aligned.
+# @length: Length of poison to inject - must be a
Re: [PATCH v5 2/2] target/riscv: redirect XVentanaCondOps to use the Zicond functions
On Wed, Mar 8, 2023 at 4:10 AM Philipp Tomsich wrote:
>
> The Zicond standard extension implements the same instruction
> semantics as XVentanaCondOps, although using different mnemonics and
> opcodes.
>
> Point XVentanaCondOps to the (newly implemented) Zicond implementation
> to reduce the future maintenance burden.
>
> Also updating MAINTAINERS as trans_xventanacondops.c.inc.
>
> Reviewed-by: Richard Henderson
>
> Signed-off-by: Philipp Tomsich
Acked-by: Alistair Francis
Alistair
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Don't downgrade to "Odd Fixes", but rather to "Maintained" (we are
> not being paid to look after this, but will look after it
> nonetheless).
>
> Changes in v2:
> - Calls into the gen_czero_{eqz,nez} helpers instead of calling
> trans_czero_{eqz,nez} to bypass the require-check and ensure that
> XVentanaCondOps can be enabled/disabled independently of Zicond.
>
> MAINTAINERS| 2 +-
> .../insn_trans/trans_xventanacondops.c.inc | 18 +++---
> 2 files changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 011fd85a09..1ad3c6fc9a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -311,7 +311,7 @@ F: target/riscv/xthead*.decode
> RISC-V XVentanaCondOps extension
> M: Philipp Tomsich
> L: qemu-ri...@nongnu.org
> -S: Supported
> +S: Maintained
> F: target/riscv/XVentanaCondOps.decode
> F: target/riscv/insn_trans/trans_xventanacondops.c.inc
>
> diff --git a/target/riscv/insn_trans/trans_xventanacondops.c.inc
> b/target/riscv/insn_trans/trans_xventanacondops.c.inc
> index 16849e6d4e..38c15f2825 100644
> --- a/target/riscv/insn_trans/trans_xventanacondops.c.inc
> +++ b/target/riscv/insn_trans/trans_xventanacondops.c.inc
> @@ -1,7 +1,7 @@
> /*
> * RISC-V translation routines for the XVentanaCondOps extension.
> *
> - * Copyright (c) 2021-2022 VRULL GmbH.
> + * Copyright (c) 2021-2023 VRULL GmbH.
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms and conditions of the GNU General Public License,
> @@ -16,24 +16,12 @@
> * this program. If not, see
Re: [PATCH v5 1/2] target/riscv: refactor Zicond support
On Wed, Mar 8, 2023 at 4:10 AM Philipp Tomsich wrote:
>
> After the original Zicond support was stuck/fell through the cracks on
> the mailing list at v3 (and a different implementation was merged in
> the meanwhile), we need to refactor Zicond to prepare it to be reused
> by XVentanaCondOps.
>
> This commit lifts the common logic out into gen_czero and uses this
> via gen_logic and 2 helper functions (effectively partial closures).
>
> Reviewed-by: Richard Henderson
>
> Signed-off-by: Philipp Tomsich
Acked-by: Alistair Francis
Alistair
> ---
>
> Changes in v5:
> - fix a rebase artifact
> - drop the 'inline' specifiers (as per review comments)
>
> Changes in v4:
> - rebase onto master
>
> Changes in v3:
> - don't add this to MAINTAINERS, as it is an official extension
>
> Changes in v2:
> - gates availability of the instructions through a REQUIRE_ZICOND
> macro (these were previously always enabled)
>
> target/riscv/insn_trans/trans_rvzicond.c.inc | 36
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvzicond.c.inc
> b/target/riscv/insn_trans/trans_rvzicond.c.inc
> index 645260164e..c8e43fa325 100644
> --- a/target/riscv/insn_trans/trans_rvzicond.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzicond.c.inc
> @@ -2,6 +2,7 @@
> * RISC-V translation routines for the Zicond Standard Extension.
> *
> * Copyright (c) 2020-2023 PLCT Lab
> + * Copyright (c) 2022 VRULL GmbH.
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms and conditions of the GNU General Public License,
> @@ -22,28 +23,33 @@
> } \
> } while (0)
>
> -static bool trans_czero_eqz(DisasContext *ctx, arg_czero_eqz *a)
> +/* Emits "$rd = ($rs2 $zero) ? $zero : $rs1" */
> +static void gen_czero(TCGv dest, TCGv src1, TCGv src2, TCGCond cond)
> {
> -REQUIRE_ZICOND(ctx);
> +TCGv zero = tcg_constant_tl(0);
> +tcg_gen_movcond_tl(cond, dest, src2, zero, zero, src1);
> +}
>
> -TCGv dest = dest_gpr(ctx, a->rd);
> -TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> -TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +static void gen_czero_eqz(TCGv dest, TCGv src1, TCGv src2)
> +{
> +gen_czero(dest, src1, src2, TCG_COND_EQ);
> +}
>
> -tcg_gen_movcond_tl(TCG_COND_EQ, dest, src2, ctx->zero, ctx->zero, src1);
> -gen_set_gpr(ctx, a->rd, dest);
> -return true;
> +static void gen_czero_nez(TCGv dest, TCGv src1, TCGv src2)
> +{
> +gen_czero(dest, src1, src2, TCG_COND_NE);
> }
>
> -static bool trans_czero_nez(DisasContext *ctx, arg_czero_nez *a)
> +static bool trans_czero_eqz(DisasContext *ctx, arg_r *a)
> {
> REQUIRE_ZICOND(ctx);
>
> -TCGv dest = dest_gpr(ctx, a->rd);
> -TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> -TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +return gen_logic(ctx, a, gen_czero_eqz);
> +}
> +
> +static bool trans_czero_nez(DisasContext *ctx, arg_r *a)
> +{
> +REQUIRE_ZICOND(ctx);
>
> -tcg_gen_movcond_tl(TCG_COND_NE, dest, src2, ctx->zero, ctx->zero, src1);
> -gen_set_gpr(ctx, a->rd, dest);
> -return true;
> +return gen_logic(ctx, a, gen_czero_nez);
> }
> --
> 2.34.1
>
>
Re: [PATCH v4 3/6] bswap: Add the ability to store to an unaligned 24 bit field
On 3/3/23 16:09, Jonathan Cameron wrote:
From: Ira Weiny
CXL has 24 bit unaligned fields which need to be stored to. CXL is
specified as little endian.
Define st24_le_p() and the supporting functions to store such a field
from a 32 bit host native value.
The use of b, w, l, q as the size specifier is limiting. So "24" was
used for the size part of the function name.
Reviewed-by: Fan Ni
Signed-off-by: Ira Weiny
Signed-off-by: Jonathan Cameron
---
v8:
- Picked up tag from Fan Ni.
---
include/qemu/bswap.h | 23 +++
1 file changed, 23 insertions(+)
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 15a78c0db5..ee71cbeaaa 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -8,11 +8,23 @@
#undef bswap64
#define bswap64(_x) __builtin_bswap64(_x)
+static inline uint32_t bswap24(uint32_t x)
+{
Could it be safer to add:
assert(x & 0xff00U == 0);
...
+return (((x & 0x00ffU) << 16) |
+((x & 0xff00U) << 0) |
+((x & 0x00ffU) >> 16));
+}
+
static inline void bswap16s(uint16_t *s)
{
*s = __builtin_bswap16(*s);
}
+static inline void bswap24s(uint32_t *s)
+{
+*s = bswap24(*s);
... and here use:
*s = bswap24(*s & 0x00ffU);
?
+}
+
static inline void bswap32s(uint32_t *s)
{
*s = __builtin_bswap32(*s);
@@ -176,6 +188,7 @@ CPU_CONVERT(le, 64, uint64_t)
* size is:
* b: 8 bits
* w: 16 bits
+ * 24: 24 bits
Following the pattern, shouldn't this be 's' for sēsquiword?
Regardless you need to update the doc in docs/devel/loads-stores.rst.
* l: 32 bits
* q: 64 bits
*
@@ -248,6 +261,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
__builtin_memcpy(ptr, , sizeof(v));
}
+static inline void st24_he_p(void *ptr, uint32_t v)
+{
+__builtin_memcpy(ptr, , 3);
+}
+
static inline int ldl_he_p(const void *ptr)
{
int32_t r;
@@ -297,6 +315,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
stw_he_p(ptr, le_bswap(v, 16));
}
+static inline void st24_le_p(void *ptr, uint32_t v)
+{
+st24_he_p(ptr, le_bswap(v, 24));
+}
