Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.
Hi Cédric, On 8/6/21 8:00 PM, Cédric Le Goater wrote: > It includes support for the POWER10 processor and the QEMU platform. 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here? > > Built from submodule. 2/ Could we have a CI job building this, during 6.2 cycle? (See .gitlab-ci.d/edk2.yml and .gitlab-ci.d/opensbi.yml) > > Signed-off-by: Cédric Le Goater > --- > pc-bios/skiboot.lid | Bin 1667280 -> 2528128 bytes > roms/skiboot| 2 +- > 2 files changed, 1 insertion(+), 1 deletion(-) > > diff --git a/pc-bios/skiboot.lid b/pc-bios/skiboot.lid > index > 504b95e8b6611aff3a934ff10f789934680591f9..8a3c278512a428a034ed5b1ddbed017ae8c0a9d0 > 100644 > GIT binary patch > literal 2528128 Consider using 'git-format-patch --no-binary' and a reference to your repository to fetch a such big binary patch. > diff --git a/roms/skiboot b/roms/skiboot > index 3a6fdede6ce1..820d43c0a775 16 > --- a/roms/skiboot > +++ b/roms/skiboot > @@ -1 +1 @@ > -Subproject commit 3a6fdede6ce117facec0108afe716cf5d0472c3f > +Subproject commit 820d43c0a7751e75a8830561f35535dfffd522bd Thanks, Phil.
[PATCH] target/riscv: Add User CSRs read-only check
Signed-off-by: LIU Zhiwei
---
target/riscv/csr.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9a4ed18ac5..ea62d9e653 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1422,11 +1422,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
csrno,
RISCVException ret;
target_ulong old_value;
RISCVCPU *cpu = env_archcpu(env);
+int read_only = get_field(csrno, 0xC00) == 3;
/* check privileges and return -1 if check fails */
#if !defined(CONFIG_USER_ONLY)
int effective_priv = env->priv;
-int read_only = get_field(csrno, 0xC00) == 3;
if (riscv_has_ext(env, RVH) &&
env->priv == PRV_S &&
@@ -1443,6 +1443,10 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
(!env->debugger && (effective_priv < get_field(csrno, 0x300 {
return RISCV_EXCP_ILLEGAL_INST;
}
+#else
+if (write_mask && read_only) {
+return -RISCV_EXCP_ILLEGAL_INST;
+}
#endif
/* ensure the CSR extension is enabled. */
--
2.17.1
Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.
On 8/9/21 5:37 AM, David Gibson wrote:
> On Fri, Aug 06, 2021 at 08:00:40PM +0200, Cédric Le Goater wrote:
>> It includes support for the POWER10 processor and the QEMU platform.
>>
>> Built from submodule.
>>
>> Signed-off-by: Cédric Le Goater
>
> Applied to ppc-for-6.2, thanks.
FYI asked description update.
>
>> ---
>> pc-bios/skiboot.lid | Bin 1667280 -> 2528128 bytes
>> roms/skiboot| 2 +-
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/skiboot.lid b/pc-bios/skiboot.lid
>> index
>> 504b95e8b6611aff3a934ff10f789934680591f9..8a3c278512a428a034ed5b1ddbed017ae8c0a9d0
>> 100644
>> GIT binary patch
>> literal 2528128
>> zcmeEv4SZD9nf|#mfearBV;kF`SO$qY6Qd1^^@hpE1kqj+w86rP2r^+niGqe*T_@&F
>> zW&&eNTH1fGwhIL{s7ayK7I$?^Td~0p7Om}K)h@1z!OscUHCC+&f!zP|oO>skM6~U0
>> zx4T=<+)FHw#O
[Bug 1939179] Re: qemu-ga fsfreeze crashes the kernel
We are currently in progress of retiring this bug tracker here... could you please open a new ticket on gitlab instead: https://gitlab.com/qemu-project/qemu/-/issues Thanks and sorry for the inconvenience. ** 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/1939179 Title: qemu-ga fsfreeze crashes the kernel Status in QEMU: Incomplete Bug description: Hello, Still required your attention, duplicate from: https://bugs.launchpad.net/bugs/1807073 https://bugs.launchpad.net/bugs/1813045 We use mainly Cloudlinux, Debian and Centos. We experienced many crashes on our qemu instances based on Cloudlinux during a snapshot. The issue is not related to CloudLinux directly, but to Qemu agent, which does not freeze the file system(s) correctly. What is actually happening: When VM backup is invoked, Qemu agent freezes the file systems, so no single change will be made during the backup. But Qemu agent does not respect the loop* devices in freezing order (we have checked its sources), which leads to the next situation: 1) freeze loopback fs ---> send async reqs to loopback thread 2) freeze main fs 3) loopback thread wakes up and trying to write data to the main fs, which is still frozen, and this finally leads to the hung task and kernel crash. Moreover, a lot of Proxmox users are complaining about the issue as well: https://forum.proxmox.com/threads/error-vm-100-qmp-command-guest-fsfreeze-thaw-failed-got-timeout.68082/ https://forum.proxmox.com/threads/problem-with-fsfreeze-freeze-and-qemu-guest-agent.65707/ To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1939179/+subscriptions
Re: [PATCH-for-6.2 1/2] disas/nios2: Fix style in print_insn_nios2()
+Peter for overall style recommendation. On 8/9/21 8:08 AM, Thomas Huth wrote: > On 07/08/2021 13.09, Philippe Mathieu-Daudé wrote: >> We are going to modify this function, fix its style first. >> >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> disas/nios2.c | 53 +-- >> 1 file changed, 26 insertions(+), 27 deletions(-) > > I guess it'd make sense to either re-indent the whole file or to bite > the bullet and life with the checkpatch warnings here ... otherwise you > now have one function that matches the QEMU coding style while the rest > of the file looks completely different. Yeah I didn't know what to do here, I remember a discussion (3 years ago?) "disas/..." is old import from binutils libopcode, so better not diverge the style. But the capstone updates shuffled things a bit. And nobody is tracking the parent project. I'm tempted to keep the style and ignore the warnings. > OTOH, we still can clean that up later, so in case you want to keep this > patch: > > Reviewed-by: Thomas Huth Thanks.
Re: [RFC PATCH 04/13] target/riscv: Support UXL32 for slit/sltiu
On 2021/8/6 上午3:09, Richard Henderson wrote:
On 8/4/21 4:53 PM, LIU Zhiwei wrote:
+static bool gen_arith_simm_tl(DisasContext *ctx, arg_i *a,
+ void (*func)(TCGv, TCGv, TCGv))
+{
+ TCGv dest = gpr_dst(ctx, a->rd);
+ TCGv src1 = gpr_src_s(ctx, a->rs1);
+ TCGv src2 = tcg_constant_tl(a->imm);
+
+ (*func)(dest, src1, src2);
+ return true;
+}
+
+static bool gen_arith_uimm_tl(DisasContext *ctx, arg_i *a,
+ void (*func)(TCGv, TCGv, TCGv))
+{
+ TCGv dest = gpr_dst(ctx, a->rd);
+ TCGv src1 = gpr_src_u(ctx, a->rs1);
+ TCGv src2 = tcg_constant_tl(ctx->uxl32 ? a->imm & UINT32_MAX :
a->imm);
+
+ (*func)(dest, src1, src2);
+ return true;
+}
Again, unsigned comparisions work fine with sign-extended inputs.
Thanks. Fix it in next patch set.
Zhiwei
r~
Re: [PATCH] vmxnet3: add stub for encapsulation offload
On 8/7/21 10:25 PM, Alexander Bulekov wrote:
> On 210807 1019, Philippe Mathieu-Daudé wrote:
>> On 8/7/21 12:23 AM, Alexander Bulekov wrote:
>>> Encapsulation offload (offload mode 1) is a valid mode present in the
>>> kernel that isn't implemented in QEMU, yet.
>>>
>>> https://lore.kernel.org/lkml/20200528015426.8285-4-dos...@vmware.com/
>>>
>>> Add a stub for this mode, to avoid the guest-triggerable assertion.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/517
>>> Signed-off-by: Alexander Bulekov
>>> ---
>>> hw/net/vmxnet3.c | 5 +
>>> hw/net/vmxnet3.h | 1 +
>>> 2 files changed, 6 insertions(+)
>>>
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index 41f796a247..62e11d0e3e 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -443,6 +443,11 @@ vmxnet3_setup_tx_offloads(VMXNET3State *s)
>>> net_tx_pkt_build_vheader(s->tx_pkt, false, false, 0);
>>> break;
>>>
>>> +case VMXNET3_OM_ENCAP:
>>> +VMW_PKPRN("Encapsulation offload requested, but not available\n");
>>> +return false;
>>> +break;
>>
>> No need to break if you returned (unreachable).
>
> I included it in case the feature ends up being added, so there is one
> less thing to worry about, but I can remove it. There are a couple of
> places in QEMU where this occurs, so I wasn't sure about the best
> practice.
I don't know if there is an official "best practice". My style /
recommendation is to try to use single exit point when possible
so I can easily add trace events.
Re: [PATCH] hw: arm: aspeed: Enable eth0 interface for aspeed-ast2600-evb
On 8/8/21 10:04 PM, Guenter Roeck wrote:
> Commit 7582591ae7 ("aspeed: Support AST2600A1 silicon revision") switched
> the silicon revision for AST2600 to revision A1. On revision A1, the first
> Ethernet interface is operational. Enable it.
Indeed.
I see that commit ba56f464f0c ("ARM: dts: aspeed: ast2600evb: Add MAC0")
reintroduced it a while ago. But my A0 doesn't support it. I am missing
something.
Joel, why this patch didn't reach the OpenBMC kernel ?
Thanks,
C.
>
> Signed-off-by: Guenter Roeck
> ---
> hw/arm/aspeed.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 9d43e26c51..ecf0c9cfac 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -959,7 +959,8 @@ static void
> aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
> amc->fmc_model = "w25q512jv";
> amc->spi_model = "mx66u51235f";
> amc->num_cs= 1;
> -amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON;
> +amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON |
> + ASPEED_MAC3_ON;
> amc->i2c_init = ast2600_evb_i2c_init;
> mc->default_ram_size = 1 * GiB;
> mc->default_cpus = mc->min_cpus = mc->max_cpus =
>
Re: [RFC PATCH 05/13] target/riscv: Support UXL32 for shift instruction
On 2021/8/6 上午6:17, Richard Henderson wrote:
On 8/4/21 4:53 PM, LIU Zhiwei wrote:
static bool trans_srli(DisasContext *ctx, arg_srli *a)
{
+ if (ctx->uxl32) {
+ return trans_srliw(ctx, a);
+ }
return gen_shifti(ctx, a, tcg_gen_shr_tl);
}
First, trans_srliw begins with REQUIRE_64BIT, which *should* fail when
RV32 is in effect. This means there's a missing change to is_32bit().
As I have replied in another patch, ctx->uxl32 already indicats 64 bit
CPU. Anyway, I will think more about how to merge is_32bit() and uxl32
in next patch set.
Second, the current decode for srli allows 7 bits of shift, while
srilw only allows 5. As a consequence, gen_shifti contains a check
for TARGET_LONG_BITS and trans_srliw does not contain a check at all.
We need to diagnose an out-of-range shift for RV32 somewhere.
Yes, it's not proper directly use *w here. Fix it in next patch set.
Zhiwei
I recommend extending the gen_shift* family of helpers.
static bool gen_shifti_imm(DisasContext *ctx, arg_shift *a, int width,
void (*func)(TCGv, TCGv, target_long))
{
TCGv dest, src1;
if (a->shamt >= width) {
return false;
}
dest = gpr_dst(ctx, a->rd);
src1 = gpr_src(ctx, a->rs1);
func(dest, src1, a->shamt);
return true;
}
static bool gen_shifti(DisasContext *ctx, arg_shift *a, int width,
void (*func)(TCGv, TCGv, TCGv))
{...}
static void gen_srliw(TCGv dest, TCGv src1, target_long shamt)
{
tcg_gen_extract_tl(dest, src1, shamt, 32 - shamt);
tcg_gen_ext32s_tl(dest, dest);
}
static bool trans_srliw(DisasContext *ctx, arg_shift *a)
{
REQUIRE_64BIT(ctx);
return gen_shifti_imm(ctx, a, 32, gen_srliw);
}
static bool trans_srli(DisasContext *ctx, arg_shift *a)
{
int xlen = is_32bit(ctx) ? 32 : 64;
return gen_shifti_imm(ctx, a, xlen,
xlen == TARGET_LONG_BITS
? tcg_gen_shri_tl : gen_srliw);
}
etc. Perhaps that is_32bit() check above could be consolidated into
some macro/inline.
r~
Re: [RFC PATCH 06/13] target/riscv: Fix div instructions
On 2021/8/6 上午6:18, Richard Henderson wrote: On 8/4/21 4:53 PM, LIU Zhiwei wrote: Don't overwrite global source register after https://lists.gnu.org/archive/html/qemu-riscv/2021-07/msg00058.html. Signed-off-by: LIU Zhiwei --- target/riscv/translate.c | 46 +++- 1 file changed, 26 insertions(+), 20 deletions(-) FWIW, I have a cleanup to these routines that does more than add temps. I've been slow to re-post the series; sorry about that. Look forward to your next patch set. I will send my next patch set based on your new post. Thanks, Zhiwei r~
Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.
Hello Phil, On 8/9/21 9:06 AM, Philippe Mathieu-Daudé wrote: > Hi Cédric, > > On 8/6/21 8:00 PM, Cédric Le Goater wrote: >> It includes support for the POWER10 processor and the QEMU platform. > > 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here? OK. See attachement. >> >> Built from submodule. > > 2/ Could we have a CI job building this, during 6.2 cycle? >(See .gitlab-ci.d/edk2.yml and .gitlab-ci.d/opensbi.yml) Sure. It doesn't look too complex. I plan to add acceptance tests for the QEMU powernv machines also once the OpenPOWER files (zImage.epapr and rootfs.cpio.xz) are published on GH. >> >> Signed-off-by: Cédric Le Goater >> --- >> pc-bios/skiboot.lid | Bin 1667280 -> 2528128 bytes >> roms/skiboot| 2 +- >> 2 files changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/pc-bios/skiboot.lid b/pc-bios/skiboot.lid >> index >> 504b95e8b6611aff3a934ff10f789934680591f9..8a3c278512a428a034ed5b1ddbed017ae8c0a9d0 >> 100644 >> GIT binary patch >> literal 2528128 > > Consider using 'git-format-patch --no-binary' and a reference > to your repository to fetch a such big binary patch. David would pull from my tree then ? So that's like doing a PR. We can do that next time I send an update if David is OK with that. I should send an update for v7.0 tag. Thanks, C. Aaron Sawdey (1): external/mambo: support mambo COW mode for PMEM disk Alexey Kardashevskiy (3): npu2: Invalidate entire TCE cache if many entries requested npu2: Clear fence on all bricks phb4: Add PHB options get/set OPAL calls Alistair Popple (3): skiboot.tcl: Add chip-id to pmem device tree node hw/phys-map/p10: Add P10 MMIO map platforms: Add Rainier Andrew Donnellan (3): npu2-opencapi: Fix integer promotion bug in LPC allocation hw/npu2-opencapi: Support multiple LPC devices hw/phys-map: Fix OCAPI_MEM BAR values Andrew Geissler (1): Support BMC IPMI heartbeat command Andrew Jeffery (10): gard: Fix data corruption when clearing single records blocklevel: smart_write: Terminate line for debug output in no-change case blocklevel: smart_write: Rename write buffer blocklevel: smart_write: Rename size variable for clarity blocklevel: smart_write: Avoid indirectly testing formal parameters blocklevel: smart_write: Deny writes intersecting ECC protected regions blocklevel: smart_write: Avoid reuse of formal parameters blocklevel: smart_write: Tidy local variable declarations blocklevel: smart_write: Fix unaligned writes to ECC partitions libflash: ipmi-hiomap: Document error handling strategy in write path Anju T Sudhakar (4): hw/imc: Cleanup code to define scom addr for IMC at run time hw/imc: Use the xscom macros for IMC based on platform hw/imc: Do scoms on the secondary core in fused core mode for core-imc counters hw/imc: Power10 support Anton Blanchard (2): external/mambo Update SIM_CTRL1 Don't warn about stack size on host binaries Artem Senichev (1): platforms/nicole: Fixup the system VPD EEPROM size Balamuruhan S (2): occ-sensor: clean dt properties if sensor is not available chip: enable HOMER/OCC common area region in Qemu emulated PowerNV host Benjamin Herrenschmidt (7): xive: Set the fused core mode properly chip: Fix pir_to_thread_id for fused cores cpu: Keep track of the "ec_primary" in big core more direct-ctl: Use the EC primary for special wakeups slw: Limit fused cores P9 to STOP0/1/2 cpu: Make cpu_get_core_index() return the fused core number imc: Use pir_to_core_id() rather than cpu_get_core_index() Claudio Carvalho (1): core/flash.c: add SECBOOT read and write support Cédric Le Goater (61): xive: fix return value of opal_xive_allocate_irq() xive/p9: introduce header files for the registers xive/p9: minor cleanup of the interface xive/p9: use MMIO access for VC_EQC_CONFIG xive/p9: remove code not using indirect mode xive/p9: remove code not using block group mode xive/p9: remove dead code xive/p9: obsolete OPAL_XIVE_IRQ_*_VIA_FW flags xive/p9: obsolete OPAL_XIVE_IRQ_SHIFT_BUG flags xive/p9: fix EQ bitmap assignment when allocation fails xive/p9: introduce definitions for priorities xive/p9: fix silent escalation EQ setup xive/p9: cleanup all EQs when a VP block is freed. xive/p9: remove ACK# setting in the NVT xive/p9: introduce NVT_SHIFT xive/p9: remove XIVE_INT_SAFETY_GAP xive/p9: use predefined bitmasks to manipulate EQ addresses xive/p9: introduce the ESB magic MMIO offsets plat/qemu: use "/qemu" device tree node to identify the QEMU platform plat/qemu: add a POWER10 platform xive/p9: Introduce XIVE_INT_ORDER xive/p9: Clarify the global IRQ number encoding xive/p9: Introduce XIVE_ESB_SHIFT xive/p9:
Re: [PATCH] hw: arm: aspeed: Enable eth0 interface for aspeed-ast2600-evb
On Mon, 9 Aug 2021 at 07:45, Cédric Le Goater wrote:
>
> On 8/8/21 10:04 PM, Guenter Roeck wrote:
> > Commit 7582591ae7 ("aspeed: Support AST2600A1 silicon revision") switched
> > the silicon revision for AST2600 to revision A1. On revision A1, the first
> > Ethernet interface is operational. Enable it.
>
> Indeed.
>
> I see that commit ba56f464f0c ("ARM: dts: aspeed: ast2600evb: Add MAC0")
> reintroduced it a while ago. But my A0 doesn't support it. I am missing
> something.
>
> Joel, why this patch didn't reach the OpenBMC kernel ?
Because your a0 doesn't support it, so it would break that. That's the
only reason.
For this patch,
Reviewed-by: Joel Stanley
>
> Thanks,
>
> C.
>
>
> >
> > Signed-off-by: Guenter Roeck
> > ---
> > hw/arm/aspeed.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 9d43e26c51..ecf0c9cfac 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -959,7 +959,8 @@ static void
> > aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
> > amc->fmc_model = "w25q512jv";
> > amc->spi_model = "mx66u51235f";
> > amc->num_cs= 1;
> > -amc->macs_mask = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON;
> > +amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON |
> > + ASPEED_MAC3_ON;
> > amc->i2c_init = ast2600_evb_i2c_init;
> > mc->default_ram_size = 1 * GiB;
> > mc->default_cpus = mc->min_cpus = mc->max_cpus =
> >
>
Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.
On 8/9/21 9:55 AM, Cédric Le Goater wrote: > Hello Phil, > > On 8/9/21 9:06 AM, Philippe Mathieu-Daudé wrote: >> Hi Cédric, >> >> On 8/6/21 8:00 PM, Cédric Le Goater wrote: >>> It includes support for the POWER10 processor and the QEMU platform. >> >> 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here? > > OK. See attachement. By "here" I meant in the commit description ;) >>> >>> Built from submodule. >> >> 2/ Could we have a CI job building this, during 6.2 cycle? >>(See .gitlab-ci.d/edk2.yml and .gitlab-ci.d/opensbi.yml) > > Sure. It doesn't look too complex. > > I plan to add acceptance tests for the QEMU powernv machines also > once the OpenPOWER files (zImage.epapr and rootfs.cpio.xz) are > published on GH. > >>> >>> Signed-off-by: Cédric Le Goater >>> --- >>> pc-bios/skiboot.lid | Bin 1667280 -> 2528128 bytes >>> roms/skiboot| 2 +- >>> 2 files changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/pc-bios/skiboot.lid b/pc-bios/skiboot.lid >>> index >>> 504b95e8b6611aff3a934ff10f789934680591f9..8a3c278512a428a034ed5b1ddbed017ae8c0a9d0 >>> 100644 >>> GIT binary patch >>> literal 2528128 >> >> Consider using 'git-format-patch --no-binary' and a reference >> to your repository to fetch a such big binary patch. > > David would pull from my tree then ? So that's like doing a PR. > We can do that next time I send an update if David is OK with > that. I should send an update for v7.0 tag. As you wish. Big patches gave us troubles, i.e. they make crash the 'patches' instance. 2.5MiB is probably borderline and I'm being nit-picky.
Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.
On 8/9/21 10:06 AM, Philippe Mathieu-Daudé wrote: > On 8/9/21 9:55 AM, Cédric Le Goater wrote: >> Hello Phil, >> >> On 8/9/21 9:06 AM, Philippe Mathieu-Daudé wrote: >>> Hi Cédric, >>> >>> On 8/6/21 8:00 PM, Cédric Le Goater wrote: It includes support for the POWER10 processor and the QEMU platform. >>> >>> 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here? >> >> OK. See attachement. > > By "here" I meant in the commit description ;) yeah I know but David queued the patch already. > Built from submodule. >>> >>> 2/ Could we have a CI job building this, during 6.2 cycle? >>>(See .gitlab-ci.d/edk2.yml and .gitlab-ci.d/opensbi.yml) >> >> Sure. It doesn't look too complex. >> >> I plan to add acceptance tests for the QEMU powernv machines also >> once the OpenPOWER files (zImage.epapr and rootfs.cpio.xz) are >> published on GH. >> Signed-off-by: Cédric Le Goater --- pc-bios/skiboot.lid | Bin 1667280 -> 2528128 bytes roms/skiboot| 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/pc-bios/skiboot.lid b/pc-bios/skiboot.lid index 504b95e8b6611aff3a934ff10f789934680591f9..8a3c278512a428a034ed5b1ddbed017ae8c0a9d0 100644 GIT binary patch literal 2528128 >>> >>> Consider using 'git-format-patch --no-binary' and a reference >>> to your repository to fetch a such big binary patch. >> >> David would pull from my tree then ? So that's like doing a PR. >> We can do that next time I send an update if David is OK with >> that. I should send an update for v7.0 tag. > > As you wish. Big patches gave us troubles, i.e. they make crash > the 'patches' instance. 2.5MiB is probably borderline and I'm > being nit-picky. > If we can do a PR next time, all should be fine. Thanks, C.
Re: [PATCH] hw: arm: aspeed: Enable eth0 interface for aspeed-ast2600-evb
On 8/9/21 10:01 AM, Joel Stanley wrote:
> On Mon, 9 Aug 2021 at 07:45, Cédric Le Goater wrote:
>>
>> On 8/8/21 10:04 PM, Guenter Roeck wrote:
>>> Commit 7582591ae7 ("aspeed: Support AST2600A1 silicon revision") switched
>>> the silicon revision for AST2600 to revision A1. On revision A1, the first
>>> Ethernet interface is operational. Enable it.
>>
>> Indeed.
>>
>> I see that commit ba56f464f0c ("ARM: dts: aspeed: ast2600evb: Add MAC0")
>> reintroduced it a while ago. But my A0 doesn't support it. I am missing
>> something.
>>
>> Joel, why this patch didn't reach the OpenBMC kernel ?
>
> Because your a0 doesn't support it, so it would break that. That's the
> only reason.
OK then.
> For this patch,
>
> Reviewed-by: Joel Stanley
Reviewed-by: Cédric Le Goater
It's queued to aspeed-6.2.
Thanks
C.
Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.
On 8/9/21 10:14 AM, Cédric Le Goater wrote: > On 8/9/21 10:06 AM, Philippe Mathieu-Daudé wrote: >> On 8/9/21 9:55 AM, Cédric Le Goater wrote: >>> Hello Phil, >>> >>> On 8/9/21 9:06 AM, Philippe Mathieu-Daudé wrote: Hi Cédric, On 8/6/21 8:00 PM, Cédric Le Goater wrote: > It includes support for the POWER10 processor and the QEMU platform. 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here? >>> >>> OK. See attachement. >> >> By "here" I meant in the commit description ;) > > yeah I know but David queued the patch already. Queued for 6.2 ;) David doesn't have problem to update an unmerged queue. So are you expecting him to amend the attachment to your commit?
Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.
On 8/9/21 10:18 AM, Philippe Mathieu-Daudé wrote: > On 8/9/21 10:14 AM, Cédric Le Goater wrote: >> On 8/9/21 10:06 AM, Philippe Mathieu-Daudé wrote: >>> On 8/9/21 9:55 AM, Cédric Le Goater wrote: Hello Phil, On 8/9/21 9:06 AM, Philippe Mathieu-Daudé wrote: > Hi Cédric, > > On 8/6/21 8:00 PM, Cédric Le Goater wrote: >> It includes support for the POWER10 processor and the QEMU platform. > > 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here? OK. See attachement. >>> >>> By "here" I meant in the commit description ;) >> >> yeah I know but David queued the patch already. > > Queued for 6.2 ;) David doesn't have problem to update an > unmerged queue. So are you expecting him to amend the attachment > to your commit? skiboot history is available on GH. We can improve the process next time. C.
Re: [PATCH v8 08/16] qemu-iotests: add gdbserver option to script tests too
diff --git a/tests/qemu-iotests/common.qemu
b/tests/qemu-iotests/common.qemu
index 0fc52d20d7..cbca757b49 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -85,7 +85,12 @@ _timed_wait_for()
timeout=yes
QEMU_STATUS[$h]=0
- while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+ read_timeout="-t ${QEMU_COMM_TIMEOUT}"
+ if [ ! -z ${GDB_OPTIONS} ]; then
Shouldn’t we quote "${GDB_OPTIONS}" so that `test` won’t interpret it as
its own parameters (if something in there starts with `--`, which I
don’t think is the intended usage for $GDB_OPTIONS, but, well...)?
(Also, `! -z` is the same as `-n`, but I suppose choosing between the
two can be a matter of style.)
+
+ GDB=""
+ if [ ! -z ${GDB_OPTIONS} ]; then
Here, too. (Sorry for not noticing in v3 already...)
Sorry, I forgot to reply to this email. I agree, I will put quotes and
change `! -z` in `-n`.
I will send v9 because after all this time this serie has some minor
conflicts with current QEMU version.
Hopefully this will be the last one :)
Thank you,
Emanuele
RE: [PULL V3 for 6.2 1/6] qapi/net: Add IPFlowSpec and QMP command for filter passthrough
> -Original Message-
> From: Markus Armbruster
> Sent: Saturday, August 7, 2021 7:32 PM
> To: Zhang, Chen
> Cc: Jason Wang ; Eric Blake ;
> Dr. David Alan Gilbert ; qemu-dev de...@nongnu.org>; Daniel P.Berrangé ; Gerd
> Hoffmann ; Li Zhijian ; Lukas
> Straub
> Subject: Re: [PULL V3 for 6.2 1/6] qapi/net: Add IPFlowSpec and QMP
> command for filter passthrough
>
> I see Jason queued this while I was failing at keeping up with review.
> I apologize for dropping the ball.
>
> There still are a few unresolved issues I raised in prior review. The
> documentation is not ready, and there is no consensus on the design of
> passthrough-filter-del.
>
> If we merge this as is for 6.2, then I want my review to be addressed on top.
OK, please hold the ball and let me know if I missed something.
I will try to do this well.
>
> Zhang Chen writes:
>
> > Since the real user scenario does not need to monitor all traffic.
> > Add passthrough-filter-add and passthrough-filter-del to maintain a
> > network passthrough list in object with network packet processing
> > function. Add IPFlowSpec struct for all QMP commands.
> > Most the fields of IPFlowSpec are optional,except object-name.
> >
> > Signed-off-by: Zhang Chen
> > ---
>
> [...]
>
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 7fab2e7cd8..bfe38faab5 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -7,6 +7,7 @@
> > ##
> >
> > { 'include': 'common.json' }
> > +{ 'include': 'sockets.json' }
> >
> > ##
> > # @set_link:
> > @@ -696,3 +697,80 @@
> > ##
> > { 'event': 'FAILOVER_NEGOTIATED',
> >'data': {'device-id': 'str'} }
> > +
> > +##
> > +# @IPFlowSpec:
> > +#
> > +# IP flow specification.
> > +#
> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is
> the
> > +#string instead of enum, because it can be passed to
> getprotobyname(3)
> > +#and avoid duplication with /etc/protocols.
>
> In review of v8, I wrote:
>
> The rationale is good, but it doesn't really belong into the interface
> documentation. Suggest:
>
># @protocol: Transport layer protocol like TCP/UDP, etc. This will be
>#passed to getprotobyname(3).
>
> to which you replied "OK." Please apply the change.
Sorry, I missed it.
I thought that more comments would make it clearer and avoid
misunderstandings like Eric Blake comments why not enum in V7.
I will change it as your comments here.
>
> > +#
> > +# @object-name: The @object-name means a qemu object with network
> packet
> > +# processing function, for example colo-compare,
> > filtr-redirector
> > +# filtr-mirror, etc. VM can running with multi network packet
>
> s/qemu/QEMU/
>
> s/filtr/filter/ two times here, and more below.
>
> s/VM/The VM/
>
> s/multi/multiple/
>
> Also, limit doc comment line length to 70 characters or so.
>
OK, I will fix it.
> > +# processing function objects. They can control different
> > network
> > +# data paths from netdev or chardev. So it needs the
> > object-name
> > +# to set the effective module.
>
> Again, this is rationale, which doesn't really belong here.
>
> What does belong here, but isn't: meaning of @object-name, i.e. how it is
> resolved to a "qemu object with network packet processing function",
> whatever that is.
>
> I'm *guessing* it's the QOM path of a QOM object that provides a certain
> interface. Correct?
>
> If yes, which interface exactly? Is it a QOM interface?
>
> The comment could then look like
>
> # QOM path to a QOM object that implements the MUMBLE interface.
>
> with the details corrected / fleshed out.
Yes, the QOM object need to maintain/apply their own passthrough list while
working.
I will remove original comments and change it to:
#QOM path to a QOM object that implements their own passthrough work in
#the original data processing flow. What is exposed to the outside is an
operable
#passthrough list.
>
> > +#
> > +# @source: Source address and port.
> > +#
> > +# @destination: Destination address and port.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'IPFlowSpec',
> > + 'data': { '*protocol': 'str', 'object-name': 'str',
> > +'*source': 'InetSocketAddressBase',
> > +'*destination': 'InetSocketAddressBase' } }
> > +
> > +##
> > +# @passthrough-filter-add:
> > +#
> > +# Add passthrough entry IPFlowSpec to a qemu object with network
> > +packet # processing function, for example filtr-mirror, COLO-compare, etc.
> > +# The object-name is necessary. The protocol and source/destination
> > +IP and # source/destination ports are optional. if only inputs part
> > +of the
>
> Start your sentences with a capital letter, please.
>
> More importantly, the paragraph is confusing. I suggested
>
># Add an entry to the COLO network passthrough list.
># Absent protocol, host addresses and ports match anything.
Current passth
[PATCH] block/export/fuse.c: fix musl build
Fix the following build failure on musl raised since version 6.0.0 and
https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
because musl does not define FALLOC_FL_ZERO_RANGE:
../block/export/fuse.c: In function 'fuse_fallocate':
../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared (first
use in this function)
563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
| ^~~~
Fixes:
-
http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
Signed-off-by: Fabrice Fontaine
---
block/export/fuse.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/export/fuse.c b/block/export/fuse.c
index ada9e263eb..07e31129a6 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -635,6 +635,7 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
+#ifdef FALLOC_FL_ZERO_RANGE
} else if (mode & FALLOC_FL_ZERO_RANGE) {
if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
/* No need for zeroes, we are going to write them ourselves */
@@ -654,6 +655,7 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
+#endif
} else if (!mode) {
/* We can only fallocate at the EOF with a truncate */
if (offset < blk_len) {
--
2.30.2
[PATCH] xive: Remove extra '0x' prefix in trace events
Cc: th...@redhat.com
Fixes: 4e960974d4ee ("xive: Add trace events")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/519
Signed-off-by: Cédric Le Goater
---
hw/intc/trace-events | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index e56e7dd3b667..6a17d38998d9 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -219,14 +219,14 @@ kvm_xive_source_reset(uint32_t srcno) "IRQ 0x%x"
xive_tctx_accept(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x
CPPR=0x%02x NSR=0x%02x ACK"
xive_tctx_notify(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x
CPPR=0x%02x NSR=0x%02x raise !"
xive_tctx_set_cppr(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x new
CPPR=0x%02x NSR=0x%02x"
-xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value)
"@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
-xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value)
"@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
+xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value)
"@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
+xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value)
"@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
xive_router_end_notify(uint8_t end_blk, uint32_t end_idx, uint32_t end_data)
"END 0x%02x/0x%04x -> enqueue 0x%08x"
xive_router_end_escalate(uint8_t end_blk, uint32_t end_idx, uint8_t esc_blk,
uint32_t esc_idx, uint32_t end_data) "END 0x%02x/0x%04x -> escalate END
0x%02x/0x%04x data 0x%08x"
-xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value)
"@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
-xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value)
"@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
+xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value)
"@0x%"PRIx64" sz=%d val=0x%" PRIx64
+xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value)
"@0x%"PRIx64" sz=%d val=0x%" PRIx64
xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring) "found
NVT 0x%x/0x%x ring=0x%x"
-xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END
0x%x/0x%x @0x0x%"PRIx64
+xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END
0x%x/0x%x @0x%"PRIx64
# pnv_xive.c
pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64"
val=0x%"PRIx64
--
2.31.1
[PATCH v9 02/16] python: Reduce strictness of pylint's duplicate-code check
From: John Snow Pylint prior to 2.8.3 (We pin at >= 2.8.0) includes function and method signatures as part of its duplicate checking algorithm. This check does not listen to pragmas, so the only way to disable it is to turn it off completely or increase the minimum duplicate lines so that it doesn't trigger for functions with long, multi-line signatures. When we decide to upgrade to pylint 2.8.3 or greater, we will be able to use 'ignore-signatures = true' to the config instead. I'd prefer not to keep us on the very bleeding edge of pylint if I can help it -- 2.8.3 came out only three days ago at time of writing. See: https://github.com/PyCQA/pylint/pull/4474 Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Acked-by: John Snow Reviewed-by: Max Reitz --- python/setup.cfg | 5 + 1 file changed, 5 insertions(+) diff --git a/python/setup.cfg b/python/setup.cfg index 14bab90288..83909c1c97 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -105,6 +105,11 @@ good-names=i, # Ignore imports when computing similarities. ignore-imports=yes +# Minimum lines number of a similarity. +# TODO: Remove after we opt in to Pylint 2.8.3. See commit msg. +min-similarity-lines=6 + + [isort] force_grid_wrap=4 force_sort_within_sections=True -- 2.31.1
[PATCH v9 03/16] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow Reviewed-by: Max Reitz Acked-by: John Snow --- python/qemu/machine/qtest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py index 592be263e0..395cc8fbfe 100644 --- a/python/qemu/machine/qtest.py +++ b/python/qemu/machine/qtest.py @@ -112,6 +112,7 @@ class QEMUQtestMachine(QEMUMachine): def __init__(self, binary: str, args: Sequence[str] = (), + wrapper: Sequence[str] = (), name: Optional[str] = None, base_temp_dir: str = "/var/tmp", socket_scm_helper: Optional[str] = None, @@ -123,7 +124,8 @@ def __init__(self, name = "qemu-%d" % os.getpid() if sock_dir is None: sock_dir = base_temp_dir -super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, +super().__init__(binary, args, wrapper=wrapper, name=name, + base_temp_dir=base_temp_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir, qmp_timer=qmp_timer) self._qtest: Optional[QEMUQtestProtocol] = None -- 2.31.1
[PATCH v9 00/16] qemu_iotests: improve debugging options
This series adds the option to attach gdbserver and valgrind to the QEMU binary running in qemu_iotests. It also allows to redirect QEMU binaries output of the python tests to the stdout, instead of a log file. Patches 1-9 introduce the -gdb option to both python and bash tests, 10-14 extend the already existing -valgrind flag to work also on python tests, and patch 15-16 introduces -p to enable logging to stdout. In particular, patches 1,6,8,11 focus on extending the QMP socket timers when using gdb/valgrind, otherwise the python tests will fail due to delays in the QMP responses. Signed-off-by: Emanuele Giuseppe Esposito --- v9: * Replace `! -z` with `-n` in bash scripts (patch 8), and quote $GDB_OPTIONS in the same if condition [Max] * Add r-b from Max to all patches except 8, remove r-b from Vladimir on patch 8 Emanuele Giuseppe Esposito (15): python: qemu: add timer parameter for qmp.accept socket python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine docs/devel/testing: add debug section to the QEMU iotests chapter qemu-iotests: add option to attach gdbserver qemu-iotests: delay QMP socket timers qemu_iotests: insert gdbserver command line as wrapper for qemu binary qemu-iotests: add gdbserver option to script tests too docs/devel/testing: add -gdb option to the debugging section of QEMU iotests qemu-iotests: extend the check script to prepare supporting valgrind for python tests qemu-iotests: extend QMP socket timeout when using valgrind qemu-iotests: allow valgrind to read/delete the generated log file qemu-iotests: insert valgrind command line as wrapper for qemu binary docs/devel/testing: add -valgrind option to the debug section of QEMU iotests qemu-iotests: add option to show qemu binary logs on stdout docs/devel/testing: add -p option to the debug section of QEMU iotests John Snow (1): python: Reduce strictness of pylint's duplicate-code check docs/devel/testing.rst | 29 python/qemu/machine/machine.py | 16 +++ python/qemu/machine/qtest.py | 9 --- python/setup.cfg | 5 tests/qemu-iotests/check | 15 --- tests/qemu-iotests/common.qemu | 7 - tests/qemu-iotests/common.rc | 8 +- tests/qemu-iotests/iotests.py | 49 -- tests/qemu-iotests/testenv.py | 23 ++-- 9 files changed, 143 insertions(+), 18 deletions(-) -- 2.31.1
[PATCH v9 05/16] qemu-iotests: add option to attach gdbserver
Define -gdb flag and GDB_OPTIONS environment variable
to python tests to attach a gdbserver to each qemu instance.
This patch only adds and parses this flag, it does not yet add
the implementation for it.
if -gdb is not provided but $GDB_OPTIONS is set, ignore the
environment variable.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
tests/qemu-iotests/check | 6 +-
tests/qemu-iotests/iotests.py | 5 +
tests/qemu-iotests/testenv.py | 17 +++--
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2dd529eb75..4365bb8066 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,9 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-gdb', action='store_true',
+ help="start gdbserver with $GDB_OPTIONS options \
+('localhost:12345' if $GDB_OPTIONS is empty)")
p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -114,7 +117,8 @@ if __name__ == '__main__':
env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
aiomode=args.aiomode, cachemode=args.cachemode,
imgopts=args.imgopts, misalign=args.misalign,
- debug=args.debug, valgrind=args.valgrind)
+ debug=args.debug, valgrind=args.valgrind,
+ gdb=args.gdb)
if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
if not args.tests:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6b0db4ce54..c86f239d81 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -74,6 +74,11 @@
qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
+gdb_qemu_env = os.environ.get('GDB_OPTIONS')
+qemu_gdb = []
+if gdb_qemu_env:
+qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
output_dir = os.environ.get('OUTPUT_DIR', '.')
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 0c3fe75636..8501c6caf5 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -27,6 +27,7 @@
import glob
from typing import List, Dict, Any, Optional, ContextManager
+DEF_GDB_OPTIONS = 'localhost:12345'
def isxfile(path: str) -> bool:
return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -72,7 +73,8 @@ class TestEnv(ContextManager['TestEnv']):
'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
- 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
+ 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
+ 'GDB_OPTIONS']
def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
if self.debug:
@@ -178,7 +180,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
imgopts: Optional[str] = None,
misalign: bool = False,
debug: bool = False,
- valgrind: bool = False) -> None:
+ valgrind: bool = False,
+ gdb: bool = False) -> None:
self.imgfmt = imgfmt
self.imgproto = imgproto
self.aiomode = aiomode
@@ -186,6 +189,15 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode:
str,
self.misalign = misalign
self.debug = debug
+if gdb:
+self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS)
+if not self.gdb_options:
+# cover the case 'export GDB_OPTIONS='
+self.gdb_options = DEF_GDB_OPTIONS
+elif 'GDB_OPTIONS' in os.environ:
+# to not propagate it in prepare_subprocess()
+del os.environ['GDB_OPTIONS']
+
if valgrind:
self.valgrind_qemu = 'y'
@@ -285,6 +297,7 @@ def print_env(self) -> None:
TEST_DIR -- {TEST_DIR}
SOCK_DIR -- {SOCK_DIR}
SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
+GDB_OPTIONS -- {GDB_OPTIONS}
"""
args = collections.defaultdict(str, self.get_env())
--
2.31.1
[PATCH v9 08/16] qemu-iotests: add gdbserver option to script tests too
Remove read timer in test script when GDB_OPTIONS are set,
so that the bash tests won't timeout while running gdb.
The only limitation here is that running a script with gdbserver
will make the test output mismatch with the expected
results, making the test fail.
Signed-off-by: Emanuele Giuseppe Esposito
---
tests/qemu-iotests/common.qemu | 7 ++-
tests/qemu-iotests/common.rc | 8 +++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 0fc52d20d7..0f1fecc68e 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -85,7 +85,12 @@ _timed_wait_for()
timeout=yes
QEMU_STATUS[$h]=0
-while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+read_timeout="-t ${QEMU_COMM_TIMEOUT}"
+if [ -n "${GDB_OPTIONS}" ]; then
+read_timeout=
+fi
+
+while IFS= read ${read_timeout} resp <&${QEMU_OUT[$h]}
do
if [ -n "$capture_events" ]; then
capture=0
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 609d82de89..d8582454de 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -166,8 +166,14 @@ _qemu_wrapper()
if [ -n "${QEMU_NEED_PID}" ]; then
echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
fi
+
+GDB=""
+if [ -n "${GDB_OPTIONS}" ]; then
+GDB="gdbserver ${GDB_OPTIONS}"
+fi
+
VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec
"${VALGRIND_LOGFILE}" \
-"$QEMU_PROG" $QEMU_OPTIONS "$@"
+$GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"
)
RETVAL=$?
_qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
--
2.31.1
[PATCH v9 01/16] python: qemu: add timer parameter for qmp.accept socket
Also add a new _qmp_timer field to the QEMUMachine class. Let's change the default socket timeout to None, so that if a subclass needs to add a timer, it can be done by modifying this private field. At the same time, restore the timer to be 15 seconds in iotests.py, to give an upper bound to the QMP monitor test command execution. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow Acked-by: John Snow Reviewed-by: Max Reitz --- python/qemu/machine/machine.py | 7 +-- python/qemu/machine/qtest.py | 5 +++-- tests/qemu-iotests/iotests.py | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 971ed7e8c6..14c4d17eca 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -97,7 +97,8 @@ def __init__(self, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None, - log_dir: Optional[str] = None): + log_dir: Optional[str] = None, + qmp_timer: Optional[float] = None): ''' Initialize a QEMUMachine @@ -112,6 +113,7 @@ def __init__(self, @param drain_console: (optional) True to drain console socket to buffer @param console_log: (optional) path to console log file @param log_dir: where to create and keep log files +@param qmp_timer: (optional) default QMP socket timeout @note: Qemu process is not started until launch() is used. ''' # pylint: disable=too-many-arguments @@ -121,6 +123,7 @@ def __init__(self, self._binary = binary self._args = list(args) self._wrapper = wrapper +self._qmp_timer = qmp_timer self._name = name or "qemu-%d" % os.getpid() self._base_temp_dir = base_temp_dir @@ -343,7 +346,7 @@ def _pre_launch(self) -> None: def _post_launch(self) -> None: if self._qmp_connection: -self._qmp.accept() +self._qmp.accept(self._qmp_timer) def _post_shutdown(self) -> None: """ diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py index d6d9c6a34a..592be263e0 100644 --- a/python/qemu/machine/qtest.py +++ b/python/qemu/machine/qtest.py @@ -115,7 +115,8 @@ def __init__(self, name: Optional[str] = None, base_temp_dir: str = "/var/tmp", socket_scm_helper: Optional[str] = None, - sock_dir: Optional[str] = None): + sock_dir: Optional[str] = None, + qmp_timer: Optional[float] = None): # pylint: disable=too-many-arguments if name is None: @@ -124,7 +125,7 @@ def __init__(self, sock_dir = base_temp_dir super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, socket_scm_helper=socket_scm_helper, - sock_dir=sock_dir) + sock_dir=sock_dir, qmp_timer=qmp_timer) self._qtest: Optional[QEMUQtestProtocol] = None self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock") diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 89663dac06..6b0db4ce54 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -570,10 +570,11 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) +timer = 15.0 super().__init__(qemu_prog, qemu_opts, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, - sock_dir=sock_dir) + sock_dir=sock_dir, qmp_timer=timer) self._num_drives = 0 def add_object(self, opts): -- 2.31.1
[PATCH v9 04/16] docs/devel/testing: add debug section to the QEMU iotests chapter
Introduce the "Debugging a test case" section, in preparation to the additional flags that will be added in the next patches. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- docs/devel/testing.rst | 8 1 file changed, 8 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 8a9cda33a5..8359f2ae37 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -224,6 +224,14 @@ another application on the host may have locked the file, possibly leading to a test failure. If using such devices are explicitly desired, consider adding ``locking=off`` option to disable image locking. +Debugging a test case +--- +The following options to the ``check`` script can be useful when debugging +a failing test: + +* ``-d`` (debug) just increases the logging verbosity, showing + for example the QMP commands and answers. + Test case groups -- 2.31.1
[PATCH v9 11/16] qemu-iotests: extend QMP socket timeout when using valgrind
As with gdbserver, valgrind delays the test execution, so the default QMP socket timeout and the generic class Timeout in iotests.py timeouts too soon. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- tests/qemu-iotests/iotests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 6aa1dc48ba..26c580f9e7 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -488,13 +488,13 @@ def __init__(self, seconds, errmsg="Timeout"): self.seconds = seconds self.errmsg = errmsg def __enter__(self): -if qemu_gdb: +if qemu_gdb or qemu_valgrind: return self signal.signal(signal.SIGALRM, self.timeout) signal.setitimer(signal.ITIMER_REAL, self.seconds) return self def __exit__(self, exc_type, value, traceback): -if qemu_gdb: +if qemu_gdb or qemu_valgrind: return False signal.setitimer(signal.ITIMER_REAL, 0) return False @@ -590,7 +590,7 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) -timer = 15.0 if not qemu_gdb else None +timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb, name=name, base_temp_dir=test_dir, -- 2.31.1
[PATCH v9 06/16] qemu-iotests: delay QMP socket timers
Attaching gdbserver implies that the qmp socket should wait indefinitely for an answer from QEMU. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Max Reitz --- tests/qemu-iotests/iotests.py | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index c86f239d81..e176a84620 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -477,10 +477,14 @@ def __init__(self, seconds, errmsg="Timeout"): self.seconds = seconds self.errmsg = errmsg def __enter__(self): +if qemu_gdb: +return self signal.signal(signal.SIGALRM, self.timeout) signal.setitimer(signal.ITIMER_REAL, self.seconds) return self def __exit__(self, exc_type, value, traceback): +if qemu_gdb: +return False signal.setitimer(signal.ITIMER_REAL, 0) return False def timeout(self, signum, frame): @@ -575,7 +579,7 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) -timer = 15.0 +timer = 15.0 if not qemu_gdb else None super().__init__(qemu_prog, qemu_opts, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, -- 2.31.1
[PATCH v9 07/16] qemu_iotests: insert gdbserver command line as wrapper for qemu binary
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- tests/qemu-iotests/iotests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index e176a84620..e7e3d92d3e 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -580,7 +580,8 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) timer = 15.0 if not qemu_gdb else None -super().__init__(qemu_prog, qemu_opts, name=name, +super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb, + name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir, qmp_timer=timer) -- 2.31.1
[PATCH v9 13/16] qemu-iotests: insert valgrind command line as wrapper for qemu binary
If -gdb and -valgrind are both defined, return an error.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
tests/qemu-iotests/iotests.py | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 85d8c0abbb..74fa56840d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -591,7 +591,11 @@ class VM(qtest.QEMUQtestMachine):
def __init__(self, path_suffix=''):
name = "qemu%s-%d" % (path_suffix, os.getpid())
timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
-super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+if qemu_gdb and qemu_valgrind:
+sys.stderr.write('gdb and valgrind are mutually exclusive\n')
+sys.exit(1)
+wrapper = qemu_gdb if qemu_gdb else qemu_valgrind
+super().__init__(qemu_prog, qemu_opts, wrapper=wrapper,
name=name,
base_temp_dir=test_dir,
socket_scm_helper=socket_scm_helper,
--
2.31.1
[PATCH v9 09/16] docs/devel/testing: add -gdb option to the debugging section of QEMU iotests
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- docs/devel/testing.rst | 11 +++ 1 file changed, 11 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 8359f2ae37..01e1919873 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -229,6 +229,17 @@ Debugging a test case The following options to the ``check`` script can be useful when debugging a failing test: +* ``-gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a + connection from a gdb client. The options given to ``gdbserver`` (e.g. the + address on which to listen for connections) are taken from the ``$GDB_OPTIONS`` + environment variable. By default (if ``$GDB_OPTIONS`` is empty), it listens on + ``localhost:12345``. + It is possible to connect to it for example with + ``gdb -iex "target remote $addr"``, where ``$addr`` is the address + ``gdbserver`` listens on. + If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored, + regardless of whether it is set or not. + * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. -- 2.31.1
Re: [PATCH v2] vhost: make SET_VRING_ADDR, SET_FEATURES send replies
On 03.08.2021 18:05, Michael S. Tsirkin wrote:
On Mon, Jul 19, 2021 at 05:21:38PM +0300, Denis Plotnikov wrote:
On vhost-user-blk migration, qemu normally sends a number of commands
to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
VHOST_USER_SET_VRING_ADDR per each started ring to enable "used ring"
data logging.
The issue is that qemu doesn't wait for reply from the vhost daemon
for these commands which may result in races between qemu expectation
of logging starting and actual login starting in vhost daemon.
The race can appear as follows: on migration setup, qemu enables dirty page
logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
vhost-user-blk daemon immediately and the daemon needs some time to turn the
logging on internally. If qemu doesn't wait for reply, after sending the
command, qemu may start migrate memory pages to a destination. At this time,
the logging may not be actually turned on in the daemon but some guest pages,
which the daemon is about to write to, may have already been transferred
without logging to the destination. Since the logging wasn't turned on,
those pages won't be transferred again as dirty. So we may end up with
corrupted data on the destination.
The same scenario is applicable for "used ring" data logging, which is
turned on with VHOST_USER_SET_VRING_ADDR command.
To resolve this issue, this patch makes qemu wait for the commands result
explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated and
logging is enabled.
Signed-off-by: Denis Plotnikov
---
v1 -> v2:
* send reply only when logging is enabled [mst]
v0 -> v1:
* send reply for SET_VRING_ADDR, SET_FEATURES only [mst]
hw/virtio/vhost-user.c | 37 ++---
1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index ee57abe04526..133588b3961e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1095,6 +1095,11 @@ static int vhost_user_set_mem_table(struct vhost_dev
*dev,
return 0;
}
+static bool log_enabled(uint64_t features)
+{
+return !!(features & (0x1ULL << VHOST_F_LOG_ALL));
+}
+
static int vhost_user_set_vring_addr(struct vhost_dev *dev,
struct vhost_vring_addr *addr)
{
@@ -1105,10 +1110,21 @@ static int vhost_user_set_vring_addr(struct vhost_dev
*dev,
.hdr.size = sizeof(msg.payload.addr),
};
+bool reply_supported = virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+if (reply_supported && log_enabled(msg.hdr.flags)) {
+msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
+}
+
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
return -1;
}
+if (msg.hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
+return process_message_reply(dev, &msg);
+}
+
return 0;
}
OK this is good, but the problem is that we then still have a race
if VHOST_USER_PROTOCOL_F_REPLY_ACK is not set. Bummer.
Let's send VHOST_USER_GET_FEATURES in this case to flush out outstanding
messages?
Ok, I've already sent v3 with related changes.
@@ -1288,7 +1304,8 @@ static int vhost_user_set_vring_call(struct vhost_dev
*dev,
return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
}
-static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
+static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64,
+ bool need_reply)
{
VhostUserMsg msg = {
.hdr.request = request,
@@ -1297,23 +1314,37 @@ static int vhost_user_set_u64(struct vhost_dev *dev,
int request, uint64_t u64)
.hdr.size = sizeof(msg.payload.u64),
};
+if (need_reply) {
+bool reply_supported = virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_REPLY_ACK);
+if (reply_supported) {
+msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
+}
+}
+
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
return -1;
}
+if (msg.hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
+return process_message_reply(dev, &msg);
+}
+
return 0;
}
static int vhost_user_set_features(struct vhost_dev *dev,
uint64_t features)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
+return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
+ log_enabled(features));
}
static int vhost_user_set_protocol_features(struct vhost_dev *dev,
uint64_t features)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features);
+return vhost_user_set_u64(dev, VHO
[PATCH v9 14/16] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Max Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy --- docs/devel/testing.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 01e1919873..8ebcf85a31 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -240,6 +240,12 @@ a failing test: If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored, regardless of whether it is set or not. +* ``-valgrind`` attaches a valgrind instance to QEMU. If it detects + warnings, it will print and save the log in + ``$TEST_DIR/.valgrind``. + The final command line will be ``valgrind --log-file=$TEST_DIR/ + .valgrind --error-exitcode=99 $QEMU ...`` + * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. -- 2.31.1
[PATCH v9 10/16] qemu-iotests: extend the check script to prepare supporting valgrind for python tests
Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.
Because the python script does not know in advance the valgrind
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Max Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/check | 7 ---
tests/qemu-iotests/iotests.py | 11 +++
tests/qemu-iotests/testenv.py | 1 +
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 4365bb8066..ebd27946db 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -39,6 +39,10 @@ def make_argparser() -> argparse.ArgumentParser:
p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
('localhost:12345' if $GDB_OPTIONS is empty)")
+p.add_argument('-valgrind', action='store_true',
+help='use valgrind, sets VALGRIND_QEMU environment '
+'variable')
+
p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -88,9 +92,6 @@ def make_argparser() -> argparse.ArgumentParser:
g_bash.add_argument('-o', dest='imgopts',
help='options to pass to qemu-img create/convert, '
'sets IMGOPTS environment variable')
-g_bash.add_argument('-valgrind', action='store_true',
-help='use valgrind, sets VALGRIND_QEMU environment '
-'variable')
g_sel = p.add_argument_group('test selecting options',
'The following options specify test set '
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e7e3d92d3e..6aa1dc48ba 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -96,6 +96,17 @@
sys.stderr.write('Please run this test via the "check" script\n')
sys.exit(os.EX_USAGE)
+qemu_valgrind = []
+if os.environ.get('VALGRIND_QEMU') == "y" and \
+os.environ.get('NO_VALGRIND') != "y":
+valgrind_logfile = "--log-file=" + test_dir
+# %p allows to put the valgrind process PID, since
+# we don't know it a priori (subprocess.Popen is
+# not yet invoked)
+valgrind_logfile += "/%p.valgrind"
+
+qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
+
socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
luks_default_secret_object = 'secret,id=keysec0,data=' + \
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8501c6caf5..8bf154376f 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -298,6 +298,7 @@ def print_env(self) -> None:
SOCK_DIR -- {SOCK_DIR}
SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
GDB_OPTIONS -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
"""
args = collections.defaultdict(str, self.get_env())
--
2.31.1
[PATCH v9 15/16] qemu-iotests: add option to show qemu binary logs on stdout
Using the flag -p, allow the qemu binary to print to stdout.
Also create the common function _close_qemu_log_file() to
avoid accessing machine.py private fields directly and have
duplicate code.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
python/qemu/machine/machine.py | 9 ++---
tests/qemu-iotests/check | 4 +++-
tests/qemu-iotests/iotests.py | 8
tests/qemu-iotests/testenv.py | 9 +++--
4 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 14c4d17eca..8b935813e9 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -348,6 +348,11 @@ def _post_launch(self) -> None:
if self._qmp_connection:
self._qmp.accept(self._qmp_timer)
+def _close_qemu_log_file(self) -> None:
+if self._qemu_log_file is not None:
+self._qemu_log_file.close()
+self._qemu_log_file = None
+
def _post_shutdown(self) -> None:
"""
Called to cleanup the VM instance after the process has exited.
@@ -360,9 +365,7 @@ def _post_shutdown(self) -> None:
self._qmp.close()
self._qmp_connection = None
-if self._qemu_log_file is not None:
-self._qemu_log_file.close()
-self._qemu_log_file = None
+self._close_qemu_log_file()
self._load_io_log()
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index ebd27946db..da1bfb839e 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,8 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-p', dest='print', action='store_true',
+help='redirects qemu\'s stdout and stderr to the test output')
p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
('localhost:12345' if $GDB_OPTIONS is empty)")
@@ -119,7 +121,7 @@ if __name__ == '__main__':
aiomode=args.aiomode, cachemode=args.cachemode,
imgopts=args.imgopts, misalign=args.misalign,
debug=args.debug, valgrind=args.valgrind,
- gdb=args.gdb)
+ gdb=args.gdb, qprint=args.print)
if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
if not args.tests:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 74fa56840d..2cf5ff965b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -79,6 +79,8 @@
if gdb_qemu_env:
qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+qemu_print = os.environ.get('PRINT_QEMU', False)
+
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
output_dir = os.environ.get('OUTPUT_DIR', '.')
@@ -613,6 +615,12 @@ def _post_shutdown(self) -> None:
else:
os.remove(valgrind_filename)
+def _pre_launch(self) -> None:
+super()._pre_launch()
+if qemu_print:
+# set QEMU binary output to stdout
+self._close_qemu_log_file()
+
def add_object(self, opts):
self._args.append('-object')
self._args.append(opts)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8bf154376f..70da0d60c8 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -74,7 +74,7 @@ class TestEnv(ContextManager['TestEnv']):
'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
- 'GDB_OPTIONS']
+ 'GDB_OPTIONS', 'PRINT_QEMU']
def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
if self.debug:
@@ -181,7 +181,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
misalign: bool = False,
debug: bool = False,
valgrind: bool = False,
- gdb: bool = False) -> None:
+ gdb: bool = False,
+ qprint: bool = False) -> None:
self.imgfmt = imgfmt
self.imgproto = imgproto
self.aiomode = aiomode
@@ -189,6 +190,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
self.misalign = misalign
self.debug = debug
+if qprint:
+self.print_qemu = 'y'
+
if gdb:
self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS)
if not self.gdb_options:
@@ -299,6 +303,7 @@ def print_env(self) -> None:
SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
GDB_OPT
[PATCH v9 12/16] qemu-iotests: allow valgrind to read/delete the generated log file
When using -valgrind on the script tests, it generates a log file
in $TEST_DIR that is either read (if valgrind finds problems) or
otherwise deleted. Provide the same exact behavior when using
-valgrind on the python tests.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
tests/qemu-iotests/iotests.py | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 26c580f9e7..85d8c0abbb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -598,6 +598,17 @@ def __init__(self, path_suffix=''):
sock_dir=sock_dir, qmp_timer=timer)
self._num_drives = 0
+def _post_shutdown(self) -> None:
+super()._post_shutdown()
+if not qemu_valgrind or not self._popen:
+return
+valgrind_filename = f"{test_dir}/{self._popen.pid}.valgrind"
+if self.exitcode() == 99:
+with open(valgrind_filename) as f:
+print(f.read())
+else:
+os.remove(valgrind_filename)
+
def add_object(self, opts):
self._args.append('-object')
self._args.append(opts)
--
2.31.1
[PATCH v3] vhost: make SET_VRING_ADDR, SET_FEATURES send replies
On vhost-user-blk migration, qemu normally sends a number of commands
to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
VHOST_USER_SET_VRING_ADDR per each started ring to enable "used ring"
data logging.
The issue is that qemu doesn't wait for reply from the vhost daemon
for these commands which may result in races between qemu expectation
of logging starting and actual login starting in vhost daemon.
The race can appear as follows: on migration setup, qemu enables dirty page
logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
vhost-user-blk daemon immediately and the daemon needs some time to turn the
logging on internally. If qemu doesn't wait for reply, after sending the
command, qemu may start migrate memory pages to a destination. At this time,
the logging may not be actually turned on in the daemon but some guest pages,
which the daemon is about to write to, may have already been transferred
without logging to the destination. Since the logging wasn't turned on,
those pages won't be transferred again as dirty. So we may end up with
corrupted data on the destination.
The same scenario is applicable for "used ring" data logging, which is
turned on with VHOST_USER_SET_VRING_ADDR command.
To resolve this issue, this patch makes qemu wait for the commands result
explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated and logging enabled.
Signed-off-by: Denis Plotnikov
---
v2 -> v3:
* send VHOST_USER_GET_FEATURES to flush out outstanding messages [mst]
v1 -> v2:
* send reply only when logging is enabled [mst]
v0 -> v1:
* send reply for SET_VRING_ADDR, SET_FEATURES only [mst]
---
hw/virtio/vhost-user.c | 130 -
1 file changed, 89 insertions(+), 41 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index ee57abe04526..18f685df549f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1095,23 +1095,6 @@ static int vhost_user_set_mem_table(struct vhost_dev
*dev,
return 0;
}
-static int vhost_user_set_vring_addr(struct vhost_dev *dev,
- struct vhost_vring_addr *addr)
-{
-VhostUserMsg msg = {
-.hdr.request = VHOST_USER_SET_VRING_ADDR,
-.hdr.flags = VHOST_USER_VERSION,
-.payload.addr = *addr,
-.hdr.size = sizeof(msg.payload.addr),
-};
-
-if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
-return -1;
-}
-
-return 0;
-}
-
static int vhost_user_set_vring_endian(struct vhost_dev *dev,
struct vhost_vring_state *ring)
{
@@ -1288,72 +1271,137 @@ static int vhost_user_set_vring_call(struct vhost_dev
*dev,
return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
}
-static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
+
+static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t
*u64)
{
VhostUserMsg msg = {
.hdr.request = request,
.hdr.flags = VHOST_USER_VERSION,
-.payload.u64 = u64,
-.hdr.size = sizeof(msg.payload.u64),
};
+if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
+return 0;
+}
+
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
return -1;
}
+if (vhost_user_read(dev, &msg) < 0) {
+return -1;
+}
+
+if (msg.hdr.request != request) {
+error_report("Received unexpected msg type. Expected %d received %d",
+ request, msg.hdr.request);
+return -1;
+}
+
+if (msg.hdr.size != sizeof(msg.payload.u64)) {
+error_report("Received bad msg size.");
+return -1;
+}
+
+*u64 = msg.payload.u64;
+
return 0;
}
-static int vhost_user_set_features(struct vhost_dev *dev,
- uint64_t features)
+static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
+return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
}
-static int vhost_user_set_protocol_features(struct vhost_dev *dev,
-uint64_t features)
+static int enforce_reply(struct vhost_dev *dev)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features);
+ /*
+* we need a reply but can't get it from some command directly,
+* so send the command which must send a reply to make sure
+* the command we sent before is actually completed.
+*/
+uint64_t dummy;
+return vhost_user_get_features(dev, &dummy);
}
-static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t
*u64)
+static int vhost_user_set_vring_addr(struct vhost_dev *dev,
+ struct vhost_vring_addr *addr)
{
VhostUserMsg msg = {
-.hdr.request
[PATCH v9 16/16] docs/devel/testing: add -p option to the debug section of QEMU iotests
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- docs/devel/testing.rst | 4 1 file changed, 4 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 8ebcf85a31..4a0abbf23d 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -249,6 +249,10 @@ a failing test: * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. +* ``-p`` (print) redirects QEMU’s stdout and stderr to the test output, + instead of saving it into a log file in + ``$TEST_DIR/qemu-machine-``. + Test case groups -- 2.31.1
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read
Hi, > As a side note(not strictly related to this fix) should we continue > joining reproducer patches with the fixes? In order to test the > reproducer, you need to cleave the fix off the patch. At the same time > we don't want to mess up bisection, so does it make sense to have the > reproducer patch be separate but come last in the series? Yes, I think it makes sense to send the testcase as separate patch, and the ordering (fix first, testcase second) makes sense too. If they are separated it is easy enough to create a local test branch with a different order, or to just temporarily revert the fix to test the testcase. take care, Gerd
Re: [PATCH 0/5] [RfC] monitor/hmp: command register support
On Sat, Aug 07, 2021 at 12:36:13PM +0200, Markus Armbruster wrote: > Gerd Hoffmann writes: > > > Helps making qemu more modular, > > see commit messages for details. > > > > Depends on the "modules: add meta-data database" patch series. > > This series is about HMP. Do we have equivalent functionality for QMP > already? Didn't check as the commands I've needed it this for are hmp-only. But, yes, when going forward with building more code modular we might need this for qmp too at some point ... take care, Gerd
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read
On Fri, 6 Aug 2021 at 15:43, Alexander Bulekov wrote: > As a side note(not strictly related to this fix) should we continue > joining reproducer patches with the fixes? In order to test the > reproducer, you need to cleave the fix off the patch. At the same time > we don't want to mess up bisection, so does it make sense to have the > reproducer patch be separate but come last in the series? My preference is for the test case as a separate patch, last in the series. For this kind of minor easy-to-review fix it matters less, but sometimes the right fix for a problem might be larger or more complicated, and then having the test case in the same patch makes that patch awkwardly large. Also the person able to review the code change and the person able to review the test case might not be the same... thanks -- PMM
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read
On 8/6/21 4:42 PM, Alexander Bulekov wrote:
> On 210804 1451, Qiang Liu wrote:
>> xlnx_dp_read allows an out-of-bounds read at its default branch because
>> of an improper index.
>>
>> According to
>> https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
>> (DP Module), registers 0x3A4/0x3A4/0x3AC are allowed.
>>
>> DP_INT_MASK 0x03A4 32 mixed 0xF03F Interrupt
>> Mask Register for intrN.
>> DP_INT_EN 0x03A8 32 mixed 0x Interrupt
>> Enable Register.
>> DP_INT_DS 0x03AC 32 mixed 0x Interrupt
>> Disable Register.
>>
>> In xlnx_dp_write, when the offset is 0x3A8 and 0x3AC, the virtual device
>> will write s->core_registers[0x3A4
2]. That is to say, the maxize of s->core_registers could be ((0x3A4
2) + 1). However, the current size of s->core_registers is (0x3AF >>
2), that is ((0x3A4 >> 2) + 2), which is out of the range.
>> In xlxn_dp_read, the access to offset 0x3A8 or 0x3AC will be directed to
>> the offset 0x3A8 (incorrect functionality) or 0x3AC (out-of-bounds read)
>> rather than 0x3A4.
>>
>> This patch enforces the read access to offset 0x3A8 and 0x3AC to 0x3A4,
>> but does not adjust the size of s->core_registers to avoid breaking
>> migration.
>>
>> Fixes: 58ac482a66de ("introduce xlnx-dp")
>> Signed-off-by: Qiang Liu
>
> Acked-by: Alexander Bulekov
>
> If there is somehow a regression, the test won't fail in a fatal way, so
> maybe it makes sense to throw in a setenv(UBSAN_OPTIONS=halt_on_error=1)
Where? Main meson? qtests meson? setenv() in the test (but would
override preset variable)?
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read
Thank you for all the insightful comments about the separated patches. This would be my first time to format a serial of patches. Does it look like below? [PATCH v3 00/2] title [PATCH v3 01/2] fix [PATCH v3 02/2] test Best, Qiang On Mon, Aug 9, 2021 at 11:24 AM Peter Maydell wrote: > > On Fri, 6 Aug 2021 at 15:43, Alexander Bulekov wrote: > > As a side note(not strictly related to this fix) should we continue > > joining reproducer patches with the fixes? In order to test the > > reproducer, you need to cleave the fix off the patch. At the same time > > we don't want to mess up bisection, so does it make sense to have the > > reproducer patch be separate but come last in the series? > > My preference is for the test case as a separate patch, last > in the series. For this kind of minor easy-to-review fix it > matters less, but sometimes the right fix for a problem might > be larger or more complicated, and then having the test case > in the same patch makes that patch awkwardly large. > > Also the person able to review the code change and the person > able to review the test case might not be the same... > > thanks > -- PMM
Re: [PATCH v3] vhost: make SET_VRING_ADDR, SET_FEATURES send replies
Looks good. Some cosmetics:
On Mon, Aug 09, 2021 at 12:03:30PM +0300, Denis Plotnikov wrote:
> On vhost-user-blk migration, qemu normally sends a number of commands
> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
> VHOST_USER_SET_VRING_ADDR per each started ring to enable "used ring"
> data logging.
> The issue is that qemu doesn't wait for reply from the vhost daemon
> for these commands which may result in races between qemu expectation
> of logging starting and actual login starting in vhost daemon.
>
> The race can appear as follows: on migration setup, qemu enables dirty page
> logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
> vhost-user-blk daemon immediately and the daemon needs some time to turn the
> logging on internally. If qemu doesn't wait for reply, after sending the
> command, qemu may start migrate memory pages to a destination. At this time,
start migrating
> the logging may not be actually turned on in the daemon but some guest pages,
> which the daemon is about to write to, may have already been transferred
> without logging to the destination. Since the logging wasn't turned on,
> those pages won't be transferred again as dirty. So we may end up with
> corrupted data on the destination.
> The same scenario is applicable for "used ring" data logging, which is
> turned on with VHOST_USER_SET_VRING_ADDR command.
>
> To resolve this issue, this patch makes qemu wait for the commands result
command result
> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated and logging
> enabled.
typo
>
> Signed-off-by: Denis Plotnikov
>
> ---
> v2 -> v3:
> * send VHOST_USER_GET_FEATURES to flush out outstanding messages [mst]
>
> v1 -> v2:
> * send reply only when logging is enabled [mst]
>
> v0 -> v1:
> * send reply for SET_VRING_ADDR, SET_FEATURES only [mst]
> ---
> hw/virtio/vhost-user.c | 130 -
> 1 file changed, 89 insertions(+), 41 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index ee57abe04526..18f685df549f 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1095,23 +1095,6 @@ static int vhost_user_set_mem_table(struct vhost_dev
> *dev,
> return 0;
> }
>
> -static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> - struct vhost_vring_addr *addr)
> -{
> -VhostUserMsg msg = {
> -.hdr.request = VHOST_USER_SET_VRING_ADDR,
> -.hdr.flags = VHOST_USER_VERSION,
> -.payload.addr = *addr,
> -.hdr.size = sizeof(msg.payload.addr),
> -};
> -
> -if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> -return -1;
> -}
> -
> -return 0;
> -}
> -
> static int vhost_user_set_vring_endian(struct vhost_dev *dev,
> struct vhost_vring_state *ring)
> {
> @@ -1288,72 +1271,137 @@ static int vhost_user_set_vring_call(struct
> vhost_dev *dev,
> return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
> }
>
> -static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t
> u64)
> +
> +static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t
> *u64)
> {
> VhostUserMsg msg = {
> .hdr.request = request,
> .hdr.flags = VHOST_USER_VERSION,
> -.payload.u64 = u64,
> -.hdr.size = sizeof(msg.payload.u64),
> };
>
> +if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
> +return 0;
> +}
> +
> if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> return -1;
> }
>
> +if (vhost_user_read(dev, &msg) < 0) {
> +return -1;
> +}
> +
> +if (msg.hdr.request != request) {
> +error_report("Received unexpected msg type. Expected %d received %d",
> + request, msg.hdr.request);
> +return -1;
> +}
> +
> +if (msg.hdr.size != sizeof(msg.payload.u64)) {
> +error_report("Received bad msg size.");
> +return -1;
> +}
> +
> +*u64 = msg.payload.u64;
> +
> return 0;
> }
>
> -static int vhost_user_set_features(struct vhost_dev *dev,
> - uint64_t features)
> +static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
> {
> -return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
> +return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
> }
>
> -static int vhost_user_set_protocol_features(struct vhost_dev *dev,
> -uint64_t features)
> +static int enforce_reply(struct vhost_dev *dev)
> {
> -return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES,
> features);
> + /*
> +* we need a reply but can't get it from some command directly,
> +* so send the command which must send a reply
> to make su
Re: [PATCH] target/riscv: Add User CSRs read-only check
On Mon, Aug 9, 2021 at 3:09 PM LIU Zhiwei wrote:
>
nits: please write something in the commit message
> Signed-off-by: LIU Zhiwei
> ---
> target/riscv/csr.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 9a4ed18ac5..ea62d9e653 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1422,11 +1422,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
> csrno,
> RISCVException ret;
> target_ulong old_value;
> RISCVCPU *cpu = env_archcpu(env);
> +int read_only = get_field(csrno, 0xC00) == 3;
>
> /* check privileges and return -1 if check fails */
> #if !defined(CONFIG_USER_ONLY)
> int effective_priv = env->priv;
> -int read_only = get_field(csrno, 0xC00) == 3;
>
> if (riscv_has_ext(env, RVH) &&
> env->priv == PRV_S &&
> @@ -1443,6 +1443,10 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
> csrno,
> (!env->debugger && (effective_priv < get_field(csrno, 0x300 {
> return RISCV_EXCP_ILLEGAL_INST;
> }
> +#else
> +if (write_mask && read_only) {
This can be merged by the !CONFIG_USER_ONLY case.
> +return -RISCV_EXCP_ILLEGAL_INST;
> +}
> #endif
>
> /* ensure the CSR extension is enabled. */
Regards,
Bin
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read
On 210809 1131, Philippe Mathieu-Daudé wrote:
> On 8/6/21 4:42 PM, Alexander Bulekov wrote:
> > On 210804 1451, Qiang Liu wrote:
> >> xlnx_dp_read allows an out-of-bounds read at its default branch because
> >> of an improper index.
> >>
> >> According to
> >> https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
> >> (DP Module), registers 0x3A4/0x3A4/0x3AC are allowed.
> >>
> >> DP_INT_MASK 0x03A4 32 mixed 0xF03F Interrupt
> >> Mask Register for intrN.
> >> DP_INT_EN 0x03A8 32 mixed 0x Interrupt
> >> Enable Register.
> >> DP_INT_DS 0x03AC 32 mixed 0x Interrupt
> >> Disable Register.
> >>
> >> In xlnx_dp_write, when the offset is 0x3A8 and 0x3AC, the virtual device
> >> will write s->core_registers[0x3A4
> 2]. That is to say, the maxize of s->core_registers could be ((0x3A4
> 2) + 1). However, the current size of s->core_registers is (0x3AF >>
> 2), that is ((0x3A4 >> 2) + 2), which is out of the range.
> >> In xlxn_dp_read, the access to offset 0x3A8 or 0x3AC will be directed to
> >> the offset 0x3A8 (incorrect functionality) or 0x3AC (out-of-bounds read)
> >> rather than 0x3A4.
> >>
> >> This patch enforces the read access to offset 0x3A8 and 0x3AC to 0x3A4,
> >> but does not adjust the size of s->core_registers to avoid breaking
> >> migration.
> >>
> >> Fixes: 58ac482a66de ("introduce xlnx-dp")
> >> Signed-off-by: Qiang Liu
> >
> > Acked-by: Alexander Bulekov
> >
> > If there is somehow a regression, the test won't fail in a fatal way, so
> > maybe it makes sense to throw in a setenv(UBSAN_OPTIONS=halt_on_error=1)
>
> Where? Main meson? qtests meson? setenv() in the test (but would
> override preset variable)?
>
Probably in the test, with overwrite = 0 ? Without halt_on_error the
test will succeed even if the problem returns..
-Alex
Re: [PATCH] block/export/fuse.c: fix musl build
On 8/9/21 10:50 AM, Fabrice Fontaine wrote:
> Fix the following build failure on musl raised since version 6.0.0 and
> https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
> because musl does not define FALLOC_FL_ZERO_RANGE:
>
> ../block/export/fuse.c: In function 'fuse_fallocate':
> ../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared
> (first use in this function)
> 563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
> | ^~~~
>
> Fixes:
> -
> http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
>
> Signed-off-by: Fabrice Fontaine
> ---
> block/export/fuse.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index ada9e263eb..07e31129a6 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -635,6 +635,7 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> +#ifdef FALLOC_FL_ZERO_RANGE
Please use CONFIG_FALLOCATE_ZERO_RANGE.
> } else if (mode & FALLOC_FL_ZERO_RANGE) {
> if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
> /* No need for zeroes, we are going to write them ourselves */
> @@ -654,6 +655,7 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> +#endif
> } else if (!mode) {
> /* We can only fallocate at the EOF with a truncate */
> if (offset < blk_len) {
>
Maybe safer #ifdef'ry as:
-- >8 --
diff --git a/block/export/fuse.c b/block/export/fuse.c
index ada9e263ebb..fc7b07d2b57 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req,
fuse_ino_t inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (mode & FALLOC_FL_ZERO_RANGE) {
+}
+#ifdef CONFIG_FALLOCATE_ZERO_RANGE
+else if (mode & FALLOC_FL_ZERO_RANGE) {
if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
/* No need for zeroes, we are going to write them ourselves */
ret = fuse_do_truncate(exp, offset + length, false,
@@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req,
fuse_ino_t inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (!mode) {
+}
+#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
+else if (!mode) {
/* We can only fallocate at the EOF with a truncate */
if (offset < blk_len) {
fuse_reply_err(req, EOPNOTSUPP);
---
Re: [PATCH] xive: Remove extra '0x' prefix in trace events
On 8/9/21 10:52 AM, Cédric Le Goater wrote:
> Cc: th...@redhat.com
> Fixes: 4e960974d4ee ("xive: Add trace events")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/519
> Signed-off-by: Cédric Le Goater
> ---
> hw/intc/trace-events | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index e56e7dd3b667..6a17d38998d9 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -219,14 +219,14 @@ kvm_xive_source_reset(uint32_t srcno) "IRQ 0x%x"
> xive_tctx_accept(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x
> CPPR=0x%02x NSR=0x%02x ACK"
> xive_tctx_notify(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x
> CPPR=0x%02x NSR=0x%02x raise !"
> xive_tctx_set_cppr(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x new
> CPPR=0x%02x NSR=0x%02x"
> -xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
> -xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
> +xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
> +xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
> xive_router_end_notify(uint8_t end_blk, uint32_t end_idx, uint32_t end_data)
> "END 0x%02x/0x%04x -> enqueue 0x%08x"
> xive_router_end_escalate(uint8_t end_blk, uint32_t end_idx, uint8_t esc_blk,
> uint32_t esc_idx, uint32_t end_data) "END 0x%02x/0x%04x -> escalate END
> 0x%02x/0x%04x data 0x%08x"
> -xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value)
> "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
> -xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value)
> "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
> +xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value)
> "@0x%"PRIx64" sz=%d val=0x%" PRIx64
> +xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value)
> "@0x%"PRIx64" sz=%d val=0x%" PRIx64
> xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring)
> "found NVT 0x%x/0x%x ring=0x%x"
> -xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END
> 0x%x/0x%x @0x0x%"PRIx64
> +xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END
> 0x%x/0x%x @0x%"PRIx64
>
> # pnv_xive.c
> pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64"
> val=0x%"PRIx64
>
Acceptable for 6.1 IMHO.
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH] xive: Remove extra '0x' prefix in trace events
Le 09/08/2021 à 10:52, Cédric Le Goater a écrit :
> Cc: th...@redhat.com
> Fixes: 4e960974d4ee ("xive: Add trace events")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/519
> Signed-off-by: Cédric Le Goater
> ---
> hw/intc/trace-events | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index e56e7dd3b667..6a17d38998d9 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -219,14 +219,14 @@ kvm_xive_source_reset(uint32_t srcno) "IRQ 0x%x"
> xive_tctx_accept(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x
> CPPR=0x%02x NSR=0x%02x ACK"
> xive_tctx_notify(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x
> CPPR=0x%02x NSR=0x%02x raise !"
> xive_tctx_set_cppr(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr,
> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x new
> CPPR=0x%02x NSR=0x%02x"
> -xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
> -xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
> +xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
> +xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value)
> "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
> xive_router_end_notify(uint8_t end_blk, uint32_t end_idx, uint32_t end_data)
> "END 0x%02x/0x%04x -> enqueue 0x%08x"
> xive_router_end_escalate(uint8_t end_blk, uint32_t end_idx, uint8_t esc_blk,
> uint32_t esc_idx, uint32_t end_data) "END 0x%02x/0x%04x -> escalate END
> 0x%02x/0x%04x data 0x%08x"
> -xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value)
> "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
> -xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value)
> "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
> +xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value)
> "@0x%"PRIx64" sz=%d val=0x%" PRIx64
> +xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value)
> "@0x%"PRIx64" sz=%d val=0x%" PRIx64
> xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring)
> "found NVT 0x%x/0x%x ring=0x%x"
> -xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END
> 0x%x/0x%x @0x0x%"PRIx64
> +xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END
> 0x%x/0x%x @0x%"PRIx64
>
> # pnv_xive.c
> pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64"
> val=0x%"PRIx64
>
Reviewed-by: Laurent Vivier
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read
On 8/9/21 11:33 AM, Qiang Liu wrote: > Thank you for all the insightful comments about the separated patches. > This would be my first time to format a serial of patches. Does it > look like below? > [PATCH v3 00/2] title > [PATCH v3 01/2] fix > [PATCH v3 02/2] test Exactly. Otherwise, adding the test before the fix would make an incremental build to fail.
Re: [PATCH] target/riscv: Add User CSRs read-only check
On 2021/8/9 下午5:35, Bin Meng wrote:
On Mon, Aug 9, 2021 at 3:09 PM LIU Zhiwei wrote:
nits: please write something in the commit message
OK
Signed-off-by: LIU Zhiwei
---
target/riscv/csr.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9a4ed18ac5..ea62d9e653 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1422,11 +1422,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
csrno,
RISCVException ret;
target_ulong old_value;
RISCVCPU *cpu = env_archcpu(env);
+int read_only = get_field(csrno, 0xC00) == 3;
/* check privileges and return -1 if check fails */
#if !defined(CONFIG_USER_ONLY)
int effective_priv = env->priv;
-int read_only = get_field(csrno, 0xC00) == 3;
if (riscv_has_ext(env, RVH) &&
env->priv == PRV_S &&
@@ -1443,6 +1443,10 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
(!env->debugger && (effective_priv < get_field(csrno, 0x300 {
return RISCV_EXCP_ILLEGAL_INST;
}
+#else
+if (write_mask && read_only) {
This can be merged by the !CONFIG_USER_ONLY case.
Do you mean
#if !defined(CONFIG_USER_ONLY)
if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
return -RISCV_EXCP_ILLEGAL_INST;
}
#else
if (write_mask && read_only) {
return -RISCV_EXCP_ILLEGAL_INST;
}
#endif
Thanks,
Zhiwei
+return -RISCV_EXCP_ILLEGAL_INST;
+}
#endif
/* ensure the CSR extension is enabled. */
Regards,
Bin
[PULL 1/1] meson: fix logic for gnutls check
From: Alyssa Ross
The logic before was
if not get_option('gnutls').auto() or have_system
Which is equivalent to
if get_option('gnutls').enabled() or get_option('gnutls').disabled() or
have_system
This means that the check for gnutls is performed even if gnutls is
disabled, which means that the build system will insist on having
libtasn1 if gnutls is found, even if gnutls support is disabled.
When gnutls is disabled, the check for gnutls shouldn't be performed,
to ensure that further build system logic (like the check for
libtasn1) doesn't make decisions based on the presence of gnutls,
rather than the gnutls option.
After making this change, I can successfully ./configure --disable-gnutls
on my system with gnutls installed, but not libtasn1.
Signed-off-by: Alyssa Ross
Message-Id: <20210806144947.321647-1...@alyssa.is>
Acked-by: Paolo Bonzini
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index af9bbb83db..b3e7ec0e92 100644
--- a/meson.build
+++ b/meson.build
@@ -824,7 +824,7 @@ endif
gnutls = not_found
gnutls_crypto = not_found
-if not get_option('gnutls').auto() or have_system
+if get_option('gnutls').enabled() or (get_option('gnutls').auto() and
have_system)
# For general TLS support our min gnutls matches
# that implied by our platform support matrix
#
--
2.31.1
[PULL 0/1] Build system patch for 6.1-rc3
The following changes since commit dee64246ded3aa7dbada68b96ce1c64e5bea327d: Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging (2021-08-06 10:28:33 +0100) are available in the Git repository at: https://gitlab.com/bonzini/qemu.git tags/for-upstream for you to fetch changes up to abc14fd05606274d8350f1f90d1ec7bc9e51aa21: meson: fix logic for gnutls check (2021-08-06 15:36:11 +) Fix for gnutls-crypto detection Alyssa Ross (1): meson: fix logic for gnutls check meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.31.1
Re: [PULL 05/18] hw/riscv: virt: Allow creating multiple NUMA sockets
On Tue, 25 Aug 2020 at 20:03, Alistair Francis wrote:
>
> From: Anup Patel
>
> We extend RISC-V virt machine to allow creating a multi-socket
> machine. Each RISC-V virt machine socket is a NUMA node having
> a set of HARTs, a memory instance, a CLINT instance, and a PLIC
> instance. Other devices are shared between all sockets. We also
> update the generated device tree accordingly.
Hi; Coverity (CID 1460752) points out that this code has
a misunderstanding of the length argument to strncat().
(I think this patch is just doing code-movement of this block of code,
but it seemed like the easiest place to send an email about the issue.)
> +/* Per-socket PLIC hart topology configuration string */
> +plic_hart_config_len =
> +(strlen(VIRT_PLIC_HART_CONFIG) + 1) * hart_count;
> +plic_hart_config = g_malloc0(plic_hart_config_len);
> +for (j = 0; j < hart_count; j++) {
> +if (j != 0) {
> +strncat(plic_hart_config, ",", plic_hart_config_len);
> +}
> +strncat(plic_hart_config, VIRT_PLIC_HART_CONFIG,
> +plic_hart_config_len);
> +plic_hart_config_len -= (strlen(VIRT_PLIC_HART_CONFIG) + 1);
> +}
The length argument to strncat() is here being used as if it were
"do not write more than length bytes", but strncat() will write
length+1 bytes in the "source too long" case (length characters
from the source string plus the trailing NUL). This isn't actually
an issue here because we carefully precalculate the allocation length
to be exactly correct, but it means that the code looks like it has
a guard against accidental miscalculation and overrun but it doesn't.
It might be preferable to write this to use glib string methods
rather than raw strlen/strncat, for example:
const char **vals;
const char *hart_config = VIRT_PLIC_HART_CONFIG;
int i;
vals = g_new(char *, hart_count + 1);
for (i = 0; i < hart_count; i++) {
vals[i] = (gchar *)hart_config;
}
vals[i] = NULL;
/* g_strjoinv() obliges us to discard 'const' here :-( */
plic_hart_config = g_strjoinv(",", (char **)vals);
Untested, but same structure as used in ppc_compat_add_property()
and qemu_rbd_mon_host(). Relieves us of the obligation to
carefully calculate string lengths and makes it clearer that
we're constructing a comma-separated string.
thanks
-- PMM
Re: [PATCH-for-6.2 2/2] disas/nios2: Simplify endianess conversion
On 8/9/21 8:12 AM, Thomas Huth wrote:
> On 07/08/2021 13.09, Philippe Mathieu-Daudé wrote:
>> Since commit 12b6e9b27d4 ("disas: Clean up CPUDebug initialization")
>> the disassemble_info->bfd_endian enum is set for all targets in
>> target_disas(). We can directly call print_insn_nios2() and simplify.
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> include/disas/dis-asm.h | 3 +--
>> disas/nios2.c | 22 +++---
>> target/nios2/cpu.c | 6 +-
>> 3 files changed, 5 insertions(+), 26 deletions(-)
> [...]
>> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
>> index 5e37defef80..5b503b5bb99 100644
>> --- a/target/nios2/cpu.c
>> +++ b/target/nios2/cpu.c
>> @@ -146,11 +146,7 @@ static void nios2_cpu_disas_set_info(CPUState
>> *cpu, disassemble_info *info)
>> {
>> /* NOTE: NiosII R2 is not supported yet. */
>> info->mach = bfd_arch_nios2;
>> -#ifdef TARGET_WORDS_BIGENDIAN
>> - info->print_insn = print_insn_big_nios2;
>> -#else
>> - info->print_insn = print_insn_little_nios2;
>> -#endif
>> + info->print_insn = print_insn_nios2;
>> }
>
> Oh, wow, I didn't even know that nios2 could be compiled with different
> endianness? I mean, we only have a "nios2-softmmu" target, not something
> like "nios2be-softmmu" and "nios2le-softmmu" ?
$ git grep ENDIAN configs/targets/nios2-*
$
We only build little-endian targets, but looking at commit
b7862564880 ("nios2: Add Altera 10M50 GHRD emulation")
hw/nios2/boot.c is ready to load big-endian ELF if we were
building the big-endian targets.
> Anyway, your patch makes a lot of sense to clean this up.
>
> Reviewed-by: Thomas Huth
>
>
[RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables
Signed-off-by: Kostiantyn Kostiuk
---
qga/commands-win32.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 724ce76a0e..a8a601776d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -539,9 +539,9 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
-PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
+g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data =
NULL;
STORAGE_DEVICE_NUMBER sdn;
-char *parent_dev_id = NULL;
+g_autofree char *parent_dev_id = NULL;
HDEVINFO parent_dev_info;
SP_DEVINFO_DATA parent_dev_info_data;
DWORD j;
--
2.25.1
[RFC 1/4] gqa-win: get_pci_info: Clean dev_info if handle is valid
Signed-off-by: Kostiantyn Kostiuk
---
qga/commands-win32.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 27baf17d6c..2ad8593b82 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -514,7 +514,7 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
static GuestPCIAddress *get_pci_info(int number, Error **errp)
{
-HDEVINFO dev_info;
+HDEVINFO dev_info = INVALID_HANDLE_VALUE;
SP_DEVINFO_DATA dev_info_data;
SP_DEVICE_INTERFACE_DATA dev_iface_data;
HANDLE dev_file;
@@ -749,7 +749,9 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
}
free_dev_info:
-SetupDiDestroyDeviceInfoList(dev_info);
+if (dev_info != INVALID_HANDLE_VALUE) {
+SetupDiDestroyDeviceInfoList(dev_info);
+}
out:
return pci;
}
--
2.25.1
[RFC 4/4] gqa-win: get_pci_info: Free parent_dev_info properly
In case when the function fails to get parent device data,
the parent_dev_info variable will be initialized, but not freed.
Signed-off-by: Kostiantyn Kostiuk
---
qga/commands-win32.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index a8a601776d..520c520cb8 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -515,6 +515,8 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
static GuestPCIAddress *get_pci_info(int number, Error **errp)
{
HDEVINFO dev_info = INVALID_HANDLE_VALUE;
+HDEVINFO parent_dev_info = INVALID_HANDLE_VALUE;
+
SP_DEVINFO_DATA dev_info_data;
SP_DEVICE_INTERFACE_DATA dev_iface_data;
HANDLE dev_file;
@@ -542,7 +544,6 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data =
NULL;
STORAGE_DEVICE_NUMBER sdn;
g_autofree char *parent_dev_id = NULL;
-HDEVINFO parent_dev_info;
SP_DEVINFO_DATA parent_dev_info_data;
DWORD j;
DWORD size = 0;
@@ -745,6 +746,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
}
}
SetupDiDestroyDeviceInfoList(parent_dev_info);
+parent_dev_info = INVALID_HANDLE_VALUE;
break;
}
@@ -752,6 +754,9 @@ cleanup:
if (dev_info != INVALID_HANDLE_VALUE) {
SetupDiDestroyDeviceInfoList(dev_info);
}
+if (parent_dev_info != INVALID_HANDLE_VALUE) {
+SetupDiDestroyDeviceInfoList(parent_dev_info);
+}
return pci;
}
--
2.25.1
[Bug 1813045] Re: qemu-ga fsfreeze crashes the kernel
See also https://gitlab.com/qemu-project/qemu/-/issues/520 ** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #520 https://gitlab.com/qemu-project/qemu/-/issues/520 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1813045 Title: qemu-ga fsfreeze crashes the kernel Status in QEMU: Expired Bug description: We use mainly Cloudlinux, Debian and Centos. We experienced many crashes on our qemu instances based on Cloudlinux during a snapshot. The issue is not related to CloudLinux directly, but to Qemu agent, which does not freeze the file system(s) correctly. What is actually happening: When VM backup is invoked, Qemu agent freezes the file systems, so no single change will be made during the backup. But Qemu agent does not respect the loop* devices in freezing order (we have checked its sources), which leads to the next situation: 1) freeze loopback fs ---> send async reqs to loopback thread 2) freeze main fs 3) loopback thread wakes up and trying to write data to the main fs, which is still frozen, and this finally leads to the hung task and kernel crash. I believe this is the culprit: /dev/loop0 /tmp ext3 rw,nosuid,noexec,relatime,data=ordered 0 0 /dev/loop0 /var/tmp ext3 rw,nosuid,noexec,relatime,data=ordered 0 0 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1813045/+subscriptions
[RFC 2/4] gqa-win: get_pci_info: Use common 'cleanup' label
To prevent memory leaks, always try to free initialized variables.
Signed-off-by: Kostiantyn Kostiuk
---
qga/commands-win32.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 2ad8593b82..724ce76a0e 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -532,7 +532,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
if (dev_info == INVALID_HANDLE_VALUE) {
error_setg_win32(errp, GetLastError(), "failed to get devices tree");
-goto out;
+goto cleanup;
}
g_debug("enumerating devices");
@@ -562,7 +562,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
} else {
error_setg_win32(errp, GetLastError(),
"failed to get device interfaces");
-goto free_dev_info;
+goto cleanup;
}
}
@@ -576,7 +576,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
CloseHandle(dev_file);
error_setg_win32(errp, GetLastError(),
"failed to get device slot number");
-goto free_dev_info;
+goto cleanup;
}
CloseHandle(dev_file);
@@ -586,7 +586,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
} else {
error_setg_win32(errp, GetLastError(),
"failed to get device interfaces");
-goto free_dev_info;
+goto cleanup;
}
g_debug("found device slot %d. Getting storage controller", number);
@@ -603,7 +603,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
} else {
error_setg_win32(errp, GetLastError(),
"failed to get device instance ID");
-goto out;
+goto cleanup;
}
}
@@ -617,14 +617,14 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
g_error("CM_Locate_DevInst failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get device instance");
-goto out;
+goto cleanup;
}
cr = CM_Get_Parent(&parent_dev_inst, dev_inst, 0);
if (cr != CR_SUCCESS) {
g_error("CM_Get_Parent failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get parent device instance");
-goto out;
+goto cleanup;
}
cr = CM_Get_Device_ID_Size(&dev_id_size, parent_dev_inst, 0);
@@ -632,7 +632,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
g_error("CM_Get_Device_ID_Size failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get parent device ID length");
-goto out;
+goto cleanup;
}
++dev_id_size;
@@ -647,7 +647,7 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
g_error("CM_Get_Device_ID failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get parent device ID");
-goto out;
+goto cleanup;
}
}
@@ -661,14 +661,14 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
if (parent_dev_info == INVALID_HANDLE_VALUE) {
error_setg_win32(errp, GetLastError(),
"failed to get parent device");
-goto out;
+goto cleanup;
}
parent_dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
if (!SetupDiEnumDeviceInfo(parent_dev_info, 0, &parent_dev_info_data))
{
error_setg_win32(errp, GetLastError(),
"failed to get parent device data");
-goto out;
+goto cleanup;
}
for (j = 0;
@@ -748,11 +748,10 @@ static GuestPCIAddress *get_pci_info(int number, Error
**errp)
break;
}
-free_dev_info:
+cleanup:
if (dev_info != INVALID_HANDLE_VALUE) {
SetupDiDestroyDeviceInfoList(dev_info);
}
-out:
return pci;
}
--
2.25.1
Re: [PATCH] target/riscv: Add User CSRs read-only check
On Mon, Aug 9, 2021 at 5:45 PM LIU Zhiwei wrote:
>
>
> On 2021/8/9 下午5:35, Bin Meng wrote:
> > On Mon, Aug 9, 2021 at 3:09 PM LIU Zhiwei wrote:
> > nits: please write something in the commit message
> OK
> >
> >> Signed-off-by: LIU Zhiwei
> >> ---
> >> target/riscv/csr.c | 6 +-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 9a4ed18ac5..ea62d9e653 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -1422,11 +1422,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
> >> csrno,
> >> RISCVException ret;
> >> target_ulong old_value;
> >> RISCVCPU *cpu = env_archcpu(env);
> >> +int read_only = get_field(csrno, 0xC00) == 3;
> >>
> >> /* check privileges and return -1 if check fails */
> >> #if !defined(CONFIG_USER_ONLY)
> >> int effective_priv = env->priv;
> >> -int read_only = get_field(csrno, 0xC00) == 3;
> >>
> >> if (riscv_has_ext(env, RVH) &&
> >> env->priv == PRV_S &&
> >> @@ -1443,6 +1443,10 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
> >> csrno,
> >> (!env->debugger && (effective_priv < get_field(csrno, 0x300 {
> >> return RISCV_EXCP_ILLEGAL_INST;
> >> }
> >> +#else
> >> +if (write_mask && read_only) {
> > This can be merged by the !CONFIG_USER_ONLY case.
>
> Do you mean
>
> #if !defined(CONFIG_USER_ONLY)
> if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
> return -RISCV_EXCP_ILLEGAL_INST;
> }
> #else
Should be #endif
> if (write_mask && read_only) {
> return -RISCV_EXCP_ILLEGAL_INST;
> }
>
> #endif
and drop this #endif
Regards,
Bin
[Bug 1939179] Re: qemu-ga fsfreeze crashes the kernel
https://gitlab.com/qemu-project/qemu/-/issues/520 ... thanks! ** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #520 https://gitlab.com/qemu-project/qemu/-/issues/520 ** Changed in: qemu Status: Incomplete => Invalid -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1939179 Title: qemu-ga fsfreeze crashes the kernel Status in QEMU: Invalid Bug description: Hello, Still required your attention, duplicate from: https://bugs.launchpad.net/bugs/1807073 https://bugs.launchpad.net/bugs/1813045 We use mainly Cloudlinux, Debian and Centos. We experienced many crashes on our qemu instances based on Cloudlinux during a snapshot. The issue is not related to CloudLinux directly, but to Qemu agent, which does not freeze the file system(s) correctly. What is actually happening: When VM backup is invoked, Qemu agent freezes the file systems, so no single change will be made during the backup. But Qemu agent does not respect the loop* devices in freezing order (we have checked its sources), which leads to the next situation: 1) freeze loopback fs ---> send async reqs to loopback thread 2) freeze main fs 3) loopback thread wakes up and trying to write data to the main fs, which is still frozen, and this finally leads to the hung task and kernel crash. Moreover, a lot of Proxmox users are complaining about the issue as well: https://forum.proxmox.com/threads/error-vm-100-qmp-command-guest-fsfreeze-thaw-failed-got-timeout.68082/ https://forum.proxmox.com/threads/problem-with-fsfreeze-freeze-and-qemu-guest-agent.65707/ To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1939179/+subscriptions
[Bug 1807073] Re: qemu-guest-agent stop work when fsfreeze
Re-opened in the new bug tracker here: https://gitlab.com/qemu- project/qemu/-/issues/520 ** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #520 https://gitlab.com/qemu-project/qemu/-/issues/520 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1807073 Title: qemu-guest-agent stop work when fsfreeze Status in QEMU: Expired Bug description: Create a live snapshot, we should first to fsfreeze the file system. We do have only one disk mounted to /: Filesystem Size Used Avail Use% Mounted on udev 48G 0 48G 0% /dev tmpfs 9.5G 8.7M 9.5G 1% /run /dev/vda1 485G 1.5G 484G 1% / tmpfs48G 0 48G 0% /dev/shm tmpfs 5.0M 0 5.0M 0% /run/lock tmpfs48G 0 48G 0% /sys/fs/cgroup tmpfs 9.5G 0 9.5G 0% /run/user/0 snapshot action is OK, when we restore the snapshot, the file system became read-only, and syslog seems stop writing until we fsck /dev/vda1 and mount -o rw,remount /: Dec 5 00:39:16 systemd[1]: Started Session 180 of user root. Dec 5 00:45:05 qemu-ga: info: guest-fsfreeze called Dec 5 07:00:45 kernel: [ 114.623823] EXT4-fs (vda1): re-mounted. Opts: (null) So after snapshoting, wo do fsthaw the file system, maybe the qga dose not respond or stop work, this action dose not execute successfully and there is no log to show the status of qemu-guest- agent. Version: libvirt 1.2.17 qemu 2.3.0 qemu-guest-agent 2.5 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1807073/+subscriptions
[PATCH v2] block/export/fuse.c: fix musl build
Fix the following build failure on musl raised since version 6.0.0 and
https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
because musl does not define FALLOC_FL_ZERO_RANGE:
../block/export/fuse.c: In function 'fuse_fallocate':
../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared (first
use in this function)
563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
| ^~~~
Fixes:
-
http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
Signed-off-by: Fabrice Fontaine
---
Changes v1 -> v2 (after review of Philippe Mathieu-Daudé):
- Use CONFIG_FALLOCATE_ZERO_RANGE and make safer #ifdef'ry
block/export/fuse.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/block/export/fuse.c b/block/export/fuse.c
index ada9e263eb..fc7b07d2b5 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (mode & FALLOC_FL_ZERO_RANGE) {
+}
+#ifdef CONFIG_FALLOCATE_ZERO_RANGE
+else if (mode & FALLOC_FL_ZERO_RANGE) {
if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
/* No need for zeroes, we are going to write them ourselves */
ret = fuse_do_truncate(exp, offset + length, false,
@@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (!mode) {
+}
+#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
+else if (!mode) {
/* We can only fallocate at the EOF with a truncate */
if (offset < blk_len) {
fuse_reply_err(req, EOPNOTSUPP);
--
2.30.2
Re: [PATCH 2/3] ps2: use a separate keyboard command reply queue
Hi, > A PS/2 keyboard has a separate command reply queue that is > independant of the key queue. This prevents that command replies > and keyboard input mix. Keyboard command replies take precedence > over queued keystrokes. A new keyboard command removes any > remaining command replies from the command reply queue. > > Implement a separate keyboard command reply queue and clear the > command reply queue before command execution. This brings the > PS/2 keyboard emulation much closer to a real PS/2 keyboard. It seems you place both queues in the existing 256-byte buffer, probably to avoid breaking live migration. Could you explain the logic a bit more, how the three pointers q->*ptr variables play together here? thanks, Gerd
Re: [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read
Hi Volker
On Sat, Aug 7, 2021 at 11:32 PM Volker Rümelin wrote:
> Since commit 584af1f1d9 (ui/gtk: add a keyboard fifo to the VTE
> consoles) a GTK VTE console chardev backend relies on the
> connected chardev frontend to call qemu_chr_fe_accept_input()
> whenever it can receive new characters. The HMP monitor doesn't
> do this. It only schedules a call to qemu_chr_fe_accept_input()
> after it handled a HMP command in monitor_command_cb().
>
> This is a problem if you paste a few characters into the GTK
> monitor console. Even entering a UTF-8 multibyte character leads
> to the same problem.
>
> Schedule a call to qemu_chr_fe_accept_input() after handling the
> received bytes to fix the HMP monitor.
>
> Signed-off-by: Volker Rümelin
>
Wouldn't it work to make gd_vc_send_chars() write in a loop (until it
fails)?
If not, I think monitor/qmp.c should be adjusted too.
---
> monitor/hmp.c | 1 +
> monitor/monitor-internal.h | 1 +
> monitor/monitor.c | 19 +--
> 3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d50c3124e1..470f56a71d 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1349,6 +1349,7 @@ static void monitor_read(void *opaque, const uint8_t
> *buf, int size)
> for (i = 0; i < size; i++) {
> readline_handle_byte(mon->rs, buf[i]);
> }
> +monitor_accept_input(&mon->common);
> } else {
> if (size == 0 || buf[size - 1] != 0) {
> monitor_printf(&mon->common, "corrupted command\n");
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..af33c3c617 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -170,6 +170,7 @@ int monitor_puts(Monitor *mon, const char *str);
> void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
> bool use_io_thread);
> void monitor_data_destroy(Monitor *mon);
> +void monitor_accept_input(Monitor *mon);
> int monitor_can_read(void *opaque);
> void monitor_list_append(Monitor *mon);
> void monitor_fdsets_cleanup(void);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 46a171bca6..8e3cf4ad98 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -519,13 +519,28 @@ int monitor_suspend(Monitor *mon)
> return 0;
> }
>
> -static void monitor_accept_input(void *opaque)
> +static void monitor_accept_input_bh(void *opaque)
> {
> Monitor *mon = opaque;
>
> qemu_chr_fe_accept_input(&mon->chr);
> }
>
> +void monitor_accept_input(Monitor *mon)
> +{
> +if (!qatomic_mb_read(&mon->suspend_cnt)) {
> +AioContext *ctx;
> +
> +if (mon->use_io_thread) {
> +ctx = iothread_get_aio_context(mon_iothread);
> +} else {
> +ctx = qemu_get_aio_context();
> +}
> +
> +aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
> +}
> +}
> +
> void monitor_resume(Monitor *mon)
> {
> if (monitor_is_hmp_non_interactive(mon)) {
> @@ -547,7 +562,7 @@ void monitor_resume(Monitor *mon)
> readline_show_prompt(hmp_mon->rs);
> }
>
> -aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
> +aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
> }
>
> trace_monitor_suspend(mon, -1);
> --
> 2.26.2
>
>
>
--
Marc-André Lureau
Re: [PULL 24/30] spapr_pci: populate ibm,loc-code
On Tue, 7 Jul 2015 at 16:49, Alexander Graf wrote:
>
> From: Nikunj A Dadhania
>
> Each hardware instance has a platform unique location code. The OF
> device tree that describes a part of a hardware entity must include
> the “ibm,loc-code” property with a value that represents the location
> code for that hardware entity.
>
> Populate ibm,loc-code.
Ancient patch, but Coverity has just noticed a bug in it
which is still present in current QEMU (CID 1460454):
> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice
> *pdev)
> +{
> +char *path = NULL, *buf = NULL, *host = NULL;
> +
> +/* Get the PCI VFIO host id */
> +host = object_property_get_str(OBJECT(pdev), "host", NULL);
> +if (!host) {
> +goto err_out;
> +}
> +
> +/* Construct the path of the file that will give us the DT location */
> +path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
> +g_free(host);
> +if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) {
> +goto err_out;
> +}
> +g_free(path);
Here we create a 'path' string, use it as the argument to
g_file_get_contents() and then free it (either here or in the err_out path)...
> +
> +/* Construct and read from host device tree the loc-code */
> +path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
> +g_free(buf);
> +if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) {
> +goto err_out;
> +}
> +return buf;
...but here we forget to free it before returning in the success case.
> +
> +err_out:
> +g_free(path);
> +return NULL;
> +}
Cleanest fix would be to declare 'path' and 'host' as
g_autofree char *path = NULL;
g_autofree char *host = NULL;
and then you can remove all the manual g_free(path) and g_free(host) calls.
thanks
-- PMM
Re: [RFC PATCH v2] Add a post for the new TCG cache modelling plugin
Mahmoud Mandour writes: > This post introduces the new TCG plugin `cache` that's used for cache > modelling. This plugin is a part of my GSoC 2021 participation. > > Signed-off-by: Mahmoud Mandour > --- > > v1 -> v2: > Elaborated some more in the introduction and broken up some > sentences. > > Changed the example of blocked matrix multiplication to a simpler > solution that requires less explanation since matrix multiplication > itself is out of the scope of the post. > > Added the code for the `mm` function but did not directly give away > the cache-problematic portion and preferred to "investigate" the > problem since it's the job of the plugin to help knowing where the > problem is. > > Added an epilogue in which I summarize the work and the patches > posted until now. > > .../2021-06-17-tcg-cache-modelling-plugin.md | 266 ++ > screenshots/2021-06-17-cache-structure.png| Bin 0 -> 19675 bytes > 2 files changed, 266 insertions(+) > create mode 100644 _posts/2021-06-17-tcg-cache-modelling-plugin.md > create mode 100644 screenshots/2021-06-17-cache-structure.png > > diff --git a/_posts/2021-06-17-tcg-cache-modelling-plugin.md > b/_posts/2021-06-17-tcg-cache-modelling-plugin.md > new file mode 100644 > index 000..5360916 > --- /dev/null > +++ b/_posts/2021-06-17-tcg-cache-modelling-plugin.md > @@ -0,0 +1,266 @@ > +--- > +layout: post > +title: "Cache Modelling TCG Plugin" > +date: 2021-06-17 06:00:00 +0200 > +author: Mahmoud Mandour > +categories: [TCG plugins, GSOC] > +--- > + > +TCG plugins provide means to instrument generated code for both user-mode and > +full system emulation. This includes the ability to intercept every memory > +access and instruction execution. This post introduces a new TCG plugin > that's > +used to simulate configurable L1 separate instruction cache and data > cache. I think we should maybe add a sentence or two just to briefly explain the purpose of a cache - perhaps with a link to a more complete description on wikipedia? Perhaps: "Caches are a key way modern CPUs keep running at full speed by avoiding the need to fetch data and instructions from the comparatively slow system memory. As a result understanding cache behaviour is a key part of performance optimisation." > + > +The plugin aims to simply model different configurations of icache and dcache > +rather than simulate intricate microarchitectural details. That's because > those > +details generally vary from one microarchitecture to another. Maybe reflow: "While different microarchitectures often have different different approaches at the very low level the core concepts of caching are universal. As QEMU is not microarchitectural emulator we model an ideal cache with a few simple parameters..." > Also, those > +details will be very different between TCG and real behavior. L1 caches are > +usually of low associativity, so simulating them will give us a good idea of > +what happens in real life with code that thrashes the cache. > + > +## Overview > + > +The plugin simulates how L1 user-configured caches would behave when given a > +working set defined by a program in user-mode, or system-wide working set. > +Subsequently, it logs performance statistics along with the most N > +cache-thrashing instructions. > + > +### Configurability > + > +The plugin is configurable in terms of: > + > +* icache size parameters: `arg="I=CACHE_SIZE ASSOC BLOCK_SIZE"` > +* dcache size parameters: `arg="D=CACHE_SIZE ASSOC BLOCK_SIZE"` > +* Eviction policy: `arg="evict=lru|rand|fifo"` > +* How many top-most thrashing instructions to log: `arg="limit=TOP_N"` > +* How many core caches to keep track of: `arg="cores=N_CORES"` > + > +### Multicore caching > + > +Multicore caching is achieved by having independent L1 caches for each > available > +core. > + > +In __full-system emulation__, the number of available vCPUs is known to the > +plugin at plugin installation time, so separate caches are maintained for > those. > + > +In __user-space emulation__, the index of the vCPU initiating a memory access > +monotonically increases and is limited with however much the kernel allows > +creating. The approach used is that we allocate a static number of caches, > and > +fit all memory accesses into those cores. This is viable having more threads > +than cores will result in interleaving those threads between the available > cores > +so they might thrash each other anyway. > + > +## Design and implementation > + > +### General structure > + > +A generic cache data structure, `struct Cache`, is used to model either an > +icache or dcache. For each known core, the plugin maintains an icache and a > +dcache. On a memory access coming from a core, the corresponding cache is > +interrogated. > + > +Each cache has a number of cache sets that are used to store the actual > cached > +locations alongside metadata that backs eviction algorithms. The str
Re: [PATCH for-6.2 v3 01/12] job: Context changes in job_completed_txn_abort()
On 06.08.21 21:16, Eric Blake wrote:
On Fri, Aug 06, 2021 at 11:38:48AM +0200, Max Reitz wrote:
Finalizing the job may cause its AioContext to change. This is noted by
job_exit(), which points at job_txn_apply() to take this fact into
account.
However, job_completed() does not necessarily invoke job_txn_apply()
(through job_completed_txn_success()), but potentially also
job_completed_txn_abort(). The latter stores the context in a local
variable, and so always acquires the same context at its end that it has
released in the beginning -- which may be a different context from the
one that job_exit() releases at its end. If it is different, qemu
aborts ("qemu_mutex_unlock_impl: Operation not permitted").
Is this a bug fix that needs to make it into 6.1?
Well, I only encountered it as part of this series (which I really don’t
think is 6.2 material at this point), and so I don’t know.
Can’t hurt, I suppose, but if we wanted this to be in 6.1, we’d better
have a specific test for it, I think.
Drop the local @outer_ctx variable from job_completed_txn_abort(), and
instead re-acquire the actual job's context at the end of the function,
so job_exit() will release the same.
Signed-off-by: Max Reitz
---
job.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
The commit message makes sense, and does a good job at explaining the
change. I'm still a bit fuzzy on how jobs are supposed to play nice
with contexts,
I can relate :)
but since your patch matches the commit message, I'm
happy to give:
Reviewed-by: Eric Blake
Thanks!
Re: [PATCH for-6.2 v3 05/12] job: @force parameter for job_cancel_sync{, _all}()
On 06.08.21 21:39, Eric Blake wrote:
On Fri, Aug 06, 2021 at 11:38:52AM +0200, Max Reitz wrote:
Callers should be able to specify whether they want job_cancel_sync() to
force-cancel the job or not.
In fact, almost all invocations do not care about consistency of the
result and just want the job to terminate as soon as possible, so they
should pass force=true. The replication block driver is the exception.
This changes some iotest outputs, because quitting qemu while a mirror
job is active will now lead to it being cancelled instead of completed,
which is what we want. (Cancelling a READY mirror job with force=false
may take an indefinite amount of time, which we do not want when
quitting. If users want consistent results, they must have all jobs be
done before they quit qemu.)
Feels somewhat like a bug fix, but I also understand why you'd prefer
to delay this to 6.2 (it is not a fresh regression, but a longstanding
issue).
It is, hence the “Buglink” tag below. However, only all of this series
together really fixes that bug (or at least patches 5+7+9 together),
just taking one wouldn’t help much. And together, it’s just too much
for 6.2 at this point.
Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
Signed-off-by: Max Reitz
---
+++ b/job.c
@@ -982,12 +982,24 @@ static void job_cancel_err(Job *job, Error **errp)
job_cancel(job, false);
}
-int job_cancel_sync(Job *job)
+/**
+ * Same as job_cancel_err(), but force-cancel.
+ */
+static void job_force_cancel_err(Job *job, Error **errp)
{
-return job_finish_sync(job, &job_cancel_err, NULL);
+job_cancel(job, true);
+}
In isolation, it looks odd that errp is passed but not used. But
looking further, it's because this is a callback that must have a
given signature, so it's okay.
Reviewed-by: Eric Blake
Re: [PATCH 3/3] ps2: migration support for command reply queue
Hi,
> +static bool ps2_keyboard_cqueue_needed(void *opaque)
> +{
> +PS2KbdState *s = opaque;
> +
> +return s->common.queue.cwptr != -1; /* the queue is mostly empty */
> +}
> +
> +static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
> +.name = "ps2kbd/command_reply_queue",
> +.needed = ps2_keyboard_cqueue_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_INT32(common.queue.cwptr, PS2KbdState),
> +VMSTATE_END_OF_LIST()
> +}
> +};
Not going to work (the existing vmstate_ps2_keyboard_need_high_bit has
the same problem btw). Chicken and egg problem on the receiving side:
How you properly evaluate ps2_keyboard_cqueue_needed() without having
common.queue.cwptr received yet?
With "cqueue not needed" being the common case migration will work
nicely in most cases nevertheless, but when the source machine actually
sends cqueue state things will break ...
Looking at data sent via vmstate works but ordering is important. You
could check write_cmd because that is sent in the migration data stream
before ps2_keyboard_cqueue_needed() will be evaluated (just an random
example for the ordering, probably wouldn't help much in this case).
There is some redundancy in the data stream (wptr + rptr would be
enough, but we also send count). Maybe that can be used somehow.
Of course the tricky part is to not confuse old qemu versions on
the receiving end.
If we can't find something we can add a property simliar to the one
for the extended keyboard state.
take care,
Gerd
Re: [PULL 06/23] hw/nvme: namespace parameter for EUI-64
On Tue, 29 Jun 2021 at 19:48, Klaus Jensen wrote:
>
> From: Heinrich Schuchardt
>
> The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
> paths. Add a new namespace property "eui64", that provides the user the
> option to specify the EUI-64.
Hi; Coverity complains about some uses of uninitialized data in this
code (CID 1458835 1459295 1459580). I think the bug was present in
the previous version of this function, but this was the last change
to touch it...
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 7dea64b72e6a..762bb82e3cac 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4426,19 +4426,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
> *n, NvmeRequest *req)
> NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> uint32_t nsid = le32_to_cpu(c->nsid);
> uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> -
> -struct data {
> -struct {
> -NvmeIdNsDescr hdr;
> -uint8_t v[NVME_NIDL_UUID];
> -} uuid;
> -struct {
> -NvmeIdNsDescr hdr;
> -uint8_t v;
> -} csi;
> -};
> -
> -struct data *ns_descrs = (struct data *)list;
> +uint8_t *pos = list;
> +struct {
> +NvmeIdNsDescr hdr;
> +uint8_t v[NVME_NIDL_UUID];
> +} QEMU_PACKED uuid;
> +struct {
> +NvmeIdNsDescr hdr;
> +uint64_t v;
> +} QEMU_PACKED eui64;
> +struct {
> +NvmeIdNsDescr hdr;
> +uint8_t v;
> +} QEMU_PACKED csi;
Here we define locals 'uuid', 'eui64', 'csi', without an initializer.
> trace_pci_nvme_identify_ns_descr_list(nsid);
>
> @@ -4452,17 +4452,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
> *n, NvmeRequest *req)
> }
>
> /*
> - * Because the NGUID and EUI64 fields are 0 in the Identify Namespace
> data
> - * structure, a Namespace UUID (nidt = 3h) must be reported in the
> - * Namespace Identification Descriptor. Add the namespace UUID here.
> + * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must
> + * provide a valid Namespace UUID in the Namespace Identification
> Descriptor
> + * data structure. QEMU does not yet support setting NGUID.
> */
> -ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
> -ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
> -memcpy(&ns_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
> +uuid.hdr.nidt = NVME_NIDT_UUID;
> +uuid.hdr.nidl = NVME_NIDL_UUID;
> +memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
Here we fill in some fields of uuid, but we don't touch uuid.hdr.rsvd2[],
which remains thus 2 bytes of uninitialized junk from our stack.
> +memcpy(pos, &uuid, sizeof(uuid));
Here we copy all of uuid to a buffer which we're going to hand
to the guest, so we've just given it two bytes of QEMU stack data
that it shouldn't really be able to look at.
> +pos += sizeof(uuid);
>
> -ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
> -ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
> -ns_descrs->csi.v = ns->csi;
> +if (ns->params.eui64) {
> +eui64.hdr.nidt = NVME_NIDT_EUI64;
> +eui64.hdr.nidl = NVME_NIDL_EUI64;
> +eui64.v = cpu_to_be64(ns->params.eui64);
> +memcpy(pos, &eui64, sizeof(eui64));
> +pos += sizeof(eui64);
> +}
> +
> +csi.hdr.nidt = NVME_NIDT_CSI;
> +csi.hdr.nidl = NVME_NIDL_CSI;
> +csi.v = ns->csi;
> +memcpy(pos, &csi, sizeof(csi));
> +pos += sizeof(csi);
We do the same thing for the rsvd2[] bytes in csi.hdr and eui64.hdr.
> return nvme_c2h(n, list, sizeof(list), req);
> }
Explicitly zero-initializing uuid, csi, eui64 with "= { }" would
be the most robust fix. If you think it's worth avoiding "zero
init and then overwrite 90% of the fields anyway" then you could
explicitly zero the .hdr.rsvd2 bytes.
thanks
-- PMM
Re: [Question] Reduce the msix_load cost for VFIO device
Hi Alex,
在 2021/8/3 22:19, Alex Williamson 写道:
> On Tue, 3 Aug 2021 16:43:07 +0800
> "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
> wrote:
>
>> Hi Alex,
>>
>> We found that the msix_load() will cost 40~50ms if the VF has 60+ interrupts,
>> the following code cost too much for each interrupt:
>>
>> msix_load:
>> for (vector = 0; vector < 60; vector++)
>> msix_handle_mask_update
>> vfio_msix_vector_do_use
>> vfio_add_kvm_msi_virq
>> kvm_irqchip_add_msi_route
>> kvm_irqchip_commit_routes <-- cost 0.8ms each time
>>
>> In irq remapping mode, the VF interrupts are not routed through KVM irqchip
>
> I'm not sure what this means. Your MSIX interrupts are going through
> QEMU anyway? Why?
>
>> in fact, so maybe we can reduce this cost by "x-no-kvm-msix=ture", right?
>> Are there any risks if we do in this way ?
>
> You're obviously free to configure your device this way, but the
> expectation is that any sort of startup latency is more than offset by
> improved runtime latency through the KVM route. This option is usually
> reserved for debugging, when we want to see all interaction with the
> device in QEMU.
>
> If there's a case where we're not detecting that a KVM route is
> ineffective, then we should figure out how to detect that and skip this
> code, but again the expectation is that the KVM route is worthwhile.
>
> If this is specifically about kvm_irqchip_commit_routes(), maybe the
> setup path needs a way to batch multiple routes and defer the commit,
> if that's possible. Thanks,
>
I've implemented two options and did some simple tests, both options can reduce
the vfio_msix_enable/msix_load cost, especially in HW live migration case, it
can significantly reduce the downtime if the VF has more than 60+ vectors.
Option-1 is "defer the commit": add a kvm virq without immediate commit, the
virq would be kept in kvm_state.irq_routes for the moment, and only commit once
at last. But it's not easy to fallback to QEMU route if someone fails.
Option-2 is "defer the setup": use QEMU route as default in migration resume
phase, try to switch to KVM route when the vector first firing. The first draft
of the code is as below, do you have any suggestion ?
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e1ea1d8..5656f3a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -347,6 +347,11 @@ static void vfio_msi_interrupt(void *opaque)
get_msg = msix_get_message;
notify = msix_notify;
+if (vector->need_switch) {
+vector->need_switch = false;
+vfio_add_kvm_msix_virq(vdev, vector, nr);
+}
+
/* A masked vector firing needs to use the PBA, enable it */
if (msix_is_masked(&vdev->pdev, nr)) {
set_bit(nr, vdev->msix->pending);
@@ -438,6 +443,26 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev,
VFIOMSIVector *vector,
vector->virq = virq;
}
+static void
+vfio_add_kvm_msix_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector, int nr)
+{
+VFIOMSIVector *vector = &vdev->msi_vectors[nr];
+Error *err = NULL;
+int fd;
+
+vfio_add_kvm_msi_virq(vdev, vector, nr, true);
+if (vector->virq < 0) {
+return;
+}
+
+fd = event_notifier_get_fd(&vector->kvm_interrupt);
+if (vfio_set_irq_signaling(&vdev->vbasedev,
+ VFIO_PCI_MSIX_IRQ_INDEX, nr,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+}
+}
+
static void vfio_remove_kvm_msi_virq(VFIOMSIVector *vector)
{
kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &vector->kvm_interrupt,
@@ -490,7 +515,11 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
unsigned int nr,
}
} else {
if (msg) {
-vfio_add_kvm_msi_virq(vdev, vector, nr, true);
+if (vdev->lazy_set_virq) {
+vector->need_switch = true;
+} else {
+vfio_add_kvm_msi_virq(vdev, vector, nr, true);
+}
}
}
@@ -566,6 +595,16 @@ static void vfio_msix_vector_release(PCIDevice *pdev,
unsigned int nr)
}
}
+static void vfio_msix_pre_enable(VFIOPCIDevice *vdev)
+{
+vdev->lazy_set_virq = true;
+}
+
+static void vfio_msix_post_enable(VFIOPCIDevice *vdev)
+{
+vdev->lazy_set_virq = false;
+}
+
static void vfio_msix_enable(VFIOPCIDevice *vdev)
{
PCIDevice *pdev = &vdev->pdev;
@@ -2466,7 +2505,9 @@ static int vfio_pci_load_config(VFIODevice *vbasedev,
QEMUFile *f)
if (msi_enabled(pdev)) {
vfio_msi_enable(vdev);
} else if (msix_enabled(pdev)) {
+vfio_msix_pre_enable(vdev);
vfio_msix_enable(vdev);
+vfio_msix_post_enable(vdev);
}
return ret;
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 6477751..31390f2 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -95,6 +95,7 @@ typedef struct VFIOMSIVector {
struct VFIOPCIDevice
Re: [PATCH-for-6.1 v2] block/export/fuse.c: fix musl build
On 8/9/21 11:51 AM, Fabrice Fontaine wrote:
> Fix the following build failure on musl raised since version 6.0.0 and
> https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
> because musl does not define FALLOC_FL_ZERO_RANGE:
>
> ../block/export/fuse.c: In function 'fuse_fallocate':
> ../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared
> (first use in this function)
> 563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
> | ^~~~
>
> Fixes:
> -
> http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
>
> Signed-off-by: Fabrice Fontaine
> ---
> Changes v1 -> v2 (after review of Philippe Mathieu-Daudé):
> - Use CONFIG_FALLOCATE_ZERO_RANGE and make safer #ifdef'ry
>
> block/export/fuse.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index ada9e263eb..fc7b07d2b5 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> -} else if (mode & FALLOC_FL_ZERO_RANGE) {
> +}
> +#ifdef CONFIG_FALLOCATE_ZERO_RANGE
> +else if (mode & FALLOC_FL_ZERO_RANGE) {
> if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
> /* No need for zeroes, we are going to write them ourselves */
> ret = fuse_do_truncate(exp, offset + length, false,
> @@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> -} else if (!mode) {
> +}
> +#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
> +else if (!mode) {
> /* We can only fallocate at the EOF with a truncate */
> if (offset < blk_len) {
> fuse_reply_err(req, EOPNOTSUPP);
>
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 01/10] virtiofsd: Limit setxattr()'s creds-dropped region
On 06.08.21 16:16, Vivek Goyal wrote:
On Fri, Jul 30, 2021 at 05:01:25PM +0200, Max Reitz wrote:
We only need to drop/switch our credentials for the (f)setxattr() call
alone, not for the openat() or fchdir() around it.
(Right now, this may not be that big of a problem, but with inodes being
identified by file handles instead of an O_PATH fd, we will need
open_by_handle_at() calls here, which is really fickle when it comes to
credentials being dropped.)
Signed-off-by: Max Reitz
---
tools/virtiofsd/passthrough_ll.c | 34 +++-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 38b2af8599..1f27eeabc5 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3121,6 +3121,7 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino,
const char *in_name,
bool switched_creds = false;
bool cap_fsetid_dropped = false;
struct lo_cred old = {};
+bool open_inode;
if (block_xattr(lo, in_name)) {
fuse_reply_err(req, EOPNOTSUPP);
@@ -3155,7 +3156,24 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino,
const char *in_name,
fuse_log(FUSE_LOG_DEBUG, "lo_setxattr(ino=%" PRIu64
", name=%s value=%s size=%zd)\n", ino, name, value, size);
+/*
+ * We can only open regular files or directories. If the inode is
+ * something else, we have to enter /proc/self/fd and use
+ * setxattr() on the link's filename there.
+ */
+open_inode = S_ISREG(inode->filetype) || S_ISDIR(inode->filetype);
sprintf(procname, "%i", inode->fd);
+if (open_inode) {
+fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+if (fd < 0) {
+saverr = errno;
+goto out;
+}
+} else {
+/* fchdir should not fail here */
+FCHDIR_NOFAIL(lo->proc_self_fd);
+}
+
/*
* If we are setting posix access acl and if SGID needs to be
* cleared, then switch to caller's gid and drop CAP_FSETID
@@ -3176,20 +3194,13 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino,
const char *in_name,
}
switched_creds = true;
}
-if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
-fd = openat(lo->proc_self_fd, procname, O_RDONLY);
-if (fd < 0) {
-saverr = errno;
-goto out;
-}
+if (open_inode) {
+assert(fd >= 0);
ret = fsetxattr(fd, name, value, size, flags);
saverr = ret == -1 ? errno : 0;
} else {
-/* fchdir should not fail here */
-FCHDIR_NOFAIL(lo->proc_self_fd);
ret = setxattr(procname, name, value, size, flags);
saverr = ret == -1 ? errno : 0;
-FCHDIR_NOFAIL(lo->root.fd);
}
if (switched_creds) {
if (cap_fsetid_dropped)
@@ -3198,6 +3209,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino,
const char *in_name,
lo_restore_cred(&old, false);
}
+if (!open_inode) {
+/* Change CWD back, fchdir should not fail here */
+FCHDIR_NOFAIL(lo->root.fd);
+}
+
This FCHDIR_NOFAIL() will also need to be called if lo_drop_cap_change_cred()
fails.
ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
&cap_fsetid_dropped);
if (ret) {
saverr = ret;
goto out;
}
Oh, right, thanks!
Max
[PATCH] target/riscv: Add User CSRs read-only check
For U-mode CSRs, read-only check is also needed.
Signed-off-by: LIU Zhiwei
---
target/riscv/csr.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9a4ed18ac5..360e4bfa33 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1422,11 +1422,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
csrno,
RISCVException ret;
target_ulong old_value;
RISCVCPU *cpu = env_archcpu(env);
+int read_only = get_field(csrno, 0xC00) == 3;
/* check privileges and return -1 if check fails */
#if !defined(CONFIG_USER_ONLY)
int effective_priv = env->priv;
-int read_only = get_field(csrno, 0xC00) == 3;
if (riscv_has_ext(env, RVH) &&
env->priv == PRV_S &&
@@ -1439,11 +1439,13 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
csrno,
effective_priv++;
}
-if ((write_mask && read_only) ||
-(!env->debugger && (effective_priv < get_field(csrno, 0x300 {
+if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
return RISCV_EXCP_ILLEGAL_INST;
}
#endif
+if (write_mask && read_only) {
+return -RISCV_EXCP_ILLEGAL_INST;
+}
/* ensure the CSR extension is enabled. */
if (!cpu->cfg.ext_icsr) {
--
2.17.1
Re: [PATCH] target/riscv: Add User CSRs read-only check
On Mon, Aug 9, 2021 at 6:37 PM LIU Zhiwei wrote:
>
> For U-mode CSRs, read-only check is also needed.
>
> Signed-off-by: LIU Zhiwei
> ---
> target/riscv/csr.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 9a4ed18ac5..360e4bfa33 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1422,11 +1422,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
> csrno,
> RISCVException ret;
> target_ulong old_value;
> RISCVCPU *cpu = env_archcpu(env);
> +int read_only = get_field(csrno, 0xC00) == 3;
>
> /* check privileges and return -1 if check fails */
> #if !defined(CONFIG_USER_ONLY)
> int effective_priv = env->priv;
> -int read_only = get_field(csrno, 0xC00) == 3;
>
> if (riscv_has_ext(env, RVH) &&
> env->priv == PRV_S &&
> @@ -1439,11 +1439,13 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
> csrno,
> effective_priv++;
> }
>
> -if ((write_mask && read_only) ||
> -(!env->debugger && (effective_priv < get_field(csrno, 0x300 {
> +if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
> return RISCV_EXCP_ILLEGAL_INST;
> }
> #endif
> +if (write_mask && read_only) {
> +return -RISCV_EXCP_ILLEGAL_INST;
This should be RISCV_EXCP_ILLEGAL_INST
> +}
>
> /* ensure the CSR extension is enabled. */
> if (!cpu->cfg.ext_icsr) {
Otherwise,
Reviewed-by: Bin Meng
Re: [PULL 8/9] qapi: introduce forwarding visitor
On Sat, 24 Jul 2021 at 10:00, Paolo Bonzini wrote:
>
> This new adaptor visitor takes a single field of the adaptee, and exposes it
> with a different name.
>
> This will be used for QOM alias properties. Alias targets can of course
> have a different name than the alias property itself (e.g. a machine's
> pflash0 might be an alias of a property named 'drive'). When the target's
> getter or setter invokes the visitor, it will use a different name than
> what the caller expects, and the visitor will not be able to find it
> (or will consume erroneously).
>
> The solution is for alias getters and setters to wrap the incoming
> visitor, and forward the sole field that the target is expecting while
> renaming it appropriately.
>
> Reviewed-by: Eric Blake
> Signed-off-by: Paolo Bonzini
Hi; Coverity complains here (CID 1459068) that the call to
visit_optional() is ignoring its return value (which we check
in 983 out of the other 989 callsites).
> +static void forward_field_optional(Visitor *v, const char *name, bool
> *present)
> +{
> +ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +if (!forward_field_translate_name(ffv, &name, NULL)) {
> +*present = false;
> +return;
> +}
> +visit_optional(ffv->target, name, present);
> +}
Is it right, or is this its "looks like this is returning an error
indication" heuristic misfiring again ?
My guess is the latter and it's caused by a mismatch
between the prototype of visit_optional() (returns a
status both by setting *present and in its return value)
and the Visitor::optional method (returns a status only
by setting *present, return void). I guess ideally we'd
standardize on whether these things were intended to return
a value or not.
thanks
-- PMM
[PATCH] hw/nvme: fix missing variable initializers
From: Klaus Jensen
Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
we set most of the fields, we do not explicitly set the rsvd2 field in
the NvmeIdNsDescr header.
Fix this by explicitly zero-initializing the variables.
Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
Suggested-by: Peter Maydell
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 43dfaeac9f54..6baf9e0420d5 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4663,15 +4663,15 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
*n, NvmeRequest *req)
struct {
NvmeIdNsDescr hdr;
uint8_t v[NVME_NIDL_UUID];
-} QEMU_PACKED uuid;
+} QEMU_PACKED uuid = {};
struct {
NvmeIdNsDescr hdr;
uint64_t v;
-} QEMU_PACKED eui64;
+} QEMU_PACKED eui64 = {};
struct {
NvmeIdNsDescr hdr;
uint8_t v;
-} QEMU_PACKED csi;
+} QEMU_PACKED csi = {};
trace_pci_nvme_identify_ns_descr_list(nsid);
--
2.32.0
Re: [PULL 06/23] hw/nvme: namespace parameter for EUI-64
On Aug 9 11:18, Peter Maydell wrote:
> On Tue, 29 Jun 2021 at 19:48, Klaus Jensen wrote:
> >
> > From: Heinrich Schuchardt
> >
> > The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
> > paths. Add a new namespace property "eui64", that provides the user the
> > option to specify the EUI-64.
>
> Hi; Coverity complains about some uses of uninitialized data in this
> code (CID 1458835 1459295 1459580). I think the bug was present in
> the previous version of this function, but this was the last change
> to touch it...
>
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 7dea64b72e6a..762bb82e3cac 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -4426,19 +4426,19 @@ static uint16_t
> > nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
> > NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> > uint32_t nsid = le32_to_cpu(c->nsid);
> > uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> > -
> > -struct data {
> > -struct {
> > -NvmeIdNsDescr hdr;
> > -uint8_t v[NVME_NIDL_UUID];
> > -} uuid;
> > -struct {
> > -NvmeIdNsDescr hdr;
> > -uint8_t v;
> > -} csi;
> > -};
> > -
> > -struct data *ns_descrs = (struct data *)list;
> > +uint8_t *pos = list;
> > +struct {
> > +NvmeIdNsDescr hdr;
> > +uint8_t v[NVME_NIDL_UUID];
> > +} QEMU_PACKED uuid;
> > +struct {
> > +NvmeIdNsDescr hdr;
> > +uint64_t v;
> > +} QEMU_PACKED eui64;
> > +struct {
> > +NvmeIdNsDescr hdr;
> > +uint8_t v;
> > +} QEMU_PACKED csi;
>
> Here we define locals 'uuid', 'eui64', 'csi', without an initializer.
>
> > trace_pci_nvme_identify_ns_descr_list(nsid);
> >
> > @@ -4452,17 +4452,29 @@ static uint16_t
> > nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
> > }
> >
> > /*
> > - * Because the NGUID and EUI64 fields are 0 in the Identify Namespace
> > data
> > - * structure, a Namespace UUID (nidt = 3h) must be reported in the
> > - * Namespace Identification Descriptor. Add the namespace UUID here.
> > + * If the EUI-64 field is 0 and the NGUID field is 0, the namespace
> > must
> > + * provide a valid Namespace UUID in the Namespace Identification
> > Descriptor
> > + * data structure. QEMU does not yet support setting NGUID.
> > */
> > -ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
> > -ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
> > -memcpy(&ns_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
> > +uuid.hdr.nidt = NVME_NIDT_UUID;
> > +uuid.hdr.nidl = NVME_NIDL_UUID;
> > +memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
>
> Here we fill in some fields of uuid, but we don't touch uuid.hdr.rsvd2[],
> which remains thus 2 bytes of uninitialized junk from our stack.
>
> > +memcpy(pos, &uuid, sizeof(uuid));
>
> Here we copy all of uuid to a buffer which we're going to hand
> to the guest, so we've just given it two bytes of QEMU stack data
> that it shouldn't really be able to look at.
>
> > +pos += sizeof(uuid);
>
> >
> > -ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
> > -ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
> > -ns_descrs->csi.v = ns->csi;
> > +if (ns->params.eui64) {
> > +eui64.hdr.nidt = NVME_NIDT_EUI64;
> > +eui64.hdr.nidl = NVME_NIDL_EUI64;
> > +eui64.v = cpu_to_be64(ns->params.eui64);
> > +memcpy(pos, &eui64, sizeof(eui64));
> > +pos += sizeof(eui64);
> > +}
> > +
> > +csi.hdr.nidt = NVME_NIDT_CSI;
> > +csi.hdr.nidl = NVME_NIDL_CSI;
> > +csi.v = ns->csi;
> > +memcpy(pos, &csi, sizeof(csi));
> > +pos += sizeof(csi);
>
> We do the same thing for the rsvd2[] bytes in csi.hdr and eui64.hdr.
>
> > return nvme_c2h(n, list, sizeof(list), req);
> > }
>
> Explicitly zero-initializing uuid, csi, eui64 with "= { }" would
> be the most robust fix. If you think it's worth avoiding "zero
> init and then overwrite 90% of the fields anyway" then you could
> explicitly zero the .hdr.rsvd2 bytes.
>
Thanks Peter,
Fix posted!
signature.asc
Description: PGP signature
Re: [PATCH-for-6.1?] hw/nvme: fix missing variable initializers
On 8/9/21 12:43 PM, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
> we set most of the fields, we do not explicitly set the rsvd2 field in
> the NvmeIdNsDescr header.
>
> Fix this by explicitly zero-initializing the variables.
>
> Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
> Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
> Suggested-by: Peter Maydell
> Signed-off-by: Klaus Jensen
> ---
> hw/nvme/ctrl.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 04/10] virtiofsd: Add lo_inode_fd() helper
On 06.08.21 20:25, Vivek Goyal wrote:
On Fri, Jul 30, 2021 at 05:01:28PM +0200, Max Reitz wrote:
[..]
@@ -1335,12 +1359,18 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t
parent,
return;
}
+res = lo_inode_fd(dir, &dir_fd);
+if (res < 0) {
+saverr = -res;
+goto out;
+}
+
saverr = lo_change_cred(req, &old, lo->change_umask && !S_ISLNK(mode));
if (saverr) {
goto out;
}
-res = mknod_wrapper(dir->fd, name, link, mode, rdev);
+res = mknod_wrapper(dir_fd.fd, name, link, mode, rdev);
saverr = errno;
@@ -1388,6 +1418,8 @@ static void lo_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
const char *name)
{
+g_auto(TempFd) inode_fd = TEMP_FD_INIT;
+g_auto(TempFd) parent_fd = TEMP_FD_INIT;
int res;
struct lo_data *lo = lo_data(req);
struct lo_inode *parent_inode;
@@ -1413,18 +1445,31 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino,
fuse_ino_t parent,
goto out_err;
}
+res = lo_inode_fd(inode, &inode_fd);
+if (res < 0) {
+errno = -res;
In previous function, we saved error to "saverr" and jumped to "out"
label, instead of overwriting to errno.
I would think that it will be good to use a single pattern. Either
save error in saverr or overwrite errno. I personally prefer saving
error into "saverr".
Absolutely, will do.
+goto out_err;
+}
+
+res = lo_inode_fd(parent_inode, &parent_fd);
+if (res < 0) {
+errno = -res;
+goto out_err;
+}
+
memset(&e, 0, sizeof(struct fuse_entry_param));
e.attr_timeout = lo->timeout;
e.entry_timeout = lo->timeout;
-sprintf(procname, "%i", inode->fd);
-res = linkat(lo->proc_self_fd, procname, parent_inode->fd, name,
+sprintf(procname, "%i", inode_fd.fd);
+res = linkat(lo->proc_self_fd, procname, parent_fd.fd, name,
AT_SYMLINK_FOLLOW);
if (res == -1) {
goto out_err;
}
-res = fstatat(inode->fd, "", &e.attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
+res = fstatat(inode_fd.fd, "", &e.attr,
+ AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
if (res == -1) {
goto out_err;
}
@@ -1453,23 +1498,33 @@ out_err:
static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
const char *name)
{
+g_auto(TempFd) dir_fd = TEMP_FD_INIT;
int res;
uint64_t mnt_id;
struct stat attr;
struct lo_data *lo = lo_data(req);
struct lo_inode *dir = lo_inode(req, parent);
+struct lo_inode *inode = NULL;
if (!dir) {
-return NULL;
+goto out;
Should we continue to just call "return NULL". dir is NULL. That means
lo_inode() failed. That means we never got the reference. So we don't
have to put the reference. If we do "goto out", it will call
lo_inode_put() which is not needed.
Yes, but lo_inode_put() will handle this gracefully, so it isn’t wrong.
My personal preference is that if there is an clean-up path, it should
be used everywhere instead of having pure returns at the beginning of a
function (where not many resources have been initialized yet), so that
no clean-up will be forgotten. Like, if we were to add some resource
acquisition in the declarations above (and clean-up code in the clean-up
path), we would need to change the return to a goto here. Or maybe we’d
forget that, and then we’d leak something.
So I prefer having clean-up sections be generic enough that they can be
used from anywhere within the function, and then also use it from
anywhere within the function, even if they end up being no-ops.
}
-res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
-lo_inode_put(lo, &dir);
+res = lo_inode_fd(dir, &dir_fd);
+if (res < 0) {
+goto out;
+}
+
+res = do_statx(lo, dir_fd.fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
if (res == -1) {
-return NULL;
+goto out;
}
-return lo_find(lo, &attr, mnt_id);
+inode = lo_find(lo, &attr, mnt_id);
+
+out:
+lo_inode_put(lo, &dir);
+return inode;
}
Thanks
Vivek
Re: [PATCH v3 02/10] virtiofsd: Add TempFd structure
On 06.08.21 16:41, Vivek Goyal wrote:
On Fri, Jul 30, 2021 at 05:01:26PM +0200, Max Reitz wrote:
We are planning to add file handles to lo_inode objects as an
alternative to lo_inode.fd. That means that everywhere where we
currently reference lo_inode.fd, we will have to open a temporary file
descriptor that needs to be closed after use.
So instead of directly accessing lo_inode.fd, there will be a helper
function (lo_inode_fd()) that either returns lo_inode.fd, or opens a new
file descriptor with open_by_handle_at(). It encapsulates this result
in a TempFd structure to let the caller know whether the FD needs to be
closed after use (opened from the handle) or not (copied from
lo_inode.fd).
I am wondering why this notion of "owned". Why not have this requirement
of always closing "fd". If we copied it from lo_inode.fd, then we will
need to dup() it. Otherwise we opened it from file handle and we will
need to close it anyway.
I guess you are trying to avoid having to call dup() and that's why
this notion of "owned" fd.
Yes, I don’t want to dup() it. One reason is that I’d rather just not.
It’s something that we can avoid, and dup-ing every time wouldn’t make
the code that much simpler (I think, without having tried).
One other is because this affects the current behavior (with O_PATH
FDs), which I don’t want to alter.
Well, and finally, as a pragmatic reason, virtiofsd-rs uses the same
structure and I don’t really want C virtiofsd and virtiofsd-rs to differ
too much.
By using g_auto(TempFd) to store this result, callers will not even have
to care about closing a temporary FD after use. It will be done
automatically once the object goes out of scope.
Signed-off-by: Max Reitz
Reviewed-by: Connor Kuehl
---
tools/virtiofsd/passthrough_ll.c | 49
1 file changed, 49 insertions(+)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 1f27eeabc5..fb5e073e6a 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -178,6 +178,28 @@ struct lo_data {
int user_posix_acl, posix_acl;
};
+/**
+ * Represents a file descriptor that may either be owned by this
+ * TempFd, or only referenced (i.e. the ownership belongs to some
+ * other object, and the value has just been copied into this TempFd).
+ *
+ * The purpose of this encapsulation is to be used as g_auto(TempFd)
+ * to automatically clean up owned file descriptors when this object
+ * goes out of scope.
+ *
+ * Use temp_fd_steal() to get an owned file descriptor that will not
+ * be closed when the TempFd goes out of scope.
+ */
+typedef struct {
+int fd;
+bool owned; /* fd owned by this object? */
+} TempFd;
+
+#define TEMP_FD_INIT ((TempFd) { .fd = -1, .owned = false })
+
+static void temp_fd_clear(TempFd *temp_fd);
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(TempFd, temp_fd_clear);
+
static const struct fuse_opt lo_opts[] = {
{ "sandbox=namespace",
offsetof(struct lo_data, sandbox),
@@ -255,6 +277,33 @@ static struct lo_data *lo_data(fuse_req_t req)
return (struct lo_data *)fuse_req_userdata(req);
}
+/**
+ * Clean-up function for TempFds
+ */
+static void temp_fd_clear(TempFd *temp_fd)
+{
+if (temp_fd->owned) {
+close(temp_fd->fd);
+*temp_fd = TEMP_FD_INIT;
+}
+}
+
+/**
+ * Return an owned fd from *temp_fd that will not be closed when
+ * *temp_fd goes out of scope.
+ *
+ * (TODO: Remove __attribute__ once this is used.)
+ */
+static __attribute__((unused)) int temp_fd_steal(TempFd *temp_fd)
+{
+if (temp_fd->owned) {
+temp_fd->owned = false;
+return temp_fd->fd;
+} else {
+return dup(temp_fd->fd);
+}
+}
This also will be simpler if we always called dup() and every caller
will close() fd.
I think only downside is having to call dup()/close(). Not sure if this
is an expensive operation or not.
Vivek
Re: [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables
Hi Kostiantyn,
On 8/9/21 11:48 AM, Kostiantyn Kostiuk wrote:
> Signed-off-by: Kostiantyn Kostiuk
I'm not sure what you are trying to do here, fix a memory leak?
> ---
> qga/commands-win32.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 724ce76a0e..a8a601776d 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -539,9 +539,9 @@ static GuestPCIAddress *get_pci_info(int number, Error
> **errp)
> dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
> for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> -PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
> +g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data =
> NULL;
> STORAGE_DEVICE_NUMBER sdn;
> -char *parent_dev_id = NULL;
> +g_autofree char *parent_dev_id = NULL;
> HDEVINFO parent_dev_info;
> SP_DEVINFO_DATA parent_dev_info_data;
> DWORD j;
>
Anyhow this function is confuse.
I think it would be easier to review by replacing the while()
by 2 calls, as suggested in the documentation:
https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila
-- >8 --
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7bac0c5d422..2188c5dd80d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -539,7 +539,6 @@ static GuestPCIAddress *get_pci_info(int number,
Error **errp)
dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
-PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
STORAGE_DEVICE_NUMBER sdn;
char *parent_dev_id = NULL;
HDEVINFO parent_dev_info;
@@ -551,25 +550,36 @@ static GuestPCIAddress *get_pci_info(int number,
Error **errp)
if (SetupDiEnumDeviceInterfaces(dev_info, &dev_info_data,
&GUID_DEVINTERFACE_DISK, 0,
&dev_iface_data)) {
-while (!SetupDiGetDeviceInterfaceDetail(dev_info,
&dev_iface_data,
-pdev_iface_detail_data,
-size, &size,
-&dev_info_data)) {
-if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
-pdev_iface_detail_data = g_malloc(size);
-pdev_iface_detail_data->cbSize =
-sizeof(*pdev_iface_detail_data);
-} else {
-error_setg_win32(errp, GetLastError(),
- "failed to get device interfaces");
-goto free_dev_info;
-}
+g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA
+ pdev_iface_detail_data = NULL;
+
+/* Get the required buffer size. */
+if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+ NULL, 0, &size,
+ &dev_info_data)
+&& GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
+error_setg_win32(errp, GetLastError(),
+ "failed to get device interfaces
buffer size");
+goto free_dev_info;
+}
+
+/* Allocate an appropriately sized buffer. */
+pdev_iface_detail_data = g_malloc(size);
+pdev_iface_detail_data->cbSize =
sizeof(*pdev_iface_detail_data);
+
+/* Get the interface details. */
+if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+ pdev_iface_detail_data,
+ size, &size,
+ &dev_info_data)) {
+error_setg_win32(errp, GetLastError(),
+ "failed to get device interfaces");
+goto free_dev_info;
}
dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
NULL);
-g_free(pdev_iface_detail_data);
if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
NULL, 0, &sdn, sizeof(sdn), &size,
NULL)) {
---
[PATCH v4] vhost: make SET_VRING_ADDR, SET_FEATURES send replies
On vhost-user-blk migration, qemu normally sends a number of commands
to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
VHOST_USER_SET_VRING_ADDR per each started ring to enable "used ring"
data logging.
The issue is that qemu doesn't wait for reply from the vhost daemon
for these commands which may result in races between qemu expectation
of logging starting and actual login starting in vhost daemon.
The race can appear as follows: on migration setup, qemu enables dirty page
logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
vhost-user-blk daemon immediately and the daemon needs some time to turn the
logging on internally. If qemu doesn't wait for reply, after sending the
command, qemu may start migrateing memory pages to a destination. At this time,
the logging may not be actually turned on in the daemon but some guest pages,
which the daemon is about to write to, may have already been transferred
without logging to the destination. Since the logging wasn't turned on,
those pages won't be transferred again as dirty. So we may end up with
corrupted data on the destination.
The same scenario is applicable for "used ring" data logging, which is
turned on with VHOST_USER_SET_VRING_ADDR command.
To resolve this issue, this patch makes qemu wait for the command result
explicitly if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated and logging enabled.
Signed-off-by: Denis Plotnikov
---
v3 -> v4:
* join acked reply and get_features in enforce_reply [mst]
* typos, rewording, cosmetic changes [mst]
v2 -> v3:
* send VHOST_USER_GET_FEATURES to flush out outstanding messages [mst]
v1 -> v2:
* send reply only when logging is enabled [mst]
v0 -> v1:
* send reply for SET_VRING_ADDR, SET_FEATURES only [mst]
---
hw/virtio/vhost-user.c | 139 +
1 file changed, 98 insertions(+), 41 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index ee57abe04526..5bb9254acd21 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1095,23 +1095,6 @@ static int vhost_user_set_mem_table(struct vhost_dev
*dev,
return 0;
}
-static int vhost_user_set_vring_addr(struct vhost_dev *dev,
- struct vhost_vring_addr *addr)
-{
-VhostUserMsg msg = {
-.hdr.request = VHOST_USER_SET_VRING_ADDR,
-.hdr.flags = VHOST_USER_VERSION,
-.payload.addr = *addr,
-.hdr.size = sizeof(msg.payload.addr),
-};
-
-if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
-return -1;
-}
-
-return 0;
-}
-
static int vhost_user_set_vring_endian(struct vhost_dev *dev,
struct vhost_vring_state *ring)
{
@@ -1288,72 +1271,146 @@ static int vhost_user_set_vring_call(struct vhost_dev
*dev,
return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
}
-static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
+
+static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t
*u64)
{
VhostUserMsg msg = {
.hdr.request = request,
.hdr.flags = VHOST_USER_VERSION,
-.payload.u64 = u64,
-.hdr.size = sizeof(msg.payload.u64),
};
+if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
+return 0;
+}
+
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
return -1;
}
+if (vhost_user_read(dev, &msg) < 0) {
+return -1;
+}
+
+if (msg.hdr.request != request) {
+error_report("Received unexpected msg type. Expected %d received %d",
+ request, msg.hdr.request);
+return -1;
+}
+
+if (msg.hdr.size != sizeof(msg.payload.u64)) {
+error_report("Received bad msg size.");
+return -1;
+}
+
+*u64 = msg.payload.u64;
+
return 0;
}
-static int vhost_user_set_features(struct vhost_dev *dev,
- uint64_t features)
+static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
+return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
}
-static int vhost_user_set_protocol_features(struct vhost_dev *dev,
-uint64_t features)
+static int enforce_reply(struct vhost_dev *dev,
+ const VhostUserMsg *msg)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features);
+uint64_t dummy;
+
+if (msg->hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
+return process_message_reply(dev, msg);
+}
+
+ /*
+* We need to wait for a reply but the backend does not
+* support replies for the command we just sent.
+* Send VHOST_USER_GET_FEATURES which makes all backends
+* send a reply.
+*/
+return vhost_user_get_
Re: [PATCH v3] vhost: make SET_VRING_ADDR, SET_FEATURES send replies
On 09.08.2021 12:34, Michael S. Tsirkin wrote:
Looks good. Some cosmetics:
all comments addressed in already sent v4
On Mon, Aug 09, 2021 at 12:03:30PM +0300, Denis Plotnikov wrote:
On vhost-user-blk migration, qemu normally sends a number of commands
to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
VHOST_USER_SET_VRING_ADDR per each started ring to enable "used ring"
data logging.
The issue is that qemu doesn't wait for reply from the vhost daemon
for these commands which may result in races between qemu expectation
of logging starting and actual login starting in vhost daemon.
The race can appear as follows: on migration setup, qemu enables dirty page
logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
vhost-user-blk daemon immediately and the daemon needs some time to turn the
logging on internally. If qemu doesn't wait for reply, after sending the
command, qemu may start migrate memory pages to a destination. At this time,
start migrating
the logging may not be actually turned on in the daemon but some guest pages,
which the daemon is about to write to, may have already been transferred
without logging to the destination. Since the logging wasn't turned on,
those pages won't be transferred again as dirty. So we may end up with
corrupted data on the destination.
The same scenario is applicable for "used ring" data logging, which is
turned on with VHOST_USER_SET_VRING_ADDR command.
To resolve this issue, this patch makes qemu wait for the commands result
command result
explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated and logging enabled.
typo
Signed-off-by: Denis Plotnikov
---
v2 -> v3:
* send VHOST_USER_GET_FEATURES to flush out outstanding messages [mst]
v1 -> v2:
* send reply only when logging is enabled [mst]
v0 -> v1:
* send reply for SET_VRING_ADDR, SET_FEATURES only [mst]
---
hw/virtio/vhost-user.c | 130 -
1 file changed, 89 insertions(+), 41 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index ee57abe04526..18f685df549f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1095,23 +1095,6 @@ static int vhost_user_set_mem_table(struct vhost_dev
*dev,
return 0;
}
-static int vhost_user_set_vring_addr(struct vhost_dev *dev,
- struct vhost_vring_addr *addr)
-{
-VhostUserMsg msg = {
-.hdr.request = VHOST_USER_SET_VRING_ADDR,
-.hdr.flags = VHOST_USER_VERSION,
-.payload.addr = *addr,
-.hdr.size = sizeof(msg.payload.addr),
-};
-
-if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
-return -1;
-}
-
-return 0;
-}
-
static int vhost_user_set_vring_endian(struct vhost_dev *dev,
struct vhost_vring_state *ring)
{
@@ -1288,72 +1271,137 @@ static int vhost_user_set_vring_call(struct vhost_dev
*dev,
return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
}
-static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
+
+static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t
*u64)
{
VhostUserMsg msg = {
.hdr.request = request,
.hdr.flags = VHOST_USER_VERSION,
-.payload.u64 = u64,
-.hdr.size = sizeof(msg.payload.u64),
};
+if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
+return 0;
+}
+
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
return -1;
}
+if (vhost_user_read(dev, &msg) < 0) {
+return -1;
+}
+
+if (msg.hdr.request != request) {
+error_report("Received unexpected msg type. Expected %d received %d",
+ request, msg.hdr.request);
+return -1;
+}
+
+if (msg.hdr.size != sizeof(msg.payload.u64)) {
+error_report("Received bad msg size.");
+return -1;
+}
+
+*u64 = msg.payload.u64;
+
return 0;
}
-static int vhost_user_set_features(struct vhost_dev *dev,
- uint64_t features)
+static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
+return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
}
-static int vhost_user_set_protocol_features(struct vhost_dev *dev,
-uint64_t features)
+static int enforce_reply(struct vhost_dev *dev)
{
-return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features);
+ /*
+* we need a reply but can't get it from some command directly,
+* so send the command which must send a reply
to make sure
+* the command we sent before is actually completed.
better:
We need to wait for a reply but the backend does not
support repli
Re: [PATCH-for-6.1?] hw/nvme: fix missing variable initializers
On Aug 9 12:47, Philippe Mathieu-Daudé wrote:
> On 8/9/21 12:43 PM, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
> > we set most of the fields, we do not explicitly set the rsvd2 field in
> > the NvmeIdNsDescr header.
> >
> > Fix this by explicitly zero-initializing the variables.
> >
> > Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
> > Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
> > Suggested-by: Peter Maydell
> > Signed-off-by: Klaus Jensen
> > ---
> > hw/nvme/ctrl.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
>
> Reviewed-by: Philippe Mathieu-Daudé
>
Swift as always Philippe, thanks!
Yes, I'll PR this for -rc3 immediately.
signature.asc
Description: PGP signature
Re: [RFC v3] virtio/vsock: add two more queues for datagram types
On Thu, Aug 05, 2021 at 12:00:05PM -0700, Jiang Wang . wrote:
On Tue, Aug 3, 2021 at 11:49 PM Stefano Garzarella wrote:
On Wed, Aug 4, 2021 at 8:41 AM Stefano Garzarella
wrote:
>
> On Tue, Aug 03, 2021 at 11:58:27AM -0700, Jiang Wang . wrote:
> >On Wed, Jul 7, 2021 at 10:27 AM Stefano Garzarella
wrote:
> >> On Wed, Jul 07, 2021 at 09:52:46AM -0700, Jiang Wang . wrote:
> >> >On Wed, Jul 7, 2021 at 1:33 AM Stefano Garzarella
wrote:
> >> >> On Tue, Jul 06, 2021 at 10:26:07PM +, Jiang Wang wrote:
>
> [...]
>
> >> >> >+
> >> >> >+if (nvqs < 0)
> >> >> >+nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> >> >+
> >> >> >+if (nvqs == MAX_VQS_WITH_DGRAM) {
> >> >> >+vvc->dgram_recv_vq = virtio_add_queue(vdev,
VHOST_VSOCK_QUEUE_SIZE,
> >> >> >+
vhost_vsock_common_handle_output);
> >> >> >+vvc->dgram_trans_vq = virtio_add_queue(vdev,
VHOST_VSOCK_QUEUE_SIZE,
> >> >> >+
vhost_vsock_common_handle_output);
> >> >> >+}
> >> >> >+
> >> >> > /* The event queue belongs to QEMU */
> >> >> > vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> >> >
vhost_vsock_common_handle_output);
> >> >>
> >> >> Did you do a test with a guest that doesn't support datagram with QEMU
> >> >> and hosts that do?
> >> >>
> >> >Yes, and it works.
> >> >
> >> >> I repost my thoughts that I had on v2:
> >> >>
> >> >> What happen if the guest doesn't support dgram?
> >> >>
> >> >> I think we should dynamically use the 3rd queue or the 5th queue
for
> >> >> the events at runtime after the guest acked the features.
> >> >>
> >> >> Maybe better to switch to an array of VirtQueue.
> >> >>
> >> >I think in current V3, it already dynamically use 3rd or 5th queue
> >> >depending
> >> >on the feature bit.
> >>
> >> I'm not sure. IIUC when vhost_vsock_common_realize() is called, we don't
> >> know the features acked by the guest, so how can it be dynamic?
> >>
> >> Here we should know only if the host kernel supports it.
> >>
> >> Maybe it works, because in QEMU we use the event queue only after a
> >> migration to send a reset event, so you can try to migrate a guest to
> >> check this path.
> >>
> >I tried VM migration and didn't see any problems. The migration looks fine
> >and vsock dgram still works after migration. Is there any more specific test
> >you want to run to check for this code path?
> >
>
> I meant a migration of a guest from QEMU without this patch to a QEMU
> with this patch. Of course in that case testing a socket stream.
>
Sorry, I meant the opposite.
You should try to migrate a guest that does not support dgrams starting
from a QEMU with this patch (and kernel that supports dgram, so qemu
uses the 5th queue for event), to a QEMU without this patch.
Got it. I tried what you said and saw errors on the destination qemu. Then
I moved event queue up to be number 3 (before adding dgram vqs),
as the following:
+/* The event queue belongs to QEMU */
+vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
+ vhost_vsock_common_handle_output);
+
nvqs = vhost_vsock_get_max_qps(enable_dgram);
@@ -253,10 +257,6 @@ void vhost_vsock_common_realize(VirtIODevice
*vdev, const char *name, bool enabl
vhost_vsock_common_handle_output);
}
-/* The event queue belongs to QEMU */
-vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
- vhost_vsock_common_handle_output);
-
But I still see following errors on the destination qemu:
qemu-system-x86_64: Error starting vhost vsock: 14
Any idea if my above code change is wrong or missing something?
No, sorry.
Do you have some kernel messages in the host?
Take one step back, what should be the host kernel version? With or
without dgram support? I tried both. The new dest kernel shows the above error.
The old dest kernel shows a msr error probably not related to vsock.
I think the best is to try the host kernel with DGRAM support, so QEMU
will allocate all the queues.
To explain the above qemu 14 error, I think the issue is that the
source host kernel
supports dgram by always setting the DGRAM feature bit(in my
implementation). Then the source
qemu query the source host kernel, and use 5 for event vq. Even if the source
guest kernel does not support dgram, it currently has no way to tell the source
host or the source qemu.
Initially I think is better to try the migration on the same host, so we
can exclude other issues. When it works properly, you can try to migrate
to a different host kernel.
On the source machine, when we start qemu process, and the guest VM
is still in BIOS or early boot process ( before vsock is initialized), I think
at this time, the qemu vhost_vsock_common_realize() is already called.
So qemu can only check if the host kernel supports dgram
[PULL for-6.1 0/1] hw/nvme fixes
From: Klaus Jensen Hi Peter, The following changes since commit dee64246ded3aa7dbada68b96ce1c64e5bea327d: Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging (2021-08-06 10:28:33 +0100) are available in the Git repository at: git://git.infradead.org/qemu-nvme.git tags/nvme-next-pull-request for you to fetch changes up to 5f4884c4412318a1adc105dea9cc28f7625ce730: hw/nvme: fix missing variable initializers (2021-08-09 12:52:16 +0200) hw/nvme fixes * coverity fixes Klaus Jensen (1): hw/nvme: fix missing variable initializers hw/nvme/ctrl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.32.0
[PULL for-6.1 1/1] hw/nvme: fix missing variable initializers
From: Klaus Jensen
Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
we set most of the fields, we do not explicitly set the rsvd2 field in
the NvmeIdNsDescr header.
Fix this by explicitly zero-initializing the variables.
Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
Suggested-by: Peter Maydell
Signed-off-by: Klaus Jensen
Reviewed-by: Philippe Mathieu-Daudé
---
hw/nvme/ctrl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 43dfaeac9f54..6baf9e0420d5 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4663,15 +4663,15 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
*n, NvmeRequest *req)
struct {
NvmeIdNsDescr hdr;
uint8_t v[NVME_NIDL_UUID];
-} QEMU_PACKED uuid;
+} QEMU_PACKED uuid = {};
struct {
NvmeIdNsDescr hdr;
uint64_t v;
-} QEMU_PACKED eui64;
+} QEMU_PACKED eui64 = {};
struct {
NvmeIdNsDescr hdr;
uint8_t v;
-} QEMU_PACKED csi;
+} QEMU_PACKED csi = {};
trace_pci_nvme_identify_ns_descr_list(nsid);
--
2.32.0
Re: [PATCH for-6.2 1/6] acpi: Delete broken ACPI_GED_X86 macro
On Thu, Aug 05, 2021 at 03:34:26PM -0400, Eduardo Habkost wrote: > The macro never worked and never will, because the > AcpiGedX86State type never existed. > > Signed-off-by: Eduardo Habkost > -#define ACPI_GED_X86(obj) \ > -OBJECT_CHECK(AcpiGedX86State, (obj), TYPE_ACPI_GED_X86) Oops. Leftover from an earlier patch revision where the struct did actually exist. Thanks for cleaning up. Reviewed-by: Gerd Hoffmann take care, Gerd
Re: [PATCH v4] virtio/vsock: add two more queues for datagram types
On Thu, Aug 05, 2021 at 12:07:02PM -0700, Jiang Wang . wrote:
On Wed, Aug 4, 2021 at 1:13 AM Stefano Garzarella wrote:
On Tue, Aug 03, 2021 at 11:41:32PM +, Jiang Wang wrote:
>Datagram sockets are connectionless and unreliable.
>The sender does not know the capacity of the receiver
>and may send more packets than the receiver can handle.
>
>Add two more dedicate virtqueues for datagram sockets,
>so that it will not unfairly steal resources from
>stream and future connection-oriented sockets.
>
>Signed-off-by: Jiang Wang
>---
>v1 -> v2: use qemu cmd option to control number of queues,
> removed configuration settings for dgram.
>v2 -> v3: use ioctl to get features and decide number of
>virt queues, instead of qemu cmd option.
>v3 -> v4: change DGRAM feature bit value to 2. Add an argument
> in vhost_vsock_common_realize to indicate dgram is supported or not.
>
> hw/virtio/vhost-user-vsock.c | 2 +-
> hw/virtio/vhost-vsock-common.c| 58 ++-
> hw/virtio/vhost-vsock.c | 5 +-
> include/hw/virtio/vhost-vsock-common.h| 6 +-
> include/hw/virtio/vhost-vsock.h | 4 ++
> include/standard-headers/linux/virtio_vsock.h | 1 +
> 6 files changed, 69 insertions(+), 7 deletions(-)
>
>diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>index 6095ed7349..e9ec0e1c00 100644
>--- a/hw/virtio/vhost-user-vsock.c
>+++ b/hw/virtio/vhost-user-vsock.c
>@@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev, Error
**errp)
> return;
> }
>
>-vhost_vsock_common_realize(vdev, "vhost-user-vsock");
>+vhost_vsock_common_realize(vdev, "vhost-user-vsock", false);
>
> vhost_dev_set_config_notifier(&vvc->vhost_dev, &vsock_ops);
>
>diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>index 4ad6e234ad..c78536911a 100644
>--- a/hw/virtio/vhost-vsock-common.c
>+++ b/hw/virtio/vhost-vsock-common.c
>@@ -17,6 +17,8 @@
> #include "hw/virtio/vhost-vsock.h"
> #include "qemu/iov.h"
> #include "monitor/monitor.h"
>+#include
>+#include
>
> int vhost_vsock_common_start(VirtIODevice *vdev)
> {
>@@ -196,9 +198,39 @@ int vhost_vsock_common_post_load(void *opaque, int
version_id)
> return 0;
> }
>
>-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
>+static int vhost_vsock_get_max_qps(bool enable_dgram)
>+{
>+uint64_t features;
>+int ret;
>+int fd = -1;
>+
>+if (!enable_dgram)
>+return MAX_VQS_WITHOUT_DGRAM;
>+
>+fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
As I said in the previous version, we cannot directly open
/dev/vhost-vsock, for two reasons:
1. this code is common with vhost-user-vsock which does not use
/dev/vhost-vsock.
2. the fd may have been passed from the management layer and qemu may
not be able to directly open /dev/vhost-vsock.
I think is better to move this function in hw/virtio/vhost-vsock.c,
using the `vhostfd`, returning true or false if dgram is supported, then
you can use it for `enable_dgram` param ...
Yes, you are right. Now I remember you said that before but I forgot about that
when I changed the code. I will fix it. Sorry about that.
No problem :-)
>+if (fd == -1) {
>+error_report("vhost-vsock: failed to open device. %s",
strerror(errno));
>+return -1;
>+}
>+
>+ret = ioctl(fd, VHOST_GET_FEATURES, &features);
>+if (ret) {
>+error_report("vhost-vsock: failed to read device. %s",
strerror(errno));
>+qemu_close(fd);
>+return ret;
>+}
>+
>+qemu_close(fd);
>+if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
>+return MAX_VQS_WITH_DGRAM;
>+
>+return MAX_VQS_WITHOUT_DGRAM;
>+}
>+
>+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, bool
enable_dgram)
> {
> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>+int nvqs = MAX_VQS_WITHOUT_DGRAM;
>
> virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> sizeof(struct virtio_vsock_config));
>@@ -209,12 +241,24 @@ void vhost_vsock_common_realize(VirtIODevice
>*vdev, const char *name)
> vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>vhost_vsock_common_handle_output);
>
>+nvqs = vhost_vsock_get_max_qps(enable_dgram);
>+
>+if (nvqs < 0)
>+nvqs = MAX_VQS_WITHOUT_DGRAM;
... and here, if `enable_dgram` is true, you can set `nvqs =
MAX_VQS_WITH_DGRAM``
sure.
>+
>+if (nvqs == MAX_VQS_WITH_DGRAM) {
>+vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>+
vhost_vsock_common_handle_output);
>+vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>+
>vhost_vsock_common_handle_output);
>+}
>+
I'm still concerned about compatibility with guests that don't
support
dgram, as I mentioned in the previous email.
I need to do some testing
Re: [PATCH-for-6.1 v2] block/export/fuse.c: fix musl build
On 8/9/21 1:27 PM, Philippe Mathieu-Daudé wrote:
> On 8/9/21 11:51 AM, Fabrice Fontaine wrote:
>> Fix the following build failure on musl raised since version 6.0.0 and
>> https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
>> because musl does not define FALLOC_FL_ZERO_RANGE:
>>
>> ../block/export/fuse.c: In function 'fuse_fallocate':
>> ../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared
>> (first use in this function)
>> 563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
>> | ^~~~
>>
>> Fixes:
>> -
>> http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
>>
>> Signed-off-by: Fabrice Fontaine
>> ---
>> Changes v1 -> v2 (after review of Philippe Mathieu-Daudé):
>> - Use CONFIG_FALLOCATE_ZERO_RANGE and make safer #ifdef'ry
>>
>> block/export/fuse.c | 8 ++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/export/fuse.c b/block/export/fuse.c
>> index ada9e263eb..fc7b07d2b5 100644
>> --- a/block/export/fuse.c
>> +++ b/block/export/fuse.c
>> @@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
>> inode, int mode,
>> offset += size;
>> length -= size;
>> } while (ret == 0 && length > 0);
>> -} else if (mode & FALLOC_FL_ZERO_RANGE) {
>> +}
>> +#ifdef CONFIG_FALLOCATE_ZERO_RANGE
>> +else if (mode & FALLOC_FL_ZERO_RANGE) {
>> if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
>> /* No need for zeroes, we are going to write them ourselves */
>> ret = fuse_do_truncate(exp, offset + length, false,
>> @@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
>> inode, int mode,
>> offset += size;
>> length -= size;
>> } while (ret == 0 && length > 0);
>> -} else if (!mode) {
>> +}
>> +#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
>> +else if (!mode) {
>> /* We can only fallocate at the EOF with a truncate */
>> if (offset < blk_len) {
>> fuse_reply_err(req, EOPNOTSUPP);
>>
> Reviewed-by: Philippe Mathieu-Daudé
>
Reviewed-by: Denis V. Lunev
Re: [PULL 06/35] Hexagon (disas) disassembler
On Wed, 17 Feb 2021 at 23:40, Richard Henderson
wrote:
>
> From: Taylor Simpson
>
> Add hexagon to disas/meson.build
> Add disas/hexagon.c
> Add hexagon to include/disas/dis-asm.h
>
> Signed-off-by: Taylor Simpson
> Tested-by: Philippe Mathieu-Daudé
> Reviewed-by: Philippe Mathieu-Daudé
> Message-Id: <1612763186-18161-6-git-send-email-tsimp...@quicinc.com>
> Signed-off-by: Richard Henderson
Coverity reports a memory leak in this code (CID 1460121):
> +int print_insn_hexagon(bfd_vma memaddr, struct disassemble_info *info)
> +{
> +uint32_t words[PACKET_WORDS_MAX];
> +bool found_end = false;
> +GString *buf = g_string_sized_new(PACKET_BUFFER_LEN);
We allocate buf here...
> +int i, len;
> +
> +for (i = 0; i < PACKET_WORDS_MAX && !found_end; i++) {
> +int status = (*info->read_memory_func)(memaddr + i *
> sizeof(uint32_t),
> + (bfd_byte *)&words[i],
> + sizeof(uint32_t), info);
> +if (status) {
> +if (i > 0) {
> +break;
> +}
> +(*info->memory_error_func)(status, memaddr, info);
> +return status;
...but in the early error return cases here...
> +}
> +if (is_packet_end(words[i])) {
> +found_end = true;
> +}
> +}
> +
> +if (!found_end) {
> +(*info->fprintf_func)(info->stream, "");
> +return PACKET_WORDS_MAX * sizeof(uint32_t);
...and here we do not free it.
> +}
> +
Easiest fix is to move the allocation
buf = g_string_sized_new(PACKET_BUFFER_LEN);
down to here, just above the point where we're going to use it.
> +len = disassemble_hexagon(words, i, memaddr, buf);
> +(*info->fprintf_func)(info->stream, "%s", buf->str);
> +g_string_free(buf, true);
> +
> +return len;
> +}
thanks
-- PMM
Re: [PATCH v2 0/2] s390x: improve subchannel error handling (vfio)
On Wed, Aug 04 2021, Matthew Rosato wrote: > On 8/4/21 8:30 PM, Jared Rossi wrote: >> I've exercised the error paths and it appears to all work correctly. >> >> On 7/19/21 11:09 AM, Jared Rossi wrote: >>> I will take a look and see if I can exercise the error paths. >>> >>> Regards, >>> >>> Jared Rossi > > Thanks Jared! So, with that I'd suggest a > > Tested-by: Jared Rossi I took the liberty to include that. > > and as I said earlier the code LGTM -- so for the series: > > Reviewed-by: Matthew Rosato Thanks to you both! > >>> >>> On 7/19/21 10:16 AM, Matthew Rosato wrote: On 7/5/21 12:39 PM, Cornelia Huck wrote: > This is a followup on the first version (which I had sent out in May, > and which kind of fell through the cracks.) While the first patch > is mostly unchanged, I added a second patch to address some possible > problems with the generated unit exceptions; non-vfio subchannels > are not affected by this. > > As before, this works on the good path, and I have not managed to > actually get my system to exercise the error path :( Sorry for the silence, was out of office for a bit and Eric is unavailable -- Anyway the code LGTM and matches what I see in the POPs, I'd be willing to ACK but I'd feel better if we could exercise the error paths before merging. @Jared/@Mike, you've both had eyes on this area of code recently, would one of you be willing to take a crack at a tested-by (non-zero CCs on HSCH/CSCH + also drive the sch_gen_unit_exception path)? > > v1->v2: > - add comments regarding -ENODEV/-EACCES handling > - add second patch > > Cornelia Huck (2): > vfio-ccw: forward halt/clear errors > css: fix actl handling for unit exceptions > > hw/s390x/css.c | 38 ++ > hw/vfio/ccw.c | 4 ++-- > include/hw/s390x/css.h | 3 ++- > 3 files changed, 38 insertions(+), 7 deletions(-) > Queued to s390-next.
[PATCH] fuzz: avoid building twice, when running on gitlab
On oss-fuzz, we build twice, to put together a build that is portable to the runner containers. On gitlab ci, this is wasteful and contributes to timeouts on the build-oss-fuzz job. Avoid building twice on gitlab, at the remote cost of potentially missing some cases that break oss-fuzz builds. Signed-off-by: Alexander Bulekov --- >From a couple test runs it looks like this can shave off 15-20 minutes. scripts/oss-fuzz/build.sh | 24 +--- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh index c1af43fded..98b56e0521 100755 --- a/scripts/oss-fuzz/build.sh +++ b/scripts/oss-fuzz/build.sh @@ -73,17 +73,19 @@ if ! make "-j$(nproc)" qemu-fuzz-i386; then "\nFor example: CC=clang CXX=clang++ $0" fi -for i in $(ldd ./qemu-fuzz-i386 | cut -f3 -d' '); do -cp "$i" "$DEST_DIR/lib/" -done -rm qemu-fuzz-i386 - -# Build a second time to build the final binary with correct rpath -../configure --disable-werror --cc="$CC" --cxx="$CXX" --enable-fuzzing \ ---prefix="$DEST_DIR" --bindir="$DEST_DIR" --datadir="$DEST_DIR/data/" \ ---extra-cflags="$EXTRA_CFLAGS" --extra-ldflags="-Wl,-rpath,\$ORIGIN/lib" \ ---target-list="i386-softmmu" -make "-j$(nproc)" qemu-fuzz-i386 V=1 +if [ "$GITLAB_CI" != "true" ]; then +for i in $(ldd ./qemu-fuzz-i386 | cut -f3 -d' '); do +cp "$i" "$DEST_DIR/lib/" +done +rm qemu-fuzz-i386 + +# Build a second time to build the final binary with correct rpath +../configure --disable-werror --cc="$CC" --cxx="$CXX" --enable-fuzzing \ +--prefix="$DEST_DIR" --bindir="$DEST_DIR" --datadir="$DEST_DIR/data/" \ +--extra-cflags="$EXTRA_CFLAGS" --extra-ldflags="-Wl,-rpath,\$ORIGIN/lib" \ +--target-list="i386-softmmu" +make "-j$(nproc)" qemu-fuzz-i386 V=1 +fi # Copy over the datadir cp -r ../pc-bios/ "$DEST_DIR/pc-bios" -- 2.30.2
Re: [PULL v1 3/5] qga: convert to use error checked base64 decode
On Fri, 18 Dec 2015 at 16:53, Daniel P. Berrange wrote:
>
> Switch from using g_base64_decode over to qbase64_decode
> in order to get error checking of the base64 input data.
>
> Reviewed-by: Eric Blake
> Signed-off-by: Daniel P. Berrange
> ---
> qga/commands-posix.c | 11 +--
> qga/commands-win32.c | 11 +--
> qga/commands.c | 13 -
> 3 files changed, 30 insertions(+), 5 deletions(-)
Hi; this is an old commit, but Coverity has just noticed that
it introduced a resource leak on an error-exit codepath (CID 1460005):
> @@ -393,10 +394,19 @@ GuestExec *qmp_guest_exec(const char *path,
> GIOChannel *in_ch, *out_ch, *err_ch;
> GSpawnFlags flags;
> bool has_output = (has_capture_output && capture_output);
> +uint8_t *input = NULL;
> +size_t ninput = 0;
>
> arglist.value = (char *)path;
> arglist.next = has_arg ? arg : NULL;
>
> +if (has_input_data) {
> +input = qbase64_decode(input_data, -1, &ninput, err);
> +if (!input) {
> +return NULL;
> +}
qbase64_decode() allocates memory. We free this in the
guest_exec_child_watch callback, but moving the call to
the base64 decode function up here has put it above the call
to g_spawn_async_with_pipes(), which can fail and whose
error-exit codepath doesn't free 'input'.
> +}
> +
> argv = guest_exec_get_args(&arglist, true);
> envp = has_env ? guest_exec_get_args(env, false) : NULL;
>
> @@ -425,7 +435,8 @@ GuestExec *qmp_guest_exec(const char *path,
> g_child_watch_add(pid, guest_exec_child_watch, gei);
>
> if (has_input_data) {
> -gei->in.data = g_base64_decode(input_data, &gei->in.size);
> +gei->in.data = input;
> +gei->in.size = ninput;
The old callsite for the decode function was below the call
to g_child_watch_add(), so it could rely on the watch function
being called to free the data.
-- PMM
[PATCH] qga: fix leak of base64 decoded data on command error
If the guest command fails to be spawned, then we would leak the decoded
base64 input used for the command's stdin feed.
Signed-off-by: Daniel P. Berrangé
---
qga/commands.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qga/commands.c b/qga/commands.c
index a6491d2cf8..80501e4a73 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -402,7 +402,7 @@ GuestExec *qmp_guest_exec(const char *path,
GIOChannel *in_ch, *out_ch, *err_ch;
GSpawnFlags flags;
bool has_output = (has_capture_output && capture_output);
-uint8_t *input = NULL;
+g_autofree uint8_t *input = NULL;
size_t ninput = 0;
arglist.value = (char *)path;
@@ -441,7 +441,7 @@ GuestExec *qmp_guest_exec(const char *path,
g_child_watch_add(pid, guest_exec_child_watch, gei);
if (has_input_data) {
-gei->in.data = input;
+gei->in.data = g_steal_pointer(&input);
gei->in.size = ninput;
#ifdef G_OS_WIN32
in_ch = g_io_channel_win32_new_fd(in_fd);
--
2.31.1
