Re: [question]vhost-user: atuo fix network link broken during migration
On 2020/3/23 下午4:17, yangke (J) wrote:
We find an issue when host mce trigger openvswitch(dpdk) restart in source host
during guest migration,
Did you mean the vhost-user netev was deleted from the source host?
VM is still link down in frontend after migration, it cause the network in VM
never be up again.
virtio_net_load_device:
/* nc.link_down can't be migrated, so infer link_down according
* to link status bit in n->status */
link_down = (n->status & VIRTIO_NET_S_LINK_UP) == 0;
for (i = 0; i < n->max_queues; i++) {
qemu_get_subqueue(n->nic, i)->link_down = link_down;
}
guset:migrate begin ---> vCPU pause > vmsate load
--->migrate finish
^ ^
^
| |
|
openvswitch in source host: begin to restart restarting
started
^ ^
^
| |
|
nc in frontend in source: link down link down
link down
^ ^
^
| |
|
nc in frontend in destination: link up link up
link down
^ ^
^
| |
|
guset network:broken broken
broken
^ ^
^
| |
|
nc in backend in source:link down link down
link up
^ ^
^
| |
|
nc in backend in destination: link up link up
link up
The link_down of frontend was loaded from n->status, n->status is link down in
source, so the link_down of
frontend is true. The backend in destination host is link up, but the frontend
in destination host is
link down, it cause the network in gust never be up again until an guest cold
reboot.
Is there a way to auto fix the link status? or just abort the migration in
virtio net device load?
Maybe we can try to sync link status after migration?
Thanks
Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
On 2020/3/24 上午9:29, Li Qiang wrote:
P J P mailto:ppan...@redhat.com>> 于2020年3月23日周一
下午8:24写道：
From: Prasad J Pandit mailto:p...@fedoraproject.org>>
Tulip network driver while copying tx/rx buffers does not check
frame size against r/w data length. This may lead to OOB buffer
access. Add check to avoid it.
Limit iterations over descriptors to avoid potential infinite
loop issue in tulip_xmit_list_update.
Reported-by: Li Qiang mailto:pangpei...@antfin.com>>
Reported-by: Ziming Zhang mailto:ezrak...@gmail.com>>
Reported-by: Jason Wang mailto:jasow...@redhat.com>>
Signed-off-by: Prasad J Pandit mailto:p...@fedoraproject.org>>
Tested-by: Li Qiang mailto:liq...@gmail.com>>
But I have a minor question
---
hw/net/tulip.c | 36 +++-
1 file changed, 27 insertions(+), 9 deletions(-)
Update v3: return a value from tulip_copy_tx_buffers() and avoid
infinite loop
->
https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index cfac2719d3..fbe40095da 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState
*s, struct tulip_descriptor *desc)
} else {
len = s->rx_frame_len;
}
+
+ if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
+ return;
+ }
Why here is '>=' instead of '>'.
IIUC the total sending length can reach to sizeof(s->rx_frame).
Same in the other place in this patch.
Yes, this need to be fixed.
PS: I have planned to write a qtest case. But my personal qemu dev
environment is broken.
I will try to write it tonight or tomorrow night.
Cool, good to know this.
Thanks
Thanks,
Li Qiang
pci_dma_write(>dev, desc->buf_addr1, s->rx_frame +
(s->rx_frame_size - s->rx_frame_len), len);
s->rx_frame_len -= len;
@@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState
*s, struct tulip_descriptor *desc)
} else {
len = s->rx_frame_len;
}
+
+ if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
+ return;
+ }
pci_dma_write(>dev, desc->buf_addr2, s->rx_frame +
(s->rx_frame_size - s->rx_frame_len), len);
s->rx_frame_len -= len;
@@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s,
const uint8_t *buf, size_t size)
trace_tulip_receive(buf, size);
- if (size < 14 || size > 2048 || s->rx_frame_len ||
tulip_rx_stopped(s)) {
+ if (size < 14 || size > sizeof(s->rx_frame) - 4
+ || s->rx_frame_len || tulip_rx_stopped(s)) {
return 0;
}
@@ -275,7 +284,6 @@ static ssize_t tulip_receive_nc(NetClientState
*nc,
return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
}
-
static NetClientInfo net_tulip_info = {
.type = NET_CLIENT_DRIVER_NIC,
.size = sizeof(NICState),
@@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct
tulip_descriptor *desc)
if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
/* Internal or external Loopback */
tulip_receive(s, s->tx_frame, s->tx_frame_len);
- } else {
+ } else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
qemu_send_packet(qemu_get_queue(s->nic),
s->tx_frame, s->tx_frame_len);
}
@@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct
tulip_descriptor *desc)
}
}
-static void tulip_copy_tx_buffers(TULIPState *s, struct
tulip_descriptor *desc)
+static int tulip_copy_tx_buffers(TULIPState *s, struct
tulip_descriptor *desc)
{
int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
TDES1_BUF1_SIZE_MASK;
int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
TDES1_BUF2_SIZE_MASK;
+ if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
+ return -1;
+ }
if (len1) {
pci_dma_read(>dev, desc->buf_addr1,
s->tx_frame + s->tx_frame_len, len1);
s->tx_frame_len += len1;
}
+ if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
+ return -1;
+ }
if (len2) {
pci_dma_read(>dev, desc->buf_addr2,
s->tx_frame + s->tx_frame_len, len2);
s->tx_frame_len += len2;
}
desc->status = (len1 + len2) ? 0 : 0x7fff;
+
+ return 0;
}
static void tulip_setup_filter_addr(TULIPState *s, uint8_t *buf,
int n)
@@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
static void tulip_xmit_list_update(TULIPState *s)
{
+#define TULIP_DESC_MAX 128
Re: [PATCH v6 2/2] net: tulip: add .can_receive routine
On 2020/3/24 上午10:04, Li Qiang wrote:
P J P mailto:ppan...@redhat.com>> 于2020年3月23日周一
下午8:25写道：
From: Prasad J Pandit mailto:p...@fedoraproject.org>>
Define .can_receive routine to do sanity checks before receiving
packet data. And call qemu_flush_queued_packets to flush queued
packets once they are read in tulip_receive().
Suggested-by: Jason Wang mailto:jasow...@redhat.com>>
Signed-off-by: Prasad J Pandit mailto:p...@fedoraproject.org>>
---
hw/net/tulip.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
Update v6: merge earlier patch 2 & 3 into one
->
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06868.html
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index fbe40095da..8d8c9519e7 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState
*s, const uint8_t *addr)
return ret;
}
+static int
+tulip_can_receive(NetClientState *nc)
+{
+ TULIPState *s = qemu_get_nic_opaque(nc);
+
+ if (s->rx_frame_len || tulip_rx_stopped(s)) {
+ return false;
+ }
+
+ return true;
+}
+
static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf,
size_t size)
{
struct tulip_descriptor desc;
@@ -236,7 +248,7 @@ static ssize_t tulip_receive(TULIPState *s,
const uint8_t *buf, size_t size)
trace_tulip_receive(buf, size);
if (size < 14 || size > sizeof(s->rx_frame) - 4
- || s->rx_frame_len || tulip_rx_stopped(s)) {
+ || !tulip_can_receive(s->nic->ncs)) {
return 0;
}
@@ -275,6 +287,8 @@ static ssize_t tulip_receive(TULIPState *s,
const uint8_t *buf, size_t size)
tulip_desc_write(s, s->current_rx_desc, );
tulip_next_rx_descriptor(s, );
} while (s->rx_frame_len);
+
+ qemu_flush_queued_packets(qemu_get_queue(s->nic));
Hi Prasad ans Jason,
I want to know why here we need call 'qemu_flush_queued_packets'.
AFAICS, the other networking card emulation need no this.
Right, I thought we need this because rx_frame_len is checked in
tulip_can_receive().
But not I think it's unnecessary. According to kernel driver code, the
rx is not necessarily stopped when rx buffer is overrun, e.g,
tulip_rx_refill() only toggle the doorbell when chip is LC82C168. But
own emulation did DC21143.
This means using can_receive() is wrong.
So I think we can drop this patch.
Thanks for the checking and sorry for the wrong suggestion.
Thanks,
Li Qiang
return size;
}
@@ -288,6 +302,7 @@ static NetClientInfo net_tulip_info = {
.type = NET_CLIENT_DRIVER_NIC,
.size = sizeof(NICState),
.receive = tulip_receive_nc,
+ .can_receive = tulip_can_receive,
};
static const char *tulip_reg_name(const hwaddr addr)
--
2.25.1
[PULL 4/7] target/ppc: don't byte swap ELFv2 signal handler
From: Vincent Fazio
Previously, the signal handler would be byte swapped if the target and
host CPU used different endianness. This would cause a SIGSEGV when
attempting to translate the opcode pointed to by the swapped address.
Thread 1 "qemu-ppc64" received signal SIGSEGV, Segmentation fault.
0x600a9257 in ldl_he_p (ptr=0x4c2c0610) at
qemu/include/qemu/bswap.h:351
351__builtin_memcpy(, ptr, sizeof(r));
#0 0x600a9257 in ldl_he_p (ptr=0x4c2c0610) at
qemu/include/qemu/bswap.h:351
#1 0x600a92fe in ldl_be_p (ptr=0x4c2c0610) at
qemu/include/qemu/bswap.h:449
#2 0x600c0790 in translator_ldl_swap at
qemu/include/exec/translator.h:201
#3 0x6011c1ab in ppc_tr_translate_insn at
qemu/target/ppc/translate.c:7856
#4 0x6005ae70 in translator_loop at qemu/accel/tcg/translator.c:102
The signal handler will be byte swapped as a result of the __get_user()
call in sigaction() if it is necessary, no additional swap is required.
Signed-off-by: Vincent Fazio
Reviewed-by: Laurent Vivier
Reviewed-by: Richard Henderson
Message-Id: <20200319133244.8818-1-vfa...@xes-inc.com>
Signed-off-by: David Gibson
---
linux-user/ppc/signal.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index 0c4e7ba54c..ecd99736b7 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -567,10 +567,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
env->nip = tswapl(handler->entry);
env->gpr[2] = tswapl(handler->toc);
} else {
-/* ELFv2 PPC64 function pointers are entry points, but R12
- * must also be set */
-env->nip = tswapl((target_ulong) ka->_sa_handler);
-env->gpr[12] = env->nip;
+/* ELFv2 PPC64 function pointers are entry points. R12 must also be
set. */
+env->gpr[12] = env->nip = ka->_sa_handler;
}
#else
env->nip = (target_ulong) ka->_sa_handler;
--
2.25.1
[PULL 7/7] ppc/ppc405_boards: Remove unnecessary NULL check
From: Philippe Mathieu-Daudé
This code is inside the "if (dinfo)" condition, so testing
again here whether it is NULL is unnecessary.
Fixes: dd59bcae7 (Don't size flash memory to match backing image)
Reported-by: Coverity (CID 1421917)
Suggested-by: Peter Maydell
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20200320155740.5342-1-phi...@redhat.com>
Reviewed-by: Markus Armbruster
Signed-off-by: David Gibson
---
hw/ppc/ppc405_boards.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index e6bffb9e1a..6198ec1035 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -191,7 +191,7 @@ static void ref405ep_init(MachineState *machine)
bios_size = 8 * MiB;
pflash_cfi02_register((uint32_t)(-bios_size),
"ef405ep.bios", bios_size,
- dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+ blk_by_legacy_dinfo(dinfo),
64 * KiB, 1,
2, 0x0001, 0x22DA, 0x, 0x, 0x555, 0x2AA,
1);
@@ -459,7 +459,7 @@ static void taihu_405ep_init(MachineState *machine)
bios_size = 2 * MiB;
pflash_cfi02_register(0xFFE0,
"taihu_405ep.bios", bios_size,
- dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+ blk_by_legacy_dinfo(dinfo),
64 * KiB, 1,
4, 0x0001, 0x22DA, 0x, 0x, 0x555, 0x2AA,
1);
@@ -494,7 +494,7 @@ static void taihu_405ep_init(MachineState *machine)
if (dinfo) {
bios_size = 32 * MiB;
pflash_cfi02_register(0xfc00, "taihu_405ep.flash", bios_size,
- dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+ blk_by_legacy_dinfo(dinfo),
64 * KiB, 1,
4, 0x0001, 0x22DA, 0x, 0x, 0x555, 0x2AA,
1);
--
2.25.1
[PULL 2/7] target/ppc: Fix slbia TLB invalidation gap
From: Nicholas Piggin
slbia must invalidate TLBs even if it does not remove a valid SLB
entry, because slbmte can overwrite valid entries without removing
their TLBs.
As the architecture says, slbia invalidates all lookaside information,
not conditionally based on if it removed valid entries.
It does not seem possible for POWER8 or earlier Linux kernels to hit
this bug because it never changes its kernel SLB translations, and it
should always have valid entries if any accesses are made to userspace
regions. However other operating systems which may modify SLB entry 0
or do more fancy things with segments might be affected.
When POWER9 slbia support is added in the next patch, this becomes a
real problem because some new slbia variants don't invalidate all
non-zero entries.
Signed-off-by: Nicholas Piggin
Message-Id: <20200318044135.851716-1-npig...@gmail.com>
Reviewed-by: Cédric Le Goater
Reviewed-by: Greg Kurz
Signed-off-by: David Gibson
---
target/ppc/mmu-hash64.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 34f6009b1e..373d44de74 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -100,20 +100,29 @@ void helper_slbia(CPUPPCState *env)
PowerPCCPU *cpu = env_archcpu(env);
int n;
+/*
+ * slbia must always flush all TLB (which is equivalent to ERAT in ppc
+ * architecture). Matching on SLB_ESID_V is not good enough, because slbmte
+ * can overwrite a valid SLB without flushing its lookaside information.
+ *
+ * It would be possible to keep the TLB in synch with the SLB by flushing
+ * when a valid entry is overwritten by slbmte, and therefore slbia would
+ * not have to flush unless it evicts a valid SLB entry. However it is
+ * expected that slbmte is more common than slbia, and slbia is usually
+ * going to evict valid SLB entries, so that tradeoff is unlikely to be a
+ * good one.
+ */
+
/* XXX: Warning: slbia never invalidates the first segment */
for (n = 1; n < cpu->hash64_opts->slb_size; n++) {
ppc_slb_t *slb = >slb[n];
if (slb->esid & SLB_ESID_V) {
slb->esid &= ~SLB_ESID_V;
-/*
- * XXX: given the fact that segment size is 256 MB or 1TB,
- * and we still don't have a tlb_flush_mask(env, n, mask)
- * in QEMU, we just invalidate all TLBs
- */
-env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
}
}
+
+env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
}
static void __helper_slbie(CPUPPCState *env, target_ulong addr,
--
2.25.1
[PULL 3/7] target/ppc: Fix ISA v3.0 (POWER9) slbia implementation
From: Nicholas Piggin
The new ISA v3.0 slbia variants have not been implemented for TCG,
which can lead to crashing when a POWER9 machine boots Linux using
the hash MMU, for example ("disable_radix" kernel command line).
Add them.
Signed-off-by: Nicholas Piggin
Message-Id: <20200319064439.1020571-1-npig...@gmail.com>
Reviewed-by: Cédric Le Goater
[dwg: Fixed compile error for USER_ONLY builds]
Signed-off-by: David Gibson
---
target/ppc/helper.h | 2 +-
target/ppc/mmu-hash64.c | 56 +++--
target/ppc/translate.c | 5 +++-
3 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index cfb4c07085..a95c010391 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -614,7 +614,7 @@ DEF_HELPER_FLAGS_3(store_slb, TCG_CALL_NO_RWG, void, env,
tl, tl)
DEF_HELPER_2(load_slb_esid, tl, env, tl)
DEF_HELPER_2(load_slb_vsid, tl, env, tl)
DEF_HELPER_2(find_slb_vsid, tl, env, tl)
-DEF_HELPER_FLAGS_1(slbia, TCG_CALL_NO_RWG, void, env)
+DEF_HELPER_FLAGS_2(slbia, TCG_CALL_NO_RWG, void, env, i32)
DEF_HELPER_FLAGS_2(slbie, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_FLAGS_2(slbieg, TCG_CALL_NO_RWG, void, env, tl)
#endif
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 373d44de74..e5baabf0e1 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -95,9 +95,10 @@ void dump_slb(PowerPCCPU *cpu)
}
}
-void helper_slbia(CPUPPCState *env)
+void helper_slbia(CPUPPCState *env, uint32_t ih)
{
PowerPCCPU *cpu = env_archcpu(env);
+int starting_entry;
int n;
/*
@@ -111,18 +112,59 @@ void helper_slbia(CPUPPCState *env)
* expected that slbmte is more common than slbia, and slbia is usually
* going to evict valid SLB entries, so that tradeoff is unlikely to be a
* good one.
+ *
+ * ISA v2.05 introduced IH field with values 0,1,2,6. These all invalidate
+ * the same SLB entries (everything but entry 0), but differ in what
+ * "lookaside information" is invalidated. TCG can ignore this and flush
+ * everything.
+ *
+ * ISA v3.0 introduced additional values 3,4,7, which change what SLBs are
+ * invalidated.
*/
-/* XXX: Warning: slbia never invalidates the first segment */
-for (n = 1; n < cpu->hash64_opts->slb_size; n++) {
-ppc_slb_t *slb = >slb[n];
+env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
+
+starting_entry = 1; /* default for IH=0,1,2,6 */
+
+if (env->mmu_model == POWERPC_MMU_3_00) {
+switch (ih) {
+case 0x7:
+/* invalidate no SLBs, but all lookaside information */
+return;
-if (slb->esid & SLB_ESID_V) {
-slb->esid &= ~SLB_ESID_V;
+case 0x3:
+case 0x4:
+/* also considers SLB entry 0 */
+starting_entry = 0;
+break;
+
+case 0x5:
+/* treat undefined values as ih==0, and warn */
+qemu_log_mask(LOG_GUEST_ERROR,
+ "slbia undefined IH field %u.\n", ih);
+break;
+
+default:
+/* 0,1,2,6 */
+break;
}
}
-env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
+for (n = starting_entry; n < cpu->hash64_opts->slb_size; n++) {
+ppc_slb_t *slb = >slb[n];
+
+if (!(slb->esid & SLB_ESID_V)) {
+continue;
+}
+if (env->mmu_model == POWERPC_MMU_3_00) {
+if (ih == 0x3 && (slb->vsid & SLB_VSID_C) == 0) {
+/* preserves entries with a class value of 0 */
+continue;
+}
+}
+
+slb->esid &= ~SLB_ESID_V;
+}
}
static void __helper_slbie(CPUPPCState *env, target_ulong addr,
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 127c82a24e..b207fb5386 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4997,9 +4997,12 @@ static void gen_slbia(DisasContext *ctx)
#if defined(CONFIG_USER_ONLY)
GEN_PRIV;
#else
+uint32_t ih = (ctx->opcode >> 21) & 0x7;
+TCGv_i32 t0 = tcg_const_i32(ih);
+
CHK_SV;
-gen_helper_slbia(cpu_env);
+gen_helper_slbia(cpu_env, t0);
#endif /* defined(CONFIG_USER_ONLY) */
}
--
2.25.1
[PULL 5/7] spapr: Fix memory leak in h_client_architecture_support()
From: Greg Kurz
This is the only error path that needs to free the previously allocated
ov1.
Reported-by: Coverity (CID 1421924)
Fixes: cbd0d7f36322 "spapr: Fail CAS if option vector table cannot be parsed"
Signed-off-by: Greg Kurz
Message-Id: <158481206205.336182.16106097429336044843.st...@bahia.lan>
Signed-off-by: David Gibson
Reviewed-by: Philippe Mathieu-Daudé
---
hw/ppc/spapr_hcall.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 40c86e91eb..0d50fc9117 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1726,6 +1726,7 @@ static target_ulong
h_client_architecture_support(PowerPCCPU *cpu,
}
ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
if (!ov5_guest) {
+spapr_ovec_cleanup(ov1_guest);
warn_report("guest didn't provide option vector 5");
return H_PARAMETER;
}
--
2.25.1
[PULL 0/7] ppc-for-5.0 queue 20200324
The following changes since commit c532b954d96f96d361ca31308f75f1b95bd4df76: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200323' into staging (2020-03-23 17:41:21 +) are available in the Git repository at: git://github.com/dgibson/qemu.git tags/ppc-for-5.0-20200324 for you to fetch changes up to 1583794b9b36911df116cc726750dadbeeac506a: ppc/ppc405_boards: Remove unnecessary NULL check (2020-03-24 11:56:37 +1100) ppc patch queue for 2020-03-24 Here's a final pull request before the qemu-5.0 hard freeze. We have an implementation of the POWER9 forms of the slbia instruction, a small cleanup and a handful of assorted fixes. Greg Kurz (1): spapr: Fix memory leak in h_client_architecture_support() Mahesh Salgaonkar (1): ppc/spapr: Set the effective address provided flag in mc error log. Nicholas Piggin (2): target/ppc: Fix slbia TLB invalidation gap target/ppc: Fix ISA v3.0 (POWER9) slbia implementation Peter Maydell (1): hw/ppc: Take QEMU lock when calling ppc_dcr_read/write() Philippe Mathieu-Daudé (1): ppc/ppc405_boards: Remove unnecessary NULL check Vincent Fazio (1): target/ppc: don't byte swap ELFv2 signal handler hw/ppc/ppc405_boards.c | 6 ++-- hw/ppc/spapr_events.c| 26 hw/ppc/spapr_hcall.c | 1 + linux-user/ppc/signal.c | 6 ++-- target/ppc/helper.h | 2 +- target/ppc/mmu-hash64.c | 73 +--- target/ppc/timebase_helper.c | 40 +++- target/ppc/translate.c | 5 ++- 8 files changed, 125 insertions(+), 34 deletions(-)
[PULL 6/7] hw/ppc: Take QEMU lock when calling ppc_dcr_read/write()
From: Peter Maydell
The ppc_dcr_read() and ppc_dcr_write() functions call into callbacks
in device code, so we need to hold the QEMU iothread lock while
calling them. This is the case already for the callsites in
kvmppc_handle_dcr_read/write(), but we must also take the lock when
calling the helpers from TCG.
This fixes a bug where attempting to initialise the PPC405EP
SDRAM will cause an assertion when sdram_map_bcr() attempts
to remap memory regions.
Reported-by: Amit Lazar
Signed-off-by: Peter Maydell
Message-Id: <20200322192258.14039-1-peter.mayd...@linaro.org>
Signed-off-by: David Gibson
---
target/ppc/timebase_helper.c | 40 +++-
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
index 703bd9ed18..d16360ab66 100644
--- a/target/ppc/timebase_helper.c
+++ b/target/ppc/timebase_helper.c
@@ -21,6 +21,7 @@
#include "exec/helper-proto.h"
#include "exec/exec-all.h"
#include "qemu/log.h"
+#include "qemu/main-loop.h"
/*/
/* SPR accesses */
@@ -167,13 +168,19 @@ target_ulong helper_load_dcr(CPUPPCState *env,
target_ulong dcrn)
raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
POWERPC_EXCP_INVAL |
POWERPC_EXCP_INVAL_INVAL, GETPC());
-} else if (unlikely(ppc_dcr_read(env->dcr_env,
- (uint32_t)dcrn, ) != 0)) {
-qemu_log_mask(LOG_GUEST_ERROR, "DCR read error %d %03x\n",
- (uint32_t)dcrn, (uint32_t)dcrn);
-raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
- POWERPC_EXCP_INVAL |
- POWERPC_EXCP_PRIV_REG, GETPC());
+} else {
+int ret;
+
+qemu_mutex_lock_iothread();
+ret = ppc_dcr_read(env->dcr_env, (uint32_t)dcrn, );
+qemu_mutex_unlock_iothread();
+if (unlikely(ret != 0)) {
+qemu_log_mask(LOG_GUEST_ERROR, "DCR read error %d %03x\n",
+ (uint32_t)dcrn, (uint32_t)dcrn);
+raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
+ POWERPC_EXCP_INVAL |
+ POWERPC_EXCP_PRIV_REG, GETPC());
+}
}
return val;
}
@@ -185,12 +192,17 @@ void helper_store_dcr(CPUPPCState *env, target_ulong
dcrn, target_ulong val)
raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
POWERPC_EXCP_INVAL |
POWERPC_EXCP_INVAL_INVAL, GETPC());
-} else if (unlikely(ppc_dcr_write(env->dcr_env, (uint32_t)dcrn,
- (uint32_t)val) != 0)) {
-qemu_log_mask(LOG_GUEST_ERROR, "DCR write error %d %03x\n",
- (uint32_t)dcrn, (uint32_t)dcrn);
-raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
- POWERPC_EXCP_INVAL |
- POWERPC_EXCP_PRIV_REG, GETPC());
+} else {
+int ret;
+qemu_mutex_lock_iothread();
+ret = ppc_dcr_write(env->dcr_env, (uint32_t)dcrn, (uint32_t)val);
+qemu_mutex_unlock_iothread();
+if (unlikely(ret != 0)) {
+qemu_log_mask(LOG_GUEST_ERROR, "DCR write error %d %03x\n",
+ (uint32_t)dcrn, (uint32_t)dcrn);
+raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
+ POWERPC_EXCP_INVAL |
+ POWERPC_EXCP_PRIV_REG, GETPC());
+}
}
}
--
2.25.1
[PULL 1/7] ppc/spapr: Set the effective address provided flag in mc error log.
From: Mahesh Salgaonkar
Per PAPR, it is expected to set effective address provided flag in
sub_err_type member of mc extended error log (i.e
rtas_event_log_v6_mc.sub_err_type). This somehow got missed in original
fwnmi-mce patch series. The current code just updates the effective address
but does not set the flag to indicate that it is available. Hence guest
fails to extract effective address from mce rtas log. This patch fixes
that.
Without this patch guest MCE logs fails print DAR value:
[ 11.933608] Disabling lock debugging due to kernel taint
[ 11.933773] MCE: CPU0: machine check (Severe) Host TLB Multihit [Recovered]
[ 11.933979] MCE: CPU0: NIP: [c0090b34]
radix__flush_tlb_range_psize+0x194/0xf00
[ 11.934223] MCE: CPU0: Initiator CPU
[ 11.934341] MCE: CPU0: Unknown
After the change:
[ 22.454149] Disabling lock debugging due to kernel taint
[ 22.454316] MCE: CPU0: machine check (Severe) Host TLB Multihit DAR:
deadbeefdeadbeef [Recovered]
[ 22.454605] MCE: CPU0: NIP: [c03e5804] kmem_cache_alloc+0x84/0x330
[ 22.454820] MCE: CPU0: Initiator CPU
[ 22.454944] MCE: CPU0: Unknown
Signed-off-by: Mahesh Salgaonkar
Message-Id: <158451653844.22972.17999316676230071087.stgit@jupiter>
Reviewed-by: Nicholas Piggin
Signed-off-by: David Gibson
---
hw/ppc/spapr_events.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 323fcef4aa..a4a540f43d 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -243,6 +243,14 @@ struct rtas_event_log_v6_mc {
#define RTAS_LOG_V6_MC_TLB_PARITY1
#define RTAS_LOG_V6_MC_TLB_MULTIHIT 2
#define RTAS_LOG_V6_MC_TLB_INDETERMINATE 3
+/*
+ * Per PAPR,
+ * For UE error type, set bit 1 of sub_err_type to indicate effective addr is
+ * provided. For other error types (SLB/ERAT/TLB), set bit 0 to indicate
+ * same.
+ */
+#define RTAS_LOG_V6_MC_UE_EA_ADDR_PROVIDED 0x40
+#define RTAS_LOG_V6_MC_EA_ADDR_PROVIDED 0x80
uint8_t reserved_1[6];
uint64_t effective_address;
uint64_t logical_address;
@@ -726,6 +734,22 @@ void
spapr_hotplug_req_remove_by_count_indexed(SpaprDrcType drc_type,
RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, _id);
}
+static void spapr_mc_set_ea_provided_flag(struct mc_extended_log *ext_elog)
+{
+switch (ext_elog->mc.error_type) {
+case RTAS_LOG_V6_MC_TYPE_UE:
+ext_elog->mc.sub_err_type |= RTAS_LOG_V6_MC_UE_EA_ADDR_PROVIDED;
+break;
+case RTAS_LOG_V6_MC_TYPE_SLB:
+case RTAS_LOG_V6_MC_TYPE_ERAT:
+case RTAS_LOG_V6_MC_TYPE_TLB:
+ext_elog->mc.sub_err_type |= RTAS_LOG_V6_MC_EA_ADDR_PROVIDED;
+break;
+default:
+break;
+}
+}
+
static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered,
struct mc_extended_log *ext_elog)
{
@@ -751,6 +775,7 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu,
bool recovered,
ext_elog->mc.sub_err_type = mc_derror_table[i].error_subtype;
if (mc_derror_table[i].dar_valid) {
ext_elog->mc.effective_address =
cpu_to_be64(env->spr[SPR_DAR]);
+spapr_mc_set_ea_provided_flag(ext_elog);
}
summary |= mc_derror_table[i].initiator
@@ -769,6 +794,7 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu,
bool recovered,
ext_elog->mc.sub_err_type = mc_ierror_table[i].error_subtype;
if (mc_ierror_table[i].nip_valid) {
ext_elog->mc.effective_address = cpu_to_be64(env->nip);
+spapr_mc_set_ea_provided_flag(ext_elog);
}
summary |= mc_ierror_table[i].initiator
--
2.25.1
[PATCH v4] target/i386: Add notes for versioned CPU models
Add which features are added or removed in this version.
Signed-off-by: Tao Xu
---
The output is as follows:
qemu-system-x86_64 -cpu help | grep "\["
x86 Cascadelake-Server-v2 Intel Xeon Processor (Cascadelake)
[ARCH_CAPABILITIES]
x86 Cascadelake-Server-v3 Intel Xeon Processor (Cascadelake)
[ARCH_CAPABILITIES, no TSX]
x86 Denverton-v2 Intel Atom Processor (Denverton) [no MPX, no MONITOR]
x86 Icelake-Client-v2 Intel Core Processor (Icelake) [no TSX]
x86 Icelake-Server-v2 Intel Xeon Processor (Icelake) [no TSX]
Changes in v3:
- Keep the existing custom model-id (Eduardo)
Changes in v2:
- correct the note of Cascadelake v3 (Xiaoyao)
---
target/i386/cpu.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 34b511f078..1c7690baa0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3192,6 +3192,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
.versions = (X86CPUVersionDefinition[]) {
{ .version = 1 },
{ .version = 2,
+ .note = "ARCH_CAPABILITIES",
.props = (PropValue[]) {
{ "arch-capabilities", "on" },
{ "rdctl-no", "on" },
@@ -3203,6 +3204,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
},
{ .version = 3,
.alias = "Cascadelake-Server-noTSX",
+ .note = "ARCH_CAPABILITIES, no TSX",
.props = (PropValue[]) {
{ "hle", "off" },
{ "rtm", "off" },
@@ -3424,6 +3426,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
{ .version = 1 },
{
.version = 2,
+.note = "no TSX",
.alias = "Icelake-Client-noTSX",
.props = (PropValue[]) {
{ "hle", "off" },
@@ -3541,6 +3544,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
{ .version = 1 },
{
.version = 2,
+.note = "no TSX",
.alias = "Icelake-Server-noTSX",
.props = (PropValue[]) {
{ "hle", "off" },
@@ -3648,6 +3652,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
{ .version = 1 },
{
.version = 2,
+.note = "no MPX, no MONITOR",
.props = (PropValue[]) {
{ "monitor", "off" },
{ "mpx", "off" },
--
2.20.1
Re: [PULL 11/26] vfio/spapr: Allow backing bigger guest IOMMU pages with smaller physical pages
On Tue, Mar 24, 2020 at 03:08:22PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 23/03/2020 21:55, Peter Maydell wrote:
> > On Tue, 21 Aug 2018 at 05:33, David Gibson
> > wrote:
> >>
> >> From: Alexey Kardashevskiy
> >>
> >> At the moment the PPC64/pseries guest only supports 4K/64K/16M IOMMU
> >> pages and POWER8 CPU supports the exact same set of page size so
> >> so far things worked fine.
> >>
> >> However POWER9 supports different set of sizes - 4K/64K/2M/1G and
> >> the last two - 2M and 1G - are not even allowed in the paravirt interface
> >> (RTAS DDW) so we always end up using 64K IOMMU pages, although we could
> >> back guest's 16MB IOMMU pages with 2MB pages on the host.
> >>
> >> This stores the supported host IOMMU page sizes in VFIOContainer and uses
> >> this later when creating a new DMA window. This uses the system page size
> >> (64k normally, 2M/16M/1G if hugepages used) as the upper limit of
> >> the IOMMU pagesize.
> >>
> >> This changes the type of @pagesize to uint64_t as this is what
> >> memory_region_iommu_get_min_page_size() returns and clz64() takes.
> >>
> >> There should be no behavioral changes on platforms other than pseries.
> >> The guest will keep using the IOMMU page size selected by the PHB pagesize
> >> property as this only changes the underlying hardware TCE table
> >> granularity.
> >
> > Hi; Coverity has raised an issue (CID 1421903) on this code and
> > I'm not sure if it's correct or not.
> >
> >
> >> @@ -144,9 +145,27 @@ int vfio_spapr_create_window(VFIOContainer *container,
> >> {
> >> int ret;
> >> IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> >> -unsigned pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
> >> +uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
> >> unsigned entries, pages;
> >> struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create)
> >> };
> >> +long systempagesize = qemu_getrampagesize();
> >> +
> >> +/*
> >> + * The host might not support the guest supported IOMMU page size,
> >> + * so we will use smaller physical IOMMU pages to back them.
> >> + */
> >> +if (pagesize > systempagesize) {
> >> +pagesize = systempagesize;
> >> +}
>
> pagesize cannot be zero here (I checked possible code paths)...
>
>
>
> >> +pagesize = 1ULL << (63 - clz64(container->pgsizes &
> >> + (pagesize | (pagesize - 1;
> >> If the argument to clz64() is zero then it will return 64, and
> > then we will try to do a shift by -1, which is undefined
> > behaviour.
>
> ... but the clz64() argument can if lets say container->pgsizes=1<<30
> (comes from VFIO) and pagesize=1<<16 (decided by QEMU or guest).
>
>
> I'll sent a patch to handle clz64()=>64. Thanks,
Thanks, Alexey.
Peter, I don't think this is urgent however - it's really unlikely in
practice.
>
>
> > Can the expression ever be zero? It's not immediately obvious to me
> > that it can't be, so my suggestion would be that if it is
> > impossible then an assert() of that would be helpful, and if it
> > is possible then the code needs to avoid the illegal shift.
>
> >> +if (!pagesize) {
> >> +error_report("Host doesn't support page size 0x%"PRIx64
> >> + ", the supported mask is 0x%lx",
> >> + memory_region_iommu_get_min_page_size(iommu_mr),
> >> + container->pgsizes);
> >> +return -EINVAL;
> >> +}
> >
> > thanks
> > -- PMM
> >
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[Bug 1592336] Re: mouse is defunct when grabbed
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1592336 Title: mouse is defunct when grabbed Status in QEMU: Expired Bug description: I run qemu as follows: qemu-system-x86_64 -machine accel=kvm -k en-us -smp 4 -m 2371 -usb \ -device virtio-rng-pci \ -drive file=/home/new/suse-fact.img,format=raw,discard=unmap,if=none,id=hd -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=hd \ -soundhw hda \ -net user,tftp=/home/xslaby/tftp,bootfile=/pxelinux.0,hostfwd=tcp::-:22,hostfwd=tcp::3632-:3632 -net nic,model=virtio \ -serial pty -balloon virtio -vga virtio -display gtk,gl=on When I run X server with icewm inside the machine, the cursor works until I grab the mous with ctrl-alt-g. Then the cursor dismissed and the mouse is defunct. I also tried -usbdevice mouse and -usbdevice tablet with the same result. I have these versions of qemu packages: qemu-2.6.0-470.2.x86_64 qemu-ipxe-1.0.0-470.2.noarch qemu-ksm-2.6.0-470.2.x86_64 qemu-kvm-2.6.0-470.2.x86_64 qemu-ovmf-x86_64-2015+git1462940744.321151f-2.1.noarch qemu-ppc-2.6.0-470.2.x86_64 qemu-seabios-1.9.1-470.2.noarch qemu-sgabios-8-470.2.noarch qemu-tools-2.6.0-470.2.x86_64 qemu-vgabios-1.9.1-470.2.noarch qemu-x86-2.6.0-470.2.x86_64 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1592336/+subscriptions
[Bug 1661176] Re: [2.8.0] Under VNC CTRL-ALT-2 doesn't get to the monitor
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1661176 Title: [2.8.0] Under VNC CTRL-ALT-2 doesn't get to the monitor Status in QEMU: Expired Bug description: With version 2.6.2 I could access the monitor via VNC by pressing CTRL-ALT-2, while CTRL-ALT-3 brought me to the "serial0 console". CTRL-ALT-1 brought me back to the VGA console. Since 2.8.0 CTRL-ALT-2 brings me to the "serial0 console" and CTRL-ALT-3 to the "parallel0 console". The monitor is not available any more, to any CTRL-ALT-1stROW. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1661176/+subscriptions
[Bug 1660010] Re: AArch64 system emulation cannot execute virt uefi in 2.7 or 2.8
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1660010 Title: AArch64 system emulation cannot execute virt uefi in 2.7 or 2.8 Status in QEMU: Expired Bug description: The UEFI firmware file is retrieved from http://snapshots.linaro.org/components/kernel/linaro- edk2/latest/release/qemu64/QEMU_EFI.fd . The error is: ``` TODO /var/lib/abbs/build/tmp.p2dMBBlJ0D/qemu-2.7.0/tci.c:1049: tcg_qemu_tb_exec() /var/lib/abbs/build/tmp.p2dMBBlJ0D/qemu-2.7.0/tci.c:1049: tcg fatal error ``` (both 2.7.0 and 2.8.0 are tested to fail, 2.6.1 works) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1660010/+subscriptions
[Bug 1639394] Re: Unable to boot Solaris 8/9 x86 under Fedora 24
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1639394 Title: Unable to boot Solaris 8/9 x86 under Fedora 24 Status in QEMU: Expired Bug description: qemu-system-x86_64 -version QEMU emulator version 2.6.2 (qemu-2.6.2-4.fc24), Copyright (c) 2003-2008 Fabrice Bellard Try several ways without success, I think it was a regression because problem seems to be related with ide fixed on 0.6.0: - int13 CDROM BIOS fix (aka Solaris x86 install CD fix) - int15, ah=86 BIOS fix (aka Solaris x86 hardware probe hang up fix) Solaris 10/11 works without a problem, also booting with "scsi" will circumvent initial problem, but later found problems related with "scsi" cdrom boot and also will not found the "ide" disk device. qemu-system-i386 -m 712 -drive file=/dev/Virtual_hdd/beryllium0,format=raw -cdrom /repo/Isos/sol-9_905_x86.iso SunOS Secondary Boot version 3.00 prom_panic: Could not mount filesystem. Entering boot debugger: [136419] Regards, \\CA, To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1639394/+subscriptions
[Bug 1659901] Re: Regression: SIGSEGV running Java
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1659901 Title: Regression: SIGSEGV running Java Status in QEMU: Expired Bug description: I have a build script that bootstraps a Debian armhf image. Part of the process involves running a Java program while inside a chroot. I am using Debian's qemu-user-static package to run the armhf Java binary on an amd64 system. qemu-user-static version 1:2.7+dfsg-3~bpo8+2 works fine. Version 1:2.8+dfsg-1~bpo8+1 always causes Java to crash with a SIGSEGV. The location of the crash appears to be random and hasn't been the same twice. I am using the Azul Systems Zulu Embedded Java runtime, rather than the regular OpenJDK runtime, because the Zulu runtime has an arm32 JIT whereas OpenJDK is interpreter-only on arm32. I can reproduce the problem easily by mounting the image created by my build script and executing "java -XshowSettings -version" in a chroot. I can give you the image if that would help debug the problem. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1659901/+subscriptions
Re: [PULL 11/26] vfio/spapr: Allow backing bigger guest IOMMU pages with smaller physical pages
On 23/03/2020 21:55, Peter Maydell wrote:
> On Tue, 21 Aug 2018 at 05:33, David Gibson
> wrote:
>>
>> From: Alexey Kardashevskiy
>>
>> At the moment the PPC64/pseries guest only supports 4K/64K/16M IOMMU
>> pages and POWER8 CPU supports the exact same set of page size so
>> so far things worked fine.
>>
>> However POWER9 supports different set of sizes - 4K/64K/2M/1G and
>> the last two - 2M and 1G - are not even allowed in the paravirt interface
>> (RTAS DDW) so we always end up using 64K IOMMU pages, although we could
>> back guest's 16MB IOMMU pages with 2MB pages on the host.
>>
>> This stores the supported host IOMMU page sizes in VFIOContainer and uses
>> this later when creating a new DMA window. This uses the system page size
>> (64k normally, 2M/16M/1G if hugepages used) as the upper limit of
>> the IOMMU pagesize.
>>
>> This changes the type of @pagesize to uint64_t as this is what
>> memory_region_iommu_get_min_page_size() returns and clz64() takes.
>>
>> There should be no behavioral changes on platforms other than pseries.
>> The guest will keep using the IOMMU page size selected by the PHB pagesize
>> property as this only changes the underlying hardware TCE table
>> granularity.
>
> Hi; Coverity has raised an issue (CID 1421903) on this code and
> I'm not sure if it's correct or not.
>
>
>> @@ -144,9 +145,27 @@ int vfio_spapr_create_window(VFIOContainer *container,
>> {
>> int ret;
>> IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>> -unsigned pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
>> +uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
>> unsigned entries, pages;
>> struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
>> +long systempagesize = qemu_getrampagesize();
>> +
>> +/*
>> + * The host might not support the guest supported IOMMU page size,
>> + * so we will use smaller physical IOMMU pages to back them.
>> + */
>> +if (pagesize > systempagesize) {
>> +pagesize = systempagesize;
>> +}
pagesize cannot be zero here (I checked possible code paths)...
>> +pagesize = 1ULL << (63 - clz64(container->pgsizes &
>> + (pagesize | (pagesize - 1;
>> If the argument to clz64() is zero then it will return 64, and
> then we will try to do a shift by -1, which is undefined
> behaviour.
... but the clz64() argument can if lets say container->pgsizes=1<<30
(comes from VFIO) and pagesize=1<<16 (decided by QEMU or guest).
I'll sent a patch to handle clz64()=>64. Thanks,
> Can the expression ever be zero? It's not immediately obvious to me
> that it can't be, so my suggestion would be that if it is
> impossible then an assert() of that would be helpful, and if it
> is possible then the code needs to avoid the illegal shift.
>> +if (!pagesize) {
>> +error_report("Host doesn't support page size 0x%"PRIx64
>> + ", the supported mask is 0x%lx",
>> + memory_region_iommu_get_min_page_size(iommu_mr),
>> + container->pgsizes);
>> +return -EINVAL;
>> +}
>
> thanks
> -- PMM
>
--
Alexey
Re: [PATCH v4 0/2] introduction of migration_version attribute for VFIO live migration
On Tue, Mar 24, 2020 at 05:29:59AM +0800, Alex Williamson wrote: > On Mon, 3 Jun 2019 20:34:22 -0400 > Yan Zhao wrote: > > > On Tue, Jun 04, 2019 at 03:29:32AM +0800, Alex Williamson wrote: > > > On Thu, 30 May 2019 20:44:38 -0400 > > > Yan Zhao wrote: > > > > > > > This patchset introduces a migration_version attribute under sysfs of > > > > VFIO > > > > Mediated devices. > > > > > > > > This migration_version attribute is used to check migration > > > > compatibility > > > > between two mdev devices of the same mdev type. > > > > > > > > Patch 1 defines migration_version attribute in > > > > Documentation/vfio-mediated-device.txt > > > > > > > > Patch 2 uses GVT as an example to show how to expose migration_version > > > > attribute and check migration compatibility in vendor driver. > > > > > > Thanks for iterating through this, it looks like we've settled on > > > something reasonable, but now what? This is one piece of the puzzle to > > > supporting mdev migration, but I don't think it makes sense to commit > > > this upstream on its own without also defining the remainder of how we > > > actually do migration, preferably with more than one working > > > implementation and at least prototyped, if not final, QEMU support. I > > > hope that was the intent, and maybe it's now time to look at the next > > > piece of the puzzle. Thanks, > > > > > > Alex > > > > Got it. > > Also thank you and all for discussing and guiding all along:) > > We'll move to the next episode now. > > Hi Yan, > > As we're hopefully moving towards a migration API, would it make sense > to refresh this series at the same time? I think we're still expecting > a vendor driver implementing Kirti's migration API to also implement > this sysfs interface for compatibility verification. Thanks, > Hi Alex Got it! Thanks for reminding of this. And as now we have vfio-pci implementing vendor ops to allow live migration of pass-through devices, is it necessary to implement similar sysfs node for those devices? or do you think just PCI IDs of those devices are enough for libvirt to know device compatibility ? Thanks Yan
Re: [PATCH 1/1] spapr/rtas: Add MinMem to ibm,get-system-parameter RTAS call
On Mon, Mar 23, 2020 at 07:07:21PM -0300, Leonardo Bras wrote: > On Mon, 2020-03-23 at 14:24 +1100, David Gibson wrote: > > On Fri, Mar 20, 2020 at 09:39:22PM -0300, Leonardo Bras wrote: > > > Add support for MinMem SPLPAR Characteristic on emulated > > > RTAS call ibm,get-system-parameter. > > > > > > MinMem represents Minimum Memory, that is described in LOPAPR as: > > > The minimum amount of main store that is needed to power on the > > > partition. Minimum memory is expressed in MB of storage. > > > > Where exactly does LoPAPR say that? The version I'm looking at > > doesn't describe MinMem all that clearly, other than to say it must be > > <= DesMem, which currently has the same value here. > > Please look for "Minimum Memory". It's on Table 237. SPLPAR Terms. Ah, thanks. Hm, yes. In the way we manage VMs with KVM and qemu, I don't think we cal really draw any meaningful distinction between MinMem and DesMem, so it's reasonble for them to have the same value. > > > This provides a way for the OS to discern hotplugged LMBs and > > > LMBs that have started with the VM, allowing it to better provide > > > a way for memory hot-removal. > > > > This seems a bit dubious. Surely we should have this information from > > the dynamic-reconfiguration-memory stuff already? Trying to discern > > this from purely a number seems very fragile - wouldn't that mean > > making assumptions about how qemu / the host is laying out it's fixed > > and dynamic memory which might not be justified? > > I agree. > I previously sent a RFC proposing the usage of a new flag for this same > reason [1], which I thank you for positive feedback. > > Even though I think using a flag is a better solution, I am also > working in this other option (MinMem), that would use parameter already > available on the platform, in case the new flag don't get approved. > > So far, using MinMem looks like a very complex solution on kernel side, > and I think it's a poor solution. > > I decided to send this patch because it's a simple change to the > platform support, that should cause no harm and could even be useful to > other OSes. Hm, ok. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH v15 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
On Tue, Mar 24, 2020 at 02:51:14AM +0800, Dr. David Alan Gilbert wrote:
> * Alex Williamson (alex.william...@redhat.com) wrote:
> > On Mon, 23 Mar 2020 23:24:37 +0530
> > Kirti Wankhede wrote:
> >
> > > On 3/21/2020 12:29 AM, Alex Williamson wrote:
> > > > On Sat, 21 Mar 2020 00:12:04 +0530
> > > > Kirti Wankhede wrote:
> > > >
> > > >> On 3/20/2020 11:31 PM, Alex Williamson wrote:
> > > >>> On Fri, 20 Mar 2020 23:19:14 +0530
> > > >>> Kirti Wankhede wrote:
> > > >>>
> > > On 3/20/2020 4:27 AM, Alex Williamson wrote:
> > > > On Fri, 20 Mar 2020 01:46:41 +0530
> > > > Kirti Wankhede wrote:
> > > >
> > > >>
> > > >>
> > > >>
> > > >> +static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu,
> > > >> dma_addr_t iova,
> > > >> +size_t size, uint64_t pgsize,
> > > >> +u64 __user *bitmap)
> > > >> +{
> > > >> + struct vfio_dma *dma;
> > > >> + unsigned long pgshift = __ffs(pgsize);
> > > >> + unsigned int npages, bitmap_size;
> > > >> +
> > > >> + dma = vfio_find_dma(iommu, iova, 1);
> > > >> +
> > > >> + if (!dma)
> > > >> + return -EINVAL;
> > > >> +
> > > >> + if (dma->iova != iova || dma->size != size)
> > > >> + return -EINVAL;
> > > >> +
> > > >> + npages = dma->size >> pgshift;
> > > >> + bitmap_size = DIRTY_BITMAP_BYTES(npages);
> > > >> +
> > > >> + /* mark all pages dirty if all pages are pinned and mapped. */
> > > >> + if (dma->iommu_mapped)
> > > >> + bitmap_set(dma->bitmap, 0, npages);
> > > >> +
> > > >> + if (copy_to_user((void __user *)bitmap, dma->bitmap,
> > > >> bitmap_size))
> > > >> + return -EFAULT;
> > > >
> > > > We still need to reset the bitmap here, clearing and re-adding the
> > > > pages that are still pinned.
> > > >
> > > > https://lore.kernel.org/kvm/20200319070635.2ff5d...@x1.home/
> > > >
> > >
> > > I thought you agreed on my reply to it
> > > https://lore.kernel.org/kvm/31621b70-02a9-2ea5-045f-f72b671fe...@nvidia.com/
> > >
> > > > Why re-populate when there will be no change since
> > > > vfio_iova_dirty_bitmap() is called holding iommu->lock? If
> > > there is any
> > > > pin request while vfio_iova_dirty_bitmap() is still working, it
> > > will
> > > > wait till iommu->lock is released. Bitmap will be populated
> > > when page is
> > > > pinned.
> > > >>>
> > > >>> As coded, dirty bits are only ever set in the bitmap, never cleared.
> > > >>> If a page is unpinned between iterations of the user recording the
> > > >>> dirty bitmap, it should be marked dirty in the iteration immediately
> > > >>> after the unpinning and not marked dirty in the following iteration.
> > > >>> That doesn't happen here. We're reporting cumulative dirty pages
> > > >>> since
> > > >>> logging was enabled, we need to be reporting dirty pages since the
> > > >>> user
> > > >>> last retrieved the dirty bitmap. The bitmap should be cleared and
> > > >>> currently pinned pages re-added after copying to the user. Thanks,
> > > >>>
> > > >>
> > > >> Does that mean, we have to track every iteration? do we really need
> > > >> that
> > > >> tracking?
> > > >>
> > > >> Generally the flow is:
> > > >> - vendor driver pin x pages
> > > >> - Enter pre-copy-phase where vCPUs are running - user starts dirty
> > > >> pages
> > > >> tracking, then user asks dirty bitmap, x pages reported dirty by
> > > >> VFIO_IOMMU_DIRTY_PAGES ioctl with _GET flag
> > > >> - In pre-copy phase, vendor driver pins y more pages, now bitmap
> > > >> consists of x+y bits set
> > > >> - In pre-copy phase, vendor driver unpins z pages, but bitmap is not
> > > >> updated, so again bitmap consists of x+y bits set.
> > > >> - Enter in stop-and-copy phase, vCPUs are stopped, mdev devices are
> > > >> stopped
> > > >> - user asks dirty bitmap - Since here vCPU and mdev devices are
> > > >> stopped,
> > > >> pages should not get dirty by guest driver or the physical device.
> > > >> Hence, x+y dirty pages would be reported.
> > > >>
> > > >> I don't think we need to track every iteration of bitmap reporting.
> > > >
> > > > Yes, once a bitmap is read, it's reset. In your example, after
> > > > unpinning z pages the user should still see a bitmap with x+y pages,
> > > > but once they've read that bitmap, the next bitmap should be x+y-z.
> > > > Userspace can make decisions about when to switch from pre-copy to
> > > > stop-and-copy based on convergence, ie. the slope of the line recording
> > > > dirty pages per iteration. The implementation here never allows an
> > > > inflection point, dirty pages reported through vfio would always either
> > > > be flat or climbing. There might also be a case that an iommu backed
> > > > device could start
Potential Null dereference
Hi,
Nullness of needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...
While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477
if (bs->backing == NULL) { return}
Thanks,
Mansour
Re: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue()
Zhang, Chen 於 2020年3月24日 週二 上午3:24寫道：
>
>
>
> > -Original Message-
> > From: Derek Su
> > Sent: Monday, March 23, 2020 1:48 AM
> > To: qemu-devel@nongnu.org
> > Cc: Zhang, Chen ; lizhij...@cn.fujitsu.com;
> > jasow...@redhat.com; dere...@qnap.com
> > Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in
> > packet_enqueue()
> >
> > The patch is to fix the "pkt" memory leak in packet_enqueue().
> > The allocated "pkt" needs to be freed if the colo compare primary or
> > secondary queue is too big.
>
> Hi Derek,
>
> Thank you for the patch.
> I re-think this issue in a big view, looks just free the pkg is not enough in
> this situation.
> The root cause is network is too busy to compare, So, better choice is notify
> COLO frame
> to do a checkpoint and clean up all the network queue. This work maybe
> decrease
> COLO network performance but seams better than drop lots of pkg.
>
> Thanks
> Zhang Chen
>
Hello, Zhang
Got it.
What is the concern of the massive "drop packets"?
Does the behavior make the COLO do checkpoint periodically (~20 seconds)
instead of doing immediate checkpoint when encountering different
response packets?
It seems that frequent checkpoints caused by the full queue (busy
network) instead of different
response packets may harm the high speed network (10 Gbps or higher)
performance dramatically.
Thanks
Derek
> >
> > Signed-off-by: Derek Su
> > ---
> > net/colo-compare.c | 23 +++
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 7ee17f2cf8..cdd87b2aa8 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -120,6 +120,10 @@ enum {
> > SECONDARY_IN,
> > };
> >
> > +static const char *colo_mode[] = {
> > +[PRIMARY_IN] = "primary",
> > +[SECONDARY_IN] = "secondary",
> > +};
> >
> > static int compare_chr_send(CompareState *s,
> > const uint8_t *buf, @@ -215,6 +219,7 @@ static
> > int
> > packet_enqueue(CompareState *s, int mode, Connection **con)
> > ConnectionKey key;
> > Packet *pkt = NULL;
> > Connection *conn;
> > +int ret;
> >
> > if (mode == PRIMARY_IN) {
> > pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ static int
> > packet_enqueue(CompareState *s, int mode, Connection **con)
> > }
> >
> > if (mode == PRIMARY_IN) {
> > -if (!colo_insert_packet(>primary_list, pkt, >pack)) {
> > -error_report("colo compare primary queue size too big,"
> > - "drop packet");
> > -}
> > +ret = colo_insert_packet(>primary_list, pkt,
> > + >pack);
> > } else {
> > -if (!colo_insert_packet(>secondary_list, pkt, >sack)) {
> > -error_report("colo compare secondary queue size too big,"
> > - "drop packet");
> > -}
> > +ret = colo_insert_packet(>secondary_list, pkt,
> > + >sack);
> > }
> > +
> > +if (!ret) {
> > +error_report("colo compare %s queue size too big,"
> > + "drop packet", colo_mode[mode]);
> > +packet_destroy(pkt, NULL);
> > +pkt = NULL;
> > +}
> > +
> > *con = conn;
> >
> > return 0;
> > --
> > 2.17.1
>
Re: [PATCH] ext4: Give 32bit personalities 32bit hashes
On Thu, Mar 19, 2020 at 11:23:33PM +0100, Linus Walleij wrote: > OK I guess we can at least take this opportunity to add > some kerneldoc to the include file. > > > As a concrete example, should "give me 32-bit semantics > > via PER_LINUX32" mean "mmap should always return addresses > > within 4GB" ? That would seem like it would make sense -- > > Incidentally that thing in particular has its own personality > flag (personalities are additive, it's a bit schizophrenic) > so PER_LINUX_32BIT is defined as: > PER_LINUX_32BIT = 0x | ADDR_LIMIT_32BIT, > and that is specifically for limiting the address space to > 32bit. > > There is also PER_LINUX32_3GB for a 3GB lowmem > limit. > > Since the personality is kind of additive, if > we want a flag *specifically* for indicating that we want > 32bit hashes from the file system, there are bits left so we > can provide that. > > Is this what we want to do? I just think we shouldn't > decide on that lightly as we will be using up personality > bug bits, but sometimes you have to use them. I've been looking at the personality bug bits more detailed, and it looks... messy. Do we pick a new personality, or do we grab another unique flag? Another possibility, which would be messier for qemu, would be use a flag set via fcntl. That would require qemu from noticing when the guest is calling open, openat, or openat2, and then inserting a fcntl system call to set the 32-bit readdir mode. That's cleaner from the kernel interface complexity perspective, but it's messier for qemu. - Ted
Re: [PATCH v3 0/3] hw/riscv: Add a serial property to sifive_u
Hi Palmer, On Sat, Mar 7, 2020 at 5:45 AM Alistair Francis wrote: > > At present the board serial number is hard-coded to 1, and passed > to OTP model during initialization. Firmware (FSBL, U-Boot) uses > the serial number to generate a unique MAC address for the on-chip > ethernet controller. When multiple QEMU 'sifive_u' instances are > created and connected to the same subnet, they all have the same > MAC address hence it creates a unusable network. > > A new "serial" property is introduced to specify the board serial > number. When not given, the default serial number 1 is used. > Could you please take this for v5.0.0? Regards, Bin
Re: [PATCH v6 2/2] net: tulip: add .can_receive routine
P J P 于2020年3月23日周一 下午8:25写道：
> From: Prasad J Pandit
>
> Define .can_receive routine to do sanity checks before receiving
> packet data. And call qemu_flush_queued_packets to flush queued
> packets once they are read in tulip_receive().
>
> Suggested-by: Jason Wang
> Signed-off-by: Prasad J Pandit
> ---
> hw/net/tulip.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> Update v6: merge earlier patch 2 & 3 into one
> -> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06868.html
>
> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> index fbe40095da..8d8c9519e7 100644
> --- a/hw/net/tulip.c
> +++ b/hw/net/tulip.c
> @@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState *s, const
> uint8_t *addr)
> return ret;
> }
>
> +static int
> +tulip_can_receive(NetClientState *nc)
> +{
> +TULIPState *s = qemu_get_nic_opaque(nc);
> +
> +if (s->rx_frame_len || tulip_rx_stopped(s)) {
> +return false;
> +}
> +
> +return true;
> +}
> +
> static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t
> size)
> {
> struct tulip_descriptor desc;
> @@ -236,7 +248,7 @@ static ssize_t tulip_receive(TULIPState *s, const
> uint8_t *buf, size_t size)
> trace_tulip_receive(buf, size);
>
> if (size < 14 || size > sizeof(s->rx_frame) - 4
> -|| s->rx_frame_len || tulip_rx_stopped(s)) {
> +|| !tulip_can_receive(s->nic->ncs)) {
> return 0;
> }
>
> @@ -275,6 +287,8 @@ static ssize_t tulip_receive(TULIPState *s, const
> uint8_t *buf, size_t size)
> tulip_desc_write(s, s->current_rx_desc, );
> tulip_next_rx_descriptor(s, );
> } while (s->rx_frame_len);
> +
> +qemu_flush_queued_packets(qemu_get_queue(s->nic));
>
Hi Prasad ans Jason,
I want to know why here we need call 'qemu_flush_queued_packets'.
AFAICS, the other networking card emulation need no this.
Thanks,
Li Qiang
> return size;
> }
>
> @@ -288,6 +302,7 @@ static NetClientInfo net_tulip_info = {
> .type = NET_CLIENT_DRIVER_NIC,
> .size = sizeof(NICState),
> .receive = tulip_receive_nc,
> +.can_receive = tulip_can_receive,
> };
>
> static const char *tulip_reg_name(const hwaddr addr)
> --
> 2.25.1
>
>
>
[Bug 1868527] Re: alignment may overlap the TLB flags
This is an inspection yet. For ARM SMMU simulation, TARGET_PAGE_BITS_MIN is 10. All low bits of the TLB virtual address are used up by TLB flags and alignment flags. It's a little crowded. /* * ARMv7 and later CPUs have 4K pages minimum, but ARMv5 and v6 * have to support 1K tiny pages. */ # define TARGET_PAGE_BITS_VARY # define TARGET_PAGE_BITS_MIN 10 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1868527 Title: alignment may overlap the TLB flags Status in QEMU: Incomplete Bug description: Hi, In QEMU-4.2.0, or git-9b26a610936deaf436af9b7e39e4b7f0a35e4409, alignment may overlap the TLB flags. For example, the alignment: MO_ALIGN_32, MO_ALIGN_32 = 5 << MO_ASHIFT, and the TLB flag: TLB_DISCARD_WRITE #define TLB_DISCARD_WRITE (1 << (TARGET_PAGE_BITS_MIN - 6)) then, in the function "get_alignment_bits", the assert may fail: #if defined(CONFIG_SOFTMMU) /* The requested alignment cannot overlap the TLB flags. */ tcg_debug_assert((TLB_FLAGS_MASK & ((1 << a) - 1)) == 0); #endif However, the alignment of MO_ALIGN_32 is not used for now, so the assert cannot be triggered in current version. Anyway it seems like a potential conflict. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1868527/+subscriptions
[Bug 1866870] Re: KVM Guest pauses after upgrade to Ubuntu 20.04
I can confirm that this bug has been fixed (zapped). Thank you all for
your hard work and determination. A job well done indeed! As a former
programmer I love you all's zeal for attacking this bug.
As a side note, knowing what you all go through, I always look things
up, walk through at least level 1 stuff and provide logfiles. I hope
what little I did, help you all resolve this.
Again my thanks, and I believe is this is where we say, "Please close
the bug marked SOLVED".
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1866870
Title:
KVM Guest pauses after upgrade to Ubuntu 20.04
Status in QEMU:
Invalid
Status in qemu package in Ubuntu:
Invalid
Status in seabios package in Ubuntu:
Fix Released
Bug description:
Symptom:
Error unpausing domain: internal error: unable to execute QEMU command
'cont': Resetting the Virtual Machine is required
Traceback (most recent call last):
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in
cb_wrapper
callback(asyncjob, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 111, in tmpcb
callback(*args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/libvirtobject.py", line
66, in newfn
ret = fn(self, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/domain.py", line 1311, in
resume
self._backend.resume()
File "/usr/lib/python3/dist-packages/libvirt.py", line 2174, in resume
if ret == -1: raise libvirtError ('virDomainResume() failed', dom=self)
libvirt.libvirtError: internal error: unable to execute QEMU command 'cont':
Resetting the Virtual Machine is required
---
As outlined here:
https://bugs.launchpad.net/qemu/+bug/1813165/comments/15
After upgrade, all KVM guests are in a default pause state. Even after
forcing them off via virsh, and restarting them the guests are paused.
These Guests are not nested.
A lot of diganostic information are outlined in the previous bug
report link provided. The solution mentioned in previous report had
been allegedly integrated into the downstream updates.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1866870/+subscriptions
Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
P J P 于2020年3月23日周一 下午8:24写道：
> From: Prasad J Pandit
>
> Tulip network driver while copying tx/rx buffers does not check
> frame size against r/w data length. This may lead to OOB buffer
> access. Add check to avoid it.
>
> Limit iterations over descriptors to avoid potential infinite
> loop issue in tulip_xmit_list_update.
>
> Reported-by: Li Qiang
> Reported-by: Ziming Zhang
> Reported-by: Jason Wang
> Signed-off-by: Prasad J Pandit
>
Tested-by: Li Qiang
But I have a minor question
> ---
> hw/net/tulip.c | 36 +++-
> 1 file changed, 27 insertions(+), 9 deletions(-)
>
> Update v3: return a value from tulip_copy_tx_buffers() and avoid infinite
> loop
> -> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
>
> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> index cfac2719d3..fbe40095da 100644
> --- a/hw/net/tulip.c
> +++ b/hw/net/tulip.c
> @@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
> tulip_descriptor *desc)
> } else {
> len = s->rx_frame_len;
> }
> +
> +if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> +return;
> +}
>
Why here is '>=' instead of '>'.
IIUC the total sending length can reach to sizeof(s->rx_frame).
Same in the other place in this patch.
PS: I have planned to write a qtest case. But my personal qemu dev
environment is broken.
I will try to write it tonight or tomorrow night.
Thanks,
Li Qiang
> pci_dma_write(>dev, desc->buf_addr1, s->rx_frame +
> (s->rx_frame_size - s->rx_frame_len), len);
> s->rx_frame_len -= len;
> @@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
> tulip_descriptor *desc)
> } else {
> len = s->rx_frame_len;
> }
> +
> +if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> +return;
> +}
> pci_dma_write(>dev, desc->buf_addr2, s->rx_frame +
> (s->rx_frame_size - s->rx_frame_len), len);
> s->rx_frame_len -= len;
> @@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s, const
> uint8_t *buf, size_t size)
>
> trace_tulip_receive(buf, size);
>
> -if (size < 14 || size > 2048 || s->rx_frame_len ||
> tulip_rx_stopped(s)) {
> +if (size < 14 || size > sizeof(s->rx_frame) - 4
> +|| s->rx_frame_len || tulip_rx_stopped(s)) {
> return 0;
> }
>
> @@ -275,7 +284,6 @@ static ssize_t tulip_receive_nc(NetClientState *nc,
> return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
> }
>
> -
> static NetClientInfo net_tulip_info = {
> .type = NET_CLIENT_DRIVER_NIC,
> .size = sizeof(NICState),
> @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct
> tulip_descriptor *desc)
> if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
> /* Internal or external Loopback */
> tulip_receive(s, s->tx_frame, s->tx_frame_len);
> -} else {
> +} else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
> qemu_send_packet(qemu_get_queue(s->nic),
> s->tx_frame, s->tx_frame_len);
> }
> @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct
> tulip_descriptor *desc)
> }
> }
>
> -static void tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor
> *desc)
> +static int tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor
> *desc)
> {
> int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
> TDES1_BUF1_SIZE_MASK;
> int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
> TDES1_BUF2_SIZE_MASK;
>
> +if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> +return -1;
> +}
> if (len1) {
> pci_dma_read(>dev, desc->buf_addr1,
> s->tx_frame + s->tx_frame_len, len1);
> s->tx_frame_len += len1;
> }
>
> +if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> +return -1;
> +}
> if (len2) {
> pci_dma_read(>dev, desc->buf_addr2,
> s->tx_frame + s->tx_frame_len, len2);
> s->tx_frame_len += len2;
> }
> desc->status = (len1 + len2) ? 0 : 0x7fff;
> +
> +return 0;
> }
>
> static void tulip_setup_filter_addr(TULIPState *s, uint8_t *buf, int n)
> @@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
>
> static void tulip_xmit_list_update(TULIPState *s)
> {
> +#define TULIP_DESC_MAX 128
> +uint8_t i = 0;
> struct tulip_descriptor desc;
>
> if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> return;
> }
>
> -for (;;) {
> +for (i = 0; i < TULIP_DESC_MAX; i++) {
> tulip_desc_read(s, s->current_tx_desc, );
> tulip_dump_tx_descriptor(s, );
>
> @@ -675,10 +693,10 @@ static void tulip_xmit_list_update(TULIPState *s)
> s->tx_frame_len = 0;
> }
>
> -tulip_copy_tx_buffers(s, );
> -
> -if (desc.control & TDES1_LS) {
> -
Re: [PATCH v2] target/i386: Add ARCH_CAPABILITIES related bits into Icelake-Server CPU model
On 3/24/2020 2:39 AM, Eduardo Habkost wrote:
On Mon, Mar 23, 2020 at 10:58:16AM +0800, Xiaoyao Li wrote:
On 3/23/2020 10:32 AM, Tao Xu wrote:
Hi Xiaoyao,
May be you can add .note for this new version.
for example:
+ .version = 3,
+ .note = "ARCH_CAPABILITIES",
+ .props = (PropValue[]) {
Hi Paolo and Eduardo,
Need I spin a new version to add the .note ?
Maybe you can add it when queue?
Please send a follow up patch so we don't hold a bug fix because
of something that's just cosmetic. I will queue this patch. We
still need a new version of "target/i386: Add notes for versioned
CPU models"[1], don't we?
[1] https://lore.kernel.org/qemu-devel/20200228215253.gb494...@habkost.net/
I am sorry for misunderstanding your comments in that patch[1]. I will
submit a new version of this patch.
Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on mmio region translation by viommu
[Cc +dwg who originated this warning]
On Mon, 23 Mar 2020 14:16:09 +0530
Bharat Bhushan wrote:
> On ARM, the MSI doorbell is translated by the virtual IOMMU.
> As such address_space_translate() returns the MSI controller
> MMIO region and we get an "iommu map to non memory area"
> message. Let's remove this latter.
>
> Signed-off-by: Eric Auger
> Signed-off-by: Bharat Bhushan
> ---
> hw/vfio/common.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 5ca11488d6..c586edf47a 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -426,8 +426,6 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb, void
> **vaddr,
> , , writable,
> MEMTXATTRS_UNSPECIFIED);
> if (!memory_region_is_ram(mr)) {
> -error_report("iommu map to non memory area %"HWADDR_PRIx"",
> - xlat);
> return false;
> }
>
I'm a bit confused here, I think we need more justification beyond "we
hit this warning and we don't want to because it's ok in this one
special case, therefore remove it". I assume the special case is that
the device MSI address is managed via the SET_IRQS ioctl and therefore
we won't actually get DMAs to this range. But I imagine the case that
was in mind when adding this warning was general peer-to-peer between
and assigned and emulated device. Maybe there's an argument to be made
that such a p2p mapping might also be used in a non-vIOMMU case. We
skip creating those mappings and drivers continue to work, maybe
because nobody attempts to do p2p DMA with the types of devices we
emulate, maybe because p2p DMA is not absolutely reliable on bare metal
and drivers test it before using it. Anyway, I need a better argument
why this is ok. Thanks,
Alex
Re: [PATCH v4 0/2] Replaced locks with lock guard macros
On 3/23/20 2:46 PM, Daniel Brodsky wrote:
> On Mon, Mar 23, 2020 at 6:25 AM Stefan Hajnoczi wrote:
>>
>> On Fri, Mar 20, 2020 at 06:43:23AM -0700, no-re...@patchew.org wrote:
>>> /tmp/qemu-test/src/util/thread-pool.c:213:5: error: unused variable
>>> 'qemu_lockable_auto1' [-Werror,-Wunused-variable]
>>> QEMU_LOCK_GUARD(>lock);
>>> ^
>>> /tmp/qemu-test/src/include/qemu/lockable.h:173:29: note: expanded from
>>> macro 'QEMU_LOCK_GUARD'
>>
>> Apparently gcc suppresses "unused variable" warnings with g_autoptr() so
>> we didn't see this warning before.
>>
>> clang does report them so let's silence the warning manually. Please
>> add G_GNUC_UNUSED onto the g_autoptr variable definition in the
>> QEMU_LOCK_GUARD() macro:
>>
>> #define QEMU_LOCK_GUARD(x) \
>> g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ G_GNUC_UNUSED
>> = \
>> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
>>
>> The WITH_*_LOCK_GUARD() macros should not require changes because the
>> variable is both read and written.
>>
>> You can test locally by building with clang (llvm) instead of gcc:
>>
>> ./configure --cc=clang
>
> Using --cc=clang is causing the following error in several different places:
> qemu/target/ppc/translate.c:1894:18: error: result of comparison
> 'target_ulong' (aka 'unsigned int') <= 4294967295
> is always true [-Werror,-Wtautological-type-limit-compare]
> if (mask <= 0xu) {
> ^ ~~~
>
> these errors don't come up when building with gcc. Any idea how to fix
> this? Or should I just suppress it?
I'm of the opinion that it should be suppressed.
This is the *correct* test for ppc64, and the warning for ppc32 is simply
because target_ulong == uint32_t. True is the correct result for ppc32.
We simply want the compiler to DTRT: simplify this test and remove the else as
unreachable.
r~
Re: [PULL 00/20] Ide patches
On 19/03/2020 18:02, John Snow wrote: > On 3/19/20 8:33 AM, Peter Maydell wrote: >> On Tue, 17 Mar 2020 at 23:23, John Snow wrote: >>> >>> The following changes since commit 373c7068dd610e97f0b551b5a6d0a27cd6da4506: >>> >>> qemu.nsi: Install Sphinx documentation (2020-03-09 16:45:00 +) >>> >>> are available in the Git repository at: >>> >>> https://github.com/jnsnow/qemu.git tags/ide-pull-request >>> >>> for you to fetch changes up to 7d0776ca7f853d466b6174d96daa5c8afc43d1a4: >>> >>> hw/ide: Remove unneeded inclusion of hw/ide.h (2020-03-17 12:22:36 -0400) >>> >>> >>> Pull request >>> >>> >>> >> >> >> Applied, thanks. >> >> Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0 >> for any user-visible changes. >> >> -- PMM >> > > Mark, I'm sorry to foist this on you, but would you mind updating the > changelog? > > --js Done. I've added them under https://wiki.qemu.org/ChangeLog/5.0#Block_devices since there doesn't seem to be a dedicated IDE section. ATB, Mark.
Re: [PATCH 1/1] spapr/rtas: Add MinMem to ibm,get-system-parameter RTAS call
On Mon, 2020-03-23 at 14:24 +1100, David Gibson wrote: > On Fri, Mar 20, 2020 at 09:39:22PM -0300, Leonardo Bras wrote: > > Add support for MinMem SPLPAR Characteristic on emulated > > RTAS call ibm,get-system-parameter. > > > > MinMem represents Minimum Memory, that is described in LOPAPR as: > > The minimum amount of main store that is needed to power on the > > partition. Minimum memory is expressed in MB of storage. > > Where exactly does LoPAPR say that? The version I'm looking at > doesn't describe MinMem all that clearly, other than to say it must be > <= DesMem, which currently has the same value here. Please look for "Minimum Memory". It's on Table 237. SPLPAR Terms. > > This provides a way for the OS to discern hotplugged LMBs and > > LMBs that have started with the VM, allowing it to better provide > > a way for memory hot-removal. > > This seems a bit dubious. Surely we should have this information from > the dynamic-reconfiguration-memory stuff already? Trying to discern > this from purely a number seems very fragile - wouldn't that mean > making assumptions about how qemu / the host is laying out it's fixed > and dynamic memory which might not be justified? I agree. I previously sent a RFC proposing the usage of a new flag for this same reason [1], which I thank you for positive feedback. Even though I think using a flag is a better solution, I am also working in this other option (MinMem), that would use parameter already available on the platform, in case the new flag don't get approved. So far, using MinMem looks like a very complex solution on kernel side, and I think it's a poor solution. I decided to send this patch because it's a simple change to the platform support, that should cause no harm and could even be useful to other OSes. Best regards, Leonardo [1] http://patchwork.ozlabs.org/patch/1249931/ signature.asc Description: This is a digitally signed message part
Re: [PATCH v1 09/22] vfio/common: check PASID alloc/free availability
On Sun, Mar 22, 2020 at 05:36:06AM -0700, Liu Yi L wrote:
[...]
> @@ -1256,11 +1334,19 @@ static int vfio_init_container(VFIOContainer
> *container, int group_fd,
> }
>
> if (iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
> -/*
> - * TODO: config flags per host IOMMU nesting capability
> - * e.g. check if VFIO_TYPE1_NESTING_IOMMU supports PASID
> - * alloc/free
> - */
> +struct vfio_iommu_type1_info_cap_nesting nesting = {
> + .nesting_capabilities = 0x0,
> + .stage1_formats = 0, };
> +
> +ret = vfio_get_nesting_iommu_cap(container, );
> +if (ret) {
> +error_setg_errno(errp, -ret,
> + "Failed to get nesting iommu cap");
> +return ret;
> +}
> +
> +flags |= (nesting.nesting_capabilities & VFIO_IOMMU_PASID_REQS) ?
> + HOST_IOMMU_PASID_REQUEST : 0;
I replied in the previous patch but I forgot to use reply-all...
Anyway I'll comment again here - I think it'll be slightly better we
use the previous patch to only offer the vfio specific hooks, and this
patch to do all the rest including host_iommu_ctx_init() below, which
will avoid creating the host_iommu_ctx_init().
Thanks,
> host_iommu_ctx_init(>host_icx,
> sizeof(container->host_icx),
> TYPE_VFIO_HOST_IOMMU_CONTEXT,
> --
> 2.7.4
>
--
Peter Xu
Potential missing checks
Hi QEMU developers, I noticed the following two potential missing checks by static analysis and detecting inconsistencies on the source code of QEMU. here is the result: 1) Missing check on offset: https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L2728-L2733 While it is checked here: https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L1748-L1752 2) Missing check on bmds->dirty_bitmap: https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/migration/block.c#L377-L378 While it is checked here: https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/migration/block.c#L363-L365 Thanks, Mansour
Re: [PATCH v4 0/2] Replaced locks with lock guard macros
On Mon, Mar 23, 2020 at 6:25 AM Stefan Hajnoczi wrote:
>
> On Fri, Mar 20, 2020 at 06:43:23AM -0700, no-re...@patchew.org wrote:
> > /tmp/qemu-test/src/util/thread-pool.c:213:5: error: unused variable
> > 'qemu_lockable_auto1' [-Werror,-Wunused-variable]
> > QEMU_LOCK_GUARD(>lock);
> > ^
> > /tmp/qemu-test/src/include/qemu/lockable.h:173:29: note: expanded from
> > macro 'QEMU_LOCK_GUARD'
>
> Apparently gcc suppresses "unused variable" warnings with g_autoptr() so
> we didn't see this warning before.
>
> clang does report them so let's silence the warning manually. Please
> add G_GNUC_UNUSED onto the g_autoptr variable definition in the
> QEMU_LOCK_GUARD() macro:
>
> #define QEMU_LOCK_GUARD(x) \
> g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ G_GNUC_UNUSED =
> \
> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
>
> The WITH_*_LOCK_GUARD() macros should not require changes because the
> variable is both read and written.
>
> You can test locally by building with clang (llvm) instead of gcc:
>
> ./configure --cc=clang
Using --cc=clang is causing the following error in several different places:
qemu/target/ppc/translate.c:1894:18: error: result of comparison
'target_ulong' (aka 'unsigned int') <= 4294967295
is always true [-Werror,-Wtautological-type-limit-compare]
if (mask <= 0xu) {
^ ~~~
these errors don't come up when building with gcc. Any idea how to fix
this? Or should I just suppress it?
Re: [PATCH v4 0/2] introduction of migration_version attribute for VFIO live migration
On Mon, 3 Jun 2019 20:34:22 -0400 Yan Zhao wrote: > On Tue, Jun 04, 2019 at 03:29:32AM +0800, Alex Williamson wrote: > > On Thu, 30 May 2019 20:44:38 -0400 > > Yan Zhao wrote: > > > > > This patchset introduces a migration_version attribute under sysfs of VFIO > > > Mediated devices. > > > > > > This migration_version attribute is used to check migration compatibility > > > between two mdev devices of the same mdev type. > > > > > > Patch 1 defines migration_version attribute in > > > Documentation/vfio-mediated-device.txt > > > > > > Patch 2 uses GVT as an example to show how to expose migration_version > > > attribute and check migration compatibility in vendor driver. > > > > Thanks for iterating through this, it looks like we've settled on > > something reasonable, but now what? This is one piece of the puzzle to > > supporting mdev migration, but I don't think it makes sense to commit > > this upstream on its own without also defining the remainder of how we > > actually do migration, preferably with more than one working > > implementation and at least prototyped, if not final, QEMU support. I > > hope that was the intent, and maybe it's now time to look at the next > > piece of the puzzle. Thanks, > > > > Alex > > Got it. > Also thank you and all for discussing and guiding all along:) > We'll move to the next episode now. Hi Yan, As we're hopefully moving towards a migration API, would it make sense to refresh this series at the same time? I think we're still expecting a vendor driver implementing Kirti's migration API to also implement this sysfs interface for compatibility verification. Thanks, Alex
Re: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context callback
On Sun, Mar 22, 2020 at 05:36:04AM -0700, Liu Yi L wrote:
> This patch adds set/unset_iommu_context() impelementation in Intel
> vIOMMU. For Intel platform, pass-through modules (e.g. VFIO) could
> set HostIOMMUContext to Intel vIOMMU emulator.
>
> Cc: Kevin Tian
> Cc: Jacob Pan
> Cc: Peter Xu
> Cc: Yi Sun
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Signed-off-by: Liu Yi L
> ---
> hw/i386/intel_iommu.c | 70
> +++
> include/hw/i386/intel_iommu.h | 17 +--
> 2 files changed, 80 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 4b22910..8d9204f 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3354,23 +3354,35 @@ static const MemoryRegionOps vtd_mem_ir_ops = {
> },
> };
>
> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> +/**
> + * Fetch a VTDBus instance for given PCIBus. If no existing instance,
> + * allocate one.
> + */
> +static VTDBus *vtd_find_add_bus(IntelIOMMUState *s, PCIBus *bus)
> {
> uintptr_t key = (uintptr_t)bus;
> VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, );
> -VTDAddressSpace *vtd_dev_as;
> -char name[128];
>
> if (!vtd_bus) {
> uintptr_t *new_key = g_malloc(sizeof(*new_key));
> *new_key = (uintptr_t)bus;
> /* No corresponding free() */
> -vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * \
> -PCI_DEVFN_MAX);
> +vtd_bus = g_malloc0(sizeof(VTDBus) + PCI_DEVFN_MAX * \
> +(sizeof(VTDAddressSpace *) + \
> + sizeof(VTDHostIOMMUContext *)));
IIRC I commented on this before... Shouldn't sizeof(VTDBus) be
enough?
> vtd_bus->bus = bus;
> g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus);
> }
> +return vtd_bus;
> +}
> +
> +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> +{
> +VTDBus *vtd_bus;
> +VTDAddressSpace *vtd_dev_as;
> +char name[128];
>
> +vtd_bus = vtd_find_add_bus(s, bus);
> vtd_dev_as = vtd_bus->dev_as[devfn];
>
> if (!vtd_dev_as) {
> @@ -3436,6 +3448,52 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
> PCIBus *bus, int devfn)
> return vtd_dev_as;
> }
>
> +static int vtd_dev_set_iommu_context(PCIBus *bus, void *opaque,
> + int devfn,
> + HostIOMMUContext *host_icx)
> +{
> +IntelIOMMUState *s = opaque;
> +VTDBus *vtd_bus;
> +VTDHostIOMMUContext *vtd_dev_icx;
> +
> +assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
> +
> +vtd_bus = vtd_find_add_bus(s, bus);
> +
> +vtd_iommu_lock(s);
> +vtd_dev_icx = vtd_bus->dev_icx[devfn];
> +
> +if (!vtd_dev_icx) {
We can assert this directly I think, in case we accidentally set the
context twice without notice.
> +vtd_bus->dev_icx[devfn] = vtd_dev_icx =
> +g_malloc0(sizeof(VTDHostIOMMUContext));
> +vtd_dev_icx->vtd_bus = vtd_bus;
> +vtd_dev_icx->devfn = (uint8_t)devfn;
> +vtd_dev_icx->iommu_state = s;
> +vtd_dev_icx->host_icx = host_icx;
> +}
> +vtd_iommu_unlock(s);
> +
> +return 0;
> +}
> +
> +static void vtd_dev_unset_iommu_context(PCIBus *bus, void *opaque, int devfn)
> +{
> +IntelIOMMUState *s = opaque;
> +VTDBus *vtd_bus;
> +VTDHostIOMMUContext *vtd_dev_icx;
> +
> +assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
> +
> +vtd_bus = vtd_find_add_bus(s, bus);
> +
> +vtd_iommu_lock(s);
> +
> +vtd_dev_icx = vtd_bus->dev_icx[devfn];
> +g_free(vtd_dev_icx);
Better set it as NULL, and can also drop vtd_dev_icx which seems
meaningless..
g_free(vtd_bus->dev_icx[devfn]);
vtd_bus->dev_icx[devfn] = NULL;
> +
> +vtd_iommu_unlock(s);
> +}
> +
> static uint64_t get_naturally_aligned_size(uint64_t start,
> uint64_t size, int gaw)
> {
> @@ -3731,6 +3789,8 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus,
> void *opaque, int devfn)
>
> static PCIIOMMUOps vtd_iommu_ops = {
> .get_address_space = vtd_host_dma_iommu,
> +.set_iommu_context = vtd_dev_set_iommu_context,
> +.unset_iommu_context = vtd_dev_unset_iommu_context,
> };
>
> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 3870052..9b4fc0a 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -64,6 +64,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
> typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
> typedef struct VTDPASIDDirEntry VTDPASIDDirEntry;
> typedef struct VTDPASIDEntry VTDPASIDEntry;
> +typedef struct VTDHostIOMMUContext VTDHostIOMMUContext;
>
> /* Context-Entry */
>
[Bug 1866870] Re: KVM Guest pauses after upgrade to Ubuntu 20.04
Works for me. F31 KVM guest is installing on Q9550 box.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1866870
Title:
KVM Guest pauses after upgrade to Ubuntu 20.04
Status in QEMU:
Invalid
Status in qemu package in Ubuntu:
Invalid
Status in seabios package in Ubuntu:
Fix Released
Bug description:
Symptom:
Error unpausing domain: internal error: unable to execute QEMU command
'cont': Resetting the Virtual Machine is required
Traceback (most recent call last):
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in
cb_wrapper
callback(asyncjob, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 111, in tmpcb
callback(*args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/libvirtobject.py", line
66, in newfn
ret = fn(self, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/domain.py", line 1311, in
resume
self._backend.resume()
File "/usr/lib/python3/dist-packages/libvirt.py", line 2174, in resume
if ret == -1: raise libvirtError ('virDomainResume() failed', dom=self)
libvirt.libvirtError: internal error: unable to execute QEMU command 'cont':
Resetting the Virtual Machine is required
---
As outlined here:
https://bugs.launchpad.net/qemu/+bug/1813165/comments/15
After upgrade, all KVM guests are in a default pause state. Even after
forcing them off via virsh, and restarting them the guests are paused.
These Guests are not nested.
A lot of diganostic information are outlined in the previous bug
report link provided. The solution mentioned in previous report had
been allegedly integrated into the downstream updates.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1866870/+subscriptions
Re: [PATCH v1 06/22] hw/pci: introduce pci_device_set/unset_iommu_context()
On Sun, Mar 22, 2020 at 05:36:03AM -0700, Liu Yi L wrote:
[...]
> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +{
> +PCIBus *bus;
> +uint8_t devfn;
> +
> +pci_device_get_iommu_bus_devfn(dev, , );
> +if (bus && bus->iommu_ops &&
> + bus->iommu_ops->get_address_space) {
Nit: Since we're moving it around, maybe re-align it to left bracket?
Same to below two places.
With the indent fixed:
Reviewed-by: Peter Xu
> +return bus->iommu_ops->get_address_space(bus,
> +bus->iommu_opaque, devfn);
> }
> return _space_memory;
> }
--
Peter Xu
Re: [PATCH v15 Kernel 3/7] vfio iommu: Add ioctl definition for dirty pages tracking.
Hi Kirti,
On 3/19/20 9:16 PM, Kirti Wankhede wrote:
> IOMMU container maintains a list of all pages pinned by vfio_pin_pages API.
> All pages pinned by vendor driver through this API should be considered as
> dirty during migration. When container consists of IOMMU capable device and
> all pages are pinned and mapped, then all pages are marked dirty.
> Added support to start/stop dirtied pages tracking and to get bitmap of all
> dirtied pages for requested IO virtual address range.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> ---
> include/uapi/linux/vfio.h | 55
> +++
> 1 file changed, 55 insertions(+)
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index d0021467af53..8138f94cac15 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -995,6 +995,12 @@ struct vfio_iommu_type1_dma_map {
>
> #define VFIO_IOMMU_MAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 13)
>
> +struct vfio_bitmap {
> + __u64pgsize;/* page size for bitmap */
in bytes as well
> + __u64size; /* in bytes */
> + __u64 __user *data; /* one bit per page */
> +};
> +
> /**
> * VFIO_IOMMU_UNMAP_DMA - _IOWR(VFIO_TYPE, VFIO_BASE + 14,
> * struct vfio_dma_unmap)
> @@ -1021,6 +1027,55 @@ struct vfio_iommu_type1_dma_unmap {
> #define VFIO_IOMMU_ENABLE_IO(VFIO_TYPE, VFIO_BASE + 15)
> #define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16)
>
> +/**
> + * VFIO_IOMMU_DIRTY_PAGES - _IOWR(VFIO_TYPE, VFIO_BASE + 17,
> + * struct vfio_iommu_type1_dirty_bitmap)
> + * IOCTL is used for dirty pages tracking. Caller sets argsz, which is size
> of> + * struct vfio_iommu_type1_dirty_bitmap.
nit: This may become outdated when adding new fields. argz use mode is
documented at the beginning of the file.
Caller set flag depend on which
> + * operation to perform, details as below:
> + *
> + * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_START set, indicates
> + * migration is active and IOMMU module should track pages which are dirtied
> or
> + * potentially dirtied by device.
> + * Dirty pages are tracked until tracking is stopped by user application by
> + * setting VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP flag.
> + *
> + * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP set, indicates
> + * IOMMU should stop tracking dirtied pages.
> + *
> + * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP flag set,
> + * IOCTL returns dirty pages bitmap for IOMMU container during migration for
> + * given IOVA range. User must provide data[] as the structure
> + * vfio_iommu_type1_dirty_bitmap_get through which user provides IOVA range
I think the fact the IOVA range must match the vfio dma_size must be
documented.
and
> + * pgsize. This interface supports to get bitmap of smallest supported pgsize
> + * only and can be modified in future to get bitmap of specified pgsize.
> + * User must allocate memory for bitmap, zero the bitmap memory and set size
> + * of allocated memory in bitmap_size field. One bit is used to represent one
> + * page consecutively starting from iova offset. User should provide page
> size
> + * in 'pgsize'. Bit set in bitmap indicates page at that offset from iova is
> + * dirty. Caller must set argsz including size of structure
> + * vfio_iommu_type1_dirty_bitmap_get.
nit: ditto
> + *
> + * Only one of the flags _START, STOP and _GET may be specified at a time.
> + *
> + */
> +struct vfio_iommu_type1_dirty_bitmap {
> + __u32argsz;
> + __u32flags;
> +#define VFIO_IOMMU_DIRTY_PAGES_FLAG_START(1 << 0)
> +#define VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP (1 << 1)
> +#define VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP (1 << 2)
> + __u8 data[];
> +};
> +
> +struct vfio_iommu_type1_dirty_bitmap_get {
> + __u64 iova;/* IO virtual address */
> + __u64 size;/* Size of iova range */
> + struct vfio_bitmap bitmap;
> +};
> +
> +#define VFIO_IOMMU_DIRTY_PAGES _IO(VFIO_TYPE, VFIO_BASE + 17)
> +
> /* Additional API for SPAPR TCE (Server POWERPC) IOMMU */
>
> /*
>
Thanks
Eric
Re: [PULL v2 00/37] Linux user for 5.0 patches
On 3/23/20 1:33 PM, Laurent Vivier wrote: > Le 18/03/2020 à 20:46, Richard Henderson a écrit : >> On 3/18/20 6:57 AM, Peter Maydell wrote: >>> My set of "run ls for various architectures" linux-user tests >>> https://people.linaro.org/~peter.maydell/linux-user-test-pmm-20200114.tgz >>> fails with this pullreq: >>> >>> e104462:bionic:linux-user-test-0.3$ >>> /home/petmay01/linaro/qemu-for-merges/build/all-linux-static/x86_64-linux-user/qemu-x86_64 >>> -L ./gnemul/qemu-x86_64 x86_64/ls -l dummyfile >>> qemu: 0x40008117e9: unhandled CPU exception 0x101 - aborting >> >> >> I replicated this on aarch64 host, with an existing build tree and merging in >> the pull request. It does not occur when building the same merged tree from >> scratch. >> >> I have no idea what the reason for this is. Laurent suggested a file in the >> build tree that is shadowed by one in the source tree, but to me that makes >> no >> sense for this case: >> >> It's target/i386/cpu.h that defines EXCP_SYSCALL (renumbered in this series >> from 0x100 to 0x101), which is not in the build tree. It is >> linux-user/i386/cpu_loop.c that consumes EXCP_SYSCALL, and it is also not in >> the build tree. >> >> However, from the error message above, it's clear that cpu_loop.o has not >> been >> rebuilt properly. >> > > I removed this series from the final pull request as the problem doesn't > seem related to change I made in configure. > > I didn't find from where the problem comes. > > Could you check if you are always able to reproduce the problem with master? I could not replicate it today with master at 787f82407c5. r~
Re: [PATCH v1 04/22] hw/iommu: introduce HostIOMMUContext
On Sun, Mar 22, 2020 at 05:36:01AM -0700, Liu Yi L wrote: > Currently, many platform vendors provide the capability of dual stage > DMA address translation in hardware. For example, nested translation > on Intel VT-d scalable mode, nested stage translation on ARM SMMUv3, > and etc. In dual stage DMA address translation, there are two stages > address translation, stage-1 (a.k.a first-level) and stage-2 (a.k.a > second-level) translation structures. Stage-1 translation results are > also subjected to stage-2 translation structures. Take vSVA (Virtual > Shared Virtual Addressing) as an example, guest IOMMU driver owns > stage-1 translation structures (covers GVA->GPA translation), and host > IOMMU driver owns stage-2 translation structures (covers GPA->HPA > translation). VMM is responsible to bind stage-1 translation structures > to host, thus hardware could achieve GVA->GPA and then GPA->HPA > translation. For more background on SVA, refer the below links. > - https://www.youtube.com/watch?v=Kq_nfGK5MwQ > - https://events19.lfasiallc.com/wp-content/uploads/2017/11/\ > Shared-Virtual-Memory-in-KVM_Yi-Liu.pdf > > In QEMU, vIOMMU emulators expose IOMMUs to VM per their own spec (e.g. > Intel VT-d spec). Devices are pass-through to guest via device pass- > through components like VFIO. VFIO is a userspace driver framework > which exposes host IOMMU programming capability to userspace in a > secure manner. e.g. IOVA MAP/UNMAP requests. Thus the major connection > between VFIO and vIOMMU are MAP/UNMAP. However, with the dual stage > DMA translation support, there are more interactions between vIOMMU and > VFIO as below: > 1) PASID allocation (allow host to intercept in PASID allocation) > 2) bind stage-1 translation structures to host > 3) propagate stage-1 cache invalidation to host > 4) DMA address translation fault (I/O page fault) servicing etc. > > With the above new interactions in QEMU, it requires an abstract layer > to facilitate the above operations and expose to vIOMMU emulators as an > explicit way for vIOMMU emulators call into VFIO. This patch introduces > HostIOMMUContext to stand for hardware IOMMU w/ dual stage DMA address > translation capability. And introduces HostIOMMUContextClass to provide > methods for vIOMMU emulators to propagate dual-stage translation related > requests to host. As a beginning, PASID allocation/free are defined to > propagate PASID allocation/free requests to host which is helpful for the > vendors who manage PASID in system-wide. In future, there will be more > operations like bind_stage1_pgtbl, flush_stage1_cache and etc. > > Cc: Kevin Tian > Cc: Jacob Pan > Cc: Peter Xu > Cc: Eric Auger > Cc: Yi Sun > Cc: David Gibson > Cc: Michael S. Tsirkin > Signed-off-by: Liu Yi L > --- > hw/Makefile.objs | 1 + > hw/iommu/Makefile.objs| 1 + > hw/iommu/host_iommu_context.c | 112 > ++ > include/hw/iommu/host_iommu_context.h | 75 +++ > 4 files changed, 189 insertions(+) > create mode 100644 hw/iommu/Makefile.objs > create mode 100644 hw/iommu/host_iommu_context.c > create mode 100644 include/hw/iommu/host_iommu_context.h > > diff --git a/hw/Makefile.objs b/hw/Makefile.objs > index 660e2b4..cab83fe 100644 > --- a/hw/Makefile.objs > +++ b/hw/Makefile.objs > @@ -40,6 +40,7 @@ devices-dirs-$(CONFIG_MEM_DEVICE) += mem/ > devices-dirs-$(CONFIG_NUBUS) += nubus/ > devices-dirs-y += semihosting/ > devices-dirs-y += smbios/ > +devices-dirs-y += iommu/ > endif > > common-obj-y += $(devices-dirs-y) > diff --git a/hw/iommu/Makefile.objs b/hw/iommu/Makefile.objs > new file mode 100644 > index 000..e6eed4e > --- /dev/null > +++ b/hw/iommu/Makefile.objs > @@ -0,0 +1 @@ > +obj-y += host_iommu_context.o > diff --git a/hw/iommu/host_iommu_context.c b/hw/iommu/host_iommu_context.c > new file mode 100644 > index 000..af61899 > --- /dev/null > +++ b/hw/iommu/host_iommu_context.c I'm not 100% sure it's the best place to put this; I thought hw/ is for emulated devices while this is some host utility, but I could be wrong. However it'll always be some kind of backend of a vIOMMU, so... Maybe we can start with this until someone else disagrees. > @@ -0,0 +1,112 @@ > +/* > + * QEMU abstract of Host IOMMU > + * > + * Copyright (C) 2020 Intel Corporation. > + * > + * Authors: Liu Yi L > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + > + * You should have received a copy of the GNU
Re: [PULL 0/5] target-arm queue
On Mon, 23 Mar 2020 at 17:40, Peter Maydell wrote:
>
> Just a few minor bugfixes, but we might as well get them in
> for rc0 tomorrow.
>
> -- PMM
>
> The following changes since commit 787f82407c5056a8b1097e39e53d01dd1abe406b:
>
> Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20200323' into
> staging (2020-03-23 15:38:30 +)
>
> are available in the Git repository at:
>
> https://git.linaro.org/people/pmaydell/qemu-arm.git
> tags/pull-target-arm-20200323
>
> for you to fetch changes up to 550a04893c2bd4442211b353680b9a6408d94dba:
>
> target/arm: Move computation of index in handle_simd_dupe (2020-03-23
> 17:22:30 +)
>
>
> target-arm queue:
> * target/arm: avoid undefined behaviour shift in watchpoint code
> * target/arm: avoid undefined behaviour shift in handle_simd_dupe()
> * target/arm: add assert that immh != 0 in disas_simd_shift_imm()
> * aspeed/smc: Fix DMA support for AST2600
> * hw/arm/bcm283x: Correct the license text ('and' vs 'or')
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.
-- PMM
Re: [PULL v2 00/37] Linux user for 5.0 patches
Le 18/03/2020 à 20:46, Richard Henderson a écrit : > On 3/18/20 6:57 AM, Peter Maydell wrote: >> My set of "run ls for various architectures" linux-user tests >> https://people.linaro.org/~peter.maydell/linux-user-test-pmm-20200114.tgz >> fails with this pullreq: >> >> e104462:bionic:linux-user-test-0.3$ >> /home/petmay01/linaro/qemu-for-merges/build/all-linux-static/x86_64-linux-user/qemu-x86_64 >> -L ./gnemul/qemu-x86_64 x86_64/ls -l dummyfile >> qemu: 0x40008117e9: unhandled CPU exception 0x101 - aborting > > > I replicated this on aarch64 host, with an existing build tree and merging in > the pull request. It does not occur when building the same merged tree from > scratch. > > I have no idea what the reason for this is. Laurent suggested a file in the > build tree that is shadowed by one in the source tree, but to me that makes no > sense for this case: > > It's target/i386/cpu.h that defines EXCP_SYSCALL (renumbered in this series > from 0x100 to 0x101), which is not in the build tree. It is > linux-user/i386/cpu_loop.c that consumes EXCP_SYSCALL, and it is also not in > the build tree. > > However, from the error message above, it's clear that cpu_loop.o has not been > rebuilt properly. > I removed this series from the final pull request as the problem doesn't seem related to change I made in configure. I didn't find from where the problem comes. Could you check if you are always able to reproduce the problem with master? Thanks, Laurent
[Bug 1866870] Re: KVM Guest pauses after upgrade to Ubuntu 20.04
apt-get will be enough once it's published, and looks like it just was
published.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1866870
Title:
KVM Guest pauses after upgrade to Ubuntu 20.04
Status in QEMU:
Invalid
Status in qemu package in Ubuntu:
Invalid
Status in seabios package in Ubuntu:
Fix Released
Bug description:
Symptom:
Error unpausing domain: internal error: unable to execute QEMU command
'cont': Resetting the Virtual Machine is required
Traceback (most recent call last):
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in
cb_wrapper
callback(asyncjob, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 111, in tmpcb
callback(*args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/libvirtobject.py", line
66, in newfn
ret = fn(self, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/domain.py", line 1311, in
resume
self._backend.resume()
File "/usr/lib/python3/dist-packages/libvirt.py", line 2174, in resume
if ret == -1: raise libvirtError ('virDomainResume() failed', dom=self)
libvirt.libvirtError: internal error: unable to execute QEMU command 'cont':
Resetting the Virtual Machine is required
---
As outlined here:
https://bugs.launchpad.net/qemu/+bug/1813165/comments/15
After upgrade, all KVM guests are in a default pause state. Even after
forcing them off via virsh, and restarting them the guests are paused.
These Guests are not nested.
A lot of diganostic information are outlined in the previous bug
report link provided. The solution mentioned in previous report had
been allegedly integrated into the downstream updates.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1866870/+subscriptions
Re: [PATCH v15 Kernel 2/7] vfio iommu: Remove atomicity of ref_count of pinned pages
Hi Kirti,
On 3/19/20 9:16 PM, Kirti Wankhede wrote:
> vfio_pfn.ref_count is always updated by holding iommu->lock, using atomic
> variable is overkill.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> drivers/vfio/vfio_iommu_type1.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 9fdfae1cb17a..70aeab921d0f 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -112,7 +112,7 @@ struct vfio_pfn {
> struct rb_node node;
> dma_addr_t iova; /* Device address */
> unsigned long pfn;/* Host pfn */
> - atomic_tref_count;
> + unsigned intref_count;
> };
>
> struct vfio_regions {
> @@ -233,7 +233,7 @@ static int vfio_add_to_pfn_list(struct vfio_dma *dma,
> dma_addr_t iova,
>
> vpfn->iova = iova;
> vpfn->pfn = pfn;
> - atomic_set(>ref_count, 1);
> + vpfn->ref_count = 1;
> vfio_link_pfn(dma, vpfn);
> return 0;
> }
> @@ -251,7 +251,7 @@ static struct vfio_pfn *vfio_iova_get_vfio_pfn(struct
> vfio_dma *dma,
> struct vfio_pfn *vpfn = vfio_find_vpfn(dma, iova);
>
> if (vpfn)
> - atomic_inc(>ref_count);
> + vpfn->ref_count++;
> return vpfn;
> }
>
> @@ -259,7 +259,8 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma,
> struct vfio_pfn *vpfn)
> {
> int ret = 0;
>
> - if (atomic_dec_and_test(>ref_count)) {
> + vpfn->ref_count--;
> + if (!vpfn->ref_count) {
> ret = put_pfn(vpfn->pfn, dma->prot);
> vfio_remove_from_pfn_list(dma, vpfn);
> }
>
Re: [PATCH v15 Kernel 1/7] vfio: KABI for migration interface for device state
Hi Kirti,
On 3/19/20 9:16 PM, Kirti Wankhede wrote:
> - Defined MIGRATION region type and sub-type.
>
> - Defined vfio_device_migration_info structure which will be placed at the
> 0th offset of migration region to get/set VFIO device related
> information. Defined members of structure and usage on read/write access.
>
> - Defined device states and state transition details.
>
> - Defined sequence to be followed while saving and resuming VFIO device.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
Please forgive me, I have just discovered v15 was available.
hereafter, you will find the 2 main points I feel difficult to
understand when reading the documentation.
> ---
> include/uapi/linux/vfio.h | 227
> ++
> 1 file changed, 227 insertions(+)
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 9e843a147ead..d0021467af53 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -305,6 +305,7 @@ struct vfio_region_info_cap_type {
> #define VFIO_REGION_TYPE_PCI_VENDOR_MASK (0x)
> #define VFIO_REGION_TYPE_GFX(1)
> #define VFIO_REGION_TYPE_CCW (2)
> +#define VFIO_REGION_TYPE_MIGRATION (3)
>
> /* sub-types for VFIO_REGION_TYPE_PCI_* */
>
> @@ -379,6 +380,232 @@ struct vfio_region_gfx_edid {
> /* sub-types for VFIO_REGION_TYPE_CCW */
> #define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD(1)
>
> +/* sub-types for VFIO_REGION_TYPE_MIGRATION */
> +#define VFIO_REGION_SUBTYPE_MIGRATION (1)
> +
> +/*
> + * The structure vfio_device_migration_info is placed at the 0th offset of
> + * the VFIO_REGION_SUBTYPE_MIGRATION region to get and set VFIO device
> related
> + * migration information. Field accesses from this structure are only
> supported
> + * at their native width and alignment. Otherwise, the result is undefined
> and
> + * vendor drivers should return an error.
> + *
> + * device_state: (read/write)
> + * - The user application writes to this field to inform the vendor
> driver
> + *about the device state to be transitioned to.
> + * - The vendor driver should take the necessary actions to change the
> + *device state. After successful transition to a given state, the
> + *vendor driver should return success on write(device_state, state)
> + *system call. If the device state transition fails, the vendor
> driver
> + *should return an appropriate -errno for the fault condition.
> + * - On the user application side, if the device state transition fails,
> + * that is, if write(device_state, state) returns an error, read
> + * device_state again to determine the current state of the device from
> + * the vendor driver.
> + * - The vendor driver should return previous state of the device unless
> + *the vendor driver has encountered an internal error, in which case
> + *the vendor driver may report the device_state
> VFIO_DEVICE_STATE_ERROR.
> + * - The user application must use the device reset ioctl to recover the
> + *device from VFIO_DEVICE_STATE_ERROR state. If the device is
> + *indicated to be in a valid device state by reading device_state,
> the
> + *user application may attempt to transition the device to any valid
> + *state reachable from the current state or terminate itself.
> + *
> + * device_state consists of 3 bits:
> + * - If bit 0 is set, it indicates the _RUNNING state. If bit 0 is
> clear,
> + *it indicates the _STOP state. When the device state is changed to
> + *_STOP, driver should stop the device before write() returns.
> + * - If bit 1 is set, it indicates the _SAVING state, which means that
> the
> + *driver should start gathering device state information that will be
> + *provided to the VFIO user application to save the device's state.
> + * - If bit 2 is set, it indicates the _RESUMING state, which means that
> + *the driver should prepare to resume the device. Data provided
> through
> + *the migration region should be used to resume the device.
> + * Bits 3 - 31 are reserved for future use. To preserve them, the user
> + * application should perform a read-modify-write operation on this
> + * field when modifying the specified bits.
> + *
> + * +--- _RESUMING
> + * |+-- _SAVING
> + * ||+- _RUNNING
> + * |||
> + * 000b => Device Stopped, not saving or resuming
> + * 001b => Device running, which is the default state
> + * 010b => Stop the device & save the device state, stop-and-copy state
> + * 011b => Device running and save the device state, pre-copy state
> + * 100b => Device stopped and the device state is resuming
> + * 101b => Invalid state
> + * 110b => Error state
> + * 111b => Invalid state
> + *
> + * State transitions:
> + *
> + *
[Bug 1868617] Re: multiseat: route different spice tablet events to distinct vdagents
Here are two different ways i can think of to potentially solve this (i'm not qemu hacker, feel free to correct me or propose a better solution): - the spicevmc chardev's "name" parameter could be used to identify the agent numerically (e.g. "vdagent:1" instead of "vdagent") - the -device virtserialport could identify whether it is connected via a multiseat PCI bridge (pci-bridge-seat) and group it with the other monitor from there. Is one of these approaches preferable? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1868617 Title: multiseat: route different spice tablet events to distinct vdagents Status in QEMU: New Bug description: docs/multiseat.txt says: > Note on spice: Spice handles multihead just fine. But it can't do > multiseat. For tablet events the event source is sent to the spice > agent. But qemu can't figure it, so it can't do input routing. > Fixing this needs a new or extended input interface between > libspice-server and qemu. For keyboard events it is even worse: The > event source isn't included in the spice protocol, so the wire > protocol must be extended to support this. I'm not sure exactly what "can't figure it" means, but it looks to me like qemu can't route incoming tablet events from a spice client to distinct vdagent channels. I think this part of the process can be fixed within qemu. I've reported https://gitlab.freedesktop.org/spice/spice-gtk/issues/121 to address the issues with the keyboard interface at the protocol level. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1868617/+subscriptions
[Bug 1868617] [NEW] multiseat: route different spice tablet events to distinct vdagents
Public bug reported: docs/multiseat.txt says: > Note on spice: Spice handles multihead just fine. But it can't do > multiseat. For tablet events the event source is sent to the spice > agent. But qemu can't figure it, so it can't do input routing. > Fixing this needs a new or extended input interface between > libspice-server and qemu. For keyboard events it is even worse: The > event source isn't included in the spice protocol, so the wire > protocol must be extended to support this. I'm not sure exactly what "can't figure it" means, but it looks to me like qemu can't route incoming tablet events from a spice client to distinct vdagent channels. I think this part of the process can be fixed within qemu. I've reported https://gitlab.freedesktop.org/spice/spice-gtk/issues/121 to address the issues with the keyboard interface at the protocol level. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1868617 Title: multiseat: route different spice tablet events to distinct vdagents Status in QEMU: New Bug description: docs/multiseat.txt says: > Note on spice: Spice handles multihead just fine. But it can't do > multiseat. For tablet events the event source is sent to the spice > agent. But qemu can't figure it, so it can't do input routing. > Fixing this needs a new or extended input interface between > libspice-server and qemu. For keyboard events it is even worse: The > event source isn't included in the spice protocol, so the wire > protocol must be extended to support this. I'm not sure exactly what "can't figure it" means, but it looks to me like qemu can't route incoming tablet events from a spice client to distinct vdagent channels. I think this part of the process can be fixed within qemu. I've reported https://gitlab.freedesktop.org/spice/spice-gtk/issues/121 to address the issues with the keyboard interface at the protocol level. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1868617/+subscriptions
Re: [PATCH v41 01/21] target/avr: Add outward facing interfaces and core CPU logic
thanks Philippe. On Mon, Mar 23, 2020 at 9:20 PM Philippe Mathieu-Daudé wrote: > On 3/23/20 7:03 PM, Richard Henderson wrote: > > On 3/23/20 10:03 AM, Michael Rolnik wrote: > >> Hi Philippe. > >> > >> It's been a while. let me think about it and get back to you. what is > your > >> concern ? > > We are using this series with Joaquin for a Google Summit of Code > project, so we are noticing some bugs and fixing them. > As it has not been merged, we work in a fork. > Since it was posted on the list, I prefer to ask on the list than > directly to you. > > > > > It shouldn't be there. See commit 1f5c00cfdb81. > > Ah it has been moved to cpu_common_reset, thanks :) > I suppose it is because this port is based on some quite old work. > > > > >> > +memset(env->r, 0, sizeof(env->r)); > >> > + > >> > +tlb_flush(cs); > >> > >> Why are you calling tlb_flush() here? > > > > > > r~ > > > > -- Best Regards, Michael Rolnik
[Bug 1866870] Re: KVM Guest pauses after upgrade to Ubuntu 20.04
Can I just apt update && apt upgrade to get this fix or do I need to
patch?
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1866870
Title:
KVM Guest pauses after upgrade to Ubuntu 20.04
Status in QEMU:
Invalid
Status in qemu package in Ubuntu:
Invalid
Status in seabios package in Ubuntu:
Fix Released
Bug description:
Symptom:
Error unpausing domain: internal error: unable to execute QEMU command
'cont': Resetting the Virtual Machine is required
Traceback (most recent call last):
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in
cb_wrapper
callback(asyncjob, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 111, in tmpcb
callback(*args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/libvirtobject.py", line
66, in newfn
ret = fn(self, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/domain.py", line 1311, in
resume
self._backend.resume()
File "/usr/lib/python3/dist-packages/libvirt.py", line 2174, in resume
if ret == -1: raise libvirtError ('virDomainResume() failed', dom=self)
libvirt.libvirtError: internal error: unable to execute QEMU command 'cont':
Resetting the Virtual Machine is required
---
As outlined here:
https://bugs.launchpad.net/qemu/+bug/1813165/comments/15
After upgrade, all KVM guests are in a default pause state. Even after
forcing them off via virsh, and restarting them the guests are paused.
These Guests are not nested.
A lot of diganostic information are outlined in the previous bug
report link provided. The solution mentioned in previous report had
been allegedly integrated into the downstream updates.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1866870/+subscriptions
[PATCH] i386: Remove unused define's from hax and hvf
Commit acb9f95a removed boundary checks for ID and VCPU ID. After that,
the max definitions of that boundaries are not required anymore. This
commit is only a code cleanup.
Signed-off-by: Julio Faracco
---
target/i386/hax-i386.h | 2 --
target/i386/hvf/hvf-i386.h | 2 --
2 files changed, 4 deletions(-)
diff --git a/target/i386/hax-i386.h b/target/i386/hax-i386.h
index 54e9d8b057..70fe5cefeb 100644
--- a/target/i386/hax-i386.h
+++ b/target/i386/hax-i386.h
@@ -41,8 +41,6 @@ struct hax_state {
};
#define HAX_MAX_VCPU 0x10
-#define MAX_VM_ID 0x40
-#define MAX_VCPU_ID 0x40
struct hax_vm {
hax_fd fd;
diff --git a/target/i386/hvf/hvf-i386.h b/target/i386/hvf/hvf-i386.h
index 15ee4835cf..fbe4a350c5 100644
--- a/target/i386/hvf/hvf-i386.h
+++ b/target/i386/hvf/hvf-i386.h
@@ -21,8 +21,6 @@
#include "x86.h"
#define HVF_MAX_VCPU 0x10
-#define MAX_VM_ID 0x40
-#define MAX_VCPU_ID 0x40
extern struct hvf_state hvf_global;
--
2.24.1
Re: [PATCH v1 for 5.0 00/11] testing/next
On 3/23/20 9:15 AM, Alex Bennée wrote: > Alex Bennée (2): > tests/vm: fix basevm config > configure: disable MTTCG for MIPS guests > > Gerd Hoffmann (4): > tests/vm: write raw console log > tests/vm: move vga setup > tests/vm: update FreeBSD to 12.1 > tests/vm: update NetBSD to 9.0 > > Philippe Mathieu-Daudé (5): > tests/docker: Keep package list sorted > tests/docker: Install gcrypt devel package in Debian image > tests/docker: Use Python3 PyYAML in the Fedora image > tests/docker: Add libepoxy and libudev packages to the Fedora image > .travis.yml: Add a KVM-only s390x job Whole series: Tested-by: Richard Henderson r~
Re: [PATCH-for-5.0 v3] tests/migration: Reduce autoconverge initial bandwidth
* Philippe Mathieu-Daudé (phi...@redhat.com) wrote: > When using max-bandwidth=~100Mb/s, this test fails on Travis-CI > s390x when configured with --disable-tcg: > > $ make check-qtest > TESTcheck-qtest-s390x: tests/qtest/boot-serial-test > qemu-system-s390x: -accel tcg: invalid accelerator tcg > qemu-system-s390x: falling back to KVM > TESTcheck-qtest-s390x: tests/qtest/pxe-test > TESTcheck-qtest-s390x: tests/qtest/test-netfilter > TESTcheck-qtest-s390x: tests/qtest/test-filter-mirror > TESTcheck-qtest-s390x: tests/qtest/test-filter-redirector > TESTcheck-qtest-s390x: tests/qtest/drive_del-test > TESTcheck-qtest-s390x: tests/qtest/device-plug-test > TESTcheck-qtest-s390x: tests/qtest/virtio-ccw-test > TESTcheck-qtest-s390x: tests/qtest/cpu-plug-test > TESTcheck-qtest-s390x: tests/qtest/migration-test > ** > ERROR:tests/qtest/migration-test.c:1229:test_migrate_auto_converge: > 'got_stop' should be FALSE > ERROR - Bail out! > ERROR:tests/qtest/migration-test.c:1229:test_migrate_auto_converge: > 'got_stop' should be FALSE > make: *** [tests/Makefile.include:633: check-qtest-s390x] Error 1 > > Per David Gilbert, "it could just be the writing is slow on s390 > and the migration thread fast; in which case the autocomplete > wouldn't be needed. Perhaps we just need to reduce the bandwidth > limit." > > Tuning the threshold by reducing the initial bandwidth makes the > autoconverge test pass. > > Suggested-by: Dr. David Alan Gilbert > Signed-off-by: Philippe Mathieu-Daudé This is what I'm expecting to help, so if it passes testing lets try it. 'got_stop' being true just indicates the migration managed to complete. Reviewed-by: Dr. David Alan Gilbert > --- > v3: really reduce =) > --- > tests/qtest/migration-test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 3d6cc83b88..2568c9529c 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -1211,7 +1211,7 @@ static void test_migrate_auto_converge(void) > * without throttling. > */ > migrate_set_parameter_int(from, "downtime-limit", 1); > -migrate_set_parameter_int(from, "max-bandwidth", 1); /* ~100Mb/s > */ > +migrate_set_parameter_int(from, "max-bandwidth", 100); /* ~1Mb/s */ > > /* To check remaining size after precopy */ > migrate_set_capability(from, "pause-before-switchover", true); > -- > 2.21.1 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
On Mon, 23 Mar 2020, John Snow wrote:
On 3/23/20 1:04 PM, BALATON Zoltan wrote:
On Mon, 23 Mar 2020, Peter Maydell wrote:
Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.
Signed-off-by: Peter Maydell
---
This is how the 'use qdev gpio' approach to fixing the leak looks.
Disclaimer: I have only tested this with "make check", nothing more.
hw/ide/sii3112.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2b..2ae6f5d9df6 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev,
Error **errp)
{
SiI3112PCIState *d = SII3112_PCI(dev);
PCIIDEState *s = PCI_IDE(dev);
+ DeviceState *ds = DEVICE(dev);
MemoryRegion *mr;
- qemu_irq *irq;
int i;
pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev,
Error **errp)
memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", >mmio,
0, 16);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
- irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+ qdev_init_gpio_in(ds, sii3112_set_irq, 2);
for (i = 0; i < 2; i++) {
ide_bus_new(>bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
- ide_init2(>bus[i], irq[i]);
+ ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
Maybe we could just use DEVICE(dev) here and above as well just like in
ide_bus_new above just to keep it consistent and avoid the confusion
caused by having dev, d, s and now also ds. DEVICE(dev) is short and
clear enough I think.
Regards,
BALATON Zoltan
Reviewed-by: John Snow
The named temporary is fine. We probably should be using a named
temporary in the other locations, too.
Yes that's fine too if using cast more than once is less efficient. Could
you change the existing DEVICE(dev) also to ds when applying please?
Probably no need for a v2 for that.
I will run my usual tests, but admit I don't really test the non-x86
boards directly. Do you want to give a tested-by on this, if it matters
to you? Otherwise, I'm fairly content to trust Peter's judgment here.
I've tried that AmigaOS still boots on sam460ex so:
Tested-by: BALATON Zoltan
The sii3112 should also work on x86 or other platforms with Linux's
sata_sil driver but only as a 2nd non-bootable controller because we don't
have option ROM and BIOS probably has no driver. Apart from that it's a
common PCI SATA controller so likely a lot of guests have driver for it.
Regards,
BALATON Zoltan
[PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files
A discard request deallocates the selected clusters so they read back
as zeroes. This is done by clearing the cluster offset field and
setting QCOW_OFLAG_ZERO in the L2 entry.
This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible stale
data from the backing file.
Since discard is an advisory operation it's safer to simply forbid it
in this scenario.
Note that we are adding this check to qcow2_co_pdiscard() and not to
qcow2_cluster_discard() or discard_in_l2_slice() because the last
two are also used by qcow2_snapshot_create() to discard the clusters
used by the VM state. In this case there's no risk of exposing stale
data to the guest and we really want that the clusters are always
discarded.
Signed-off-by: Alberto Garcia
---
block/qcow2.c | 6 +++
tests/qemu-iotests/060 | 5 ++-
tests/qemu-iotests/060.out | 2 -
tests/qemu-iotests/289 | 90 ++
tests/qemu-iotests/289.out | 52 ++
tests/qemu-iotests/group | 1 +
6 files changed, 152 insertions(+), 4 deletions(-)
create mode 100755 tests/qemu-iotests/289
create mode 100644 tests/qemu-iotests/289.out
diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..7bb7e392e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3763,6 +3763,12 @@ static coroutine_fn int
qcow2_co_pdiscard(BlockDriverState *bs,
int ret;
BDRVQcow2State *s = bs->opaque;
+/* If the image does not support QCOW_OFLAG_ZERO then discarding
+ * clusters could expose stale data from the backing file. */
+if (s->qcow_version < 3 && bs->backing) {
+return -ENOTSUP;
+}
+
if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
assert(bytes < s->cluster_size);
/* Ignore partial clusters, except for the special case of the
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 043f12904a..4a4fdfb1e1 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
# Write two clusters, the second one enforces creation of an L2 table after
# the first data cluster.
$QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
-# Discard the first cluster. This cluster will soon enough be reallocated and
+# Free the first cluster. This cluster will soon enough be reallocated and
# used for COW.
-$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 -
L2 entry
+poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
# Now, corrupt the image by marking the second L2 table cluster as free.
poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
# Start a write operation requiring COW on the image stopping it right before
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index d27692a33c..09caaea865 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -105,8 +105,6 @@ wrote 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
wrote 65536/65536 bytes at offset 536870912
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-discard 65536/65536 bytes at offset 0
-64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
qcow2: Marking image as corrupt: Preventing invalid write on metadata
(overlaps with active L2 table); further corruption events will be suppressed
blkdebug: Suspended request '0'
write failed: Input/output error
diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
new file mode 100755
index 00..13b4984721
--- /dev/null
+++ b/tests/qemu-iotests/289
@@ -0,0 +1,90 @@
+#!/usr/bin/env bash
+#
+# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
Re: [PATCH v11 00/16] s390x: Protected Virtualization support
Patchew URL:
https://patchew.org/QEMU/20200319131921.2367-1-fran...@linux.ibm.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PATCH v11 00/16] s390x: Protected Virtualization support
Message-id: 20200319131921.2367-1-fran...@linux.ibm.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3496cd0 s390x: Add unpack facility feature to GA1
03d40c1 docs: system: Add protvirt docs
4f74e2a s390x: protvirt: Handle SIGP store status correctly
243e681 s390x: protvirt: Move IO control structures over SIDA
05aa2a2 s390x: protvirt: Disable address checks for PV guest IO emulation
9430059 s390x: protvirt: Move diag 308 data over SIDA
ca7e150 s390x: protvirt: Set guest IPL PSW
7214ecd s390x: protvirt: SCLP interpretation
6084fa0 s390x: protvirt: Move STSI data over SIDAD
12a2848 s390x: Add SIDA memory ops
f051c5a s390x: protvirt: KVM intercept changes
4a13191 s390x: protvirt: Inhibit balloon when switching to protected mode
d1afa39 s390x: protvirt: Add migration blocker
2b79f0e s390x: protvirt: Support unpack facility
cddc155 Sync pv
481c64f s390x: Move diagnose 308 subcodes and rcs into ipl.h
=== OUTPUT BEGIN ===
1/16 Checking commit 481c64f251a0 (s390x: Move diagnose 308 subcodes and rcs
into ipl.h)
2/16 Checking commit cddc15575b09 (Sync pv)
3/16 Checking commit 2b79f0e44dc7 (s390x: protvirt: Support unpack facility)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#326:
new file mode 100644
WARNING: line over 80 characters
#664: FILE: include/hw/s390x/pv.h:48:
+static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) {
return 0; }
ERROR: line over 90 characters
#665: FILE: include/hw/s390x/pv.h:49:
+static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
{ return 0; }
WARNING: line over 80 characters
#765: FILE: target/s390x/diag.c:122:
+valid = subcode == DIAG308_PV_SET ? iplb_valid_pv(iplb) :
iplb_valid(iplb);
total: 1 errors, 3 warnings, 731 lines checked
Patch 3/16 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/16 Checking commit d1afa39ebf53 (s390x: protvirt: Add migration blocker)
5/16 Checking commit 4a13191b1551 (s390x: protvirt: Inhibit balloon when
switching to protected mode)
6/16 Checking commit f051c5a6118f (s390x: protvirt: KVM intercept changes)
ERROR: switch and case should be at the same indent
#50: FILE: target/s390x/kvm.c:1701:
switch (icpt_code) {
[...]
+case ICPT_PV_INSTR:
+case ICPT_PV_INSTR_NOTIFICATION:
total: 1 errors, 0 warnings, 16 lines checked
Patch 6/16 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/16 Checking commit 12a284827066 (s390x: Add SIDA memory ops)
8/16 Checking commit 6084fa081454 (s390x: protvirt: Move STSI data over SIDAD)
9/16 Checking commit 7214ecde2623 (s390x: protvirt: SCLP interpretation)
10/16 Checking commit ca7e150313c9 (s390x: protvirt: Set guest IPL PSW)
11/16 Checking commit 94300597bcf9 (s390x: protvirt: Move diag 308 data over
SIDA)
12/16 Checking commit 05aa2a2c74ca (s390x: protvirt: Disable address checks for
PV guest IO emulation)
13/16 Checking commit 243e681cda71 (s390x: protvirt: Move IO control structures
over SIDA)
14/16 Checking commit 4f74e2afdb60 (s390x: protvirt: Handle SIGP store status
correctly)
15/16 Checking commit 03d40c1885fc (docs: system: Add protvirt docs)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#17:
new file mode 100644
total: 0 errors, 1 warnings, 68 lines checked
Patch 15/16 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
16/16 Checking commit 3496cd05ab8a (s390x: Add unpack facility feature to GA1)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200319131921.2367-1-fran...@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[PATCH v2] hw/misc/allwinner-h3-dramc: enforce 64-bit multiply when calculating row mirror address
The allwinner_h3_dramc_map_rows function simulates row addressing behavior
when bootloader software attempts to detect the amount of available SDRAM.
Currently the line that calculates the 64-bit address of the mirrored row
uses a signed 32-bit multiply operation that in theory could result in the
upper 32-bit be all 1s. This commit ensures that the row mirror address
is calculated using only 64-bit operations.
Reported-by: Peter Maydell
Signed-off-by: Niek Linnenbank
---
hw/misc/allwinner-h3-dramc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/misc/allwinner-h3-dramc.c b/hw/misc/allwinner-h3-dramc.c
index 2b5260260e..1d37cf422c 100644
--- a/hw/misc/allwinner-h3-dramc.c
+++ b/hw/misc/allwinner-h3-dramc.c
@@ -85,8 +85,8 @@ static void allwinner_h3_dramc_map_rows(AwH3DramCtlState *s,
uint8_t row_bits,
} else if (row_bits_actual) {
/* Row bits not matching ram_size, install the rows mirror */
-hwaddr row_mirror = s->ram_addr + ((1 << (row_bits_actual +
- bank_bits)) * page_size);
+hwaddr row_mirror = s->ram_addr + ((1ULL << (row_bits_actual +
+ bank_bits)) * page_size);
memory_region_set_enabled(>row_mirror_alias, true);
memory_region_set_address(>row_mirror_alias, row_mirror);
--
2.17.1
Re: [PATCH 2/2] hw/misc/allwinner-h3-dramc: enforce 64-bit multiply when calculating row mirror address
Hi Peter,
On Sun, Mar 22, 2020 at 10:18 PM Peter Maydell
wrote:
> On Sun, 22 Mar 2020 at 20:54, Niek Linnenbank
> wrote:
> >
> > The allwinner_h3_dramc_map_rows function simulates row addressing
> behavior
> > when bootloader software attempts to detect the amount of available
> SDRAM.
> >
> > Currently the line that calculates the 64-bit address of the mirrored row
> > uses a signed 32-bit multiply operation that in theory could result in
> the
> > upper 32-bit be all 1s. This commit ensures that the row mirror address
> > is calculated using only 64-bit operations.
> >
> > Reported-by: Peter Maydell
> > Signed-off-by: Niek Linnenbank
> > ---
> > hw/misc/allwinner-h3-dramc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/misc/allwinner-h3-dramc.c b/hw/misc/allwinner-h3-dramc.c
> > index 2b5260260e..f9f05b5384 100644
> > --- a/hw/misc/allwinner-h3-dramc.c
> > +++ b/hw/misc/allwinner-h3-dramc.c
> > @@ -85,8 +85,8 @@ static void
> allwinner_h3_dramc_map_rows(AwH3DramCtlState *s, uint8_t row_bits,
> >
> > } else if (row_bits_actual) {
> > /* Row bits not matching ram_size, install the rows mirror */
> > -hwaddr row_mirror = s->ram_addr + ((1 << (row_bits_actual +
> > - bank_bits)) *
> page_size);
> > +hwaddr row_mirror = s->ram_addr + ((1UL << (row_bits_actual +
> > +bank_bits)) *
> page_size);
>
> This needs to be a "ULL" suffix... (I just sent a different email
> with the rationale).
>
Ah ofcourse, it should be ULL indeed. And I can't think of any reason why I
made this mistake.
I simply overlooked it, thanks. I'm resending this patch with the proper
ULL suffix.
Regards,
Niek
>
> thanks
> -- PMM
>
--
Niek Linnenbank
RE: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in packet_enqueue()
> -Original Message-
> From: Derek Su
> Sent: Monday, March 23, 2020 1:48 AM
> To: qemu-devel@nongnu.org
> Cc: Zhang, Chen ; lizhij...@cn.fujitsu.com;
> jasow...@redhat.com; dere...@qnap.com
> Subject: [PATCH v2 1/1] net/colo-compare.c: Fix memory leak in
> packet_enqueue()
>
> The patch is to fix the "pkt" memory leak in packet_enqueue().
> The allocated "pkt" needs to be freed if the colo compare primary or
> secondary queue is too big.
Hi Derek,
Thank you for the patch.
I re-think this issue in a big view, looks just free the pkg is not enough in
this situation.
The root cause is network is too busy to compare, So, better choice is notify
COLO frame
to do a checkpoint and clean up all the network queue. This work maybe decrease
COLO network performance but seams better than drop lots of pkg.
Thanks
Zhang Chen
>
> Signed-off-by: Derek Su
> ---
> net/colo-compare.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 7ee17f2cf8..cdd87b2aa8 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -120,6 +120,10 @@ enum {
> SECONDARY_IN,
> };
>
> +static const char *colo_mode[] = {
> +[PRIMARY_IN] = "primary",
> +[SECONDARY_IN] = "secondary",
> +};
>
> static int compare_chr_send(CompareState *s,
> const uint8_t *buf, @@ -215,6 +219,7 @@ static
> int
> packet_enqueue(CompareState *s, int mode, Connection **con)
> ConnectionKey key;
> Packet *pkt = NULL;
> Connection *conn;
> +int ret;
>
> if (mode == PRIMARY_IN) {
> pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ static int
> packet_enqueue(CompareState *s, int mode, Connection **con)
> }
>
> if (mode == PRIMARY_IN) {
> -if (!colo_insert_packet(>primary_list, pkt, >pack)) {
> -error_report("colo compare primary queue size too big,"
> - "drop packet");
> -}
> +ret = colo_insert_packet(>primary_list, pkt,
> + >pack);
> } else {
> -if (!colo_insert_packet(>secondary_list, pkt, >sack)) {
> -error_report("colo compare secondary queue size too big,"
> - "drop packet");
> -}
> +ret = colo_insert_packet(>secondary_list, pkt,
> + >sack);
> }
> +
> +if (!ret) {
> +error_report("colo compare %s queue size too big,"
> + "drop packet", colo_mode[mode]);
> +packet_destroy(pkt, NULL);
> +pkt = NULL;
> +}
> +
> *con = conn;
>
> return 0;
> --
> 2.17.1
[PULL for-5.0 0/1] Block patches
The following changes since commit 29e0855c5af62bbb0b0b6fed792e004dad92ba95: Merge remote-tracking branch 'remotes/elmarco/tags/slirp-pull-request' into staging (2020-03-22 21:00:38 +) are available in the Git repository at: https://github.com/stefanha/qemu.git tags/block-pull-request for you to fetch changes up to ff807d559205a434fd37d3343f01a0ab128bd190: aio-posix: fix io_uring with external events (2020-03-23 11:05:44 +) Pull request Stefan Hajnoczi (1): aio-posix: fix io_uring with external events util/fdmon-io_uring.c | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) -- 2.24.1
[PULL for-5.0 1/1] aio-posix: fix io_uring with external events
When external event sources are disabled fdmon-io_uring falls back to
fdmon-poll. The ->need_wait() callback needs to watch for this so it
can return true when external event sources are disabled.
It is also necessary to call ->wait() when AioHandlers have changed
because io_uring is asynchronous and we must submit new sqes.
Both of these changes to ->need_wait() together fix tests/test-aio -p
/aio/external-client, which failed with:
test-aio: tests/test-aio.c:404: test_aio_external_client: Assertion
`aio_poll(ctx, false)' failed.
Reported-by: Julia Suvorova
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Stefano Garzarella
Message-id: 20200319163559.117903-1-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
util/fdmon-io_uring.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
index 893b79b622..7e143ef515 100644
--- a/util/fdmon-io_uring.c
+++ b/util/fdmon-io_uring.c
@@ -290,7 +290,18 @@ static int fdmon_io_uring_wait(AioContext *ctx,
AioHandlerList *ready_list,
static bool fdmon_io_uring_need_wait(AioContext *ctx)
{
-return io_uring_cq_ready(>fdmon_io_uring);
+/* Have io_uring events completed? */
+if (io_uring_cq_ready(>fdmon_io_uring)) {
+return true;
+}
+
+/* Do we need to submit new io_uring sqes? */
+if (!QSLIST_EMPTY_RCU(>submit_list)) {
+return true;
+}
+
+/* Are we falling back to fdmon-poll? */
+return atomic_read(>external_disable_cnt);
}
static const FDMonOps fdmon_io_uring_ops = {
--
2.24.1
Re: [PATCH v41 01/21] target/avr: Add outward facing interfaces and core CPU logic
On 3/23/20 7:03 PM, Richard Henderson wrote: On 3/23/20 10:03 AM, Michael Rolnik wrote: Hi Philippe. It's been a while. let me think about it and get back to you. what is your concern ? We are using this series with Joaquin for a Google Summit of Code project, so we are noticing some bugs and fixing them. As it has not been merged, we work in a fork. Since it was posted on the list, I prefer to ask on the list than directly to you. It shouldn't be there. See commit 1f5c00cfdb81. Ah it has been moved to cpu_common_reset, thanks :) I suppose it is because this port is based on some quite old work. > + memset(env->r, 0, sizeof(env->r)); > + > + tlb_flush(cs); Why are you calling tlb_flush() here? r~
Re: [PATCH v15 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
* Alex Williamson (alex.william...@redhat.com) wrote:
> On Mon, 23 Mar 2020 23:24:37 +0530
> Kirti Wankhede wrote:
>
> > On 3/21/2020 12:29 AM, Alex Williamson wrote:
> > > On Sat, 21 Mar 2020 00:12:04 +0530
> > > Kirti Wankhede wrote:
> > >
> > >> On 3/20/2020 11:31 PM, Alex Williamson wrote:
> > >>> On Fri, 20 Mar 2020 23:19:14 +0530
> > >>> Kirti Wankhede wrote:
> > >>>
> > On 3/20/2020 4:27 AM, Alex Williamson wrote:
> > > On Fri, 20 Mar 2020 01:46:41 +0530
> > > Kirti Wankhede wrote:
> > >
> > >>
> > >>
> > >>
> > >> +static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu,
> > >> dma_addr_t iova,
> > >> + size_t size, uint64_t pgsize,
> > >> + u64 __user *bitmap)
> > >> +{
> > >> +struct vfio_dma *dma;
> > >> +unsigned long pgshift = __ffs(pgsize);
> > >> +unsigned int npages, bitmap_size;
> > >> +
> > >> +dma = vfio_find_dma(iommu, iova, 1);
> > >> +
> > >> +if (!dma)
> > >> +return -EINVAL;
> > >> +
> > >> +if (dma->iova != iova || dma->size != size)
> > >> +return -EINVAL;
> > >> +
> > >> +npages = dma->size >> pgshift;
> > >> +bitmap_size = DIRTY_BITMAP_BYTES(npages);
> > >> +
> > >> +/* mark all pages dirty if all pages are pinned and mapped. */
> > >> +if (dma->iommu_mapped)
> > >> +bitmap_set(dma->bitmap, 0, npages);
> > >> +
> > >> +if (copy_to_user((void __user *)bitmap, dma->bitmap,
> > >> bitmap_size))
> > >> +return -EFAULT;
> > >
> > > We still need to reset the bitmap here, clearing and re-adding the
> > > pages that are still pinned.
> > >
> > > https://lore.kernel.org/kvm/20200319070635.2ff5d...@x1.home/
> > >
> >
> > I thought you agreed on my reply to it
> > https://lore.kernel.org/kvm/31621b70-02a9-2ea5-045f-f72b671fe...@nvidia.com/
> >
> > > Why re-populate when there will be no change since
> > > vfio_iova_dirty_bitmap() is called holding iommu->lock? If there
> > is any
> > > pin request while vfio_iova_dirty_bitmap() is still working, it
> > will
> > > wait till iommu->lock is released. Bitmap will be populated when
> > page is
> > > pinned.
> > >>>
> > >>> As coded, dirty bits are only ever set in the bitmap, never cleared.
> > >>> If a page is unpinned between iterations of the user recording the
> > >>> dirty bitmap, it should be marked dirty in the iteration immediately
> > >>> after the unpinning and not marked dirty in the following iteration.
> > >>> That doesn't happen here. We're reporting cumulative dirty pages since
> > >>> logging was enabled, we need to be reporting dirty pages since the user
> > >>> last retrieved the dirty bitmap. The bitmap should be cleared and
> > >>> currently pinned pages re-added after copying to the user. Thanks,
> > >>>
> > >>
> > >> Does that mean, we have to track every iteration? do we really need that
> > >> tracking?
> > >>
> > >> Generally the flow is:
> > >> - vendor driver pin x pages
> > >> - Enter pre-copy-phase where vCPUs are running - user starts dirty pages
> > >> tracking, then user asks dirty bitmap, x pages reported dirty by
> > >> VFIO_IOMMU_DIRTY_PAGES ioctl with _GET flag
> > >> - In pre-copy phase, vendor driver pins y more pages, now bitmap
> > >> consists of x+y bits set
> > >> - In pre-copy phase, vendor driver unpins z pages, but bitmap is not
> > >> updated, so again bitmap consists of x+y bits set.
> > >> - Enter in stop-and-copy phase, vCPUs are stopped, mdev devices are
> > >> stopped
> > >> - user asks dirty bitmap - Since here vCPU and mdev devices are stopped,
> > >> pages should not get dirty by guest driver or the physical device.
> > >> Hence, x+y dirty pages would be reported.
> > >>
> > >> I don't think we need to track every iteration of bitmap reporting.
> > >
> > > Yes, once a bitmap is read, it's reset. In your example, after
> > > unpinning z pages the user should still see a bitmap with x+y pages,
> > > but once they've read that bitmap, the next bitmap should be x+y-z.
> > > Userspace can make decisions about when to switch from pre-copy to
> > > stop-and-copy based on convergence, ie. the slope of the line recording
> > > dirty pages per iteration. The implementation here never allows an
> > > inflection point, dirty pages reported through vfio would always either
> > > be flat or climbing. There might also be a case that an iommu backed
> > > device could start pinning pages during the course of a migration, how
> > > would the bitmap ever revert from fully populated to only tracking the
> > > pinned pages? Thanks,
> > >
> >
> > At KVM forum we discussed this - if guest driver pins say 1024 pages
> > before migration starts, during pre-copy phase
Re: [PATCH-for-5.0 v2 05/11] hw/ide/sii3112: Remove dead assignment
On 3/21/20 10:41 AM, Philippe Mathieu-Daudé wrote:
> Fix warning reported by Clang static code analyzer:
>
> CC hw/ide/sii3112.o
> hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
> val = 0;
> ^ ~
>
> Fixes: a9dd6604
> Reported-by: Clang Static Analyzer
> Reviewed-by: BALATON Zoltan
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> v2: Fix the correct function (Aleksandar review)
> ---
> hw/ide/sii3112.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> index 06605d7af2..b2ff6dd6d9 100644
> --- a/hw/ide/sii3112.c
> +++ b/hw/ide/sii3112.c
> @@ -42,7 +42,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
> unsigned int size)
> {
> SiI3112PCIState *d = opaque;
> -uint64_t val = 0;
> +uint64_t val;
>
> switch (addr) {
> case 0x00:
> @@ -126,6 +126,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr
> addr,
> break;
> default:
> val = 0;
> +break;
> }
> trace_sii3112_read(size, addr, val);
> return val;
> @@ -201,7 +202,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
> d->regs[1].sien = (val >> 16) & 0x3eed;
> break;
> default:
> -val = 0;
> +break;
> }
> }
>
>
Acked-by: John Snow
Re: [PATCH v15 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
On Mon, 23 Mar 2020 23:24:37 +0530
Kirti Wankhede wrote:
> On 3/21/2020 12:29 AM, Alex Williamson wrote:
> > On Sat, 21 Mar 2020 00:12:04 +0530
> > Kirti Wankhede wrote:
> >
> >> On 3/20/2020 11:31 PM, Alex Williamson wrote:
> >>> On Fri, 20 Mar 2020 23:19:14 +0530
> >>> Kirti Wankhede wrote:
> >>>
> On 3/20/2020 4:27 AM, Alex Williamson wrote:
> > On Fri, 20 Mar 2020 01:46:41 +0530
> > Kirti Wankhede wrote:
> >
> >>
> >>
> >>
> >> +static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu,
> >> dma_addr_t iova,
> >> +size_t size, uint64_t pgsize,
> >> +u64 __user *bitmap)
> >> +{
> >> + struct vfio_dma *dma;
> >> + unsigned long pgshift = __ffs(pgsize);
> >> + unsigned int npages, bitmap_size;
> >> +
> >> + dma = vfio_find_dma(iommu, iova, 1);
> >> +
> >> + if (!dma)
> >> + return -EINVAL;
> >> +
> >> + if (dma->iova != iova || dma->size != size)
> >> + return -EINVAL;
> >> +
> >> + npages = dma->size >> pgshift;
> >> + bitmap_size = DIRTY_BITMAP_BYTES(npages);
> >> +
> >> + /* mark all pages dirty if all pages are pinned and mapped. */
> >> + if (dma->iommu_mapped)
> >> + bitmap_set(dma->bitmap, 0, npages);
> >> +
> >> + if (copy_to_user((void __user *)bitmap, dma->bitmap,
> >> bitmap_size))
> >> + return -EFAULT;
> >
> > We still need to reset the bitmap here, clearing and re-adding the
> > pages that are still pinned.
> >
> > https://lore.kernel.org/kvm/20200319070635.2ff5d...@x1.home/
> >
>
> I thought you agreed on my reply to it
> https://lore.kernel.org/kvm/31621b70-02a9-2ea5-045f-f72b671fe...@nvidia.com/
>
> > Why re-populate when there will be no change since
> > vfio_iova_dirty_bitmap() is called holding iommu->lock? If there is
> any
> > pin request while vfio_iova_dirty_bitmap() is still working, it will
> > wait till iommu->lock is released. Bitmap will be populated when
> page is
> > pinned.
> >>>
> >>> As coded, dirty bits are only ever set in the bitmap, never cleared.
> >>> If a page is unpinned between iterations of the user recording the
> >>> dirty bitmap, it should be marked dirty in the iteration immediately
> >>> after the unpinning and not marked dirty in the following iteration.
> >>> That doesn't happen here. We're reporting cumulative dirty pages since
> >>> logging was enabled, we need to be reporting dirty pages since the user
> >>> last retrieved the dirty bitmap. The bitmap should be cleared and
> >>> currently pinned pages re-added after copying to the user. Thanks,
> >>>
> >>
> >> Does that mean, we have to track every iteration? do we really need that
> >> tracking?
> >>
> >> Generally the flow is:
> >> - vendor driver pin x pages
> >> - Enter pre-copy-phase where vCPUs are running - user starts dirty pages
> >> tracking, then user asks dirty bitmap, x pages reported dirty by
> >> VFIO_IOMMU_DIRTY_PAGES ioctl with _GET flag
> >> - In pre-copy phase, vendor driver pins y more pages, now bitmap
> >> consists of x+y bits set
> >> - In pre-copy phase, vendor driver unpins z pages, but bitmap is not
> >> updated, so again bitmap consists of x+y bits set.
> >> - Enter in stop-and-copy phase, vCPUs are stopped, mdev devices are stopped
> >> - user asks dirty bitmap - Since here vCPU and mdev devices are stopped,
> >> pages should not get dirty by guest driver or the physical device.
> >> Hence, x+y dirty pages would be reported.
> >>
> >> I don't think we need to track every iteration of bitmap reporting.
> >
> > Yes, once a bitmap is read, it's reset. In your example, after
> > unpinning z pages the user should still see a bitmap with x+y pages,
> > but once they've read that bitmap, the next bitmap should be x+y-z.
> > Userspace can make decisions about when to switch from pre-copy to
> > stop-and-copy based on convergence, ie. the slope of the line recording
> > dirty pages per iteration. The implementation here never allows an
> > inflection point, dirty pages reported through vfio would always either
> > be flat or climbing. There might also be a case that an iommu backed
> > device could start pinning pages during the course of a migration, how
> > would the bitmap ever revert from fully populated to only tracking the
> > pinned pages? Thanks,
> >
>
> At KVM forum we discussed this - if guest driver pins say 1024 pages
> before migration starts, during pre-copy phase device can dirty 0 pages
> in best case and 1024 pages in worst case. In that case, user will
> transfer content of 1024 pages during pre-copy phase and in
> stop-and-copy phase also, that will be pages will be copied twice. So we
> decided to only
Re: [PATCH] ide/sii3112: Avoid leaking irqs array
On 3/23/20 10:32 AM, BALATON Zoltan wrote: > Coverity CID 1421984 reports a leak in allocated irqs, this patch > attempts to plug that. > > Signed-off-by: BALATON Zoltan Dequeueing in favor of Peter Maydell's patch. (Let me know if you feel that is a mistake.)
Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
On 3/23/20 1:04 PM, BALATON Zoltan wrote:
> On Mon, 23 Mar 2020, Peter Maydell wrote:
>> Coverity points out (CID 1421984) that we are leaking the
>> memory returned by qemu_allocate_irqs(). We can avoid this
>> leak by switching to using qdev_init_gpio_in(); the base
>> class finalize will free the irqs that this allocates under
>> the hood.
>>
>> Signed-off-by: Peter Maydell
>> ---
>> This is how the 'use qdev gpio' approach to fixing the leak looks.
>> Disclaimer: I have only tested this with "make check", nothing more.
>>
>> hw/ide/sii3112.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 06605d7af2b..2ae6f5d9df6 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev,
>> Error **errp)
>> {
>> SiI3112PCIState *d = SII3112_PCI(dev);
>> PCIIDEState *s = PCI_IDE(dev);
>> + DeviceState *ds = DEVICE(dev);
>> MemoryRegion *mr;
>> - qemu_irq *irq;
>> int i;
>>
>> pci_config_set_interrupt_pin(dev->config, 1);
>> @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev,
>> Error **errp)
>> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", >mmio,
>> 0, 16);
>> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>
>> - irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
>> + qdev_init_gpio_in(ds, sii3112_set_irq, 2);
>> for (i = 0; i < 2; i++) {
>> ide_bus_new(>bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
>> - ide_init2(>bus[i], irq[i]);
>> + ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
>
> Maybe we could just use DEVICE(dev) here and above as well just like in
> ide_bus_new above just to keep it consistent and avoid the confusion
> caused by having dev, d, s and now also ds. DEVICE(dev) is short and
> clear enough I think.
>
> Regards,
> BALATON Zoltan
>
Reviewed-by: John Snow
The named temporary is fine. We probably should be using a named
temporary in the other locations, too.
I will run my usual tests, but admit I don't really test the non-x86
boards directly. Do you want to give a tested-by on this, if it matters
to you? Otherwise, I'm fairly content to trust Peter's judgment here.
--js
Re: [PATCH v4 2/2] target/arm: kvm: Handle potential issue with dabt injection
On 3/23/20 4:32 AM, Beata Michalska wrote: > uint8_t ext_dabt_pending; /* Request for injecting ext DABT */ > +uint8_t ext_dabt_raised; /* Tracking/verifying injection of ext DABT */ Is there a reason these are uint8_t and not bool? r~
[PATCH-for-5.0 v3] tests/migration: Reduce autoconverge initial bandwidth
When using max-bandwidth=~100Mb/s, this test fails on Travis-CI s390x when configured with --disable-tcg: $ make check-qtest TESTcheck-qtest-s390x: tests/qtest/boot-serial-test qemu-system-s390x: -accel tcg: invalid accelerator tcg qemu-system-s390x: falling back to KVM TESTcheck-qtest-s390x: tests/qtest/pxe-test TESTcheck-qtest-s390x: tests/qtest/test-netfilter TESTcheck-qtest-s390x: tests/qtest/test-filter-mirror TESTcheck-qtest-s390x: tests/qtest/test-filter-redirector TESTcheck-qtest-s390x: tests/qtest/drive_del-test TESTcheck-qtest-s390x: tests/qtest/device-plug-test TESTcheck-qtest-s390x: tests/qtest/virtio-ccw-test TESTcheck-qtest-s390x: tests/qtest/cpu-plug-test TESTcheck-qtest-s390x: tests/qtest/migration-test ** ERROR:tests/qtest/migration-test.c:1229:test_migrate_auto_converge: 'got_stop' should be FALSE ERROR - Bail out! ERROR:tests/qtest/migration-test.c:1229:test_migrate_auto_converge: 'got_stop' should be FALSE make: *** [tests/Makefile.include:633: check-qtest-s390x] Error 1 Per David Gilbert, "it could just be the writing is slow on s390 and the migration thread fast; in which case the autocomplete wouldn't be needed. Perhaps we just need to reduce the bandwidth limit." Tuning the threshold by reducing the initial bandwidth makes the autoconverge test pass. Suggested-by: Dr. David Alan Gilbert Signed-off-by: Philippe Mathieu-Daudé --- v3: really reduce =) --- tests/qtest/migration-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 3d6cc83b88..2568c9529c 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -1211,7 +1211,7 @@ static void test_migrate_auto_converge(void) * without throttling. */ migrate_set_parameter_int(from, "downtime-limit", 1); -migrate_set_parameter_int(from, "max-bandwidth", 1); /* ~100Mb/s */ +migrate_set_parameter_int(from, "max-bandwidth", 100); /* ~1Mb/s */ /* To check remaining size after precopy */ migrate_set_capability(from, "pause-before-switchover", true); -- 2.21.1
Re: [PATCH v2] target/i386: Add ARCH_CAPABILITIES related bits into Icelake-Server CPU model
On Mon, Mar 23, 2020 at 10:58:16AM +0800, Xiaoyao Li wrote:
> On 3/23/2020 10:32 AM, Tao Xu wrote:
> > Hi Xiaoyao,
> >
> > May be you can add .note for this new version.
> >
> > for example:
> >
> > + .version = 3,
> > + .note = "ARCH_CAPABILITIES",
> > + .props = (PropValue[]) {
>
> Hi Paolo and Eduardo,
>
> Need I spin a new version to add the .note ?
> Maybe you can add it when queue?
Please send a follow up patch so we don't hold a bug fix because
of something that's just cosmetic. I will queue this patch. We
still need a new version of "target/i386: Add notes for versioned
CPU models"[1], don't we?
[1] https://lore.kernel.org/qemu-devel/20200228215253.gb494...@habkost.net/
--
Eduardo
How do I use patchew to be a good block-devel citizen? (was: Re: [PATCH] iotests/026: Move v3-exclusive test to new file)
On 3/23/20 8:23 AM, Max Reitz wrote:
> On 12.03.20 23:19, John Snow wrote:
>>
>>
>> On 3/11/20 10:07 AM, Max Reitz wrote:
>>> data_file does not work with v2, and we probably want 026 to keep
>>> working for v2 images. Thus, open a new file for v3-exclusive error
>>> path test cases.
>>>
>>> Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
>>>(“iotests/026: Test EIO on allocation in a data-file”)
>>> Signed-off-by: Max Reitz
>>
>> Let me start this reply with something good, or at least something
>> that's not bad. It's value neutral at worst.
>>
>> Reviewed-by: John Snow
>> Tested-by: John Snow
>
> Thanks. :)
>
>> Now, let's get cracking on some prime nonsense.
>>
>> I assume this patch is still 'pending'. Here's a complete tangent
>> unrelated to your patch in every single way:
>
> Reasonable, but it’s a bit of a shame you bury it here. I feel like
> this makes it unlikely to reach the people you want to reach.
>
I'm not sure if I was trying to reach anyone, exactly, but maybe I
should have been. Let's recirculate this to a wider audience.
So, directly below, find the $subject question:
>> What's the best way to use patchew to see series that are "pending" in
>> some way? I'd like to:
>>
>> - Search only the block list (to:qemu-bl...@nongnu.org. I assume this
>> catches CCs too.)
>> - Exclude series that are merged (-is:merged)
>> - Exclude obsoleted series (-is:obsolete)
>>
>> This gets a bit closer to things that are interesting in some way --
>> give or take some fuzziness with patchew's detection of "merged" or
>> "obsoleted" sometimes.
>>
>> - Exclude pull requests. (-is:pull seems broken, actually.)
>> - Exclude reviewed series (-is:reviewed -- what does patchew consider
>> 'reviewed'? does this mean fully reviewed, or any reviews?)
>>
>> This gives me something a bit more useful.
>>
>> - Exclude 'expired' series. I use 30 days as a mental model for this. It
>> might be nice to formalize this and mark patches that received no
>> replies and didn't detect any other state change as "expired" and send
>> an autoreply from the bot.
>>
>> (I.e., patches that are complete, applied, passed CI, were not
>> obsoleted, did not appear to be merged, and received no replies from
>> anyone except the patch author)
>>
>>
>> ("Hi, this patch received no replies from anyone except the author (you)
>> for 30 days. The series is being dropped from the pending queue and is
>> being marked expired. If the patches are still important, please rebase
>> them and re-send to the list.
>>
>> Please use scripts/get_maintainers.pl to identify candidate maintainers
>> and reviewers and make sure they are CC'd.
>>
>> This series appears to touch files owned by the following maintainers:
>> - Blah
>> - Etc
>> - And so on
>>
>> For more information on the contribution process, please visit:
>> ")
>>
>> We don't have anything like that, so age:<30d suffices. Alright, this
>> list is starting to look *pretty* decent.
>>
>> project:QEMU to:qemu-bl...@nongnu.org not:obsolete not:merged
>> -is:reviewed age:<30d
>>
>> Lastly, maybe we can exclude series that don't have replies yet.
>
> Maybe Patchew should ping these after 14 days or so.
>
> That’s about the only thing I can contribute, because I don’t really use
> Patchew myself... I keep patches in a subfolder of my inbox on unread;
> and I keep my own pending series in a separate folder. (Before I did
> that, I was much more prone to forgetting my own patches rather than
> someone else’s.)
>
> Max
>
>> It's
>> not clear to patchew which replies are:
>>
>> - Unrelated comments, like this one here
>> - Requests for a change
>> - A question for the submitter
>> - A softly-worded N-A-C-K
>>
>> and without a concept of designated reviewer, perhaps lack of replies is
>> good evidence that the series is untouched and needs someone to 'pick it
>> up'; (-has:replies)
>>
>> https://patchew.org/search?q=project%3AQEMU+to%3Aqemu-block%40nongnu.org+not%3Aobsolete+not%3Amerged+-is%3Areviewed+age%3A%3C30d+-has%3Areplies
>>
>> Alright, that's pretty good, actually.
>>
>> OK, yes, this patch still needs love as far as patchew understands.
>>
[Snip: no longer relevant to the topic.]
Re: [PATCH v2] target/i386: set the CPUID level to 0x14 on old machine-type
On Fri, Mar 13, 2020 at 12:48:06AM +0800, Luwei Kang wrote: > The CPUID level need to be set to 0x14 manually on old > machine-type if Intel PT is enabled in guest. E.g. the > CPUID[0].EAX(level)=7 and CPUID[7].EBX[25](intel-pt)=1 when the > Qemu with "-machine pc-i440fx-3.1 -cpu qemu64,+intel-pt" parameter. > > Some Intel PT capabilities are exposed by leaf 0x14 and the > missing capabilities will cause some MSRs access failed. > This patch add a warning message to inform the user to extend > the CPUID level. > > Suggested-by: Eduardo Habkost > Signed-off-by: Luwei Kang Queued, thanks. -- Eduardo
Re: [PATCH 3/3] iotests: Increase pause_wait() timeout
On 3/20/20 6:21 AM, Philippe Mathieu-Daudé wrote:
> On 3/13/20 9:36 AM, Kevin Wolf wrote:
>> Waiting for only 1 second proved to be too short on a loaded system,
>> resulting in false positives when testing pull requests. Increase the
>> timeout a bit to make this less likely.
>>
>> Signed-off-by: Kevin Wolf
>> ---
>> tests/qemu-iotests/iotests.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index b859c303a2..7bc4934cd2 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -925,7 +925,7 @@ class QMPTestCase(unittest.TestCase):
>> self.assert_qmp(event, 'data/type', 'mirror')
>> def pause_wait(self, job_id='job0'):
>> - with Timeout(1, "Timeout waiting for job to pause"):
>> + with Timeout(3, "Timeout waiting for job to pause"):
>> while True:
>> result = self.vm.qmp('query-block-jobs')
>> found = False
>>
>
> I wonder if this might be more accurate:
>
> load_timeout = math.ceil(os.getloadavg()[0])
> with Timeout(1 + load_timeout, "Timeout waiting for job to pause"):
> ...
>
> Anyhow:
> Reviewed-by: Philippe Mathieu-Daudé
>
They're just tests ... and this is just a worst-case timeout. I don't
think we need to get too cute with it. This only affects cases where the
test (or the binary) is broken and we have to wait without getting a
response for 3 seconds before being able to declare that the test is
indeed broken.
Optimizing this waiting time is probably not helpful; as when the test
is passing we'll never see this delay.
--js
Re: [PATCH v1 11/11] .travis.yml: Add a KVM-only s390x job
On 3/23/20 9:15 AM, Alex Bennée wrote: > From: Philippe Mathieu-Daudé > > Add a job to build QEMU on s390x with TCG disabled, so > this configuration won't bitrot over time. > > This job is quick, running check-unit: Ran for 5 min 30 sec > https://travis-ci.org/github/philmd/qemu/jobs/665456423 > > Acked-by: Cornelia Huck > Signed-off-by: Philippe Mathieu-Daudé > Signed-off-by: Alex Bennée > Message-Id: <20200322154015.25358-1-phi...@redhat.com> > --- > .travis.yml | 42 ++ > 1 file changed, 42 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v1 10/11] tests/docker: Add libepoxy and libudev packages to the Fedora image
On 3/23/20 9:15 AM, Alex Bennée wrote: > From: Philippe Mathieu-Daudé > > Install optional dependencies of QEMU to get better coverage. > > Suggested-by: Peter Maydell > Signed-off-by: Philippe Mathieu-Daudé > Signed-off-by: Alex Bennée > Message-Id: <20200322120104.21267-5-phi...@redhat.com> > --- > tests/docker/dockerfiles/fedora.docker | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v1 08/11] tests/docker: Install gcrypt devel package in Debian image
On 3/23/20 9:15 AM, Alex Bennée wrote: > From: Philippe Mathieu-Daudé > > In commit 6f8bbb374be we enabled building with the gcrypt library > on the the Debian 'x86 host', which was based on Debian Stretch. > Later in commit 698a71edbed we upgraded the Debian base image to > Buster. > > Apparently Debian Stretch was listing gcrypt as a QEMU dependency, > but this is not the case anymore in Buster, so we need to install > it manually (it it not listed by 'apt-get -s build-dep qemu' in > the common debian10.docker anymore). This fixes: > > $ ../configure $QEMU_CONFIGURE_OPTS > > ERROR: User requested feature gcrypt > configure was not able to find it. > Install gcrypt devel >= 1.5.0 > > Signed-off-by: Philippe Mathieu-Daudé > Signed-off-by: Alex Bennée > Message-Id: <20200322120104.21267-3-phi...@redhat.com> > --- Reviewed-by: Richard Henderson r~
Re: [PATCH v1 09/11] tests/docker: Use Python3 PyYAML in the Fedora image
On 3/23/20 9:15 AM, Alex Bennée wrote: > From: Philippe Mathieu-Daudé > > The Python2 PyYAML is now pointless, switch to the Python3 version. > > Fixes: bcbf27947 (docker: move tests from python2 to python3) > Signed-off-by: Philippe Mathieu-Daudé > Signed-off-by: Alex Bennée > Message-Id: <20200322120104.21267-4-phi...@redhat.com> > --- > tests/docker/dockerfiles/fedora.docker | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v1 07/11] tests/docker: Keep package list sorted
On 3/23/20 9:15 AM, Alex Bennée wrote: > From: Philippe Mathieu-Daudé > > Keep package list sorted, this eases rebase/cherry-pick. > > Fixes: 3a6784813 > Signed-off-by: Philippe Mathieu-Daudé > Signed-off-by: Alex Bennée > Message-Id: <20200322120104.21267-2-phi...@redhat.com> > --- > tests/docker/dockerfiles/centos7.docker | 6 -- > tests/docker/dockerfiles/fedora.docker | 6 -- > 2 files changed, 8 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v1 06/11] configure: disable MTTCG for MIPS guests
On 3/23/20 9:15 AM, Alex Bennée wrote: > While debugging check-acceptance failures I found an instability in > the mips64el test case. Briefly the test case: > > retry.py -n 100 -c -- ./mips64el-softmmu/qemu-system-mips64el \ > -display none -vga none -serial mon:stdio \ > -machine malta -kernel ./vmlinux-4.7.0-rc1.I6400 \ > -cpu I6400 -smp 8 -vga std \ > -append "printk.time=0 clocksource=GIC console=tty0 console=ttyS0 > panic=-1" \ > --no-reboot > > Reports about a 9% failure rate: > > Results summary: > 0: 91 times (91.00%), avg time 5.547 (0.45 varience/0.67 deviation) > -6: 9 times (9.00%), avg time 3.394 (0.02 varience/0.13 deviation) > Ran command 100 times, 91 passes > > When re-run with "--accel tcg,thread=single" the instability goes > away. > > Results summary: > 0: 100 times (100.00%), avg time 17.318 (249.76 varience/15.80 deviation) > Ran command 100 times, 100 passes > > Which seems to indicate there is some aspect of the MIPS MTTCG fixes > that has been missed. Ideally we would fix that but I'm afraid I don't > have time to investigate and am not super familiar with the > architecture anyway. In lieu of someone tracking down the failure lets > disable it for now. > > Signed-off-by: Alex Bennée > Acked-by: Philippe Mathieu-Daudé > Cc: Aleksandar Markovic > Cc: Aurelien Jarno > Cc: Aleksandar Rikalo Acked-by: Richard Henderson r~
Re: aio-context question
> If it doesn't have any effect because it just does what will be done > later anyway, it can be removed, but that doesn't buy us much. I think that code is simply unnecessary (no effect). But it is quite hard to read and understand, so I think removing that code helps to simplify things. > If it results in preventing some case (like the one fixed by 30dd65f3), > we need to check whether this case is actually safe. If it is safe, we > can remove the lines and get a new feature from it. > > In both cases, I think a test case should be written together with the > removal of the code. And if we find out that it's unsafe, we should even > more write a test case that makes sure that the operation fails. Yes, a test case would be great. Especially one that test backups with io-threads. I am hunting a bug for more than a week now. Seems bdrv_drain hangs sometimes when running a backup job on drives using io-threads ... But I still haven't found a reliable way to reproduce it. Sometimes takes me several hours to trigger it again... I will try to write a test case if I can really find that bug...
Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine
Hi On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf wrote: > > Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben: > > Kevin Wolf writes: > > > > > This moves the QMP dispatcher to a coroutine and runs all QMP command > > > handlers that declare 'coroutine': true in coroutine context so they > > > can avoid blocking the main loop while doing I/O or waiting for other > > > events. > > > > > > For commands that are not declared safe to run in a coroutine, the > > > dispatcher drops out of coroutine context by calling the QMP command > > > handler from a bottom half. > > > > > > Signed-off-by: Kevin Wolf > > > > Uh, what about @cur_mon? > > > > @cur_mon points to the current monitor while a command executes. > > Initial value is null. It is set in three places (not counting unit > > tests), and all three save, set, do something that may use @cur_mon, > > restore: > > > > * monitor_qmp_dispatch(), for use within qmp_dispatch() > > * monitor_read(), for use within handle_hmp_command() > > * qmp_human_monitor_command(), also for use within handle_hmp_command() > > > > Therefore, @cur_mon is null unless we're running within qmp_dispatch() > > or handle_hmp_command(). > > Can we make it NULL for coroutine-enabled handlers? fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp handlers that use monitor_get_fd() usually depend on cur_mon. Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist. > > > Example of use: error_report() & friends print "to current monitor if we > > have one, else to stderr." Makes sharing code between HMP and CLI > > easier. Uses @cur_mon under the hood. > > error_report() eventually prints to stderr both for cur_mon == NULL and > for QMP monitors. Is there an important difference between both cases? > > There is error_vprintf_unless_qmp(), which behaves differently for both > cases. However, it is only used in VNC code, so that code would have to > keep coroutines disabled. > > Is cur_mon used much in other functions called by QMP handlers? > > > @cur_mon is thread-local. > > > > I'm afraid we have to save, clear and restore @cur_mon around a yield. > > That sounds painful enough that I'd rather avoid it. > > Kevin > -- Marc-André Lureau
Re: [PATCH v41 01/21] target/avr: Add outward facing interfaces and core CPU logic
On 3/23/20 10:03 AM, Michael Rolnik wrote: > Hi Philippe. > > It's been a while. let me think about it and get back to you. what is your > concern ? It shouldn't be there. See commit 1f5c00cfdb81. > > + memset(env->r, 0, sizeof(env->r)); > > + > > + tlb_flush(cs); > > Why are you calling tlb_flush() here? r~
[Bug 1868527] Re: alignment may overlap the TLB flags
That is of course completely dependent on the target page size. So, yes, a target with a very small page size cannot use large alignments. The assert makes sure. Is this comment simply by inspection, or did you have an actual bug to report? ** Changed in: qemu Status: New => Incomplete -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1868527 Title: alignment may overlap the TLB flags Status in QEMU: Incomplete Bug description: Hi, In QEMU-4.2.0, or git-9b26a610936deaf436af9b7e39e4b7f0a35e4409, alignment may overlap the TLB flags. For example, the alignment: MO_ALIGN_32, MO_ALIGN_32 = 5 << MO_ASHIFT, and the TLB flag: TLB_DISCARD_WRITE #define TLB_DISCARD_WRITE (1 << (TARGET_PAGE_BITS_MIN - 6)) then, in the function "get_alignment_bits", the assert may fail: #if defined(CONFIG_SOFTMMU) /* The requested alignment cannot overlap the TLB flags. */ tcg_debug_assert((TLB_FLAGS_MASK & ((1 << a) - 1)) == 0); #endif However, the alignment of MO_ALIGN_32 is not used for now, so the assert cannot be triggered in current version. Anyway it seems like a potential conflict. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1868527/+subscriptions
Re: [PATCH v15 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
On 3/21/2020 12:29 AM, Alex Williamson wrote:
On Sat, 21 Mar 2020 00:12:04 +0530
Kirti Wankhede wrote:
On 3/20/2020 11:31 PM, Alex Williamson wrote:
On Fri, 20 Mar 2020 23:19:14 +0530
Kirti Wankhede wrote:
On 3/20/2020 4:27 AM, Alex Williamson wrote:
On Fri, 20 Mar 2020 01:46:41 +0530
Kirti Wankhede wrote:
+static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
+ size_t size, uint64_t pgsize,
+ u64 __user *bitmap)
+{
+ struct vfio_dma *dma;
+ unsigned long pgshift = __ffs(pgsize);
+ unsigned int npages, bitmap_size;
+
+ dma = vfio_find_dma(iommu, iova, 1);
+
+ if (!dma)
+ return -EINVAL;
+
+ if (dma->iova != iova || dma->size != size)
+ return -EINVAL;
+
+ npages = dma->size >> pgshift;
+ bitmap_size = DIRTY_BITMAP_BYTES(npages);
+
+ /* mark all pages dirty if all pages are pinned and mapped. */
+ if (dma->iommu_mapped)
+ bitmap_set(dma->bitmap, 0, npages);
+
+ if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
+ return -EFAULT;
We still need to reset the bitmap here, clearing and re-adding the
pages that are still pinned.
https://lore.kernel.org/kvm/20200319070635.2ff5d...@x1.home/
I thought you agreed on my reply to it
https://lore.kernel.org/kvm/31621b70-02a9-2ea5-045f-f72b671fe...@nvidia.com/
> Why re-populate when there will be no change since
> vfio_iova_dirty_bitmap() is called holding iommu->lock? If there is any
> pin request while vfio_iova_dirty_bitmap() is still working, it will
> wait till iommu->lock is released. Bitmap will be populated when page is
> pinned.
As coded, dirty bits are only ever set in the bitmap, never cleared.
If a page is unpinned between iterations of the user recording the
dirty bitmap, it should be marked dirty in the iteration immediately
after the unpinning and not marked dirty in the following iteration.
That doesn't happen here. We're reporting cumulative dirty pages since
logging was enabled, we need to be reporting dirty pages since the user
last retrieved the dirty bitmap. The bitmap should be cleared and
currently pinned pages re-added after copying to the user. Thanks,
Does that mean, we have to track every iteration? do we really need that
tracking?
Generally the flow is:
- vendor driver pin x pages
- Enter pre-copy-phase where vCPUs are running - user starts dirty pages
tracking, then user asks dirty bitmap, x pages reported dirty by
VFIO_IOMMU_DIRTY_PAGES ioctl with _GET flag
- In pre-copy phase, vendor driver pins y more pages, now bitmap
consists of x+y bits set
- In pre-copy phase, vendor driver unpins z pages, but bitmap is not
updated, so again bitmap consists of x+y bits set.
- Enter in stop-and-copy phase, vCPUs are stopped, mdev devices are stopped
- user asks dirty bitmap - Since here vCPU and mdev devices are stopped,
pages should not get dirty by guest driver or the physical device.
Hence, x+y dirty pages would be reported.
I don't think we need to track every iteration of bitmap reporting.
Yes, once a bitmap is read, it's reset. In your example, after
unpinning z pages the user should still see a bitmap with x+y pages,
but once they've read that bitmap, the next bitmap should be x+y-z.
Userspace can make decisions about when to switch from pre-copy to
stop-and-copy based on convergence, ie. the slope of the line recording
dirty pages per iteration. The implementation here never allows an
inflection point, dirty pages reported through vfio would always either
be flat or climbing. There might also be a case that an iommu backed
device could start pinning pages during the course of a migration, how
would the bitmap ever revert from fully populated to only tracking the
pinned pages? Thanks,
At KVM forum we discussed this - if guest driver pins say 1024 pages
before migration starts, during pre-copy phase device can dirty 0 pages
in best case and 1024 pages in worst case. In that case, user will
transfer content of 1024 pages during pre-copy phase and in
stop-and-copy phase also, that will be pages will be copied twice. So we
decided to only get dirty pages bitmap at stop-and-copy phase. If user
is going to get dirty pages in stop-and-copy phase only, then that will
be single iteration.
There aren't any devices yet that can track sys memory dirty pages. So
we can go ahead with this patch and support for dirty pages tracking
during pre-copy phase can be added later when there will be consumers of
that functionality.
Thanks,
Kirti
Re: [PATCH v5 09/60] target/riscv: vector single-width integer add and subtract
On 3/23/20 1:10 AM, LIU Zhiwei wrote:
>> static void gen_gvec_rsubi(unsigned vece, uint32_t dofs,
>> uint32_t aofs, int64_t c,
>> uint32_t oprsz, uint32_t maxsz)
>> {
>> tcg_debug_assert(vece <= MO_64);
>> tcg_gen_gvec_2i(dofs, aofs, oprsz, maxsz, c, _op[vece]);
>> }
> Hi Richard,
>
> When I try to add GVEC IR rsubs，I find it is some difficult to keep it
> separate from tcg-runtime-gvec.c.
>
> The .fno functions, e.g., gen_helper_gvec_rsubs8 need to be defined like
>
> void HELPER(gvec_subs8)(void *d, void *a, uint64_t b, uint32_t desc)
>
> {
>
> intptr_t oprsz = simd_oprsz(desc);
>
> vec8 vecb = (vec8)DUP16(b);
>
> intptr_t i;
>
> for (i = 0; i < oprsz; i += sizeof(vec8)) {
>
> *(vec8 *)(d + i) = vecb - *(vec8 *)(a + i);
>
> }
>
> clear_high(d, oprsz, desc);
>
> }
>
>
> The vec8 and DUP are defined in tcg-runtime-gvec.c.
Update your branch -- they're gone since commit 0a83e43a9ee6.
Just use normal integer types.
r~
[Bug 1866870] Re: KVM Guest pauses after upgrade to Ubuntu 20.04
This bug was fixed in the package seabios - 1.13.0-1ubuntu1
---
seabios (1.13.0-1ubuntu1) focal; urgency=medium
* d/p/lp-1866870-build-use-fcf-protection-none-when-available.patch
fix breakage on older chips due to fcf-protection (LP: #1866870)
-- Christian Ehrhardt Thu, 19 Mar
2020 13:10:10 +0100
** Changed in: seabios (Ubuntu)
Status: Triaged => Fix Released
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1866870
Title:
KVM Guest pauses after upgrade to Ubuntu 20.04
Status in QEMU:
Invalid
Status in qemu package in Ubuntu:
Invalid
Status in seabios package in Ubuntu:
Fix Released
Bug description:
Symptom:
Error unpausing domain: internal error: unable to execute QEMU command
'cont': Resetting the Virtual Machine is required
Traceback (most recent call last):
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in
cb_wrapper
callback(asyncjob, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/asyncjob.py", line 111, in tmpcb
callback(*args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/libvirtobject.py", line
66, in newfn
ret = fn(self, *args, **kwargs)
File "/usr/share/virt-manager/virtManager/object/domain.py", line 1311, in
resume
self._backend.resume()
File "/usr/lib/python3/dist-packages/libvirt.py", line 2174, in resume
if ret == -1: raise libvirtError ('virDomainResume() failed', dom=self)
libvirt.libvirtError: internal error: unable to execute QEMU command 'cont':
Resetting the Virtual Machine is required
---
As outlined here:
https://bugs.launchpad.net/qemu/+bug/1813165/comments/15
After upgrade, all KVM guests are in a default pause state. Even after
forcing them off via virsh, and restarting them the guests are paused.
These Guests are not nested.
A lot of diganostic information are outlined in the previous bug
report link provided. The solution mentioned in previous report had
been allegedly integrated into the downstream updates.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1866870/+subscriptions
[PULL 2/5] aspeed/smc: Fix DMA support for AST2600
From: Cédric Le Goater
Recent firmwares uses SPI DMA transfers in U-Boot to load the
different images (kernel, initrd, dtb) in the SoC DRAM. The AST2600
FMC model is missing the masks to be applied on the DMA registers
which resulted in incorrect values. Fix that and wire the SPI
controllers which have DMA support on the AST2600.
Fixes: bcaa8ddd081c ("aspeed/smc: Add AST2600 support")
Signed-off-by: Cédric Le Goater
Reviewed-by: Joel Stanley
Message-id: 20200320053923.20565-1-...@kaod.org
Signed-off-by: Peter Maydell
---
hw/arm/aspeed_ast2600.c | 6 ++
hw/ssi/aspeed_smc.c | 15 +--
hw/ssi/trace-events | 1 +
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 446b44d31cf..1a869e09b96 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -411,6 +411,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
/* SPI */
for (i = 0; i < sc->spis_num; i++) {
+object_property_set_link(OBJECT(>spi[i]), OBJECT(s->dram_mr),
+ "dram", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
object_property_set_int(OBJECT(>spi[i]), 1, "num-cs", );
object_property_set_bool(OBJECT(>spi[i]), true, "realized",
_err);
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 9d5c696d5a1..2edccef2d54 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -364,6 +364,8 @@ static const AspeedSMCController controllers[] = {
.flash_window_base = ASPEED26_SOC_FMC_FLASH_BASE,
.flash_window_size = 0x1000,
.has_dma = true,
+.dma_flash_mask= 0x0FFC,
+.dma_dram_mask = 0x3FFC,
.nregs = ASPEED_SMC_R_MAX,
.segment_to_reg= aspeed_2600_smc_segment_to_reg,
.reg_to_segment= aspeed_2600_smc_reg_to_segment,
@@ -379,7 +381,9 @@ static const AspeedSMCController controllers[] = {
.segments = aspeed_segments_ast2600_spi1,
.flash_window_base = ASPEED26_SOC_SPI_FLASH_BASE,
.flash_window_size = 0x1000,
-.has_dma = false,
+.has_dma = true,
+.dma_flash_mask= 0x0FFC,
+.dma_dram_mask = 0x3FFC,
.nregs = ASPEED_SMC_R_MAX,
.segment_to_reg= aspeed_2600_smc_segment_to_reg,
.reg_to_segment= aspeed_2600_smc_reg_to_segment,
@@ -395,7 +399,9 @@ static const AspeedSMCController controllers[] = {
.segments = aspeed_segments_ast2600_spi2,
.flash_window_base = ASPEED26_SOC_SPI2_FLASH_BASE,
.flash_window_size = 0x1000,
-.has_dma = false,
+.has_dma = true,
+.dma_flash_mask= 0x0FFC,
+.dma_dram_mask = 0x3FFC,
.nregs = ASPEED_SMC_R_MAX,
.segment_to_reg= aspeed_2600_smc_segment_to_reg,
.reg_to_segment= aspeed_2600_smc_reg_to_segment,
@@ -1135,6 +1141,11 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
MemTxResult result;
uint32_t data;
+trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE ?
+"write" : "read",
+s->regs[R_DMA_FLASH_ADDR],
+s->regs[R_DMA_DRAM_ADDR],
+s->regs[R_DMA_LEN]);
while (s->regs[R_DMA_LEN]) {
if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
data = address_space_ldl_le(>dram_as, s->regs[R_DMA_DRAM_ADDR],
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
index 0a70629801a..0ea498de910 100644
--- a/hw/ssi/trace-events
+++ b/hw/ssi/trace-events
@@ -6,5 +6,6 @@ aspeed_smc_do_snoop(int cs, int index, int dummies, int data)
"CS%d index:0x%x d
aspeed_smc_flash_write(int cs, uint64_t addr, uint32_t size, uint64_t data,
int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
aspeed_smc_read(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 "
size %u: 0x%" PRIx64
aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
+aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t dram_addr,
uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
aspeed_smc_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 "
size %u: 0x%" PRIx64
aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
--
2.20.1
Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine
Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > This moves the QMP dispatcher to a coroutine and runs all QMP command > > handlers that declare 'coroutine': true in coroutine context so they > > can avoid blocking the main loop while doing I/O or waiting for other > > events. > > > > For commands that are not declared safe to run in a coroutine, the > > dispatcher drops out of coroutine context by calling the QMP command > > handler from a bottom half. > > > > Signed-off-by: Kevin Wolf > > Uh, what about @cur_mon? > > @cur_mon points to the current monitor while a command executes. > Initial value is null. It is set in three places (not counting unit > tests), and all three save, set, do something that may use @cur_mon, > restore: > > * monitor_qmp_dispatch(), for use within qmp_dispatch() > * monitor_read(), for use within handle_hmp_command() > * qmp_human_monitor_command(), also for use within handle_hmp_command() > > Therefore, @cur_mon is null unless we're running within qmp_dispatch() > or handle_hmp_command(). Can we make it NULL for coroutine-enabled handlers? > Example of use: error_report() & friends print "to current monitor if we > have one, else to stderr." Makes sharing code between HMP and CLI > easier. Uses @cur_mon under the hood. error_report() eventually prints to stderr both for cur_mon == NULL and for QMP monitors. Is there an important difference between both cases? There is error_vprintf_unless_qmp(), which behaves differently for both cases. However, it is only used in VNC code, so that code would have to keep coroutines disabled. Is cur_mon used much in other functions called by QMP handlers? > @cur_mon is thread-local. > > I'm afraid we have to save, clear and restore @cur_mon around a yield. That sounds painful enough that I'd rather avoid it. Kevin
[PULL 1/5] hw/arm/bcm283x: Correct the license text
From: Philippe Mathieu-Daudé The license is the 'GNU General Public License v2.0 or later', not 'and': This program is free software; you can redistribute it and/ori modify it under the terms of the GNU General Public License as published by the Free Software Foundation; either version 2 of the License, or (at your option) any later version. Fix the license comment. Signed-off-by: Philippe Mathieu-Daudé Message-id: 20200312213455.15854-1-phi...@redhat.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- include/hw/arm/bcm2835_peripherals.h | 3 ++- include/hw/arm/bcm2836.h | 3 ++- include/hw/char/bcm2835_aux.h| 3 ++- include/hw/display/bcm2835_fb.h | 3 ++- include/hw/dma/bcm2835_dma.h | 4 +++- include/hw/intc/bcm2835_ic.h | 4 +++- include/hw/intc/bcm2836_control.h| 3 ++- include/hw/misc/bcm2835_mbox.h | 4 +++- include/hw/misc/bcm2835_mbox_defs.h | 4 +++- include/hw/misc/bcm2835_property.h | 4 +++- hw/arm/bcm2835_peripherals.c | 3 ++- hw/arm/bcm2836.c | 3 ++- hw/arm/raspi.c | 3 ++- hw/display/bcm2835_fb.c | 1 - hw/dma/bcm2835_dma.c | 4 +++- hw/intc/bcm2835_ic.c | 4 ++-- hw/intc/bcm2836_control.c| 4 +++- hw/misc/bcm2835_mbox.c | 4 +++- hw/misc/bcm2835_property.c | 4 +++- 19 files changed, 45 insertions(+), 20 deletions(-) diff --git a/include/hw/arm/bcm2835_peripherals.h b/include/hw/arm/bcm2835_peripherals.h index 7859281e11b..2e8655a7c2d 100644 --- a/include/hw/arm/bcm2835_peripherals.h +++ b/include/hw/arm/bcm2835_peripherals.h @@ -5,7 +5,8 @@ * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft * Written by Andrew Baumann * - * This code is licensed under the GNU GPLv2 and later. + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. */ #ifndef BCM2835_PERIPHERALS_H diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h index 92a6544816b..024af8aae4f 100644 --- a/include/hw/arm/bcm2836.h +++ b/include/hw/arm/bcm2836.h @@ -5,7 +5,8 @@ * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft * Written by Andrew Baumann * - * This code is licensed under the GNU GPLv2 and later. + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. */ #ifndef BCM2836_H diff --git a/include/hw/char/bcm2835_aux.h b/include/hw/char/bcm2835_aux.h index cdbf7e3e37e..934acf9c813 100644 --- a/include/hw/char/bcm2835_aux.h +++ b/include/hw/char/bcm2835_aux.h @@ -2,7 +2,8 @@ * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft * Written by Andrew Baumann * - * This code is licensed under the GNU GPLv2 and later. + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. */ #ifndef BCM2835_AUX_H diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h index 228988ba056..2246be74d83 100644 --- a/include/hw/display/bcm2835_fb.h +++ b/include/hw/display/bcm2835_fb.h @@ -5,7 +5,8 @@ * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft * Written by Andrew Baumann * - * This code is licensed under the GNU GPLv2 and later. + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. */ #ifndef BCM2835_FB_H diff --git a/include/hw/dma/bcm2835_dma.h b/include/hw/dma/bcm2835_dma.h index 91ed8d05d16..a6747842b76 100644 --- a/include/hw/dma/bcm2835_dma.h +++ b/include/hw/dma/bcm2835_dma.h @@ -1,6 +1,8 @@ /* * Raspberry Pi emulation (c) 2012 Gregory Estrade - * This code is licensed under the GNU GPLv2 and later. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. */ #ifndef BCM2835_DMA_H diff --git a/include/hw/intc/bcm2835_ic.h b/include/hw/intc/bcm2835_ic.h index fb75fa0064e..392ded1cb33 100644 --- a/include/hw/intc/bcm2835_ic.h +++ b/include/hw/intc/bcm2835_ic.h @@ -1,6 +1,8 @@ /* * Raspberry Pi emulation (c) 2012 Gregory Estrade - * This code is licensed under the GNU GPLv2 and later. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. */ #ifndef BCM2835_IC_H diff --git a/include/hw/intc/bcm2836_control.h b/include/hw/intc/bcm2836_control.h index de061b8929a..2c22405686b 100644 --- a/include/hw/intc/bcm2836_control.h +++ b/include/hw/intc/bcm2836_control.h @@ -8,7 +8,8 @@ * ARM Local Timer IRQ Copyright (c) 2019. Zoltán Baldaszti * Added basic IRQ_TIMER interrupt support * - * This code is licensed under the GNU GPLv2 and later. + * This work is licensed under the terms of the GNU GPL, version
[PULL 0/5] target-arm queue
Just a few minor bugfixes, but we might as well get them in
for rc0 tomorrow.
-- PMM
The following changes since commit 787f82407c5056a8b1097e39e53d01dd1abe406b:
Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20200323' into
staging (2020-03-23 15:38:30 +)
are available in the Git repository at:
https://git.linaro.org/people/pmaydell/qemu-arm.git
tags/pull-target-arm-20200323
for you to fetch changes up to 550a04893c2bd4442211b353680b9a6408d94dba:
target/arm: Move computation of index in handle_simd_dupe (2020-03-23
17:22:30 +)
target-arm queue:
* target/arm: avoid undefined behaviour shift in watchpoint code
* target/arm: avoid undefined behaviour shift in handle_simd_dupe()
* target/arm: add assert that immh != 0 in disas_simd_shift_imm()
* aspeed/smc: Fix DMA support for AST2600
* hw/arm/bcm283x: Correct the license text ('and' vs 'or')
Cédric Le Goater (1):
aspeed/smc: Fix DMA support for AST2600
Philippe Mathieu-Daudé (1):
hw/arm/bcm283x: Correct the license text
Richard Henderson (3):
target/arm: Rearrange disabled check for watchpoints
target/arm: Assert immh != 0 in disas_simd_shift_imm
target/arm: Move computation of index in handle_simd_dupe
include/hw/arm/bcm2835_peripherals.h | 3 ++-
include/hw/arm/bcm2836.h | 3 ++-
include/hw/char/bcm2835_aux.h| 3 ++-
include/hw/display/bcm2835_fb.h | 3 ++-
include/hw/dma/bcm2835_dma.h | 4 +++-
include/hw/intc/bcm2835_ic.h | 4 +++-
include/hw/intc/bcm2836_control.h| 3 ++-
include/hw/misc/bcm2835_mbox.h | 4 +++-
include/hw/misc/bcm2835_mbox_defs.h | 4 +++-
include/hw/misc/bcm2835_property.h | 4 +++-
hw/arm/aspeed_ast2600.c | 6 ++
hw/arm/bcm2835_peripherals.c | 3 ++-
hw/arm/bcm2836.c | 3 ++-
hw/arm/raspi.c | 3 ++-
hw/display/bcm2835_fb.c | 1 -
hw/dma/bcm2835_dma.c | 4 +++-
hw/intc/bcm2835_ic.c | 4 ++--
hw/intc/bcm2836_control.c| 4 +++-
hw/misc/bcm2835_mbox.c | 4 +++-
hw/misc/bcm2835_property.c | 4 +++-
hw/ssi/aspeed_smc.c | 15 +--
target/arm/helper.c | 11 ++-
target/arm/translate-a64.c | 6 +-
hw/ssi/trace-events | 1 +
24 files changed, 76 insertions(+), 28 deletions(-)
[PULL 3/5] target/arm: Rearrange disabled check for watchpoints
From: Richard Henderson
Coverity rightly notes that ctz32(bas) on 0 will return 32,
which makes the len calculation a BAD_SHIFT.
A value of 0 in DBGWCR_EL1.BAS is reserved. Simply move
the existing check we have for this case.
Reported-by: Coverity (CID 1421964)
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20200320160622.8040-2-richard.hender...@linaro.org
Reviewed-by: Peter Maydell
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index d2ec2c53510..b7b6887241d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6340,17 +6340,18 @@ void hw_watchpoint_update(ARMCPU *cpu, int n)
int bas = extract64(wcr, 5, 8);
int basstart;
-if (bas == 0) {
-/* This must act as if the watchpoint is disabled */
-return;
-}
-
if (extract64(wvr, 2, 1)) {
/* Deprecated case of an only 4-aligned address. BAS[7:4] are
* ignored, and BAS[3:0] define which bytes to watch.
*/
bas &= 0xf;
}
+
+if (bas == 0) {
+/* This must act as if the watchpoint is disabled */
+return;
+}
+
/* The BAS bits are supposed to be programmed to indicate a contiguous
* range of bytes. Otherwise it is CONSTRAINED UNPREDICTABLE whether
* we fire for each byte in the word/doubleword addressed by the WVR.
--
2.20.1
[PULL 4/5] target/arm: Assert immh != 0 in disas_simd_shift_imm
From: Richard Henderson
Coverity raised a shed-load of errors cascading from inferring
that clz32(immh) might yield 32, from immh might be 0.
While immh cannot be 0 from encoding, it is not obvious even to
a human how we've checked that: via the filtering provided by
data_proc_simd[].
Reported-by: Coverity (CID 1421923, and more)
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20200320160622.8040-3-richard.hender...@linaro.org
Reviewed-by: Peter Maydell
Signed-off-by: Peter Maydell
---
target/arm/translate-a64.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8fffb52203d..032478614c4 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -10405,6 +10405,9 @@ static void disas_simd_shift_imm(DisasContext *s,
uint32_t insn)
bool is_u = extract32(insn, 29, 1);
bool is_q = extract32(insn, 30, 1);
+/* data_proc_simd[] has sent immh == 0 to disas_simd_mod_imm. */
+assert(immh != 0);
+
switch (opcode) {
case 0x08: /* SRI */
if (!is_u) {
--
2.20.1
[PULL 5/5] target/arm: Move computation of index in handle_simd_dupe
From: Richard Henderson
Coverity reports a BAD_SHIFT with ctz32(imm5), with imm5 == 0.
This is an invalid encoding, but we diagnose that just below
by rejecting size > 3. Avoid the warning by sinking the
computation of index below the check.
Reported-by: Coverity (CID 1421965)
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20200320160622.8040-4-richard.hender...@linaro.org
Reviewed-by: Peter Maydell
Signed-off-by: Peter Maydell
---
target/arm/translate-a64.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 032478614c4..7580e463674 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -7422,7 +7422,7 @@ static void handle_simd_dupe(DisasContext *s, int is_q,
int rd, int rn,
int imm5)
{
int size = ctz32(imm5);
-int index = imm5 >> (size + 1);
+int index;
if (size > 3 || (size == 3 && !is_q)) {
unallocated_encoding(s);
@@ -7433,6 +7433,7 @@ static void handle_simd_dupe(DisasContext *s, int is_q,
int rd, int rn,
return;
}
+index = imm5 >> (size + 1);
tcg_gen_gvec_dup_mem(size, vec_full_reg_offset(s, rd),
vec_reg_offset(s, rn, index, size),
is_q ? 16 : 8, vec_full_reg_size(s));
--
2.20.1
Re: [PATCH] target/mips: Fix loongson multimedia condition instructions
On 3/20/20 9:56 PM, Jiaxun Yang wrote:
> case OPC_SLE_CP2:
> -/*
> - * ??? Document is unclear: Set FCC[CC]. Does that mean the
> - * FD field is the CC field?
> - */
> +cond = TCG_COND_LE;
> +do_cc_cond:
> +{
> +int cc = (ctx->opcode >> 8) & 0x7;
> +lab = gen_new_label();
> +tcg_gen_ori_i32(fpu_fcr31, fpu_fcr31, 1 << get_fp_bit(cc));
> +tcg_gen_brcond_i64(cond, t0, t1, lab);
> +tcg_gen_xori_i32(fpu_fcr31, fpu_fcr31, 1 << get_fp_bit(cc));
> +gen_set_label(lab);
> +}
> +goto no_rd;
> +break;
There is no need for a branch here. This is a deposit operation.
TCGv_i64 t64 = tcg_temp_new_i64();
TCGv_i32 t32 = tcg_temp_new_i32();
tcg_gen_setcond_i64(cond, t64, t0, t1);
tcg_gen_extrl_i64_i32(t32, t64);
tcg_gen_deposit_i32(cpu_fcr31, cpu_fcr31, t32,
get_fp_bit(cc), 1);
tcg_temp_free_i32(t32);
tcg_temp_free_i64(t64);
r~
Re: [PATCH v11 00/16] s390x: Protected Virtualization support
On Thu, 19 Mar 2020 09:19:05 -0400 Janosch Frank wrote: > Most of the QEMU changes for PV are related to the new IPL type with > subcodes 8 - 10 and the execution of the necessary Ultravisor calls to > IPL secure guests. Note that we can only boot into secure mode from > normal mode, i.e. stfle 161 is not active in secure mode. > > The other changes related to data gathering for emulation and > disabling addressing checks in secure mode, as well as CPU resets. > > v11: > * Review fixes > > v10: > * Moved documentation into subfolder > * Added huge page fencing > * Cleared up IO questions that were remaining > * Added exits/abbort/assert for conditions where we can't recover > > v9: > * Moved pv.h into include/hw/s390x/ > * Replaced cmd strings with macro > * Moved s390_is_pv() to pv.h > * Added new copyright dates and authors > v8: > * Removed the iplb_valid changes as they are picked > * Checkpatch fixes > * Review fixes > * Replaced env/ms->pv with s390_is_pv() > v7: > * Merged the diag 308 subcode patches and the unpack > * Moved the SIDA memops into the sync patch > * Bailout for the none machien and fencing of CONFIG_USER_ONLY > * Changes due to review > > v6: > * diag308 rc numbers were changed by architecture > * IPL pv block received one more reserved field by architecture > * Officially added the bios patch to the series > * Dropped picked constant rename patch > > v5: > * Moved docs into docs/system > * Some more enable/disable changes > * Moved enablement/disablement of pv in separate functions > * Some review fixes > > v4: > * Sync with KVM changes > * Review changes > > V3: > * Use dedicated functions to access SIDA > * Smaller cleanups and segfault fixes > * Error reporting for Ultravisor calls > * Inject of RC of diag308 subcode 10 fails > > V2: > * Split out cleanups > * Internal PV state tracking > * Review feedback > > Christian Borntraeger (1): > s390x: Add unpack facility feature to GA1 > > Janosch Frank (15): > s390x: Move diagnose 308 subcodes and rcs into ipl.h > Sync pv > s390x: protvirt: Support unpack facility > s390x: protvirt: Add migration blocker > s390x: protvirt: Inhibit balloon when switching to protected mode > s390x: protvirt: KVM intercept changes > s390x: Add SIDA memory ops > s390x: protvirt: Move STSI data over SIDAD > s390x: protvirt: SCLP interpretation > s390x: protvirt: Set guest IPL PSW > s390x: protvirt: Move diag 308 data over SIDA > s390x: protvirt: Disable address checks for PV guest IO emulation > s390x: protvirt: Move IO control structures over SIDA > s390x: protvirt: Handle SIGP store status correctly > docs: system: Add protvirt docs > > MAINTAINERS | 2 + > docs/system/s390x/protvirt.rst | 60 +++ > docs/system/target-s390x.rst| 5 + > hw/s390x/Makefile.objs | 1 + > hw/s390x/ipl.c | 59 ++- > hw/s390x/ipl.h | 102 ++- > hw/s390x/pv.c | 98 ++ > hw/s390x/s390-virtio-ccw.c | 148 +++- > hw/s390x/sclp.c | 56 --- > include/hw/s390x/pv.h | 55 +++ > include/hw/s390x/s390-virtio-ccw.h | 1 + > include/hw/s390x/sclp.h | 2 + > linux-headers/linux/kvm.h | 45 - > target/s390x/cpu.c | 27 +++-- > target/s390x/cpu.h | 7 +- > target/s390x/cpu_features_def.inc.h | 1 + > target/s390x/diag.c | 77 +++ > target/s390x/gen-features.c | 1 + > target/s390x/helper.c | 6 ++ > target/s390x/ioinst.c | 96 +- > target/s390x/kvm-stub.c | 5 + > target/s390x/kvm.c | 79 +-- > target/s390x/kvm_s390x.h| 3 + > target/s390x/mmu_helper.c | 14 +++ > 24 files changed, 870 insertions(+), 80 deletions(-) > create mode 100644 docs/system/s390x/protvirt.rst > create mode 100644 hw/s390x/pv.c > create mode 100644 include/hw/s390x/pv.h > Thanks, queued to s390-next for 5.1 (with v12 of patch 3). Patch 2 will obviously need to be replaced by a proper headers update; I'll do that when 5.1 development opens up (I assume the kernel patches will have reached Linux master before that.) (Any further s390x patches for 5.0 will go via s390-fixes, as usual.)
Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
On Mon, 23 Mar 2020 at 17:04, BALATON Zoltan wrote:
>
> On Mon, 23 Mar 2020, Peter Maydell wrote:
> > Coverity points out (CID 1421984) that we are leaking the
> > memory returned by qemu_allocate_irqs(). We can avoid this
> > leak by switching to using qdev_init_gpio_in(); the base
> > class finalize will free the irqs that this allocates under
> > the hood.
> >
> > Signed-off-by: Peter Maydell
> > ---
> > This is how the 'use qdev gpio' approach to fixing the leak looks.
> > Disclaimer: I have only tested this with "make check", nothing more.
> >
> > hw/ide/sii3112.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> > index 06605d7af2b..2ae6f5d9df6 100644
> > --- a/hw/ide/sii3112.c
> > +++ b/hw/ide/sii3112.c
> > @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error
> > **errp)
> > {
> > SiI3112PCIState *d = SII3112_PCI(dev);
> > PCIIDEState *s = PCI_IDE(dev);
> > +DeviceState *ds = DEVICE(dev);
> > MemoryRegion *mr;
> > -qemu_irq *irq;
> > int i;
> >
> > pci_config_set_interrupt_pin(dev->config, 1);
> > @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error
> > **errp)
> > memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", >mmio, 0,
> > 16);
> > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
> >
> > -irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
> > +qdev_init_gpio_in(ds, sii3112_set_irq, 2);
> > for (i = 0; i < 2; i++) {
> > ide_bus_new(>bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
> > -ide_init2(>bus[i], irq[i]);
> > +ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
>
> Maybe we could just use DEVICE(dev) here and above as well just like in
> ide_bus_new above just to keep it consistent and avoid the confusion
> caused by having dev, d, s and now also ds. DEVICE(dev) is short and clear
> enough I think.
Usual style in these methods is to have local variables if
it's going to be used more than once or twice -- QOM casts
aren't free the way C casts are. We already do that here for
the SII3112_PCI() and the PCI_IDE().
In this case things are a bit confused by the function having
used "dev" for the PCIDevice* and 's' for the PCIIDEState.
QEMU is far from consistent in this matter, but usually 's'
is the pointer to the device's own state (ie SiI3112PCIState*
in this case) and the DeviceState* is 'dev'. The PCIDevice *
is often 'pci_dev'. I would call the PCIIDEState* 'pci_ide'.
I hadn't noticed the use of DEVICE() in ide_bus_new(); you're
right that we should be consistent about whether we use the cast
macro or the local variable.
thanks
-- PMM
