[PATCH v2] Update event idx if guest has made extra buffers during double check
If guest has made some buffers available during double check,
but the total buffer size available is lower than @bufsize,
notify the guest with the latest available idx(event idx)
seen by the host.
Fixes: 06b12970174 ("virtio-net: fix network stall under load")
Signed-off-by: wencheng Yang
---
hw/net/virtio-net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9c7e85caea..23c6c8c898 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1654,6 +1654,7 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int
bufsize)
if (virtio_queue_empty(q->rx_vq) ||
(n->mergeable_rx_bufs &&
!virtqueue_avail_bytes(q->rx_vq, bufsize, 0))) {
+virtio_queue_set_notification(q->rx_vq, 1);
return 0;
}
}
--
2.39.0
Re: [PATCH v3] hw/arm/virt: Avoid unexpected warning from Linux guest on host with Fujitsu CPUs
On Thu, Jun 13, 2024 at 1:48 AM Robin Murphy wrote: > > On 2024-06-12 1:50 pm, Philippe Mathieu-Daudé wrote: > > On 12/6/24 14:48, Peter Maydell wrote: > >> On Wed, 12 Jun 2024 at 13:33, Philippe Mathieu-Daudé > >> wrote: > >>> > >>> Hi Zhenyu, > >>> Hello Philippe, > >>> On 12/6/24 04:05, Zhenyu Zhang wrote: > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 3c93c0c0a6..3cefac6d43 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -271,6 +271,17 @@ static void create_fdt(VirtMachineState *vms) > qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2); > qemu_fdt_setprop_string(fdt, "/", "model", "linux,dummy-virt"); > > +/* > + * For QEMU, all DMA is coherent. Advertising this in the root > node > + * has two benefits: > + * > + * - It avoids potential bugs where we forget to mark a DMA > + * capable device as being dma-coherent > + * - It avoids spurious warnings from the Linux kernel about > + * devices which can't do DMA at all > + */ > +qemu_fdt_setprop(fdt, "/", "dma-coherent", NULL, 0); > >>> > >>> OK, but why restrict that to the Aarch64 virt machine? > >>> Shouldn't advertise this generically in create_device_tree()? > >>> Or otherwise at least in the other virt machines? > >> > >> create_device_tree() creates an empty device tree, not one > >> with stuff in it. It seems reasonable to me for this property > >> on the root to be set in the same place we set other properties > >> of the root node. > > > > OK. Still the question about other virt machines remains > > unanswered :) > > From the DT consumer point of view, the interpretation and assumptions > around coherency *are* generally architecture- or platform-specific. For > instance on RISC-V, many platforms want to assume coherency by default > (and potentially use "dma-noncoherent" to mark individual devices that > aren't), while others may still want to do the opposite and use > "dma-coherent" in the same manner as Arm and AArch64. Neither property > existed back in ePAPR, so typical PowerPC systems wouldn't even be > looking and will just make their own assumptions by other means. > As Robin's comment says, each platform wants to assume coherency by default may be different. Adding it to all virt machines may introduce new risks. Currently, the issue is only valid on Fujitsu CPUs where the cache line size is 256 bytes, meaning the combination of kvm+virt-platform is needed to trigger the warning. So I'd be inclined to add this "dma-coherent" property for the "virt" platform first and advertise the property to other platforms if we hit the issue on those platforms. Thanks, Zhenyu
Re: [PATCH v1 00/16] vfio: QOMify VFIOContainer
On 6/17/24 3:30 AM, Duan, Zhenzhong wrote: Hi Cédric, -Original Message- From: Cédric Le Goater Subject: [PATCH v1 00/16] vfio: QOMify VFIOContainer Hello, The series starts with simple changes (patch 1-4). Two of which were initialy sent by Joao in a series adding VFIO migration support with vIOMMU [1]. The changes following prepare VFIOContainer for QOMification, switch the container models to QOM when ready and add some final cleanups. Except comment on patch 11 and 15, Others LGTM, so for other patches, yes. I will send a v2 soonish. It is on vfio-9.1 Reviewed-by: Zhenzhong Duan Thanks, C. Thanks Zhenzhong
[PULL v2 00/19] aspeed queue
The following changes since commit 05ad1440b8428b0ade9b8e5c01469adb8fbf83e3: Merge tag 'virtio-grants-v8-tag' of https://gitlab.com/sstabellini/qemu into staging (2024-06-15 20:13:06 -0700) are available in the Git repository at: https://github.com/legoater/qemu/ tags/pull-aspeed-20240617 for you to fetch changes up to 5f44521242d2fdfa190206a6be40577a58a71ef9: MAINTAINERS: Add reviewers for ASPEED BMCs (2024-06-16 21:08:54 +0200) aspeed queue: * Add AST2700 support Changes in v2: - Fixed class_size in TYPE_ASPEED_INTC definition - Fixed spelling : Unhandeled -> Unhandled Cédric Le Goater (1): aspeed/smc: Reintroduce "dram-base" property for AST2700 Jamin Lin (18): aspeed/wdt: Add AST2700 support aspeed/sli: Add AST2700 support aspeed/sdmc: remove redundant macros aspeed/sdmc: fix coding style aspeed/sdmc: Add AST2700 support aspeed/smc: correct device description aspeed/smc: support dma start length and 1 byte length unit aspeed/smc: support 64 bits dma dram address aspeed/smc: support different memory region ops for SMC flash region aspeed/smc: Add AST2700 support aspeed/scu: Add AST2700 support aspeed/intc: Add AST2700 support aspeed/soc: Add AST2700 support aspeed: Add an AST2700 eval board aspeed/soc: fix incorrect dram size for AST2700 test/avocado/machine_aspeed.py: Add AST2700 test case docs:aspeed: Add AST2700 Evaluation board MAINTAINERS: Add reviewers for ASPEED BMCs MAINTAINERS | 3 + docs/system/arm/aspeed.rst | 39 ++- include/hw/arm/aspeed_soc.h | 30 +- include/hw/intc/aspeed_intc.h| 44 +++ include/hw/misc/aspeed_scu.h | 47 ++- include/hw/misc/aspeed_sdmc.h| 5 +- include/hw/misc/aspeed_sli.h | 27 ++ include/hw/ssi/aspeed_smc.h | 3 + include/hw/watchdog/wdt_aspeed.h | 3 +- hw/arm/aspeed.c | 32 ++ hw/arm/aspeed_ast27x0.c | 648 +++ hw/intc/aspeed_intc.c| 361 ++ hw/misc/aspeed_scu.c | 306 +- hw/misc/aspeed_sdmc.c| 220 +++-- hw/misc/aspeed_sli.c | 177 +++ hw/ssi/aspeed_smc.c | 347 +++-- hw/watchdog/wdt_aspeed.c | 24 ++ hw/arm/meson.build | 1 + hw/intc/meson.build | 1 + hw/intc/trace-events | 13 + hw/misc/meson.build | 3 +- hw/misc/trace-events | 11 + hw/ssi/trace-events | 2 +- tests/avocado/machine_aspeed.py | 62 24 files changed, 2351 insertions(+), 58 deletions(-) create mode 100644 include/hw/intc/aspeed_intc.h create mode 100644 include/hw/misc/aspeed_sli.h create mode 100644 hw/arm/aspeed_ast27x0.c create mode 100644 hw/intc/aspeed_intc.c create mode 100644 hw/misc/aspeed_sli.c
Re: [PATCH] Update event idx if guest has made extra buffers during double check
On Thu, Jun 13, 2024 at 10:22 AM thomas wrote:
>
> Fixes: 06b12970174 ("virtio-net: fix network stall under load")
>
> If guest has made some buffers available during double check,
> but the total buffer size available is lower than @bufsize,
> notify the guest with the latest available idx(event idx)
> seen by the host.
> ---
> hw/net/virtio-net.c | 1 +
> 1 file changed, 1 insertion(+)
Patch looks good to me, but it misses some other stuff for example:
- the sob tag.
- fixes should be placed above sob tag
- need to cc qemu-sta...@nongnu.org
Thanks
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9c7e85caea..23c6c8c898 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1654,6 +1654,7 @@ static int virtio_net_has_buffers(VirtIONetQueue *q,
> int bufsize)
> if (virtio_queue_empty(q->rx_vq) ||
> (n->mergeable_rx_bufs &&
> !virtqueue_avail_bytes(q->rx_vq, bufsize, 0))) {
> +virtio_queue_set_notification(q->rx_vq, 1);
> return 0;
> }
> }
> --
> 2.39.0
>
RE: [PATCH v1 00/16] vfio: QOMify VFIOContainer
Hi Cédric, >-Original Message- >From: Cédric Le Goater >Subject: [PATCH v1 00/16] vfio: QOMify VFIOContainer > >Hello, > >The series starts with simple changes (patch 1-4). Two of which were >initialy sent by Joao in a series adding VFIO migration support with >vIOMMU [1]. > >The changes following prepare VFIOContainer for QOMification, switch >the container models to QOM when ready and add some final cleanups. Except comment on patch 11 and 15, Others LGTM, so for other patches, Reviewed-by: Zhenzhong Duan Thanks Zhenzhong
RE: [PATCH v3 1/7] HostIOMMUDevice: Store the VFIO/VDPA agent
Hi Cédric, >-Original Message- >From: Cédric Le Goater >Sent: Friday, June 14, 2024 6:05 PM >To: eric.au...@redhat.com; eric.auger@gmail.com; qemu- >de...@nongnu.org; qemu-...@nongnu.org; m...@redhat.com; jean- >phili...@linaro.org; peter.mayd...@linaro.org; yangh...@redhat.com; Duan, >Zhenzhong >Cc: alex.william...@redhat.com; jasow...@redhat.com; >pbonz...@redhat.com; berra...@redhat.com >Subject: Re: [PATCH v3 1/7] HostIOMMUDevice: Store the VFIO/VDPA agent > > >>> Talking of which, why are we passing a 'VFIODevice *' parameter to >>> HostIOMMUDeviceClass::realize ? I don't see a good reason >>> >>> I think a 'VFIOContainerBase *' would be more appropriate since >>> 'HostIOMMUDevice' represents a device on the host which is common >>> to all VFIO devices. >>> >>> In that case, HostIOMMUDevice::agent wouldn't need to be opaque >>> anymore. It could simply be a 'VFIOContainerBase *' and >>> hiod_legacy_vfio_get_iova_ranges() in patch 3 would grab the >>> 'iova_ranges' from the 'VFIOContainerBase *' directly. >>> >>> This means some rework : >>> >>> * vfio_device_get_aw_bits() would use a 'VFIOContainerBase *' instead. >>> * HostIOMMUDevice::name would be removed. This is just for error >>> messages. >>> * hiod_iommufd_vfio_realize() would use VFIOIOMMUFDContainer::be. >>> >>> That said, I think we need the QOMification changes first. >> >> OK I need to review your series first. At the moment I have just >> addressed Zhenzhong's comment in v4, just sent. > >Yep. Just take a look at mine. If both of you agree with above >proposal, I can care of it and resend all 3. It's a small change. I would suggest using opaque pointer and VFIODevice for two reasons, 1. in nesting series vIOMMU needs to attach/detaching hwpt which is VFIODevice operations. See https://github.com/yiliu1765/qemu/commit/3ca559d35adc9840555e361a56708af4c6338b3d 2. If we plan to support VDPA Device in future, the opaque pointer can also point to an VDPADevice structure. Thanks Zhenzhong
Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
On 6/7/24 1:57 AM, Thomas Huth wrote:
On 05/06/2024 16.48, Jared Rossi wrote:
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index c977a52b50..de3d1f0d5a 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -43,6 +43,7 @@ typedef unsigned long long u64;
#include "iplb.h"
/* start.s */
+extern char _start[];
void disabled_wait(void) __attribute__ ((__noreturn__));
void consume_sclp_int(void);
void consume_io_int(void);
@@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
static inline void panic(const char *string)
{
sclp_print(string);
+ if (load_next_iplb()) {
+ sclp_print("\nTrying next boot device...");
+ jump_to_IPL_code((long)_start);
+ }
+
disabled_wait();
}
Honestly, I am unsure whether this is a really cool idea or a very
ugly hack ... but I think I tend towards the latter, sorry. Jumping
back to the startup code might cause various problem, e.g.
pre-initialized variables don't get their values reset, causing
different behavior when the s390-ccw bios runs a function a second
time this way. Thus this sounds very fragile. Could we please try to
get things cleaned up correctly, so that functions return with error
codes instead of panicking when we can continue with another boot
device? Even if its more work right now, I think this will be much
more maintainable in the future.
Thomas
Thanks Thomas, I appreciate your insight. Your hesitation is
perfectly understandable as well. My initial design was like you
suggest, where the functions return instead of panic, but the issue I
ran into is that netboot uses a separate image, which we jump in to
at the start of IPL from a network device (see zipl_load() in
pc-bios/s390-ccw/bootmap.c). I wasn't able to come up with a simple
way to return to the main BIOS code if a netboot fails other than by
jumping back. So, it seems to me that netboot kind of throws a
monkeywrench into the basic idea of reworking the panics into returns.
I'm open to suggestions on a better way to recover from a failed
netboot, and it's certainly possible I've overlooked something, but
as far as I can tell a jump is necessary in that particular case at
least. Netboot could perhaps be handled as a special case where the
jump back is permitted whereas other device types return, but I don't
think that actually solves the main issue.
What are your thoughts on this?
Yes, I agree that jumping is currently required to get back from the
netboot code. So if you could rework your patches in a way that limits
the jumping to a failed netboot, that would be acceptable, I think.
Apart from that: We originally decided to put the netboot code into a
separate binary since the required roms/SLOF module might not always
have been checked out (it needed to be done manually), so we were not
able to compile it in all cases. But nowadays, this is handled in a
much nicer way, the submodule is automatically checked out once you
compile the s390x-softmmu target and have a s390x compiler available,
so I wonder whether we should maybe do the next step and integrate the
netboot code into the main s390-ccw.img now? Anybody got an opinion on
this?
Thomas
Hi Thomas,
I would generally defer the decision about integrating the netboot code
to someone with more insight than me, but for what it's worth, I am of
the opinion that if we want to rework all of panics into returns, then
it would make the most sense to also do the integration now so that we
can avoid using jump altogether. Unless I'm missing something simple, I
don't think the panic/return conversion will be trivial, and actually I
think it will be quite invasive since there are dozens of calls to panic
and assert that will need to be changed. It doesn't seem worthwhile to
do all of these conversions in order to avoid using jump, but then still
being exposed to possible problems caused by jumping due to netboot
requiring it anyway.
Regards,
Jared Rossi
Re: [PATCH 1/1] i386/tcg: Allow IRET from user mode to user mode for dotnet runtime
I do not think I will have the time or focus to work on improving this
patch this summer, as I will retire in 2 weeks and need to make a clean
break to focus on other things (health, for one) for a while.
If anyone wants to put into place Richard's ideas, I will not be offended!
I do not see any of this chatter in this email thread on the bug report
https://gitlab.com/qemu-project/qemu/-/issues/249
Robert Henry
On Sat, Jun 15, 2024 at 4:25 PM Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 6/11/24 09:20, Robert R. Henry wrote:
> > This fixes a bug wherein i386/tcg assumed an interrupt return using
> > the IRET instruction was always returning from kernel mode to either
> > kernel mode or user mode. This assumption is violated when IRET is used
> > as a clever way to restore thread state, as for example in the dotnet
> > runtime. There, IRET returns from user mode to user mode.
> >
> > This bug manifested itself as a page fault in the guest Linux kernel.
> >
> > This bug appears to have been in QEMU since the beginning.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/249
> > Signed-off-by: Robert R. Henry
> > ---
> > target/i386/tcg/seg_helper.c | 78 ++--
> > 1 file changed, 47 insertions(+), 31 deletions(-)
> >
> > diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
> > index 715db1f232..815d26e61d 100644
> > --- a/target/i386/tcg/seg_helper.c
> > +++ b/target/i386/tcg/seg_helper.c
> > @@ -843,20 +843,35 @@ static void do_interrupt_protected(CPUX86State
> *env, int intno, int is_int,
> >
> > #ifdef TARGET_X86_64
> >
> > -#define PUSHQ_RA(sp, val, ra) \
> > -{ \
> > -sp -= 8;\
> > -cpu_stq_kernel_ra(env, sp, (val), ra); \
> > -}
> > -
> > -#define POPQ_RA(sp, val, ra)\
> > -{ \
> > -val = cpu_ldq_kernel_ra(env, sp, ra); \
> > -sp += 8;\
> > -}
> > +#define PUSHQ_RA(sp, val, ra, cpl, dpl) \
> > + FUNC_PUSHQ_RA(env, , val, ra, cpl, dpl)
> > +
> > +static inline void FUNC_PUSHQ_RA(
> > +CPUX86State *env, target_ulong *sp,
> > +target_ulong val, target_ulong ra, int cpl, int dpl) {
> > + *sp -= 8;
> > + if (dpl == 0) {
> > +cpu_stq_kernel_ra(env, *sp, val, ra);
> > + } else {
> > +cpu_stq_data_ra(env, *sp, val, ra);
> > + }
> > +}
>
> This doesn't seem quite right.
>
> I would be much happier if we were to resolve the proper mmu index
> earlier, once, rather
> than within each call to cpu_{ld,st}*_{kernel,data}_ra. With the mmu
> index in hand, use
> cpu_{ld,st}*_mmuidx_ra instead.
>
> I believe you will want to factor out a subroutine of x86_cpu_mmu_index
> which passes in
> the pl, rather than reading cpl from env->hflags. This will also allow
> cpu_mmu_index_kernel to be eliminated or simplified, which is written to
> assume pl=0.
>
>
> r~
>
Re: [PATCH v2 2/4] target/ppc: Move VSX vector with length storage access insns to decodetree.
On 6/13/24 02:33, Chinmay Rath wrote:
+/* EA <- (ra == 0) ? 0 : GPR[ra] */
+static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
+{
+TCGv EA;
+if (!ra) {
+EA = tcg_constant_tl(0);
+return EA;
+}
+EA = tcg_temp_new();
+if (NARROW_MODE(ctx)) {
+tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
+} else {
+tcg_gen_mov_tl(EA, cpu_gpr[ra]);
Why are you making a copy, rather than just returning cpu_gpr[ra]?
If you need to modify the resulting EA, then you also need to make a copy for 0.
r~
Re: [PATCH 9/9] contrib/plugins: add ips plugin example for cost modeling
Pierrick Bouvier writes: > On 6/13/24 01:54, Philippe Mathieu-Daudé wrote: >> On 12/6/24 17:35, Alex Bennée wrote: >>> From: Pierrick Bouvier >>> >>> This plugin uses the new time control interface to make decisions >>> about the state of time during the emulation. The algorithm is >>> currently very simple. The user specifies an ips rate which applies >> ... IPS rate (Instructions Per Second) which ... >> >>> per core. If the core runs ahead of its allocated execution time the >>> plugin sleeps for a bit to let real time catch up. Either way time is >>> updated for the emulation as a function of total executed instructions >>> with some adjustments for cores that idle. >>> >>> Examples >>> >>> >>> Slow down execution of /bin/true: >>> $ num_insn=$(./build/qemu-x86_64 -plugin ./build/tests/plugin/libinsn.so -d >>> plugin /bin/true |& grep total | sed -e 's/.*: //') >>> $ time ./build/qemu-x86_64 -plugin >>> ./build/contrib/plugins/libips.so,ips=$(($num_insn/4)) /bin/true >>> real 4.000s >>> >>> Boot a Linux kernel simulating a 250MHz cpu: >>> $ /build/qemu-system-x86_64 -kernel /boot/vmlinuz-6.1.0-21-amd64 -append >>> "console=ttyS0" -plugin >>> ./build/contrib/plugins/libips.so,ips=$((250*1000*1000)) -smp 1 -m 512 >>> check time until kernel panic on serial0 >>> >>> Tested in system mode by booting a full debian system, and using: >>> $ sysbench cpu run >>> Performance decrease linearly with the given number of ips. >>> >>> Signed-off-by: Pierrick Bouvier >>> Message-Id: <20240530220610.1245424-7-pierrick.bouv...@linaro.org> >>> --- >>>contrib/plugins/ips.c| 164 +++ >>>contrib/plugins/Makefile | 1 + >>>2 files changed, 165 insertions(+) >>>create mode 100644 contrib/plugins/ips.c >>> >>> diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c >>> new file mode 100644 >>> index 00..db77729264 >>> --- /dev/null >>> +++ b/contrib/plugins/ips.c >>> @@ -0,0 +1,164 @@ >>> +/* >>> + * ips rate limiting plugin. >> The plugin names are really to packed to my taste (each time I look >> for >> one I have to open most source files to figure out the correct one); so >> please ease my life by using a more descriptive header at least: >>Instructions Per Second (IPS) rate limiting plugin. >> Thanks. >> > > I agree most of the plugin names are pretty cryptic, and they are > lacking a common "help" system, to describe what they do, and which > options are available for them. It's definitely something we could add > in the future. > > Regarding what you reported, I'm totally ok with the change. > > However, since this is a new series, I'm not if I or Alex should > change it. If it's ok for you to modify this Alex, it could be simpler > than waiting for me to push a new patch with just this. Its my tree so I'll fix it up. I'll ask you if I want a respin ;-) -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH] target/sparc: use signed denominator in sdiv helper
On 6/6/24 07:43, Clément Chigot wrote:
The result has to be done with the signed denominator (b32) instead of
the unsigned value passed in argument (b).
Fixes: 1326010322d6 ("target/sparc: Remove CC_OP_DIV")
Signed-off-by: Clément Chigot
---
target/sparc/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index 2247e243b5..7846ddd6f6 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -121,7 +121,7 @@ uint64_t helper_sdiv(CPUSPARCState *env, target_ulong a,
target_ulong b)
return (uint32_t)(b32 < 0 ? INT32_MAX : INT32_MIN) | (-1ull << 32);
}
-a64 /= b;
+a64 /= b32;
r = a64;
if (unlikely(r != a64)) {
return (uint32_t)(a64 < 0 ? INT32_MIN : INT32_MAX) | (-1ull << 32);
Queued, thanks.
r~
Re: [PATCH v2] linux-user: Make TARGET_NR_setgroups affect only the current thread
On 6/14/24 08:46, Ilya Leoshkevich wrote:
Like TARGET_NR_setuid, TARGET_NR_setgroups should affect only the
calling thread, and not the entire process. Therefore, implement it
using a syscall, and not a libc call.
Cc:qemu-sta...@nongnu.org
Fixes: 19b84f3c35d7 ("added setgroups and getgroups syscalls")
Signed-off-by: Ilya Leoshkevich
Reviewed-by: Philippe Mathieu-Daudé
---
v1:https://patchew.org/QEMU/20240131001851.15932-1-...@linux.ibm.com/
v1 -> v2: Rebase, add Philippe's R-b.
linux-user/syscall.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson
Queued.
r~
Re: [PATCH v3] accel/tcg: Fix typo causing tb->page_addr[1] to not be recorded
On 6/12/24 06:30, Anton Johansson wrote:
For TBs crossing page boundaries, the 2nd page will never be
recorded/removed, as the index of the 2nd page is computed from the
address of the 1st page. This is due to a typo, fix it.
Cc: qemu-sta...@nongnu.org
Fixes: deba78709a ("accel/tcg: Always lock pages before translation")
Signed-off-by: Anton Johansson
Reviewed-by: Manos Pitsidianakis
Reviewed-by: Philippe Mathieu-Daudé
---
accel/tcg/tb-maint.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Brown paper bag time.
Queued, thanks.
r~
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index 19ae6793f3..cc0f5afd47 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -713,7 +713,7 @@ static void tb_record(TranslationBlock *tb)
tb_page_addr_t paddr0 = tb_page_addr0(tb);
tb_page_addr_t paddr1 = tb_page_addr1(tb);
tb_page_addr_t pindex0 = paddr0 >> TARGET_PAGE_BITS;
-tb_page_addr_t pindex1 = paddr0 >> TARGET_PAGE_BITS;
+tb_page_addr_t pindex1 = paddr1 >> TARGET_PAGE_BITS;
assert(paddr0 != -1);
if (unlikely(paddr1 != -1) && pindex0 != pindex1) {
@@ -745,7 +745,7 @@ static void tb_remove(TranslationBlock *tb)
tb_page_addr_t paddr0 = tb_page_addr0(tb);
tb_page_addr_t paddr1 = tb_page_addr1(tb);
tb_page_addr_t pindex0 = paddr0 >> TARGET_PAGE_BITS;
-tb_page_addr_t pindex1 = paddr0 >> TARGET_PAGE_BITS;
+tb_page_addr_t pindex1 = paddr1 >> TARGET_PAGE_BITS;
assert(paddr0 != -1);
if (unlikely(paddr1 != -1) && pindex0 != pindex1) {
Re: [PATCH] Make TARGET_PAGE_MASK typed as target_ulong
On 6/16/24 10:40, Roman Kiryanov wrote:
Hi Richard,
thank you for looking into this.
No, this will cause failures, because we need this value to sign-extend to when
the
context includes {u}int64_t, and target_ulong is uint32_t.
I did not expect this, good catch. I see QEMU uses size_t as the
return type in qemu_target_page_size which returns TARGET_PAGE_SIZE.
Maybe use size_t for TARGET_PAGE_MASK everywhere (including
qemu_target_page_mask) as well?
No, because size_t != uint64_t.
We still support 32-bit hosts.
r~
Re: [PULL 0/5] virtio-grants-v8-tag
On 6/12/24 14:29, Stefano Stabellini wrote: The following changes since commit 80e8f0602168f451a93e71cbb1d59e93d745e62e: Merge tag 'virtio-grants-v8-tag' into staging (2024-06-09 11:21:55 -0700) are available in the Git repository at: https://gitlab.com/sstabellini/qemu.git for you to fetch changes up to 6d87a2a311fe4a8363143e6914d000697ea0cd83: hw/arm: xen: Enable use of grant mappings (2024-06-09 20:16:14 +0200) Edgar E. Iglesias (5): xen: mapcache: Make MCACHE_BUCKET_SHIFT runtime configurable xen: mapcache: Unmap first entries in buckets xen: mapcache: Pass the ram_addr offset to xen_map_cache() xen: mapcache: Add support for grant mappings hw/arm: xen: Enable use of grant mappings Applied, thanks. Please update https://wiki.qemu.org/ChangeLog/9.1 as appropriate. r~
Re: [PATCH] Make TARGET_PAGE_MASK typed as target_ulong
Hi Richard,
thank you for looking into this.
> No, this will cause failures, because we need this value to sign-extend to
> when the
> context includes {u}int64_t, and target_ulong is uint32_t.
I did not expect this, good catch. I see QEMU uses size_t as the
return type in qemu_target_page_size which returns TARGET_PAGE_SIZE.
Maybe use size_t for TARGET_PAGE_MASK everywhere (including
qemu_target_page_mask) as well?
> What options are you using, because this warning should not be generated with
> -fwrapv.
Good point, I think we missed this option.
Regards,
Roman.
Re: [PATCH v14 12/14] virtio-gpu: Handle resource blob commands
On 2024/06/16 10:03, Dmitry Osipenko wrote:
From: Antonio Caggiano
Support BLOB resources creation, mapping and unmapping by calling the
new stable virglrenderer 0.10 interface. Only enabled when available and
via the blob config. E.g. -device virtio-vga-gl,blob=true
Signed-off-by: Antonio Caggiano
Signed-off-by: Xenia Ragiadakou
Signed-off-by: Huang Rui
Signed-off-by: Dmitry Osipenko
---
hw/display/virtio-gpu-gl.c | 3 +
hw/display/virtio-gpu-virgl.c | 309 -
hw/display/virtio-gpu.c| 6 +-
include/hw/virtio/virtio-gpu.h | 2 +
4 files changed, 316 insertions(+), 4 deletions(-)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 4fe9e6a0c21c..5f27568d3ec8 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -160,6 +160,9 @@ static void virtio_gpu_gl_device_unrealize(DeviceState
*qdev)
VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
if (gl->renderer_state >= RS_INITED) {
+#if VIRGL_VERSION_MAJOR >= 1
+qemu_bh_delete(gl->cmdq_resume_bh);
+#endif
if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
timer_free(gl->print_stats);
}
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 60befab7efc2..fed3e27b2fc9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -26,6 +26,7 @@
struct virtio_gpu_virgl_resource {
struct virtio_gpu_simple_resource base;
+MemoryRegion *mr;
};
static struct virtio_gpu_virgl_resource *
@@ -49,6 +50,153 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
}
#endif
+#if VIRGL_VERSION_MAJOR >= 1
+typedef enum {
+HOSTMEM_MR_MAPPED,
HOSTMEM_MR_MAPPED is no longer used.
Re: [PATCH v14 10/14] virtio-gpu: Support blob scanout using dmabuf fd
On 2024/06/16 10:03, Dmitry Osipenko wrote:
From: Robert Beckett
Support displaying blob resources by handling SET_SCANOUT_BLOB
command.
Signed-by: Antonio Caggiano
Signed-off-by: Robert Beckett
Signed-off-by: Huang Rui
Reviewed-by: Antonio Caggiano
Signed-off-by: Dmitry Osipenko
---
hw/display/virtio-gpu-virgl.c | 109 +
hw/display/virtio-gpu.c| 12 ++--
include/hw/virtio/virtio-gpu.h | 7 +++
3 files changed, 122 insertions(+), 6 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 3ffea478e723..60befab7efc2 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,8 @@
#include "trace.h"
#include "hw/virtio/virtio.h"
#include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
+#include "hw/virtio/virtio-gpu-pixman.h"
#include "ui/egl-helpers.h"
@@ -78,6 +80,7 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
res->base.height = c2d.height;
res->base.format = c2d.format;
res->base.resource_id = c2d.resource_id;
+res->base.dmabuf_fd = -1;
QTAILQ_INSERT_HEAD(>reslist, >base, next);
args.handle = c2d.resource_id;
@@ -125,6 +128,7 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
res->base.height = c3d.height;
res->base.format = c3d.format;
res->base.resource_id = c3d.resource_id;
+res->base.dmabuf_fd = -1;
QTAILQ_INSERT_HEAD(>reslist, >base, next);
args.handle = c3d.resource_id;
@@ -509,6 +513,106 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
g_free(resp);
}
+#if VIRGL_VERSION_MAJOR >= 1
+static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
+ struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_framebuffer fb = { 0 };
+struct virgl_renderer_resource_info info;
+struct virtio_gpu_virgl_resource *res;
+struct virtio_gpu_set_scanout_blob ss;
+uint64_t fbend;
+
+VIRTIO_GPU_FILL_CMD(ss);
+virtio_gpu_scanout_blob_bswap();
+trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
+ ss.r.width, ss.r.height, ss.r.x,
+ ss.r.y);
+
+if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
+ __func__, ss.scanout_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
+return;
+}
+
+if (ss.resource_id == 0) {
+virtio_gpu_disable_scanout(g, ss.scanout_id);
+return;
+}
+
+if (ss.width < 16 ||
+ss.height < 16 ||
+ss.r.x + ss.r.width > ss.width ||
+ss.r.y + ss.r.height > ss.height) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
+ " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
+ __func__, ss.scanout_id, ss.resource_id,
+ ss.r.x, ss.r.y, ss.r.width, ss.r.height,
+ ss.width, ss.height);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
+
+res = virtio_gpu_virgl_find_resource(g, ss.resource_id);
+if (!res) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
+ __func__, ss.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+if (virgl_renderer_resource_get_info(ss.resource_id, )) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not have info %d\n",
+ __func__, ss.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+if (res->base.dmabuf_fd < 0) {
+res->base.dmabuf_fd = info.fd;
res->base.dmabuf_fd is conditionally assigned but
virgl_renderer_resource_get_info() is called unconditionally, which is
inconsistent.
The relevant code is better to be moved into
virgl_cmd_resource_create_blob() for consistenty with
virtio_gpu_resource_create_blob().
