[PATCH] virtiofsd: Add `sigreturn` to the seccomp whitelist
The virtiofsd currently crashes on s390x. This is because of a
`sigreturn` system call. See audit log below:
type=SECCOMP msg=audit(1669382477.611:459): auid=4294967295 uid=0 gid=0
ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 pid=6649
comm="virtiofsd" exe="/usr/libexec/virtiofsd" sig=31 arch=8016 syscall=119
compat=0 ip=0x3fff15f748a code=0x8000AUID="unset" UID="root" GID="root"
ARCH=s390x SYSCALL=sigreturn
Signed-off-by: Marc Hartmayer
---
tools/virtiofsd/passthrough_seccomp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/virtiofsd/passthrough_seccomp.c
b/tools/virtiofsd/passthrough_seccomp.c
index 888295c073de..0033dab4939e 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -110,6 +110,7 @@ static const int syscall_allowlist[] = {
#endif
SCMP_SYS(set_robust_list),
SCMP_SYS(setxattr),
+SCMP_SYS(sigreturn),
SCMP_SYS(symlinkat),
SCMP_SYS(syncfs),
SCMP_SYS(time), /* Rarely needed, except on static builds */
--
2.34.1
Re: [Virtio-fs] [PATCH] virtiofsd: Add `sigreturn` to the seccomp whitelist
German Maglione writes:
> On Fri, Nov 25, 2022 at 3:40 PM Marc Hartmayer wrote:
>>
>> The virtiofsd currently crashes on s390x. This is because of a
>> `sigreturn` system call. See audit log below:
>>
>> type=SECCOMP msg=audit(1669382477.611:459): auid=4294967295 uid=0 gid=0
>> ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 pid=6649
>> comm="virtiofsd" exe="/usr/libexec/virtiofsd" sig=31 arch=8016
>> syscall=119 compat=0 ip=0x3fff15f748a code=0x8000AUID="unset" UID="root"
>> GID="root" ARCH=s390x SYSCALL=sigreturn
>>
>> Signed-off-by: Marc Hartmayer
>> ---
>> tools/virtiofsd/passthrough_seccomp.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/virtiofsd/passthrough_seccomp.c
>> b/tools/virtiofsd/passthrough_seccomp.c
>> index 888295c073de..0033dab4939e 100644
>> --- a/tools/virtiofsd/passthrough_seccomp.c
>> +++ b/tools/virtiofsd/passthrough_seccomp.c
>> @@ -110,6 +110,7 @@ static const int syscall_allowlist[] = {
>> #endif
>> SCMP_SYS(set_robust_list),
>> SCMP_SYS(setxattr),
>> +SCMP_SYS(sigreturn),
>> SCMP_SYS(symlinkat),
>> SCMP_SYS(syncfs),
>> SCMP_SYS(time), /* Rarely needed, except on static builds */
>> --
>> 2.34.1
>>
>> ___
>> Virtio-fs mailing list
>> virtio...@redhat.com
>> https://listman.redhat.com/mailman/listinfo/virtio-fs
>>
>
> Reviewed-by: German Maglione
Thanks.
>
> Should we add this also in the rust version?, I see we don't have it
> enabled either.
Yep - thanks.
>
> --
> German
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH] virtiofsd: Add `sigreturn` to the seccomp whitelist
"Dr. David Alan Gilbert" writes: > * Marc Hartmayer (mhart...@linux.ibm.com) wrote: >> The virtiofsd currently crashes on s390x. This is because of a >> `sigreturn` system call. See audit log below: >> >> type=SECCOMP msg=audit(1669382477.611:459): auid=4294967295 uid=0 gid=0 >> ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 pid=6649 >> comm="virtiofsd" exe="/usr/libexec/virtiofsd" sig=31 arch=8016 >> syscall=119 compat=0 ip=0x3fff15f748a code=0x8000AUID="unset" UID="root" >> GID="root" ARCH=s390x SYSCALL=sigreturn > > I'm curious; doesn't that mean that some signal is being delivered and > you're returning? Which one? code=0x8000 means that the seccomp action SECCOMP_RET_KILL_PROCESS is taken => process is killed by a SIGSYS signal (31) [1]. At least, that’s my understanding of this log message. [1] https://man7.org/linux/man-pages/man2/seccomp.2.html […snip…] > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH] virtiofsd: Add `sigreturn` to the seccomp whitelist
Christian Borntraeger writes:
> Am 29.11.22 um 10:52 schrieb Christian Borntraeger:
>>
>>
>> Am 29.11.22 um 10:42 schrieb Dr. David Alan Gilbert:
>>> * Marc Hartmayer (mhart...@linux.ibm.com) wrote:
>>>> "Dr. David Alan Gilbert" writes:
>>>>
>>>>> * Marc Hartmayer (mhart...@linux.ibm.com) wrote:
>>>>>> The virtiofsd currently crashes on s390x. This is because of a
>>>>>> `sigreturn` system call. See audit log below:
>>>>>>
>>>>>> type=SECCOMP msg=audit(1669382477.611:459): auid=4294967295 uid=0 gid=0
>>>>>> ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 pid=6649
>>>>>> comm="virtiofsd" exe="/usr/libexec/virtiofsd" sig=31 arch=8016
>>>>>> syscall=119 compat=0 ip=0x3fff15f748a code=0x8000AUID="unset"
>>>>>> UID="root" GID="root" ARCH=s390x SYSCALL=sigreturn
>>>>>
>>>>> I'm curious; doesn't that mean that some signal is being delivered and
>>>>> you're returning? Which one?
>>>>
>>>> code=0x8000 means that the seccomp action SECCOMP_RET_KILL_PROCESS
>>>> is taken => process is killed by a SIGSYS signal (31) [1].
>>>>
>>>> At least, that’s my understanding of this log message.
>>>>
>>>> [1] https://man7.org/linux/man-pages/man2/seccomp.2.html
>>>
>>> But isn't that the fallout rather than the cause ? i.e. seccomp
>>> is sending a SIGSYS because the process used sigreturn, my question
>>> is why did the process call sigreturn in the first place - it must
>>> have received a signal to return from?
>>
>> Good question. virtiofsd seems to prepare itself for
>>
>> int fuse_set_signal_handlers(struct fuse_session *se)
>> {
>> /*
>> * If we used SIG_IGN instead of the do_nothing function,
>> * then we would be unable to tell if we set SIG_IGN (and
>> * thus should reset to SIG_DFL in fuse_remove_signal_handlers)
>> * or if it was already set to SIG_IGN (and should be left
>> * untouched.
>> */
>> if (set_one_signal_handler(SIGHUP, exit_handler, 0) == -1 ||
>> set_one_signal_handler(SIGINT, exit_handler, 0) == -1 ||
>> set_one_signal_handler(SIGTERM, exit_handler, 0) == -1 ||
>> set_one_signal_handler(SIGPIPE, do_nothing, 0) == -1) {
>> return -1;
>> }
>>
>>
>>
>> Given that rt_sigreturn was already on the seccomp list it seems
>> to be expected that those handlers are called.
>
> For me, it seems to happen on shutdown:
> Stack trace of thread 1:
> #0 0x03ffc06f348a __kernel_sigreturn (linux-vdso64.so.1
> + 0x48a)
> #1 0x03ffc06f3488 __kernel_sigreturn (linux-vdso64.so.1
> + 0x488)
> #2 0x03ff9af1be96
> __GI___futex_abstimed_wait_cancelable64 (libc.so.6 + 0x9be96)
> #3 0x03ff9af211b4 __pthread_clockjoin_ex (libc.so.6 +
> 0xa11b4)
> #4 0x03ff9af2106e pthread_join@GLIBC_2.2 (libc.so.6 +
> 0xa106e)
> #5 0x02aa35d2fe36 fv_queue_cleanup_thread (virtiofsd +
> 0x2fe36)
> #6 0x02aa35d3152c stop_all_queues (virtiofsd + 0x3152c)
> #7 0x02aa35d2869c main (virtiofsd + 0x2869c)
> #8 0x03ff9aeb4872 __libc_start_call_main (libc.so.6 +
> 0x34872)
> #9 0x03ff9aeb4950 __libc_start_main@@GLIBC_2.34
> (libc.so.6 + 0x34950)
> #10 0x02aa35d290a0 .annobin_libvhost_user.c_end.startup
> (virtiofsd + 0x290a0)
>
>
That’s also what I see.
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Virtio-fs] [PATCH] virtiofsd: Add `sigreturn` to the seccomp whitelist
German Maglione writes:
> On Mon, Nov 28, 2022 at 10:00 AM Marc Hartmayer
> wrote:
>>
>> German Maglione writes:
>>
>> > On Fri, Nov 25, 2022 at 3:40 PM Marc Hartmayer
>> > wrote:
>> >>
>> >> The virtiofsd currently crashes on s390x. This is because of a
>> >> `sigreturn` system call. See audit log below:
>> >>
>> >> type=SECCOMP msg=audit(1669382477.611:459): auid=4294967295 uid=0 gid=0
>> >> ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 pid=6649
>> >> comm="virtiofsd" exe="/usr/libexec/virtiofsd" sig=31 arch=8016
>> >> syscall=119 compat=0 ip=0x3fff15f748a code=0x8000AUID="unset"
>> >> UID="root" GID="root" ARCH=s390x SYSCALL=sigreturn
>> >>
>> >> Signed-off-by: Marc Hartmayer
>> >> ---
>> >> tools/virtiofsd/passthrough_seccomp.c | 1 +
>> >> 1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/tools/virtiofsd/passthrough_seccomp.c
>> >> b/tools/virtiofsd/passthrough_seccomp.c
>> >> index 888295c073de..0033dab4939e 100644
>> >> --- a/tools/virtiofsd/passthrough_seccomp.c
>> >> +++ b/tools/virtiofsd/passthrough_seccomp.c
>> >> @@ -110,6 +110,7 @@ static const int syscall_allowlist[] = {
>> >> #endif
>> >> SCMP_SYS(set_robust_list),
>> >> SCMP_SYS(setxattr),
>> >> +SCMP_SYS(sigreturn),
>> >> SCMP_SYS(symlinkat),
>> >> SCMP_SYS(syncfs),
>> >> SCMP_SYS(time), /* Rarely needed, except on static builds */
>> >> --
>> >> 2.34.1
>> >>
>> >> ___
>> >> Virtio-fs mailing list
>> >> virtio...@redhat.com
>> >> https://listman.redhat.com/mailman/listinfo/virtio-fs
>> >>
>> >
>> > Reviewed-by: German Maglione
>>
>> Thanks.
>>
>> >
>> > Should we add this also in the rust version?, I see we don't have it
>> > enabled either.
>>
>> Yep - thanks.
>
> Could you test this MR to see if it is ok?
> https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/144
I couldn’t reproduce the problem using the Rust version - even without
your patch. With your patch applied it’s (of course) still working.
>
> Thanks,
>
> --
> German
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH] system/qdev-monitor: move drain_call_rcu call under if (!dev) in qmp_device_add()
On Fri, Nov 03, 2023 at 01:56 PM +0300, Dmitrii Gavrilov
wrote:
> Original goal of addition of drain_call_rcu to qmp_device_add was to cover
> the failure case of qdev_device_add. It seems call of drain_call_rcu was
> misplaced in 7bed89958bfbf40df what led to waiting for pending RCU callbacks
> under happy path too. What led to overall performance degradation of
> qmp_device_add.
>
> In this patch call of drain_call_rcu moved under handling of failure of
> qdev_device_add.
>
> Signed-off-by: Dmitrii Gavrilov
I don't know the exact reason, but this commit caused udev events to
show up much slower than before (~3s vs. ~23s) when a virtio-scsi device
is hotplugged (I’ve tested this only on s390x). Importantly, this only
happens when asynchronous SCSI scanning is disabled in the *guest*
kernel (scsi_mod.scan=sync or CONFIG_SCSI_SCAN_ASYNC=n).
The `udevadm monitor` output captured while hotplugging the device
(using QEMU 012b170173bc ("system/qdev-monitor: move drain_call_rcu call
under if (!dev) in qmp_device_add()")):
…
KERNEL[2.166575] add /devices/css0/0.0.0002/0.0.0002 (ccw)
KERNEL[2.166594] bind /devices/css0/0.0.0002/0.0.0002 (ccw)
KERNEL[2.166826] add /devices/css0/0.0.0002/0.0.0002/virtio2 (virtio)
UDEV [2.166846] add /devices/css0/0.0.0002/0.0.0002 (ccw)
UDEV [2.167013] bind /devices/css0/0.0.0002/0.0.0002 (ccw)
KERNEL[2.167560] add /devices/virtual/workqueue/scsi_tmf_0 (workqueue)
UDEV [2.167977] add /devices/virtual/workqueue/scsi_tmf_0 (workqueue)
KERNEL[2.167987] add /devices/css0/0.0.0002/0.0.0002/virtio2/host0 (scsi)
KERNEL[2.167996] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/scsi_host/host0 (scsi_host)
KERNEL[2.169113] change /0:0:0:0 (scsi)
UDEV [2.169212] change /0:0:0:0 (scsi)
KERNEL[2.199500] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0 (scsi)
KERNEL[2.199513] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0 (scsi)
KERNEL[2.199523] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/scsi_device/0:0:0:0
(scsi_device)
KERNEL[2.199532] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/scsi_disk/0:0:0:0
(scsi_disk)
KERNEL[2.199564] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/scsi_generic/sg0
(scsi_generic)
KERNEL[2.199586] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/bsg/0:0:0:0
(bsg)
KERNEL[2.280482] add /devices/virtual/bdi/8:0 (bdi)
UDEV [2.280634] add /devices/virtual/bdi/8:0 (bdi)
KERNEL[3.060145] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/block/sda
(block)
KERNEL[3.060160] bind
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0 (scsi)
KERNEL[22.160147] bind /devices/css0/0.0.0002/0.0.0002/virtio2 (virtio)
KERNEL[22.160161] add /bus/virtio/drivers/virtio_scsi (drivers)
KERNEL[22.160169] add /module/virtio_scsi (module)
UDEV [22.161078] add /devices/css0/0.0.0002/0.0.0002/virtio2 (virtio)
UDEV [22.161339] add /bus/virtio/drivers/virtio_scsi (drivers)
UDEV [22.161860] add /devices/css0/0.0.0002/0.0.0002/virtio2/host0 (scsi)
UDEV [22.161869] add /module/virtio_scsi (module)
UDEV [22.161880] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0 (scsi)
UDEV [22.161890] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/scsi_host/host0 (scsi_host)
UDEV [22.161901] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0 (scsi)
UDEV [22.161911] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/scsi_disk/0:0:0:0
(scsi_disk)
UDEV [22.161924] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/bsg/0:0:0:0
(bsg)
UDEV [22.161937] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/scsi_generic/sg0
(scsi_generic)
UDEV [22.162123] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/scsi_device/0:0:0:0
(scsi_device)
UDEV [22.468924] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0/block/sda
(block)
UDEV [22.473955] bind
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/target0:0:0/0:0:0:0 (scsi)
UDEV [22.473970] bind /devices/css0/0.0.0002/0.0.0002/virtio2 (virtio)
The `udevadm monitor` output without this commit (QEMU 9876359990dd
("hw/scsi/lsi53c895a: add timer to scripts processing")):
…
KERNEL[2.091114] add /devices/virtual/workqueue/scsi_tmf_0 (workqueue)
UDEV [2.091218] add /devices/virtual/workqueue/scsi_tmf_0 (workqueue)
KERNEL[2.091408] add /devices/css0/0.0.0002/0.0.0002/virtio2/host0 (scsi)
KERNEL[2.091418] add
/devices/css0/0.0.0002/0.0.0002/virtio2/host0/scsi_host/host0 (scsi_host)
KERNEL[2.200461] bind /devices/css0/0.0.0002/0.0.0002/virtio2 (virtio)
KERNEL[2.200473] add /bus/virtio/drivers/virtio_scsi (drivers)
KERNEL[2.200481] add /module/v
Re: [PATCH] system/qdev-monitor: move drain_call_rcu call under if (!dev) in qmp_device_add()
On Fri, Apr 26, 2024 at 11:57 AM +0300, Dmitrii Gavrilov
wrote:
> 26.04.2024, 11:17, "Marc Hartmayer" :
>
> On Fri, Nov 03, 2023 at 01:56 PM +0300, Dmitrii Gavrilov
> wrote:
>
> Original goal of addition of drain_call_rcu to qmp_device_add was to cover
> the failure case of qdev_device_add. It seems call of drain_call_rcu was
> misplaced in 7bed89958bfbf40df what led to waiting for pending RCU callbacks
> under happy path too. What led to overall performance degradation of
> qmp_device_add.
>
> In this patch call of drain_call_rcu moved under handling of failure of
> qdev_device_add.
>
> Signed-off-by: Dmitrii Gavrilov
>
> I don't know the exact reason, but this commit caused udev events to
> show up much slower than before (~3s vs. ~23s) when a virtio-scsi device
> is hotplugged (I’ve tested this only on s390x). Importantly, this only
> happens when asynchronous SCSI scanning is disabled in the *guest*
> kernel (scsi_mod.scan=sync or CONFIG_SCSI_SCAN_ASYNC=n).
>
> The `udevadm monitor` output captured while hotplugging the device
> (using QEMU 012b170173bc ("system/qdev-monitor: move drain_call_rcu call
> under if (!dev) in qmp_device_add()")):
>
[…snip…]
> Any ideas?
>
> Thanks in advance.
>
> Kind regards,
> Marc
>
> Hello!
>
> Thank you for mentioning this.
>
> At first glance it seems that using scsi in synchonous mode caues the global
> QEMU mutex lock until the scanning phase is complete. Prior to 012b170173bc
> ("system/qdev-monitor: move drain_call_rcu call under if (!dev) in
> qmp_device_add()") on each device adition the lock would be forcibly removed
> allowing callbacks (including UDEV ones) to be processed after a new device
> is added.
>
> I`ll try to investigate this furter. But currently it appears to me like
> performance or observability dilemma.
I tried the test on my local laptop (x86_64) and there seems to be no
issue (I used the kernel cmdline option scsi_mod.scan=sync for the
guest) - guest and host kernel == 6.8.7. But please double check.
>
> Is behaviour you mentioning consistant?
Yep, at least for more than 50 iterations (I stopped the test then).
>
> Best regards,
> Dmitrii
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH v1 1/1] pc-bios/s390-ccw: fix sclp_get_loadparm_ascii
On Thu, Nov 28, 2019 at 01:33 PM +0100, Claudio Imbrenda
wrote:
> The existing s390 bios gets the LOADPARM information from the system using
> an SCLP call that specifies a buffer length too small to contain all the
> output.
>
> The recent fixes in the SCLP code have exposed this bug, since now the
> SCLP call will return an error (as per architecture) instead of
> writing partially and completing successfully.
>
> The solution is simply to specify the full page length as the SCCB
> length instead of a smaller size.
>
> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
> Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info")
>
> Signed-off-by: Claudio Imbrenda
> ---
> pc-bios/s390-ccw/sclp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index c0223fa..7251f9a 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm)
> ReadInfo *sccb = (void *)_sccb;
>
> memset((char *)_sccb, 0, sizeof(ReadInfo));
> -sccb->h.length = sizeof(ReadInfo);
> +sccb->h.length = SCCB_SIZE;
> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) {
> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN);
> }
> --
> 2.7.4
Tested-by: Marc Hartmayer
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH] s390x/ipl: support extended kernel command line size
In the past s390 used a fixed command line length of 896 bytes. This has changed
with the Linux commit 5ecb2da660ab ("s390: support command lines longer than 896
bytes"). There is now a parm area indicating the maximum command line size. This
parm area has always been initialized to zero, so with older kernels this field
would read zero and we must then assume that only 896 bytes are available.
Acked-by: Viktor Mihajlovski
Signed-off-by: Marc Hartmayer
---
hw/s390x/ipl.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 7ddca0127fc2..092c66b3f9f1 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -37,8 +37,9 @@
#define KERN_IMAGE_START0x01UL
#define LINUX_MAGIC_ADDR0x010008UL
+#define KERN_PARM_AREA_SIZE_ADDR0x010430UL
#define KERN_PARM_AREA 0x010480UL
-#define KERN_PARM_AREA_SIZE 0x000380UL
+#define LEGACY_KERN_PARM_AREA_SIZE 0x000380UL
#define INITRD_START0x80UL
#define INITRD_PARM_START 0x010408UL
#define PARMFILE_START 0x001000UL
@@ -110,6 +111,21 @@ static uint64_t bios_translate_addr(void *opaque, uint64_t
srcaddr)
return srcaddr + dstaddr;
}
+static uint64_t get_max_kernel_cmdline_size(void)
+{
+uint64_t *size_ptr = rom_ptr(KERN_PARM_AREA_SIZE_ADDR, sizeof(*size_ptr));
+
+if (size_ptr) {
+uint64_t size;
+
+size = be64_to_cpu(*size_ptr);
+if (size != 0) {
+return size;
+}
+}
+return LEGACY_KERN_PARM_AREA_SIZE;
+}
+
static void s390_ipl_realize(DeviceState *dev, Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
@@ -197,10 +213,11 @@ static void s390_ipl_realize(DeviceState *dev, Error
**errp)
ipl->start_addr = KERN_IMAGE_START;
/* Overwrite parameters in the kernel image, which are "rom" */
if (parm_area) {
-if (cmdline_size > KERN_PARM_AREA_SIZE) {
+uint64_t max_cmdline_size = get_max_kernel_cmdline_size();
+if (cmdline_size > max_cmdline_size) {
error_setg(errp,
"kernel command line exceeds maximum size: %zu
> %lu",
- cmdline_size, KERN_PARM_AREA_SIZE);
+ cmdline_size, max_cmdline_size);
return;
}
--
2.31.1
Re: [PATCH] s390x/ipl: support extended kernel command line size
David Hildenbrand writes:
> On 22.11.21 12:29, Marc Hartmayer wrote:
>> In the past s390 used a fixed command line length of 896 bytes. This has
>> changed
>> with the Linux commit 5ecb2da660ab ("s390: support command lines longer than
>> 896
>> bytes"). There is now a parm area indicating the maximum command line size.
>> This
>> parm area has always been initialized to zero, so with older kernels this
>> field
>> would read zero and we must then assume that only 896 bytes are available.
>>
>> Acked-by: Viktor Mihajlovski
>> Signed-off-by: Marc Hartmayer
>> ---
>> hw/s390x/ipl.c | 23 ---
>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 7ddca0127fc2..092c66b3f9f1 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -37,8 +37,9 @@
>>
>> #define KERN_IMAGE_START0x01UL
>> #define LINUX_MAGIC_ADDR0x010008UL
>> +#define KERN_PARM_AREA_SIZE_ADDR0x010430UL
>> #define KERN_PARM_AREA 0x010480UL
>> -#define KERN_PARM_AREA_SIZE 0x000380UL
>> +#define LEGACY_KERN_PARM_AREA_SIZE 0x000380UL
>> #define INITRD_START0x80UL
>> #define INITRD_PARM_START 0x010408UL
>> #define PARMFILE_START 0x001000UL
>> @@ -110,6 +111,21 @@ static uint64_t bios_translate_addr(void *opaque,
>> uint64_t srcaddr)
>> return srcaddr + dstaddr;
>> }
>>
>> +static uint64_t get_max_kernel_cmdline_size(void)
>> +{
>> +uint64_t *size_ptr = rom_ptr(KERN_PARM_AREA_SIZE_ADDR,
>> sizeof(*size_ptr));
>> +
>> +if (size_ptr) {
>> +uint64_t size;
>> +
>> +size = be64_to_cpu(*size_ptr);
>> +if (size != 0) {
>
> Could do "if (size) {"
Ok.
>
>> +return size;
>> +}
>> +}
>> +return LEGACY_KERN_PARM_AREA_SIZE;
>> +}
>> +
>> static void s390_ipl_realize(DeviceState *dev, Error **errp)
>> {
>> MachineState *ms = MACHINE(qdev_get_machine());
>> @@ -197,10 +213,11 @@ static void s390_ipl_realize(DeviceState *dev, Error
>> **errp)
>> ipl->start_addr = KERN_IMAGE_START;
>> /* Overwrite parameters in the kernel image, which are "rom" */
>> if (parm_area) {
>> -if (cmdline_size > KERN_PARM_AREA_SIZE) {
>> +uint64_t max_cmdline_size = get_max_kernel_cmdline_size();
>
> We might want an empty line here.
Yep.
>
>> +if (cmdline_size > max_cmdline_size) {
>> error_setg(errp,
>> "kernel command line exceeds maximum size:
>> %zu > %lu",
>> - cmdline_size, KERN_PARM_AREA_SIZE);
>> + cmdline_size, max_cmdline_size);
>> return;
>> }
>>
>>
>
> Reviewed-by: David Hildenbrand
Thanks for the review!
>
> --
> Thanks,
>
> David / dhildenb
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH] s390x/ipl: check kernel command line size
Check if the provided kernel command line exceeds the maximum size of the s390x
Linux kernel command line size, which is 896 bytes.
Reported-by: Sven Schnelle
Signed-off-by: Marc Hartmayer
---
hw/s390x/ipl.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 1821c6faeef3..a58ea58cc736 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -38,6 +38,7 @@
#define KERN_IMAGE_START0x01UL
#define LINUX_MAGIC_ADDR0x010008UL
#define KERN_PARM_AREA 0x010480UL
+#define KERN_PARM_AREA_SIZE 0x000380UL
#define INITRD_START0x80UL
#define INITRD_PARM_START 0x010408UL
#define PARMFILE_START 0x001000UL
@@ -190,10 +191,19 @@ static void s390_ipl_realize(DeviceState *dev, Error
**errp)
* loader) and it won't work. For this case we force it to 0x1,
too.
*/
if (pentry == KERN_IMAGE_START || pentry == 0x800) {
-char *parm_area = rom_ptr(KERN_PARM_AREA, strlen(ipl->cmdline) +
1);
+size_t cmdline_size = strlen(ipl->cmdline) + 1;
+char *parm_area = rom_ptr(KERN_PARM_AREA, cmdline_size);
+
ipl->start_addr = KERN_IMAGE_START;
/* Overwrite parameters in the kernel image, which are "rom" */
if (parm_area) {
+if (cmdline_size > KERN_PARM_AREA_SIZE) {
+error_setg(errp,
+ "kernel command line exceeds maximum size: %lu
> %lu",
+ cmdline_size, KERN_PARM_AREA_SIZE);
+return;
+}
+
strcpy(parm_area, ipl->cmdline);
}
} else {
--
2.25.1
Re: [PATCH 02/13] s390_flic: add migration-enabled property
aque=, vmdesc=vmdesc@entry=0x2aa36602280, version_id=version_id@entry=3, errp=0x3ff73efb438) at ../migration/vmstate.c:408 #13 0x02aa348b2dbe in vmstate_save_state_with_err (f=f@entry=0x2aa36602bd0, vmsd=, opaque=, vmdesc_id=vmdesc_id@entry=0x2aa36602280, errp=errp@entry=0x3ff73efb438) at ../migration/vmstate.c:347 #14 0x02aa344993ce in vmstate_save (f=f@entry=0x2aa36602bd0, se=se@entry=0x2aa365cac80, vmdesc=vmdesc@entry=0x2aa36602280, errp=, errp@entry=0x3ff73efb438) at ../migration/savevm.c:1037 #15 0x02aa3449cb80 in qemu_savevm_state_complete_precopy_non_iterable (f=f@entry=0x2aa36602bd0, in_postcopy=, in_postcopy@entry=false, inactivate_disks=false, inactivate_disks@entry=true) at ../migration/savevm.c:1554 #16 0x02aa3449d15a in qemu_savevm_state_complete_precopy (f=0x2aa36602bd0, iterable_only=iterable_only@entry=false, inactivate_disks=false) at ../migration/savevm.c:1630 #17 0x02aa3448ca00 in migration_completion_precopy (s=0x2aa3625d1f0, current_active_state=0x3ff73efb67c) at ../migration/migration.c:2710 #18 migration_completion (s=0x2aa3625d1f0) at ../migration/migration.c:2774 #19 migration_iteration_run (s=0x2aa3625d1f0) at ../migration/migration.c:3198 #20 migration_thread (opaque=opaque@entry=0x2aa3625d1f0) at ../migration/migration.c:3464 #21 0x02aa3483bc12 in qemu_thread_start (args=) at ../util/qemu-thread-posix.c:541 #22 0x03ff91bac3fa in start_thread () at /lib64/libc.so.6 #23 0x03ff91c2bb18 in thread_start () at /lib64/libc.so.6 -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH] hw/intc/s390_flic: Fix crash that occurs when saving the machine state
On Fri, May 17, 2024 at 08:15 AM +0200, Thomas Huth wrote:
> adapter_info_so_needed() treats its "opaque" parameter as a S390FLICState,
> but the function belongs to a VMStateDescription that is attached to a
> TYPE_VIRTIO_CCW_BUS device. This is currently causing a crash when the
> user tries to save or migrate the VM state. Fix it by using s390_get_flic()
> to get the correct device here instead.
>
> Reported-by: Marc Hartmayer
> Fixes: 9d1b0f5bf5 ("s390_flic: add migration-enabled property")
> Signed-off-by: Thomas Huth
> ---
> hw/intc/s390_flic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index 7f93080087..6771645699 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -459,7 +459,7 @@ type_init(qemu_s390_flic_register_types)
>
> static bool adapter_info_so_needed(void *opaque)
> {
> -S390FLICState *fs = S390_FLIC_COMMON(opaque);
> +S390FLICState *fs = s390_get_flic();
>
> return fs->migration_enabled;
> }
> --
> 2.45.0
>
Tested-by: Marc Hartmayer
Thanks.
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH] s390x/ipl: check kernel command line size
Thomas Huth writes:
> On 06/10/2021 11.26, Marc Hartmayer wrote:
>> Check if the provided kernel command line exceeds the maximum size of the
>> s390x
>> Linux kernel command line size, which is 896 bytes.
>>
>> Reported-by: Sven Schnelle
>> Signed-off-by: Marc Hartmayer
>> ---
>> hw/s390x/ipl.c | 12 +++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 1821c6faeef3..a58ea58cc736 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -38,6 +38,7 @@
>> #define KERN_IMAGE_START0x01UL
>> #define LINUX_MAGIC_ADDR0x010008UL
>> #define KERN_PARM_AREA 0x010480UL
>> +#define KERN_PARM_AREA_SIZE 0x000380UL
>> #define INITRD_START0x80UL
>> #define INITRD_PARM_START 0x010408UL
>> #define PARMFILE_START 0x001000UL
>> @@ -190,10 +191,19 @@ static void s390_ipl_realize(DeviceState *dev, Error
>> **errp)
>>* loader) and it won't work. For this case we force it to
>> 0x1, too.
>>*/
>> if (pentry == KERN_IMAGE_START || pentry == 0x800) {
>> -char *parm_area = rom_ptr(KERN_PARM_AREA, strlen(ipl->cmdline)
>> + 1);
>> +size_t cmdline_size = strlen(ipl->cmdline) + 1;
>> +char *parm_area = rom_ptr(KERN_PARM_AREA, cmdline_size);
>> +
>> ipl->start_addr = KERN_IMAGE_START;
>> /* Overwrite parameters in the kernel image, which are "rom" */
>> if (parm_area) {
>> +if (cmdline_size > KERN_PARM_AREA_SIZE) {
>> +error_setg(errp,
>> + "kernel command line exceeds maximum size:
>> %lu > %lu",
>
> I think the first %lu should be %zd instead?
Yep, makes sense - thanks!
>
> Apart from that, the patch looks fine to me... so if you agree, I can fix
> that up when picking up the patch.
Thanks.
>
> Thomas
>
>
>> + cmdline_size, KERN_PARM_AREA_SIZE);
>> +return;
>> +}
>> +
>> strcpy(parm_area, ipl->cmdline);
>> }
>> } else {
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH v6 02/10] dump: Write ELF section headers right after ELF header
Janosch Frank writes: > Let's start bundling the writes of the headers and of the data so we > have a clear ordering between them. Since the ELF header uses offsets > to the headers we can freely order them. > > Signed-off-by: Janosch Frank > --- > dump/dump.c | 31 ++- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index e7a3b54ebe..b168a25321 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -583,6 +583,8 @@ static void dump_begin(DumpState *s, Error **errp) > * -- > * | elf header | > * -- > + * | sctn_hdr | > + * -- While you’re at it, I would suggest to add the location for the program headers (phdr) as well. This would it make easier to understand the memory layout & the code below as well. I guess it looks like: … --- | sctn_hdr | --- | prog_hdr | --- … […snip]
Re: [PATCH v6 02/10] dump: Write ELF section headers right after ELF header
Janosch Frank writes: > On 10/17/22 14:49, Marc Hartmayer wrote: >> Janosch Frank writes: >> >>> Let's start bundling the writes of the headers and of the data so we >>> have a clear ordering between them. Since the ELF header uses offsets >>> to the headers we can freely order them. >>> >>> Signed-off-by: Janosch Frank >>> --- >>> dump/dump.c | 31 ++- >>> 1 file changed, 14 insertions(+), 17 deletions(-) >>> >>> diff --git a/dump/dump.c b/dump/dump.c >>> index e7a3b54ebe..b168a25321 100644 >>> --- a/dump/dump.c >>> +++ b/dump/dump.c >>> @@ -583,6 +583,8 @@ static void dump_begin(DumpState *s, Error **errp) >>>* -- >>>* | elf header | >>>* -- >>> + * | sctn_hdr | >>> + * -- >> >> While you’re at it, I would suggest to add the location for the program >> headers (phdr) as well. This would it make easier to understand the >> memory layout & the code below as well. >> >> I guess it looks like: >> >> … >> --- >> | sctn_hdr | >> --- >> | prog_hdr | >> --- >> … >> >> >> […snip] >> > > > They are already in there, have a look at the PT_* entries. I've left > them like this because I assumed that the original author wanted to make > a point by having them like this. Makes sense - I mistakenly assumed that these were the actual segment contents. […snip]
Re: [PATCH v2 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S
Thomas Huth writes:
> Providing the space of a stack frame is the duty of the caller,
> so we should reserve 160 bytes before jumping into the main function.
> Otherwise the main() function might write past the stack array.
>
> While we're at it, add a proper STACK_SIZE macro for the stack size
> instead of using magic numbers (this is also required for the following
> patch).
>
> Reviewed-by: Christian Borntraeger
> Reviewed-by: Cédric Le Goater
> Signed-off-by: Thomas Huth
> ---
> pc-bios/s390-ccw/start.S | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index d29de09cc6..29b0a9ece0 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -10,10 +10,12 @@
> * directory.
> */
>
> +#define STACK_SIZE 0x8000
> +
> .globl _start
> _start:
>
> -larl%r15,stack + 0x8000 /* Set up stack */
> +larl%r15,stack + STACK_SIZE - 160 /* Set up stack */
^^^
You can also add a macro for this
- e.g. STACK_FRAME_SIZE.
Besides that,
Reviewed-by: Marc Hartmayer
>
> /* clear bss */
> larl%r2,__bss_start
> --
> 2.39.3
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH v4 2/5] s390x: switch pv and subsystem reset ordering on reboot
On Wed, Aug 23, 2023 at 04:22 PM +0200, Steffen Eiden
wrote:
> From: Janosch Frank
>
> Bound APQNs have to be reset before tearing down the secure config via
> s390_machine_unprotect(). Otherwise the Ultravisor will return a error
> code.
>
> So let's switch the ordering around to make that happen.
>
> Reviewed-by: Christian Borntraeger
> Signed-off-by: Janosch Frank
> ---
> hw/s390x/s390-virtio-ccw.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4b36c9970e..795dd53d68 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -442,13 +442,13 @@ static void s390_machine_reset(MachineState *machine,
> ShutdownCause reason)
> switch (reset_type) {
> case S390_RESET_EXTERNAL:
> case S390_RESET_REIPL:
> +qemu_devices_reset(reason);
> +s390_crypto_reset();
> +
> if (s390_is_pv()) {
> s390_machine_unprotect(ms);
> }
>
> -qemu_devices_reset(reason);
> -s390_crypto_reset();
> -
> /* configure and start the ipl CPU only */
> run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
> break;
> --
> 2.41.0
>
Unfortunately, this breaks things for me. You can reproduce the problem
easily… Start an SE guest via direct kernel boot and reboot the guest
after the guest has booted.
e.g.
$ sudo qemu-system-s390x -smp 2 --machine accel=kvm,aes-key-wrap=off -kernel
/var/lib/libvirt/images/hades/vmlinux-s390x.se.img -m 2048 -nographic
-nodefaults -chardev stdio,id=c1,mux=on -device sclpconsole,chardev=c1 -append
"earlyprintk"
…
# reboot # (console in the guest)
…
[3.966496] systemd-shutdown[1]: Syncing filesystems and block devices.
[3.966606] systemd-shutdown[1]: Rebooting.
qemu-system-s390x: CPU reset failed on CPU 0 type aec4
qemu-system-s390x: CPU reset failed on CPU 1 type aec4
qemu-system-s390x: KVM PV command 4 (KVM_PV_VERIFY) failed: header rc 102 rrc
1b IOCTL rc: -22
Protected boot has failed: 0xa02
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[Qemu-devel] [Bug 1788582] [NEW] Race condition during shutdown
Public bug reported:
I ran into a bug when I started several VMs in parallel using
libvirt. The VMs are using only a kernel and a initrd (which includes a
minimal OS). The guest OS itself does a 'poweroff -f' as soon as the
login prompt shows up. So the expectaction is that the VMs will start,
the shutdown will be initiated, and the QEMU processes will then
end. But instead some of the QEMU processes get stuck in ppoll().
A bisect showed that the first bad commit was
0f12264e7a41458179ad10276a7c33c72024861a ("block: Allow graph changes in
bdrv_drain_all_begin/end sections").
I've already tried the current master
(13b7b188501d419a7d63c016e00065bcc693b7d4)
since the problem might be related
to the commit a1405acddeb0af6625dd9c30e8277b08e0885bd3 ("aio: Do
aio_notify_accept only during blocking aio_poll"). But the bug is still
there. I’ve reproduced the bug on x86_64 and on s390x.
The backtrace of a hanging QEMU process:
(gdb) bt
#0 0x7f5d0e251b36 in ppoll () from target:/lib64/libc.so.6
#1 0x560191052014 in qemu_poll_ns (fds=0x560193b23d60, nfds=5,
timeout=55774838936000) at /home/user/git/qemu/util/qemu-timer.c:334
#2 0x5601910531fa in os_host_main_loop_wait (timeout=55774838936000) at
/home/user/git/qemu/util/main-loop.c:233
#3 0x560191053119 in main_loop_wait (nonblocking=0) at
/home/user/git/qemu/util/main-loop.c:497
#4 0x560190baf454 in main_loop () at /home/user/git/qemu/vl.c:1866
#5 0x560190baa552 in main (argc=71, argv=0x7ffde10e41c8,
envp=0x7ffde10e4408) at /home/user/git/qemu/vl.c:4644
The used domain definition is:
test
716800
2
8
hvm
/var/lib/libvirt/images/vmlinuz-4.14.13-200.fc26.x86_64
/var/lib/libvirt/images/test-image-qemux86_64+modules-4.14.13-200.fc26.x86_64.cpio.gz
console=hvc0 STARTUP=shutdown.sh
destroy
restart
preserve
/usr/local/qemu/master/bin/qemu-system-x86_64
** Affects: qemu
Importance: Undecided
Status: New
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1788582
Title:
Race condition during shutdown
Status in QEMU:
New
Bug description:
I ran into a bug when I started several VMs in parallel using
libvirt. The VMs are using only a kernel and a initrd (which includes a
minimal OS). The guest OS itself does a 'poweroff -f' as soon as the
login prompt shows up. So the expectaction is that the VMs will start,
the shutdown will be initiated, and the QEMU processes will then
end. But instead some of the QEMU processes get stuck in ppoll().
A bisect showed that the first bad commit was
0f12264e7a41458179ad10276a7c33c72024861a ("block: Allow graph changes in
bdrv_drain_all_begin/end sections").
I've already tried the current master
(13b7b188501d419a7d63c016e00065bcc693b7d4)
since the problem might be related
to the commit a1405acddeb0af6625dd9c30e8277b08e0885bd3 ("aio: Do
aio_notify_accept only during blocking aio_poll"). But the bug is still
there. I’ve reproduced the bug on x86_64 and on s390x.
The backtrace of a hanging QEMU process:
(gdb) bt
#0 0x7f5d0e251b36 in ppoll () from target:/lib64/libc.so.6
#1 0x560191052014 in qemu_poll_ns (fds=0x560193b23d60, nfds=5,
timeout=55774838936000) at /home/user/git/qemu/util/qemu-timer.c:334
#2 0x5601910531fa in os_host_main_loop_wait (timeout=55774838936000) at
/home/user/git/qemu/util/main-loop.c:233
#3 0x560191053119 in main_loop_wait (nonblocking=0) at
/home/user/git/qemu/util/main-loop.c:497
#4 0x560190baf454 in main_loop () at /home/user/git/qemu/vl.c:1866
#5 0x560190baa552 in main (argc=71, argv=0x7ffde10e41c8,
envp=0x7ffde10e4408) at /home/user/git/qemu/vl.c:4644
The used domain definition is:
test
716800
2
8
hvm
/var/lib/libvirt/images/vmlinuz-4.14.13-200.fc26.x86_64
/var/lib/libvirt/images/test-image-qemux86_64+modules-4.14.13-200.fc26.x86_64.cpio.gz
console=hvc0 STARTUP=shutdown.sh
destroy
restart
preserve
/usr/local/qemu/master/bin/qemu-system-x86_64
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1788582/+subscriptions
Re: [Qemu-devel] [libvirt] Question about the host-model CPU mode
On Thu, Oct 12, 2017 at 02:07 PM +0200, Jiri Denemark
wrote:
> On Mon, Oct 09, 2017 at 10:16:48 +0200, Marc Hartmayer wrote:
>> On Thu, Oct 05, 2017 at 02:11 PM +0200, Jiri Denemark
>> wrote:
>> > But it's going to be a bit complicated because we ask QEMU what the
>> > host CPU is and the interface we used would need to be enhanced to
>> > give us different results for all supported machine types so that we
>> > can select the right one when a domain is started.
>>
>> So how do we deal with this?
>
> I think we need to start discussing this on qemu-devel list. If I
> remember correctly, David Hildenbrand is the original author of the
> query-cpu-model-expansion QMP command.
>
> Please, Cc me so that the thread is more visible to me.
>
> Thanks,
>
> Jirka
>
Hi all,
we recently encountered the problem that the 'host-model' [1] has to be
related to the machine type of a domain. We have following problem:
Let's assume we've a z13 system with a QEMU 2.9 and we define a
domain using the default s390-virtio-ccw machine together with the
host-model CPU mode [1]. The definition will have the machine
expanded to s390-virtio-ccw-2.9 but retain the host-model CPU mode
in the domain definition. In a next step we upgrade to QEMU 2.10
(first version to recognize z14). Everything is still fine, even
though the machine runs in 2.9 compatibility mode. Finally we
upgrade to a z14. As a consequence it is not possible to start the
domain anymore as the machine type doesn't support our CPU host
model (which is expanded at start time of the domain).
For determining the actual host-model the QMP command
'query-cpu-model-expansion' is used. This is done once per QEMU binary
and the result of it is cached by libvirt. The problem with that is
that libvirt queries with the newest machine type of the QEMU binary
for the host CPU model. But now we realized that we have to consider
the machine type of each domain for the determination of the host CPU
model of a domain.
We could now either probe the host CPU model for each QEMU binary +
machine type combination and for this we've to start a new QEMU
process each time. Or we can add a QMP command/parameter that allows
us to determine the host CPU model(s) with respect to the passed
machine type and/or all supported machine types.
The QMP command query-cpu-model-expansion is currently described in
qapi-schema.json as follows:
##
# @query-cpu-model-expansion:
#
# Expands a given CPU model (or a combination of CPU model + additional options)
# to different granularities, allowing tooling to get an understanding what a
# specific CPU model looks like in QEMU under a certain configuration.
#
# This interface can be used to query the "host" CPU model.
#
# The data returned by this command may be affected by:
#
# * QEMU version: CPU models may look different depending on the QEMU version.
# (Except for CPU models reported as "static" in query-cpu-definitions.)
# * machine-type: CPU model may look different depending on the machine-type.
# (Except for CPU models reported as "static" in query-cpu-definitions.)
# * machine options (including accelerator): in some architectures, CPU models
# may look different depending on machine and accelerator options. (Except for
# CPU models reported as "static" in query-cpu-definitions.)
# * "-cpu" arguments and global properties: arguments to the -cpu option and
# global properties may affect expansion of CPU models. Using
# query-cpu-model-expansion while using these is not advised.
#
# Some architectures may not support all expansion types. s390x supports
# "full" and "static".
#
# Returns: a CpuModelExpansionInfo. Returns an error if expanding CPU models is
# not supported, if the model cannot be expanded, if the model contains
# an unknown CPU definition name, unknown properties or properties
# with a wrong type. Also returns an error if an expansion type is
# not supported.
#
# Since: 2.8.0
##
{ 'command': 'query-cpu-model-expansion',
'data': { 'type': 'CpuModelExpansionType',
'model': 'CpuModelInfo' },
'returns': 'CpuModelExpansionInfo' }
4:46:25 PM
◄
qapi-schema.json
What do you think?
Thank you.
Marc
[1] https://libvirt.org/formatdomain.html#elementsCPU
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines
On Wed, Oct 25, 2017 at 05:50 PM +0200, David Hildenbrand wrote: > On 25.10.2017 17:09, Boris Fiuczynski wrote: >> On 10/25/2017 12:23 PM, David Hildenbrand wrote: >>> On 25.10.2017 12:18, Christian Borntraeger wrote: >>>> Ping, I plan to submit belows patch for 2.11. We can then still look into >>>> a libvirt<->qemu interface for limiting host-model depending on machine >>>> versions >>>> (or not). >>> >>> I think this would be sufficient for now. >>> >>> Having different host models, depending on the the machine type sounds >>> wrong. But maybe we'll need it in the future. >>> >> >> David, I disagree if your proposal is to generally tolerate new cpu >> features in old machine types. This *might* work for gs but how do you >> guaranty that guests do not behave differently/wrong when suddenly new >> cpu features are made available to guest when (re-)starting them. >> That is my feedback for the qemu side of this mater. > > Just re-reading this section, so what you mean is: > > a) guest is started, host model is copied and used. guest works. > b) guest is stopped. > c) QEMU/KVM/HW is updated. > d) guest is started, new host model is copied. guest no longer works. > > d) could be because there are now some additional features with e.g. > broken guest implementation or now missing features. > > > What you propose (if I am not wrong) is a to bind features somehow to a > QEMU machine. I think that should never be done. You could not catch now > missing features. What exactly do you mean by the last sentence? > > What would you think about a persistent host-model copy option? So > instead of copying at every start, only copy and replace it once in the XML? > > Easy to specify by the user and no CPU feature changes will happend > "blindly". > > > -- > > Thanks, > > David > -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 2/2] libvhost-user: handle endianness as mandated by the spec
On Mon, Aug 03, 2020 at 11:26 AM +0200, Cornelia Huck wrote: > On Thu, 30 Jul 2020 16:07:31 +0200 > Marc Hartmayer wrote: > >> Since virtio existed even before it got standardized, the virtio >> standard defines the following types of virtio devices: >> >> + legacy device (pre-virtio 1.0) >> + non-legacy or VIRTIO 1.0 device >> + transitional device (which can act both as legacy and non-legacy) >> >> Virtio 1.0 defines the fields of the virtqueues as little endian, >> while legacy uses guest's native endian [1]. Currently libvhost-user >> does not handle virtio endianness at all, i.e. it works only if the >> native endianness matches with whatever is actually needed. That means >> things break spectacularly on big-endian targets. Let us handle virtio >> endianness for non-legacy as required by the virtio specification >> [1]. > > Maybe add > > "and fence legacy virtio, as there is no safe way to figure out the > needed endianness conversions for all cases." Okay. > >> The fencing of legacy virtio devices is done in >> `vu_set_features_exec`. > > Not that I disagree with fencing legacy virtio, but looking at some > vhost-user* drivers, I'm not sure everything will work as desired for > those (I might be missing something, though.) > > - vhost-user-blk lists VERSION_1 in the supported features, but > vhost-user-scsi doesn't... is there some inheritance going on that > I'm missing? > - vhost-user-gpu-pci inherits from virtio-gpu-pci, so I guess it's fine > - vhost-user-input should also always have been virtio-1 > > So, has anybody been using vhost-user-scsi and can confirm that it > still works, or at least can be made to work? Unfortunately, I don’t have the required hardware :/ Can please anybody verify this? > >> >> [1] >> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003 >> >> Signed-off-by: Marc Hartmayer >> --- >> contrib/libvhost-user/libvhost-user.c | 77 +++ >> 1 file changed, 43 insertions(+), 34 deletions(-) > > The code change per se LGTM. Thanks for the feedback! > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[RFC 0/4] Enable virtio-fs on s390x
This RFC is about enabling virtio-fs on s390x. For that we need + some shim code (first patch), and we need + libvhost-user to deal with virtio endiannes as mandated by the spec. The second part is trickier, because unlike QEMU we are not certain about the guest's native endianness, which is needed to handle the legacy-interface appropriately. In fact, this is the reason why just RFC. One of the open questions is whether to build separate versions, one for guest little endian and one for guest big endian, or do we want something like a command line option? (Digression on the libvirt modeling) A third option would be to refuse legacy altogether. libvhost-access.h is based on hw/virtio/virtio-access.h. How to use? For general instructions how to use virtio-fs (on x86) please have a look at https://virtio-fs.gitlab.io/howto-qemu.html. Most of the instructions can also be applied on s390x. In short: 1. Install self-compiled QEMU with this patch series applied 2. Prepare host and guest kernel so they support virtio-fs Start virtiofsd on the host $ virtiofsd -f --socket-path=/tmp/vhostqemu -o source=/tmp/shared Now you can start QEMU in a separate shell on the host: $ qemu-system-s390x -machine type=s390-ccw-virtio,accel=kvm,memory-backend=mem \ -object memory-backend-file,id=mem,size=2G,mem-path=/dev/shm/virtiofs,share=on,prealloc=on,prealloc-threads=1 \ -chardev socket,id=char0,path=/tmp/vhostqemu -device vhost-user-fs-ccw,queue-size=1024,chardev=char0,tag=myfs \ -drive if=virtio,file=disk.qcow2 \ -m 2G -smp 2 -nographic Log into the guest and mount it $ mount -t virtiofs myfs /mnt Halil Pasic (1): virtio: add vhost-user-fs-ccw device Marc Hartmayer (3): libvhost-user: print invalid address on vu_panic libvhost-user: handle endianness as mandated by the spec HACK: Hard-code the libvhost-user.o-cflags for s390x Makefile.objs | 1 + contrib/libvhost-user/libvhost-access.h | 87 + contrib/libvhost-user/libvhost-user.c | 124 hw/s390x/Makefile.objs | 1 + hw/s390x/vhost-user-fs-ccw.c| 74 ++ 5 files changed, 227 insertions(+), 60 deletions(-) create mode 100644 contrib/libvhost-user/libvhost-access.h create mode 100644 hw/s390x/vhost-user-fs-ccw.c -- 2.25.4
[RFC 4/4] HACK: Hard-code the libvhost-user.o-cflags for s390x
This patch exists only to show the actual problem that libvhost-user
and it's users are architecture dependent as soon as we're trying to
support legacy virtio.
Signed-off-by: Marc Hartmayer
---
Makefile.objs | 1 +
contrib/libvhost-user/libvhost-access.h | 7 +++
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index 7ce2588b89a3..abfb1912e456 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -111,6 +111,7 @@ elf2dmp-obj-y = contrib/elf2dmp/
ivshmem-client-obj-$(CONFIG_IVSHMEM) = contrib/ivshmem-client/
ivshmem-server-obj-$(CONFIG_IVSHMEM) = contrib/ivshmem-server/
libvhost-user-obj-y = contrib/libvhost-user/
+libvhost-user.o-cflags += -iquote $(SRC_PATH)/s390x-softmmu -DNEED_CPU_H
vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS)
vhost-user-scsi.o-libs := $(LIBISCSI_LIBS)
vhost-user-scsi-obj-y = contrib/vhost-user-scsi/
diff --git a/contrib/libvhost-user/libvhost-access.h
b/contrib/libvhost-user/libvhost-access.h
index a4fc334fe134..e9451ae0fbc6 100644
--- a/contrib/libvhost-user/libvhost-access.h
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -5,10 +5,9 @@
#include "libvhost-user.h"
-/* TODO attempt to use poisoned TARGET_PPC64/ARM */
-/* #if defined(TARGET_PPC64) || defined(TARGET_ARM) */
-/* #define LEGACY_VIRTIO_IS_BIENDIAN 1 */
-/* #endif */
+#if defined(TARGET_PPC64) || defined(TARGET_ARM)
+#define LEGACY_VIRTIO_IS_BIENDIAN 1
+#endif
static inline bool vu_is_big_endian(VuDev *dev)
{
--
2.25.4
[RFC 1/4] virtio: add vhost-user-fs-ccw device
From: Halil Pasic
Wire up the CCW device for vhost-user-fs.
Signed-off-by: Halil Pasic
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/vhost-user-fs-ccw.c | 74
2 files changed, 75 insertions(+)
create mode 100644 hw/s390x/vhost-user-fs-ccw.c
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index a46a1c7894e0..c4086ec3171e 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -20,6 +20,7 @@ obj-$(CONFIG_VIRTIO_NET) += virtio-ccw-net.o
obj-$(CONFIG_VIRTIO_BLK) += virtio-ccw-blk.o
obj-$(call land,$(CONFIG_VIRTIO_9P),$(CONFIG_VIRTFS)) += virtio-ccw-9p.o
obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock-ccw.o
+obj-$(CONFIG_VHOST_USER_FS) += vhost-user-fs-ccw.o
endif
obj-y += css-bridge.o
obj-y += ccw-device.o
diff --git a/hw/s390x/vhost-user-fs-ccw.c b/hw/s390x/vhost-user-fs-ccw.c
new file mode 100644
index ..0f11a77239a5
--- /dev/null
+++ b/hw/s390x/vhost-user-fs-ccw.c
@@ -0,0 +1,74 @@
+/*
+ * Ccw transport wiring for vhost-user-fs
+ *
+ * Copyright 2020 IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "hw/virtio/vhost-user-fs.h"
+#include "virtio-ccw.h"
+
+typedef struct VHostUserFSCcw {
+VirtioCcwDevice parent_obj;
+VHostUserFS vdev;
+} VHostUserFSCcw;
+
+#define TYPE_VHOST_USER_FS_CCW "vhost-user-fs-ccw"
+#define VHOST_USER_FS_CCW(obj) \
+OBJECT_CHECK(VHostUserFSCcw, (obj), TYPE_VHOST_USER_FS_CCW)
+
+
+static Property vhost_user_fs_ccw_properties[] = {
+DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
+VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+ VIRTIO_CCW_MAX_REV),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_fs_ccw_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(ccw_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
+object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void vhost_user_fs_ccw_instance_init(Object *obj)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VHOST_USER_FS);
+}
+
+static void vhost_user_fs_ccw_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_CLASS(klass);
+
+k->realize = vhost_user_fs_ccw_realize;
+device_class_set_props(dc,vhost_user_fs_ccw_properties);
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static const TypeInfo vhost_user_fs_ccw = {
+.name = TYPE_VHOST_USER_FS_CCW,
+.parent= TYPE_VIRTIO_CCW_DEVICE,
+.instance_size = sizeof(VHostUserFSCcw),
+.instance_init = vhost_user_fs_ccw_instance_init,
+.class_init= vhost_user_fs_ccw_class_init,
+};
+
+static void vhost_user_fs_ccw_register(void)
+{
+type_register_static(&vhost_user_fs_ccw);
+}
+
+type_init(vhost_user_fs_ccw_register)
--
2.25.4
[RFC 3/4] libvhost-user: handle endianness as mandated by the spec
Since virtio existed even before it got standardized, the virtio
standard defines the following types of virtio devices:
+ legacy device (pre-virtio 1.0)
+ non-legacy or VIRTIO 1.0 device
+ transitional device (which can act both as legacy and non-legacy)
Virtio 1.0 defines the fields of the virtqueues as little endian,
while legacy uses guest's native endian [1]. Currently libvhost-user
does not handle virtio endianness at all, i.e. it works only if the
native endianness matches with whatever is actually needed. That means
things break spectacularly on big-endian targets. Let us handle
virtio endianness as required by the virtio specification [1].
[1]
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003
Signed-off-by: Marc Hartmayer
---
contrib/libvhost-user/libvhost-access.h | 88 +
contrib/libvhost-user/libvhost-user.c | 122
2 files changed, 151 insertions(+), 59 deletions(-)
create mode 100644 contrib/libvhost-user/libvhost-access.h
diff --git a/contrib/libvhost-user/libvhost-access.h
b/contrib/libvhost-user/libvhost-access.h
new file mode 100644
index ..a4fc334fe134
--- /dev/null
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -0,0 +1,88 @@
+#ifndef LIBVHOST_ACCESS_H
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+
+#include "libvhost-user.h"
+
+/* TODO attempt to use poisoned TARGET_PPC64/ARM */
+/* #if defined(TARGET_PPC64) || defined(TARGET_ARM) */
+/* #define LEGACY_VIRTIO_IS_BIENDIAN 1 */
+/* #endif */
+
+static inline bool vu_is_big_endian(VuDev *dev)
+{
+if (!vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+/* TODO there is no `device_endian` attribute for VuDev */
+/* assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN); */
+/* return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG; */
+}
+
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return false;
+}
+
+static inline bool vu_access_is_big_endian(VuDev *dev)
+{
+#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
+return vu_is_big_endian(dev);
+#elif defined(TARGET_WORDS_BIGENDIAN)
+if (vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return false;
+}
+return true;
+#else
+return false;
+#endif
+}
+
+static inline void vu_stw_p(VuDev *vdev, void *ptr, uint16_t v)
+{
+if (vu_access_is_big_endian(vdev)) {
+stw_be_p(ptr, v);
+} else {
+stw_le_p(ptr, v);
+}
+}
+
+static inline void vu_stl_p(VuDev *vdev, void *ptr, uint32_t v)
+{
+if (vu_access_is_big_endian(vdev)) {
+stl_be_p(ptr, v);
+} else {
+stl_le_p(ptr, v);
+}
+}
+
+static inline void vu_stq_p(VuDev *vdev, void *ptr, uint64_t v)
+{
+if (vu_access_is_big_endian(vdev)) {
+stq_be_p(ptr, v);
+} else {
+stq_le_p(ptr, v);
+}
+}
+
+static inline int vu_lduw_p(VuDev *vdev, const void *ptr)
+{
+if (vu_access_is_big_endian(vdev))
+return lduw_be_p(ptr);
+return lduw_le_p(ptr);
+}
+
+static inline int vu_ldl_p(VuDev *vdev, const void *ptr)
+{
+if (vu_access_is_big_endian(vdev))
+return ldl_be_p(ptr);
+return ldl_le_p(ptr);
+}
+
+static inline uint64_t vu_ldq_p(VuDev *vdev, const void *ptr)
+{
+if (vu_access_is_big_endian(vdev))
+return ldq_be_p(ptr);
+return ldq_le_p(ptr);
+}
+
+#endif /* LIBVHOST_ACCESS_h */
diff --git a/contrib/libvhost-user/libvhost-user.c
b/contrib/libvhost-user/libvhost-user.c
index 9e8750a9dabc..200a90206ed2 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -45,6 +45,10 @@
#include "qemu/memfd.h"
#include "libvhost-user.h"
+static inline bool has_feature(uint64_t features, unsigned int fbit);
+static inline bool vu_has_feature(VuDev *dev, unsigned int fbit);
+
+#include "libvhost-access.h"
/* usually provided by GLib */
#ifndef MIN
@@ -1074,7 +1078,7 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
return false;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = vu_lduw_p(dev, &vq->vring.used->idx);
if (vq->last_avail_idx != vq->used_idx) {
bool resume = dev->iface->queue_is_processed_in_order &&
@@ -1191,7 +1195,7 @@ vu_check_queue_inflights(VuDev *dev, VuVirtq *vq)
return 0;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = vu_lduw_p(dev, &vq->vring.used->idx);
vq->resubmit_num = 0;
vq->resubmit_list = NULL;
vq->counter = 0;
@@ -2019,35 +2023,35 @@ vu_queue_started(const VuDev *dev, const VuVirtq *vq)
}
static inline uint16_t
-vring_avail_flags(VuVirtq *vq)
+vring_avail_flags(VuDev *dev, VuVirtq *vq)
{
-return vq->vring.avail->flags;
+return vu_lduw_p(dev, &vq->vring.avail->
[RFC v2 0/3] Enable virtio-fs on s390x
This RFC is about enabling virtio-fs on s390x. For that we need + some shim code (first patch), and we need + libvhost-user to deal with virtio endiannes for non-legacy virtio devices as mandated by the spec. libvhost-access.h is based on hw/virtio/virtio-access.h. How to use? For general instructions how to use virtio-fs (on x86) please have a look at https://virtio-fs.gitlab.io/howto-qemu.html. Most of the instructions can also be applied on s390x. In short: 1. Install self-compiled QEMU with this patch series applied 2. Prepare host and guest kernel so they support virtio-fs Start virtiofsd on the host $ virtiofsd -f --socket-path=/tmp/vhostqemu -o source=/tmp/shared Now you can start QEMU in a separate shell on the host: $ qemu-system-s390x -machine type=s390-ccw-virtio,accel=kvm,memory-backend=mem \ -object memory-backend-file,id=mem,size=2G,mem-path=/dev/shm/virtiofs,share=on,prealloc=on,prealloc-threads=1 \ -chardev socket,id=char0,path=/tmp/vhostqemu -device vhost-user-fs-ccw,queue-size=1024,chardev=char0,tag=myfs \ -drive if=virtio,file=disk.qcow2 \ -m 2G -smp 2 -nographic Log into the guest and mount it $ mount -t virtiofs myfs /mnt Changelog: v1->v2: + rebased + drop patch "libvhost-user: print invalid address on vu_panic" as it's not related to this series + drop patch "[RFC 4/4] HACK: Hard-code the libvhost-user.o-cflags for s390x" + patch "virtio: add vhost-user-fs-ccw device": replace qdev_set_parent_bus and object_property_set_bool by qdev_realize + patch "libvhost-user: handle endianness as mandated by the spec": Drop support for legacy virtio devices + add patch to fence legacy virtio devices Halil Pasic (1): virtio: add vhost-user-fs-ccw device Marc Hartmayer (2): libvhost-user: handle endianness as mandated by the spec libvhost-user: fence legacy virtio devices contrib/libvhost-user/libvhost-access.h | 71 ++ contrib/libvhost-user/libvhost-user.c | 125 +--- hw/s390x/Makefile.objs | 1 + hw/s390x/vhost-user-fs-ccw.c| 73 ++ 4 files changed, 211 insertions(+), 59 deletions(-) create mode 100644 contrib/libvhost-user/libvhost-access.h create mode 100644 hw/s390x/vhost-user-fs-ccw.c -- 2.25.4
[RFC v2 3/3] libvhost-user: fence legacy virtio devices
libvhost-user has no support for legacy virtio devices therefore
let's fence them.
Signed-off-by: Marc Hartmayer
---
contrib/libvhost-user/libvhost-access.h | 10 ++
contrib/libvhost-user/libvhost-user.c | 6 ++
2 files changed, 16 insertions(+)
diff --git a/contrib/libvhost-user/libvhost-access.h
b/contrib/libvhost-user/libvhost-access.h
index 868ba3e7bfb8..aa505ea1ec02 100644
--- a/contrib/libvhost-user/libvhost-access.h
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -1,11 +1,21 @@
#ifndef LIBVHOST_ACCESS_H
+#include
+
#include "qemu/bswap.h"
#include "libvhost-user.h"
+static inline bool vu_has_feature(VuDev *dev, unsigned int fbit);
+
static inline bool vu_access_is_big_endian(VuDev *dev)
{
+/*
+ * TODO: can probably be removed as the fencing is already done in
+ * `vu_set_features_exec`
+ */
+assert(vu_has_feature(dev, VIRTIO_F_VERSION_1));
+
/* Devices conforming to VIRTIO 1.0 or later are always LE. */
return false;
}
diff --git a/contrib/libvhost-user/libvhost-user.c
b/contrib/libvhost-user/libvhost-user.c
index 0214b04c5291..93c4503b1f53 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -540,6 +540,12 @@ vu_set_features_exec(VuDev *dev, VhostUserMsg *vmsg)
DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64);
dev->features = vmsg->payload.u64;
+if (!vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+/* We only support devices conforming to VIRTIO 1.0 or
+ * later */
+vu_panic(dev, "virtio legacy devices aren't supported by
libvhost-user");
+return false;
+}
if (!(dev->features & VHOST_USER_F_PROTOCOL_FEATURES)) {
vu_set_enable_all_rings(dev, true);
--
2.25.4
[RFC v2 1/3] virtio: add vhost-user-fs-ccw device
From: Halil Pasic
Wire up the CCW device for vhost-user-fs.
Signed-off-by: Halil Pasic
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/vhost-user-fs-ccw.c | 73
2 files changed, 74 insertions(+)
create mode 100644 hw/s390x/vhost-user-fs-ccw.c
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index a46a1c7894e0..c4086ec3171e 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -20,6 +20,7 @@ obj-$(CONFIG_VIRTIO_NET) += virtio-ccw-net.o
obj-$(CONFIG_VIRTIO_BLK) += virtio-ccw-blk.o
obj-$(call land,$(CONFIG_VIRTIO_9P),$(CONFIG_VIRTFS)) += virtio-ccw-9p.o
obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock-ccw.o
+obj-$(CONFIG_VHOST_USER_FS) += vhost-user-fs-ccw.o
endif
obj-y += css-bridge.o
obj-y += ccw-device.o
diff --git a/hw/s390x/vhost-user-fs-ccw.c b/hw/s390x/vhost-user-fs-ccw.c
new file mode 100644
index ..88a7a11a34b4
--- /dev/null
+++ b/hw/s390x/vhost-user-fs-ccw.c
@@ -0,0 +1,73 @@
+/*
+ * Ccw transport wiring for vhost-user-fs
+ *
+ * Copyright 2020 IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "hw/virtio/vhost-user-fs.h"
+#include "virtio-ccw.h"
+
+typedef struct VHostUserFSCcw {
+VirtioCcwDevice parent_obj;
+VHostUserFS vdev;
+} VHostUserFSCcw;
+
+#define TYPE_VHOST_USER_FS_CCW "vhost-user-fs-ccw"
+#define VHOST_USER_FS_CCW(obj) \
+OBJECT_CHECK(VHostUserFSCcw, (obj), TYPE_VHOST_USER_FS_CCW)
+
+
+static Property vhost_user_fs_ccw_properties[] = {
+DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
+VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+ VIRTIO_CCW_MAX_REV),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_fs_ccw_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(ccw_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+qdev_realize(vdev, BUS(&ccw_dev->bus), errp);
+}
+
+static void vhost_user_fs_ccw_instance_init(Object *obj)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VHOST_USER_FS);
+}
+
+static void vhost_user_fs_ccw_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_CLASS(klass);
+
+k->realize = vhost_user_fs_ccw_realize;
+device_class_set_props(dc,vhost_user_fs_ccw_properties);
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static const TypeInfo vhost_user_fs_ccw = {
+.name = TYPE_VHOST_USER_FS_CCW,
+.parent= TYPE_VIRTIO_CCW_DEVICE,
+.instance_size = sizeof(VHostUserFSCcw),
+.instance_init = vhost_user_fs_ccw_instance_init,
+.class_init= vhost_user_fs_ccw_class_init,
+};
+
+static void vhost_user_fs_ccw_register(void)
+{
+type_register_static(&vhost_user_fs_ccw);
+}
+
+type_init(vhost_user_fs_ccw_register)
--
2.25.4
[RFC v2 2/3] libvhost-user: handle endianness as mandated by the spec
Since virtio existed even before it got standardized, the virtio
standard defines the following types of virtio devices:
+ legacy device (pre-virtio 1.0)
+ non-legacy or VIRTIO 1.0 device
+ transitional device (which can act both as legacy and non-legacy)
Virtio 1.0 defines the fields of the virtqueues as little endian,
while legacy uses guest's native endian [1]. Currently libvhost-user
does not handle virtio endianness at all, i.e. it works only if the
native endianness matches with whatever is actually needed. That means
things break spectacularly on big-endian targets. Let us handle virtio
endianness for non-legacy as required by the virtio specification
[1]. We will fence non-legacy virtio devices with the upcoming patch.
[1]
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003
Signed-off-by: Marc Hartmayer
---
Note: As we don't support legacy virtio devices there is dead code in
libvhost-access.h that could be removed. But for the sake of
completeness, I left it in the code.
---
contrib/libvhost-user/libvhost-access.h | 61
contrib/libvhost-user/libvhost-user.c | 119
2 files changed, 121 insertions(+), 59 deletions(-)
create mode 100644 contrib/libvhost-user/libvhost-access.h
diff --git a/contrib/libvhost-user/libvhost-access.h
b/contrib/libvhost-user/libvhost-access.h
new file mode 100644
index ..868ba3e7bfb8
--- /dev/null
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -0,0 +1,61 @@
+#ifndef LIBVHOST_ACCESS_H
+
+#include "qemu/bswap.h"
+
+#include "libvhost-user.h"
+
+static inline bool vu_access_is_big_endian(VuDev *dev)
+{
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return false;
+}
+
+static inline void vu_stw_p(VuDev *vdev, void *ptr, uint16_t v)
+{
+if (vu_access_is_big_endian(vdev)) {
+stw_be_p(ptr, v);
+} else {
+stw_le_p(ptr, v);
+}
+}
+
+static inline void vu_stl_p(VuDev *vdev, void *ptr, uint32_t v)
+{
+if (vu_access_is_big_endian(vdev)) {
+stl_be_p(ptr, v);
+} else {
+stl_le_p(ptr, v);
+}
+}
+
+static inline void vu_stq_p(VuDev *vdev, void *ptr, uint64_t v)
+{
+if (vu_access_is_big_endian(vdev)) {
+stq_be_p(ptr, v);
+} else {
+stq_le_p(ptr, v);
+}
+}
+
+static inline int vu_lduw_p(VuDev *vdev, const void *ptr)
+{
+if (vu_access_is_big_endian(vdev))
+return lduw_be_p(ptr);
+return lduw_le_p(ptr);
+}
+
+static inline int vu_ldl_p(VuDev *vdev, const void *ptr)
+{
+if (vu_access_is_big_endian(vdev))
+return ldl_be_p(ptr);
+return ldl_le_p(ptr);
+}
+
+static inline uint64_t vu_ldq_p(VuDev *vdev, const void *ptr)
+{
+if (vu_access_is_big_endian(vdev))
+return ldq_be_p(ptr);
+return ldq_le_p(ptr);
+}
+
+#endif /* LIBVHOST_ACCESS_H */
diff --git a/contrib/libvhost-user/libvhost-user.c
b/contrib/libvhost-user/libvhost-user.c
index d315db139606..0214b04c5291 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -45,6 +45,7 @@
#include "qemu/memfd.h"
#include "libvhost-user.h"
+#include "libvhost-access.h"
/* usually provided by GLib */
#ifndef MIN
@@ -1074,7 +1075,7 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
return false;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = vu_lduw_p(dev, &vq->vring.used->idx);
if (vq->last_avail_idx != vq->used_idx) {
bool resume = dev->iface->queue_is_processed_in_order &&
@@ -1191,7 +1192,7 @@ vu_check_queue_inflights(VuDev *dev, VuVirtq *vq)
return 0;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = vu_lduw_p(dev, &vq->vring.used->idx);
vq->resubmit_num = 0;
vq->resubmit_list = NULL;
vq->counter = 0;
@@ -2019,35 +2020,35 @@ vu_queue_started(const VuDev *dev, const VuVirtq *vq)
}
static inline uint16_t
-vring_avail_flags(VuVirtq *vq)
+vring_avail_flags(VuDev *dev, VuVirtq *vq)
{
-return vq->vring.avail->flags;
+return vu_lduw_p(dev, &vq->vring.avail->flags);
}
static inline uint16_t
-vring_avail_idx(VuVirtq *vq)
+vring_avail_idx(VuDev *dev, VuVirtq *vq)
{
-vq->shadow_avail_idx = vq->vring.avail->idx;
+vq->shadow_avail_idx = vu_lduw_p(dev, &vq->vring.avail->idx);
return vq->shadow_avail_idx;
}
static inline uint16_t
-vring_avail_ring(VuVirtq *vq, int i)
+vring_avail_ring(VuDev *dev, VuVirtq *vq, int i)
{
-return vq->vring.avail->ring[i];
+return vu_lduw_p(dev, &vq->vring.avail->ring[i]);
}
static inline uint16_t
-vring_get_used_event(VuVirtq *vq)
+vring_get_used_event(VuDev *dev, VuVirtq *vq)
{
-return vring_avail_ring(vq, vq->vring.num);
+return vring_avail_ring(dev, vq, vq->vring.num);
}
static i
Re: [RFC v2 2/3] libvhost-user: handle endianness as mandated by the spec
On Tue, Jul 21, 2020 at 06:44 PM +0200, Halil Pasic wrote: > On Tue, 21 Jul 2020 09:40:10 -0400 > "Michael S. Tsirkin" wrote: > >> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote: >> > Since virtio existed even before it got standardized, the virtio >> > standard defines the following types of virtio devices: >> > >> > + legacy device (pre-virtio 1.0) >> > + non-legacy or VIRTIO 1.0 device >> > + transitional device (which can act both as legacy and non-legacy) >> > >> > Virtio 1.0 defines the fields of the virtqueues as little endian, >> > while legacy uses guest's native endian [1]. Currently libvhost-user >> > does not handle virtio endianness at all, i.e. it works only if the >> > native endianness matches with whatever is actually needed. That means >> > things break spectacularly on big-endian targets. Let us handle virtio >> > endianness for non-legacy as required by the virtio specification >> > [1]. We will fence non-legacy virtio devices with the upcoming patch. >> > >> > [1] >> > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003 >> > >> > Signed-off-by: Marc Hartmayer >> > >> > --- >> > Note: As we don't support legacy virtio devices >> >> Who's "we" in this sentence? vhost user supports legacy generally ... > > In that sentence "we" is the library "libvhost-user". I would like > to avoid s390x being an oddball regarding this. Marc's previous > version made an attempt at correct endianness handling for legacy > and non-legacy. That spawned a discussion on how we don't want > legacy devices in this context. This series makes what seemed to be the > consensus reached in that discussion explicit: namely that libvhost-user > does not support legacy-virtio. Hi Michael, Polite ping - what’s your opinion? Thanks in advance. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [RFC v2 2/3] libvhost-user: handle endianness as mandated by the spec
On Wed, Jul 29, 2020 at 10:13 AM -0400, "Michael S. Tsirkin" wrote: > On Tue, Jul 28, 2020 at 12:52:11PM +0200, Marc Hartmayer wrote: >> On Tue, Jul 21, 2020 at 06:44 PM +0200, Halil Pasic >> wrote: >> > On Tue, 21 Jul 2020 09:40:10 -0400 >> > "Michael S. Tsirkin" wrote: >> > >> >> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote: >> >> > Since virtio existed even before it got standardized, the virtio >> >> > standard defines the following types of virtio devices: >> >> > >> >> > + legacy device (pre-virtio 1.0) >> >> > + non-legacy or VIRTIO 1.0 device >> >> > + transitional device (which can act both as legacy and non-legacy) >> >> > >> >> > Virtio 1.0 defines the fields of the virtqueues as little endian, >> >> > while legacy uses guest's native endian [1]. Currently libvhost-user >> >> > does not handle virtio endianness at all, i.e. it works only if the >> >> > native endianness matches with whatever is actually needed. That means >> >> > things break spectacularly on big-endian targets. Let us handle virtio >> >> > endianness for non-legacy as required by the virtio specification >> >> > [1]. We will fence non-legacy virtio devices with the upcoming patch. >> >> > >> >> > [1] >> >> > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003 >> >> > >> >> > Signed-off-by: Marc Hartmayer >> >> > >> >> > --- >> >> > Note: As we don't support legacy virtio devices >> >> >> >> Who's "we" in this sentence? vhost user supports legacy generally ... >> > >> > In that sentence "we" is the library "libvhost-user". I would like >> > to avoid s390x being an oddball regarding this. Marc's previous >> > version made an attempt at correct endianness handling for legacy >> > and non-legacy. That spawned a discussion on how we don't want >> > legacy devices in this context. This series makes what seemed to be the >> > consensus reached in that discussion explicit: namely that libvhost-user >> > does not support legacy-virtio. >> >> Hi Michael, >> >> Polite ping - what’s your opinion? Thanks in advance. >> >> […snip] > > It's a reasonable limitation, but I also don't see anything > that verifies that device is modern only. > E.g. fail setting features if VIRTIO_F_VERSION_1 is not there? That’s implemented in patch 3 to emphasize how the fencing is done. For the next series I’ll merge patch 2 and 3. > > >> -- >> Kind regards / Beste Grüße >>Marc Hartmayer >> >> IBM Deutschland Research & Development GmbH >> Vorsitzender des Aufsichtsrats: Gregor Pillen >> Geschäftsführung: Dirk Wittkopp >> Sitz der Gesellschaft: Böblingen >> Registergericht: Amtsgericht Stuttgart, HRB 243294 > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [RFC v2 2/3] libvhost-user: handle endianness as mandated by the spec
On Tue, Jul 21, 2020 at 01:48 PM +0100, Stefan Hajnoczi wrote: > On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote: >> Since virtio existed even before it got standardized, the virtio >> standard defines the following types of virtio devices: >> >> + legacy device (pre-virtio 1.0) >> + non-legacy or VIRTIO 1.0 device >> + transitional device (which can act both as legacy and non-legacy) >> >> Virtio 1.0 defines the fields of the virtqueues as little endian, >> while legacy uses guest's native endian [1]. Currently libvhost-user >> does not handle virtio endianness at all, i.e. it works only if the >> native endianness matches with whatever is actually needed. That means >> things break spectacularly on big-endian targets. Let us handle virtio >> endianness for non-legacy as required by the virtio specification >> [1]. We will fence non-legacy virtio devices with the upcoming patch. >> >> [1] >> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003 >> >> Signed-off-by: Marc Hartmayer >> >> --- >> Note: As we don't support legacy virtio devices there is dead code in >> libvhost-access.h that could be removed. But for the sake of >> completeness, I left it in the code. > > Please remove the dead code. It is unlikely that legacy device support > will be required in the future and it will just confuse people reading > the code. Done. > >> --- >> contrib/libvhost-user/libvhost-access.h | 61 >> contrib/libvhost-user/libvhost-user.c | 119 >> 2 files changed, 121 insertions(+), 59 deletions(-) >> create mode 100644 contrib/libvhost-user/libvhost-access.h >> >> diff --git a/contrib/libvhost-user/libvhost-access.h >> b/contrib/libvhost-user/libvhost-access.h >> new file mode 100644 >> index ..868ba3e7bfb8 >> --- /dev/null >> +++ b/contrib/libvhost-user/libvhost-access.h >> @@ -0,0 +1,61 @@ >> +#ifndef LIBVHOST_ACCESS_H > > License/copyright header? With the dead code removed there is no reason for having libvhost-access.h so I’ve removed it. -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH 2/2] libvhost-user: handle endianness as mandated by the spec
Since virtio existed even before it got standardized, the virtio
standard defines the following types of virtio devices:
+ legacy device (pre-virtio 1.0)
+ non-legacy or VIRTIO 1.0 device
+ transitional device (which can act both as legacy and non-legacy)
Virtio 1.0 defines the fields of the virtqueues as little endian,
while legacy uses guest's native endian [1]. Currently libvhost-user
does not handle virtio endianness at all, i.e. it works only if the
native endianness matches with whatever is actually needed. That means
things break spectacularly on big-endian targets. Let us handle virtio
endianness for non-legacy as required by the virtio specification
[1]. The fencing of legacy virtio devices is done in
`vu_set_features_exec`.
[1]
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003
Signed-off-by: Marc Hartmayer
---
contrib/libvhost-user/libvhost-user.c | 77 +++
1 file changed, 43 insertions(+), 34 deletions(-)
diff --git a/contrib/libvhost-user/libvhost-user.c
b/contrib/libvhost-user/libvhost-user.c
index 53f16bdf082c..e2238a0400c9 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -42,6 +42,7 @@
#include "qemu/atomic.h"
#include "qemu/osdep.h"
+#include "qemu/bswap.h"
#include "qemu/memfd.h"
#include "libvhost-user.h"
@@ -539,6 +540,14 @@ vu_set_features_exec(VuDev *dev, VhostUserMsg *vmsg)
DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64);
dev->features = vmsg->payload.u64;
+if (!vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+/*
+ * We only support devices conforming to VIRTIO 1.0 or
+ * later
+ */
+vu_panic(dev, "virtio legacy devices aren't supported by
libvhost-user");
+return false;
+}
if (!(dev->features & VHOST_USER_F_PROTOCOL_FEATURES)) {
vu_set_enable_all_rings(dev, true);
@@ -1074,7 +1083,7 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
return false;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = lduw_le_p(&vq->vring.used->idx);
if (vq->last_avail_idx != vq->used_idx) {
bool resume = dev->iface->queue_is_processed_in_order &&
@@ -1191,7 +1200,7 @@ vu_check_queue_inflights(VuDev *dev, VuVirtq *vq)
return 0;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = lduw_le_p(&vq->vring.used->idx);
vq->resubmit_num = 0;
vq->resubmit_list = NULL;
vq->counter = 0;
@@ -2021,13 +2030,13 @@ vu_queue_started(const VuDev *dev, const VuVirtq *vq)
static inline uint16_t
vring_avail_flags(VuVirtq *vq)
{
-return vq->vring.avail->flags;
+return lduw_le_p(&vq->vring.avail->flags);
}
static inline uint16_t
vring_avail_idx(VuVirtq *vq)
{
-vq->shadow_avail_idx = vq->vring.avail->idx;
+vq->shadow_avail_idx = lduw_le_p(&vq->vring.avail->idx);
return vq->shadow_avail_idx;
}
@@ -2035,7 +2044,7 @@ vring_avail_idx(VuVirtq *vq)
static inline uint16_t
vring_avail_ring(VuVirtq *vq, int i)
{
-return vq->vring.avail->ring[i];
+return lduw_le_p(&vq->vring.avail->ring[i]);
}
static inline uint16_t
@@ -2123,12 +2132,12 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc
*desc,
int i, unsigned int max, unsigned int *next)
{
/* If this descriptor says it doesn't chain, we're done. */
-if (!(desc[i].flags & VRING_DESC_F_NEXT)) {
+if (!(lduw_le_p(&desc[i].flags) & VRING_DESC_F_NEXT)) {
return VIRTQUEUE_READ_DESC_DONE;
}
/* Check they're not leading us off end of descriptors. */
-*next = desc[i].next;
+*next = lduw_le_p(&desc[i].next);
/* Make sure compiler knows to grab that: we don't want it changing! */
smp_wmb();
@@ -2171,8 +2180,8 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq,
unsigned int *in_bytes,
}
desc = vq->vring.desc;
-if (desc[i].flags & VRING_DESC_F_INDIRECT) {
-if (desc[i].len % sizeof(struct vring_desc)) {
+if (lduw_le_p(&desc[i].flags) & VRING_DESC_F_INDIRECT) {
+if (ldl_le_p(&desc[i].len) % sizeof(struct vring_desc)) {
vu_panic(dev, "Invalid size for indirect buffer table");
goto err;
}
@@ -2185,8 +2194,8 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq,
unsigned int *in_bytes,
/* loop over the indirect descriptor table */
indirect = 1;
-desc_addr = desc[i].addr;
-desc_len = desc[i].len;
+desc_addr = ldq_le_p(&desc[i].addr);
+desc_len = ldl_le_p(&desc[i].len);
max
[PATCH 0/2] Enable virtio-fs on s390x
This patch series is about enabling virtio-fs on s390x. For that we need + some shim code (first patch), and we need + libvhost-user to deal with virtio endiannes for non-legacy virtio devices as mandated by the spec. How to use? For general instructions how to use virtio-fs (on x86) please have a look at https://virtio-fs.gitlab.io/howto-qemu.html. Most of the instructions can also be applied on s390x. In short: 1. Install self-compiled QEMU with this patch series applied 2. Prepare host and guest kernel so they support virtio-fs Start virtiofsd on the host $ virtiofsd -f --socket-path=/tmp/vhostqemu -o source=/tmp/shared Now you can start QEMU in a separate shell on the host: $ qemu-system-s390x -machine type=s390-ccw-virtio,accel=kvm,memory-backend=mem \ -object memory-backend-file,id=mem,size=2G,mem-path=/dev/shm/virtiofs,share=on,prealloc=on,prealloc-threads=1 \ -chardev socket,id=char0,path=/tmp/vhostqemu -device vhost-user-fs-ccw,queue-size=1024,chardev=char0,tag=myfs \ -drive if=virtio,file=disk.qcow2 \ -m 2G -smp 2 -nographic Log into the guest and mount it $ mount -t virtiofs myfs /mnt Changelog: RFC v2 -> v1: - patch 1: + Added `force_revision_1 = true` (Conny) + I didn't add the r-b from Stefan Hajnoczi as I've added the changes suggested by Conny - squashed patches 2 and 3: + removed assertion in performance critical code path (Stefan) + dropped all dead code (Stefan) + removed libvhost-access.h RFC v1 -> RFC v2: + rebased + drop patch "libvhost-user: print invalid address on vu_panic" as it's not related to this series + drop patch "[RFC 4/4] HACK: Hard-code the libvhost-user.o-cflags for s390x" + patch "virtio: add vhost-user-fs-ccw device": replace qdev_set_parent_bus and object_property_set_bool by qdev_realize + patch "libvhost-user: handle endianness as mandated by the spec": Drop support for legacy virtio devices + add patch to fence legacy virtio devices *** BLURB HERE *** Halil Pasic (1): virtio: add vhost-user-fs-ccw device Marc Hartmayer (1): libvhost-user: handle endianness as mandated by the spec contrib/libvhost-user/libvhost-user.c | 77 +++ hw/s390x/Makefile.objs| 1 + hw/s390x/vhost-user-fs-ccw.c | 75 ++ 3 files changed, 119 insertions(+), 34 deletions(-) create mode 100644 hw/s390x/vhost-user-fs-ccw.c -- 2.25.4
[PATCH 1/2] virtio: add vhost-user-fs-ccw device
From: Halil Pasic
Wire up the CCW device for vhost-user-fs.
Signed-off-by: Halil Pasic
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/vhost-user-fs-ccw.c | 75
2 files changed, 76 insertions(+)
create mode 100644 hw/s390x/vhost-user-fs-ccw.c
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index a46a1c7894e0..c4086ec3171e 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -20,6 +20,7 @@ obj-$(CONFIG_VIRTIO_NET) += virtio-ccw-net.o
obj-$(CONFIG_VIRTIO_BLK) += virtio-ccw-blk.o
obj-$(call land,$(CONFIG_VIRTIO_9P),$(CONFIG_VIRTFS)) += virtio-ccw-9p.o
obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock-ccw.o
+obj-$(CONFIG_VHOST_USER_FS) += vhost-user-fs-ccw.o
endif
obj-y += css-bridge.o
obj-y += ccw-device.o
diff --git a/hw/s390x/vhost-user-fs-ccw.c b/hw/s390x/vhost-user-fs-ccw.c
new file mode 100644
index ..6c6f26929301
--- /dev/null
+++ b/hw/s390x/vhost-user-fs-ccw.c
@@ -0,0 +1,75 @@
+/*
+ * virtio ccw vhost-user-fs implementation
+ *
+ * Copyright 2020 IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "hw/virtio/vhost-user-fs.h"
+#include "virtio-ccw.h"
+
+typedef struct VHostUserFSCcw {
+VirtioCcwDevice parent_obj;
+VHostUserFS vdev;
+} VHostUserFSCcw;
+
+#define TYPE_VHOST_USER_FS_CCW "vhost-user-fs-ccw"
+#define VHOST_USER_FS_CCW(obj) \
+OBJECT_CHECK(VHostUserFSCcw, (obj), TYPE_VHOST_USER_FS_CCW)
+
+
+static Property vhost_user_fs_ccw_properties[] = {
+DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
+VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+ VIRTIO_CCW_MAX_REV),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_fs_ccw_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(ccw_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+qdev_realize(vdev, BUS(&ccw_dev->bus), errp);
+}
+
+static void vhost_user_fs_ccw_instance_init(Object *obj)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);
+VirtioCcwDevice *ccw_dev = VIRTIO_CCW_DEVICE(obj);
+
+ccw_dev->force_revision_1 = true;
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VHOST_USER_FS);
+}
+
+static void vhost_user_fs_ccw_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_CLASS(klass);
+
+k->realize = vhost_user_fs_ccw_realize;
+device_class_set_props(dc, vhost_user_fs_ccw_properties);
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static const TypeInfo vhost_user_fs_ccw = {
+.name = TYPE_VHOST_USER_FS_CCW,
+.parent= TYPE_VIRTIO_CCW_DEVICE,
+.instance_size = sizeof(VHostUserFSCcw),
+.instance_init = vhost_user_fs_ccw_instance_init,
+.class_init= vhost_user_fs_ccw_class_init,
+};
+
+static void vhost_user_fs_ccw_register(void)
+{
+type_register_static(&vhost_user_fs_ccw);
+}
+
+type_init(vhost_user_fs_ccw_register)
--
2.25.4
Re: [PATCH 2/2] libvhost-user: handle endianness as mandated by the spec
On Sun, Aug 02, 2020 at 01:13 AM -0400, "Michael S. Tsirkin" wrote: > On Thu, Jul 30, 2020 at 04:07:31PM +0200, Marc Hartmayer wrote: >> Since virtio existed even before it got standardized, the virtio >> standard defines the following types of virtio devices: >> >> + legacy device (pre-virtio 1.0) >> + non-legacy or VIRTIO 1.0 device >> + transitional device (which can act both as legacy and non-legacy) >> >> Virtio 1.0 defines the fields of the virtqueues as little endian, >> while legacy uses guest's native endian [1]. Currently libvhost-user >> does not handle virtio endianness at all, i.e. it works only if the >> native endianness matches with whatever is actually needed. That means >> things break spectacularly on big-endian targets. Let us handle virtio >> endianness for non-legacy as required by the virtio specification >> [1]. The fencing of legacy virtio devices is done in >> `vu_set_features_exec`. >> >> [1] >> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003 >> >> Signed-off-by: Marc Hartmayer > > > Reviewed-by: Michael S. Tsirkin Thanks. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[RFC 2/4] libvhost-user: print invalid address on vu_panic
This can be helpful for debugging.
Signed-off-by: Marc Hartmayer
---
contrib/libvhost-user/libvhost-user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/libvhost-user/libvhost-user.c
b/contrib/libvhost-user/libvhost-user.c
index d315db139606..9e8750a9dabc 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -2432,7 +2432,7 @@ virtqueue_map_desc(VuDev *dev,
iov[num_sg].iov_base = vu_gpa_to_va(dev, &len, pa);
if (iov[num_sg].iov_base == NULL) {
-vu_panic(dev, "virtio: invalid address for buffers");
+vu_panic(dev, "virtio: invalid address 0x%lx for buffers", pa);
return;
}
iov[num_sg].iov_len = len;
--
2.25.4
[PATCH] pc-bios/s390-ccw: don't try to read the next block if end of chunk is reached
Don't read the block if a null block number is reached, because this means that
the end of chunk is reached.
Reviewed-by: Collin Walling
Signed-off-by: Marc Hartmayer
---
pc-bios/s390-ccw/bootmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 44df7d16afca..b46997c0b7c1 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -213,7 +213,7 @@ static int eckd_get_boot_menu_index(block_number_t
s1b_block_nr)
next_block_nr = eckd_block_num(&s1b->seek[i + 1].chs);
}
-if (next_block_nr) {
+if (next_block_nr && !is_null_block_number(next_block_nr)) {
read_block(next_block_nr, s2_next_blk,
"Cannot read stage2 boot loader");
}
--
2.25.1
Re: [PATCH v2 2/2] s390x: pv: Fix diag318 PV fencing
On Thu, Oct 22, 2020 at 06:31 AM -0400, Janosch Frank
wrote:
> Diag318 fencing needs to be determined on the current VM PV state and
> not on the state that the VM has when we create the CPU model.
>
> Signed-off-by: Janosch Frank
> Reported-by: Marc Hartmayer
> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
> ---
> target/s390x/cpu_features.c | 5 +
> target/s390x/cpu_features.h | 4
> target/s390x/cpu_models.c | 4
> target/s390x/kvm.c | 3 +--
> 4 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 31ea8df246..42fe0bf4ca 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -14,6 +14,7 @@
> #include "qemu/osdep.h"
> #include "qemu/module.h"
> #include "cpu_features.h"
> +#include "hw/s390x/pv.h"
>
> #define DEF_FEAT(_FEAT, _NAME, _TYPE, _BIT, _DESC) \
> [S390_FEAT_##_FEAT] = {\
> @@ -105,6 +106,10 @@ void s390_fill_feat_block(const S390FeatBitmap features,
> S390FeatType type,
> }
> feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
> }
> +
> +if (type == S390_FEAT_TYPE_SCLP_FAC134 && s390_is_pv()) {
> +clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
> +}
> }
>
> void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index ef52ffce83..87463f064d 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -81,6 +81,10 @@ const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup
> group);
>
> #define BE_BIT_NR(BIT) (BIT ^ (BITS_PER_LONG - 1))
>
> +static inline void clear_be_bit(unsigned int bit_nr, uint8_t *array)
> +{
> +array[bit_nr / 8] &= ~(0x80 >> (bit_nr % 8));
> +}
> static inline void set_be_bit(unsigned int bit_nr, uint8_t *array)
> {
> array[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index ca484bfda7..461e0b8f4a 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -29,6 +29,7 @@
> #include "hw/pci/pci.h"
> #endif
> #include "qapi/qapi-commands-machine-target.h"
> +#include "hw/s390x/pv.h"
>
> #define CPUDEF_INIT(_type, _gen, _ec_ga, _mha_pow, _hmfai, _name, _desc) \
> {\
> @@ -238,6 +239,9 @@ bool s390_has_feat(S390Feat feat)
> }
> return 0;
> }
> +if (feat == S390_FEAT_DIAG_318 && s390_is_pv()) {
> +return false;
> +}
> return test_bit(feat, cpu->model->features);
> }
>
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index f13eff688c..baa070fdf7 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2498,8 +2498,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model,
> Error **errp)
> */
> set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>
> -/* DIAGNOSE 0x318 is not supported under protected virtualization */
> -if (!s390_is_pv() && kvm_check_extension(kvm_state,
> KVM_CAP_S390_DIAG318)) {
> +if (kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
> set_bit(S390_FEAT_DIAG_318, model->features);
> }
>
> --
> 2.25.1
Tested-by: Marc Hartmayer
[Qemu-devel] [Bug 1788582] Re: Race condition during shutdown
It was fixed with commit 4cf077b59fc73eec29f8b7 (see patch series
https://lists.gnu.org/archive/html/qemu-block/2018-09/msg00504.html).
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1788582
Title:
Race condition during shutdown
Status in QEMU:
New
Bug description:
I ran into a bug when I started several VMs in parallel using
libvirt. The VMs are using only a kernel and a initrd (which includes a
minimal OS). The guest OS itself does a 'poweroff -f' as soon as the
login prompt shows up. So the expectaction is that the VMs will start,
the shutdown will be initiated, and the QEMU processes will then
end. But instead some of the QEMU processes get stuck in ppoll().
A bisect showed that the first bad commit was
0f12264e7a41458179ad10276a7c33c72024861a ("block: Allow graph changes in
bdrv_drain_all_begin/end sections").
I've already tried the current master
(13b7b188501d419a7d63c016e00065bcc693b7d4)
since the problem might be related
to the commit a1405acddeb0af6625dd9c30e8277b08e0885bd3 ("aio: Do
aio_notify_accept only during blocking aio_poll"). But the bug is still
there. I’ve reproduced the bug on x86_64 and on s390x.
The backtrace of a hanging QEMU process:
(gdb) bt
#0 0x7f5d0e251b36 in ppoll () from target:/lib64/libc.so.6
#1 0x560191052014 in qemu_poll_ns (fds=0x560193b23d60, nfds=5,
timeout=55774838936000) at /home/user/git/qemu/util/qemu-timer.c:334
#2 0x5601910531fa in os_host_main_loop_wait (timeout=55774838936000) at
/home/user/git/qemu/util/main-loop.c:233
#3 0x560191053119 in main_loop_wait (nonblocking=0) at
/home/user/git/qemu/util/main-loop.c:497
#4 0x560190baf454 in main_loop () at /home/user/git/qemu/vl.c:1866
#5 0x560190baa552 in main (argc=71, argv=0x7ffde10e41c8,
envp=0x7ffde10e4408) at /home/user/git/qemu/vl.c:4644
The used domain definition is:
test
716800
2
8
hvm
/var/lib/libvirt/images/vmlinuz-4.14.13-200.fc26.x86_64
/var/lib/libvirt/images/test-image-qemux86_64+modules-4.14.13-200.fc26.x86_64.cpio.gz
console=hvc0 STARTUP=shutdown.sh
destroy
restart
preserve
/usr/local/qemu/master/bin/qemu-system-x86_64
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1788582/+subscriptions
[PATCH 1/3] pc-bios/s390-ccw: fix off-by-one error
This error takes effect when the magic value "zIPL" is located at the
end of a block. For example if s2_cur_blk = 0x7fe18000 and the magic
value "zIPL" is located at 0x7fe18ffc - 0x7fe18fff.
Fixes: ba831b25262a ("s390-ccw: read stage2 boot loader data to find menu")
Reviewed-by: Collin Walling
Signed-off-by: Marc Hartmayer
---
pc-bios/s390-ccw/bootmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 97205674e59a..0d29dceaa3cc 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -163,7 +163,7 @@ static bool find_zipl_boot_menu_banner(int *offset)
int i;
/* Menu banner starts with "zIPL" */
-for (i = 0; i < virtio_get_block_size() - 4; i++) {
+for (i = 0; i < virtio_get_block_size() - 3; i++) {
if (magic_match(s2_cur_blk + i, ZIPL_MAGIC_EBCDIC)) {
*offset = i;
return true;
--
2.25.4
[PATCH 2/3] pc-bios/s390-ccw: break loop if a null block number is reached
Break the loop if `cur_block_nr` is a null block number because this
means that the end of chunk is reached. In this case we will try to
boot the default entry.
Fixes: ba831b25262a ("s390-ccw: read stage2 boot loader data to find menu")
Reviewed-by: Collin Walling
Signed-off-by: Marc Hartmayer
---
pc-bios/s390-ccw/bootmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 0d29dceaa3cc..08f16c5595a3 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -192,7 +192,7 @@ static int eckd_get_boot_menu_index(block_number_t
s1b_block_nr)
for (i = 0; i < STAGE2_BLK_CNT_MAX; i++) {
cur_block_nr = eckd_block_num(&s1b->seek[i].chs);
-if (!cur_block_nr) {
+if (!cur_block_nr || is_null_block_number(cur_block_nr)) {
break;
}
--
2.25.4
[PATCH 0/3] pc-bios: s390x: fix corner cases in booting from ECKD
The first patch fixes the scan for the string "zIPL" (which is used to detect the start of the boot menu banner) in `find_zipl_boot_menu_banner`. The second patch fixes the bug of not booting the default entry if no zipl boot menu data was found. For everybody's convenience there is a branch: https://gitlab.com/mhartmay/qemu/-/tree/bios_fixes Marc Hartmayer (3): pc-bios/s390-ccw: fix off-by-one error pc-bios/s390-ccw: break loop if a null block number is reached pc-bios/s390-ccw: Update s390-ccw.img bios binary pc-bios/s390-ccw.img | Bin 42608 -> 42608 bytes pc-bios/s390-ccw/bootmap.c | 4 ++-- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.25.4
[PATCH 3/3] pc-bios/s390-ccw: Update s390-ccw.img bios binary
Contains the fixes "pc-bios/s390-ccw: fix off-by-one error" and
"pc-bios/s390-ccw: break loop if a null block number is reached".
Signed-off-by: Marc Hartmayer
---
pc-bios/s390-ccw.img | Bin 42608 -> 42608 bytes
1 file changed, 0 insertions(+), 0 deletions(-)
diff --git a/pc-bios/s390-ccw.img b/pc-bios/s390-ccw.img
index
3074686a8c7448d943aa62f810d88d3e92865ef3..d166774ece9e2a3cd1d89e02acfffb68b370a363
100644
GIT binary patch
literal 42608
zcmeHwd3;pW+5ed=5JG^Pl?fo+fe2wqm?Q`R7iO}s$Tn;>h!#RJkVr^kGC^=7YP5*9
z4bimvw$>4e7qyikY73$zeWk7K+a*#Fx{3}e8kY%RMSJZr?N;t$2F&NMN@Qsl5
zD3hOVsij30n<;t=5st6XFHBa8?kOP1kC_r)^o=yidr2NFkQHXC&?EIDT=DtID>8ki
zTN>`*^6=^wSl#@BCJ~~Qn;H4rV+)Sw-=+Vzft~W69xU2
zps@k3H~K8uazHZ`mdRzMYbKW!Po0upU6G!_wKPTozTvYME*I-(#^3$cn_D)GJ@e@g
z1LI2_DLDt_5K?}`8|EwiSO$oC)u^QM$c2-iJ-YRc8&}`@(Dw%2`s2U+-Kgs|fkGH`
zhaU@R)Gt;rKT&k|X<5#E9*puBevUBMD%&M>maDS42&+TJ5-bm|HOj>Zabsy^nQ*Tu
z$}Frd5@i)P6;)KZ1aL
zLQLQu`7QNAEPtI^`vtNQXx;Cwu<$6PmVI}MxP3yzYQuHCEmq7FF)qayL;d2N2e$ZP
zG>;hHvKswg(5xanVW`zMM_DcIRfVN#sQy_}oC8nVcwMI+VUyxUqx}NyW?*o;2-iIN
zyF|t1KW98F^w;vdUm+YC!;~1mU5OF(TAg@AyGrPq$1+LF78aM?Ry)V8jC9y-bx21b
zy@xT_TwqtMsNL#Ul?~`e_;kU>Z>NzZPJY#1F19noH-S~1rGJ6CTF`UKo;iw7SZz&X
zg?K5xRai5C!R>8X!Y&4B>C@{u7lSgNmTv2h^rs|`%iH|+fLEj#@`xmPIMsmHsRVvW
z(zyH`ZLWwy?UO#?w}a^VLn5%xr?eFyU)#1^Tp?n#0b%O
z?5jo7H2pKH^InIrSmTotnr7-cppPMmasTDoY28D)xcM!W7-Jt`?eN=?jnFRFr$Ar+
z01X-8blQQR19%#m_1S?@tQXnbH&dGoY1awWJlFsOxLGd44RM0v}qF5~X#KCWJGjvJ!2s-d<0v~<#1{dT`f
z7&8xt8ra*_7?A`$g&qJx3;1qhjD`cFwTuyCXc^TK5MdE9RI+>!XbT&3^}c~3b6=Rq
z0{4<32fZy9RyGDDd5}*WN(UU#6-dLW#0qH3S?VkL=RxPR)8}75f*M7%0I*ol+zgq)
zGPj8U%*qeK0{u8QnQJHb70X^&AZj`1P=5e!{ywqui|Lw9`&b-wZpc5{@f9;6#5?%KzG(($yA}kKCFRXxO`Pk+uwW4{nP`9dL1mqPC{-tT@x{h@G
zc1vInQk#`!GrFlj_3_M@md$fLg7{5k4#m#pakbj*+!Im`G>h%YL(w}$dkXsh1N{{(
z753H9?h+CE62$1Xigrar^gk^}I&86vR)P5`#CG&a)J9GxyVp+}2@B)VkD;&S!oDEzaX6jjQM)Z?|yz4;|+`8^aZ?rCkCm{&2xzyr%&3bY5J!$^7Km^MBoXcf8fg|
zR|fu)Yr?p6FW^~$;emEg(El{^b$7rO!dLVinUB6ZsQo2p{B~cM*pBD8d+j2oDSe2Z
zkO{l5`=KRb`>;&R^MrjPMEo4p7Y0np+K)j>cK7jpQCMkX#4xmtfK`|XeP#Zj+}5@`
z5Re#1zL*>txQ^vEyeD~2JI^$HD@fU5qU`eH%;7hJd?WpM4mk4aJNy^CLJu$`b0Bbr
zpvn;-msY^(^h*|KXFodQw6{k|sS>miS=8T>e>l8Vv_~N~qfxZON3g(}w9gdvR+~0K
z@5sTr*k}oOwG@lM8rkAk1G_{Da-TXY+vhlI+e=X56Dc{EuZ`hoS0@f@$pJz5XW!?q
z48SfON)P;1<&y2Up*7~Uf83~J&^B!pa7IZBv}ZNiPoNcH`+6*$Lyn@yF6l~AEc65MzG##z0iUl0F7OY0m=Qe61KMP)
z%Fr*=7eF&GUphx|`g{t?`)NOfg*dH$S4&n-IAa6qKAW&=dEy4FHA79FYxfCNyG5u$
z-XGMLFn>u79Cz7(w{{y9^-$n37i%-56GHfi>>exV(NOFp6T1F&5|l)c92WD2#sQKzw}|($mC~
z?;jyAX2zY`J~6X#EBFFxwWpz3ztBGq>FaA0-bm_mAl?g(=mEd#jS?e?PPR0_yr
z$y?$^TknK@sABGp7goaZfrp&VO2dD12-dYfn|>C&dP`!Vb`V;B1J>Hp`hWWsZ$Gi!
zT6phj5w%qjQAveN@A5f{H;ifCKyr2|ft~(V!h-p87tOv^s|%F+9ZhS6z@A3M$YSei
z5r)-y7oHvX3ShjQ=z3rttZ)NM?TWTxB1{a*RqI7T24rg$dV&pnsM4;AB7T(nUG-ri
zBKa`#|7g3K+78PsNbos^$GN8XgbTUtVfD$V-JpF>(Ej0Rk~cwZn?S7z4_6#o#sP;V
z!0WXB%eI@T26li}6N`VGPZdctKdf)C=I(b6^*Q{GdZ@>NQHrpp6ifqV9{pKs{ICSj
zz?Q$^J%iG&5%1Vj5!MwrO90w^Wb_OXP!Ch5cK&6kHx#x
z{;y*WuHSg1~PU(
zL#sVhHf26PH*x)MIR7Z+voo1vX_TKD&iO2EbF(bR>Z-C;g_V$AMd-Sk-2@BefUKW|
zZNw@_ea_xu<@qvjDB)|l0Xi}=@i8qWtd8{fyXX8qT0zn0@(b2E8Y6<{!FS(dZmppa
z!q$L?6Iv9MzSA}u(WgP=WiX@$T5)I;tuAbC!&A_s{c^?RF@DQqI9zIACyz50>ju_Q
zT4}%gJ9voG%=HHA%wDYVdofe3*kgQFfF6EjHR`nq&=ovMUqD;WYm#i6BHqTDv=?js
zUU2^e=o#y;T+J&IctCzu1M%^;IxShnYK|QZcFMGxqh>teciT;b-_QVa={Lli+sd_j
z!vyN#^J0uK#(LU7($&6$wg0q!)bI60%2+_s+2kGI@0H;1kU7{RBs@mrCCA`dZ+ZgZ
zHCy5YP;@nVz(?b{Syad6dE%-@;fuhGoGP6OHtY%YkGgsUMH?{I_wt07vE{r
zF6lgDn}{*df=l+&o}qaOR{i}#*=H3u152+-!{@(3PFhbFR{9-Up8hIvB`cEp_WL$#
z8dgl?3n)LejVS;89qJE$(Fz{bNWAmtFB<$~+kNX_d*kY@990;a4?OGiYiP{OKlAvh
z4C~vRAI-3yXIMU#B1zw484eBEDH0GniWGyO(a;vm)Bt=9o0<*H!TvMiIqG}=!%}xU
zrAiXL|9g^dNHr2#^I{yYl*M;x$+Rwrlc7ExBw>Y>g>J#x?`53~Xq$!59uANOjAh(L
zBNs=}EN!}e9{vi=8=HtnX%$w&sRXlZ8Bvto6KAH~&N~>5g
z{88E^&Lde-`^103w>=O2$dyPT8IK3HOL8O=QG
zY-ul=NOKPsbTrTWHJJI~oiqQlM%GskuQRDMYX1`AxWw-KE5w;)`X7;W)?~S
z^JOVnYEuLq0Qb_l&CAeSZkNF^zDV}}ri^2p2Qbc{5&eJ2v<%|>Bb;B@S)O$_=XoX0
zdY|*`|7E?-dG-slzz2-81v@3|&nRv>H4=8TfcEr|8p&}j$??9mgyVfh-R&fo7Qj>B
zoqXb-X--XjG^Y8JTi7(r)b034B
zEu?*(`9MD$f6k^DBtnZ67r{e2e^wZxLIcaoR|65`D%|yQ^P=F2u^+
zTf@vilQUMuekQOJu_NdVk1>tfZTk~rvH-AI&$7OY-0vGQfWc|~gN95_PcY*DtV4k^#eY`p6wT3;gE!rx>1
zZU=9kCjRtfJ+@UqqC7hj-eHL@CMkL04~R;Cy|>hmf=F&aNg=$}HAvaQZL4Dp24ZAh
zbGF^c&*20GWp{R88Mn;&9AI$j&InVxjRyF6txSVz9({r4kN#&pZo1zxrV{l$f
zxdVL6CJT`D;-RsD#}K6h*73y0I9M=!x!RflOPK9+1YY*LTA|wT6=G4FvF{#nDcg(~
zZ64x|nSy-MD3J+Y%!1qi5f7VZ&ruu}Sk5?DxwjGhJ^<_L#BNlfNH|7GK|344YET`5
z#-a5a`t}%M5vG$&?|l&Uy~_07yUke|e~vjl2mDpQ={k`vb)c)yaxeM+ESdP@+KkJX$*^XmGZ9ngUQMT294!;t!fPKoWe%yWtIMB>(
z;E(Yvhp(XP|Gufk3;sk{n~&TQ*csS~Rx^6CWLr-GGbz^_nC9Oa+E^!M^Qh}!JHh}N
zu}(qsD~xMC!+6!N?x}}H0z>gGXkyC}SFWY={$>}qv{7vsuM)9uQ
Re: [PATCH 1/3] pc-bios/s390-ccw: fix off-by-one error
On Thu, Sep 24, 2020 at 12:02 PM +0200, Philippe Mathieu-Daudé
wrote:
> Hi Marc,
>
> On 9/24/20 10:59 AM, Marc Hartmayer wrote:
>> This error takes effect when the magic value "zIPL" is located at the
>> end of a block. For example if s2_cur_blk = 0x7fe18000 and the magic
>> value "zIPL" is located at 0x7fe18ffc - 0x7fe18fff.
>>
>> Fixes: ba831b25262a ("s390-ccw: read stage2 boot loader data to find menu")
>> Reviewed-by: Collin Walling
>> Signed-off-by: Marc Hartmayer
>> ---
>> pc-bios/s390-ccw/bootmap.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 97205674e59a..0d29dceaa3cc 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -163,7 +163,7 @@ static bool find_zipl_boot_menu_banner(int *offset)
>> int i;
>>
>> /* Menu banner starts with "zIPL" */
>> -for (i = 0; i < virtio_get_block_size() - 4; i++) {
>> +for (i = 0; i < virtio_get_block_size() - 3; i++) {
>
> Easier to review as:
>
>for (i = 0; i <= virtio_get_block_size() - 4; i++) {
Yep.
>
> Even easier defining ZIPL_MAGIC_SIZE instead of the magic '4'.
I thought about adding such a macro as well. Makes even more sense with
your proposed change.
>
>> if (magic_match(s2_cur_blk + i, ZIPL_MAGIC_EBCDIC)) {
>> *offset = i;
>> return true;
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 2/3] pc-bios/s390-ccw: break loop if a null block number is reached
On Thu, Sep 24, 2020 at 01:49 PM +0200, Thomas Huth wrote:
> On 24/09/2020 10.59, Marc Hartmayer wrote:
>> Break the loop if `cur_block_nr` is a null block number because this
>> means that the end of chunk is reached. In this case we will try to
>> boot the default entry.
>>
>> Fixes: ba831b25262a ("s390-ccw: read stage2 boot loader data to find menu")
>> Reviewed-by: Collin Walling
>> Signed-off-by: Marc Hartmayer
>> ---
>> pc-bios/s390-ccw/bootmap.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 0d29dceaa3cc..08f16c5595a3 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -192,7 +192,7 @@ static int eckd_get_boot_menu_index(block_number_t
>> s1b_block_nr)
>> for (i = 0; i < STAGE2_BLK_CNT_MAX; i++) {
>> cur_block_nr = eckd_block_num(&s1b->seek[i].chs);
>>
>> -if (!cur_block_nr) {
>> +if (!cur_block_nr || is_null_block_number(cur_block_nr)) {
>> break;
>> }
>
> Reviewed-by: Thomas Huth
>
> I'll queue the two patches on my s390-ccw bios branch. Just let me know
> if you'd like to have the "< ... - 3" changed into "<= ... - 4" in the
> first patch, I can tweak it directly if you like.
Yes, please change it to <= … - 4. Thanks!
>
> Thomas
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH] meson: vhost-user-gpu/virtiofsd: use absolute path
The option `libexecdir` is relative to `prefix` (see
https://mesonbuild.com/Builtin-options.html), so we have to be aware
of this when creating 50-qemu-gpu.json and
50-qemu-virtiofsd.json. Otherwise, tools like libvirt will not be able
to find the executable.
Fixes: 16bf7a3326d8 ("configure: move directory options from config-host.mak to
meson")
Signed-off-by: Marc Hartmayer
---
contrib/vhost-user-gpu/meson.build | 2 +-
tools/virtiofsd/meson.build| 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/vhost-user-gpu/meson.build
b/contrib/vhost-user-gpu/meson.build
index 37ecca13cafb..c487ca72c1ff 100644
--- a/contrib/vhost-user-gpu/meson.build
+++ b/contrib/vhost-user-gpu/meson.build
@@ -9,6 +9,6 @@ if 'CONFIG_TOOLS' in config_host and 'CONFIG_VIRGL' in
config_host \
configure_file(input: '50-qemu-gpu.json.in',
output: '50-qemu-gpu.json',
- configuration: { 'libexecdir' : get_option('libexecdir') },
+ configuration: { 'libexecdir' : get_option('prefix') /
get_option('libexecdir') },
install_dir: qemu_datadir / 'vhost-user')
endif
diff --git a/tools/virtiofsd/meson.build b/tools/virtiofsd/meson.build
index e1a4dc98d9ec..17edecf55c0a 100644
--- a/tools/virtiofsd/meson.build
+++ b/tools/virtiofsd/meson.build
@@ -15,5 +15,5 @@ executable('virtiofsd', files(
configure_file(input: '50-qemu-virtiofsd.json.in',
output: '50-qemu-virtiofsd.json',
- configuration: { 'libexecdir' : get_option('libexecdir') },
+ configuration: { 'libexecdir' : get_option('prefix') /
get_option('libexecdir') },
install_dir: qemu_datadir / 'vhost-user')
--
2.25.4
Re: [PATCH] meson: vhost-user-gpu/virtiofsd: use absolute path
On Tue, Nov 03, 2020 at 12:23 PM +0100, Marc Hartmayer
wrote:
> The option `libexecdir` is relative to `prefix` (see
> https://mesonbuild.com/Builtin-options.html), so we have to be aware
> of this when creating 50-qemu-gpu.json and
> 50-qemu-virtiofsd.json. Otherwise, tools like libvirt will not be able
> to find the executable.
>
> Fixes: 16bf7a3326d8 ("configure: move directory options from config-host.mak
> to meson")
> Signed-off-by: Marc Hartmayer
> ---
> contrib/vhost-user-gpu/meson.build | 2 +-
> tools/virtiofsd/meson.build| 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/vhost-user-gpu/meson.build
> b/contrib/vhost-user-gpu/meson.build
> index 37ecca13cafb..c487ca72c1ff 100644
> --- a/contrib/vhost-user-gpu/meson.build
> +++ b/contrib/vhost-user-gpu/meson.build
> @@ -9,6 +9,6 @@ if 'CONFIG_TOOLS' in config_host and 'CONFIG_VIRGL' in
> config_host \
>
>configure_file(input: '50-qemu-gpu.json.in',
> output: '50-qemu-gpu.json',
> - configuration: { 'libexecdir' : get_option('libexecdir') },
> + configuration: { 'libexecdir' : get_option('prefix') /
> get_option('libexecdir') },
> install_dir: qemu_datadir / 'vhost-user')
> endif
> diff --git a/tools/virtiofsd/meson.build b/tools/virtiofsd/meson.build
> index e1a4dc98d9ec..17edecf55c0a 100644
> --- a/tools/virtiofsd/meson.build
> +++ b/tools/virtiofsd/meson.build
> @@ -15,5 +15,5 @@ executable('virtiofsd', files(
>
> configure_file(input: '50-qemu-virtiofsd.json.in',
> output: '50-qemu-virtiofsd.json',
> - configuration: { 'libexecdir' : get_option('libexecdir') },
> + configuration: { 'libexecdir' : get_option('prefix') /
> get_option('libexecdir') },
> install_dir: qemu_datadir / 'vhost-user')
> --
> 2.25.4
>
>
It’s probably not the best way to fix it, but at least a good hint
what’s wrong :)
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH] meson: vhost-user-gpu/virtiofsd: use absolute path
On Tue, Nov 03, 2020 at 12:55 PM +0100, Paolo Bonzini
wrote:
> On 03/11/20 12:28, Marc Hartmayer wrote:
>> On Tue, Nov 03, 2020 at 12:23 PM +0100, Marc Hartmayer
>> wrote:
>>> The option `libexecdir` is relative to `prefix` (see
>>> https://mesonbuild.com/Builtin-options.html), so we have to be aware
>>> of this when creating 50-qemu-gpu.json and
>>> 50-qemu-virtiofsd.json. Otherwise, tools like libvirt will not be able
>>> to find the executable.
>>>
>>> Fixes: 16bf7a3326d8 ("configure: move directory options from
>>> config-host.mak to meson")
>>> Signed-off-by: Marc Hartmayer
>>> ---
>>> contrib/vhost-user-gpu/meson.build | 2 +-
>>> tools/virtiofsd/meson.build| 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/contrib/vhost-user-gpu/meson.build
>>> b/contrib/vhost-user-gpu/meson.build
>>> index 37ecca13cafb..c487ca72c1ff 100644
>>> --- a/contrib/vhost-user-gpu/meson.build
>>> +++ b/contrib/vhost-user-gpu/meson.build
>>> @@ -9,6 +9,6 @@ if 'CONFIG_TOOLS' in config_host and 'CONFIG_VIRGL' in
>>> config_host \
>>>
>>>configure_file(input: '50-qemu-gpu.json.in',
>>> output: '50-qemu-gpu.json',
>>> - configuration: { 'libexecdir' : get_option('libexecdir')
>>> },
>>> + configuration: { 'libexecdir' : get_option('prefix') /
>>> get_option('libexecdir') },
>>> install_dir: qemu_datadir / 'vhost-user')
>>> endif
>>> diff --git a/tools/virtiofsd/meson.build b/tools/virtiofsd/meson.build
>>> index e1a4dc98d9ec..17edecf55c0a 100644
>>> --- a/tools/virtiofsd/meson.build
>>> +++ b/tools/virtiofsd/meson.build
>>> @@ -15,5 +15,5 @@ executable('virtiofsd', files(
>>>
>>> configure_file(input: '50-qemu-virtiofsd.json.in',
>>> output: '50-qemu-virtiofsd.json',
>>> - configuration: { 'libexecdir' : get_option('libexecdir') },
>>> + configuration: { 'libexecdir' : get_option('prefix') /
>>> get_option('libexecdir') },
>>> install_dir: qemu_datadir / 'vhost-user')
>>> --
>>> 2.25.4
>>>
>>>
>>
>> It’s probably not the best way to fix it, but at least a good hint
>> what’s wrong :)
>
> No, it's okay. I queued it.
Thanks :)
>
> Paolo
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH v2 2/2] libvhost-user: handle endianness as mandated by the spec
Since virtio existed even before it got standardized, the virtio
standard defines the following types of virtio devices:
+ legacy device (pre-virtio 1.0)
+ non-legacy or VIRTIO 1.0 device
+ transitional device (which can act both as legacy and non-legacy)
Virtio 1.0 defines the fields of the virtqueues as little endian,
while legacy uses guest's native endian [1]. Currently libvhost-user
does not handle virtio endianness at all, i.e. it works only if the
native endianness matches with whatever is actually needed. That means
things break spectacularly on big-endian targets. Let us handle virtio
endianness for non-legacy as required by the virtio specification [1]
and fence legacy virtio, as there is no safe way to figure out the
needed endianness conversions for all cases. The fencing of legacy
virtio devices is done in `vu_set_features_exec`.
[1]
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Marc Hartmayer
---
contrib/libvhost-user/libvhost-user.c | 77 +++
1 file changed, 43 insertions(+), 34 deletions(-)
diff --git a/contrib/libvhost-user/libvhost-user.c
b/contrib/libvhost-user/libvhost-user.c
index 53f16bdf082c..e2238a0400c9 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -42,6 +42,7 @@
#include "qemu/atomic.h"
#include "qemu/osdep.h"
+#include "qemu/bswap.h"
#include "qemu/memfd.h"
#include "libvhost-user.h"
@@ -539,6 +540,14 @@ vu_set_features_exec(VuDev *dev, VhostUserMsg *vmsg)
DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64);
dev->features = vmsg->payload.u64;
+if (!vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+/*
+ * We only support devices conforming to VIRTIO 1.0 or
+ * later
+ */
+vu_panic(dev, "virtio legacy devices aren't supported by
libvhost-user");
+return false;
+}
if (!(dev->features & VHOST_USER_F_PROTOCOL_FEATURES)) {
vu_set_enable_all_rings(dev, true);
@@ -1074,7 +1083,7 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
return false;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = lduw_le_p(&vq->vring.used->idx);
if (vq->last_avail_idx != vq->used_idx) {
bool resume = dev->iface->queue_is_processed_in_order &&
@@ -1191,7 +1200,7 @@ vu_check_queue_inflights(VuDev *dev, VuVirtq *vq)
return 0;
}
-vq->used_idx = vq->vring.used->idx;
+vq->used_idx = lduw_le_p(&vq->vring.used->idx);
vq->resubmit_num = 0;
vq->resubmit_list = NULL;
vq->counter = 0;
@@ -2021,13 +2030,13 @@ vu_queue_started(const VuDev *dev, const VuVirtq *vq)
static inline uint16_t
vring_avail_flags(VuVirtq *vq)
{
-return vq->vring.avail->flags;
+return lduw_le_p(&vq->vring.avail->flags);
}
static inline uint16_t
vring_avail_idx(VuVirtq *vq)
{
-vq->shadow_avail_idx = vq->vring.avail->idx;
+vq->shadow_avail_idx = lduw_le_p(&vq->vring.avail->idx);
return vq->shadow_avail_idx;
}
@@ -2035,7 +2044,7 @@ vring_avail_idx(VuVirtq *vq)
static inline uint16_t
vring_avail_ring(VuVirtq *vq, int i)
{
-return vq->vring.avail->ring[i];
+return lduw_le_p(&vq->vring.avail->ring[i]);
}
static inline uint16_t
@@ -2123,12 +2132,12 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc
*desc,
int i, unsigned int max, unsigned int *next)
{
/* If this descriptor says it doesn't chain, we're done. */
-if (!(desc[i].flags & VRING_DESC_F_NEXT)) {
+if (!(lduw_le_p(&desc[i].flags) & VRING_DESC_F_NEXT)) {
return VIRTQUEUE_READ_DESC_DONE;
}
/* Check they're not leading us off end of descriptors. */
-*next = desc[i].next;
+*next = lduw_le_p(&desc[i].next);
/* Make sure compiler knows to grab that: we don't want it changing! */
smp_wmb();
@@ -2171,8 +2180,8 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq,
unsigned int *in_bytes,
}
desc = vq->vring.desc;
-if (desc[i].flags & VRING_DESC_F_INDIRECT) {
-if (desc[i].len % sizeof(struct vring_desc)) {
+if (lduw_le_p(&desc[i].flags) & VRING_DESC_F_INDIRECT) {
+if (ldl_le_p(&desc[i].len) % sizeof(struct vring_desc)) {
vu_panic(dev, "Invalid size for indirect buffer table");
goto err;
}
@@ -2185,8 +2194,8 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq,
unsigned int *in_bytes,
/* loop over the indirect descriptor table */
indirect = 1;
-desc_addr = desc[i].addr;
-
[PATCH v2 1/2] virtio: add vhost-user-fs-ccw device
From: Halil Pasic
Wire up the CCW device for vhost-user-fs.
Reviewed-by: Cornelia Huck
Signed-off-by: Halil Pasic
---
hw/s390x/meson.build | 1 +
hw/s390x/vhost-user-fs-ccw.c | 75
2 files changed, 76 insertions(+)
create mode 100644 hw/s390x/vhost-user-fs-ccw.c
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index b63782d87ab5..948ceae7a7b3 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -41,6 +41,7 @@ virtio_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true:
files('virtio-ccw-scsi.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_SERIAL', if_true:
files('virtio-ccw-serial.c'))
virtio_ss.add(when: ['CONFIG_VIRTIO_9P', 'CONFIG_VIRTFS'], if_true:
files('virtio-ccw-blk.c'))
virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-ccw.c'))
+virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true:
files('vhost-user-fs-ccw.c'))
s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
hw_arch += {'s390x': s390x_ss}
diff --git a/hw/s390x/vhost-user-fs-ccw.c b/hw/s390x/vhost-user-fs-ccw.c
new file mode 100644
index ..6c6f26929301
--- /dev/null
+++ b/hw/s390x/vhost-user-fs-ccw.c
@@ -0,0 +1,75 @@
+/*
+ * virtio ccw vhost-user-fs implementation
+ *
+ * Copyright 2020 IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "hw/virtio/vhost-user-fs.h"
+#include "virtio-ccw.h"
+
+typedef struct VHostUserFSCcw {
+VirtioCcwDevice parent_obj;
+VHostUserFS vdev;
+} VHostUserFSCcw;
+
+#define TYPE_VHOST_USER_FS_CCW "vhost-user-fs-ccw"
+#define VHOST_USER_FS_CCW(obj) \
+OBJECT_CHECK(VHostUserFSCcw, (obj), TYPE_VHOST_USER_FS_CCW)
+
+
+static Property vhost_user_fs_ccw_properties[] = {
+DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
+VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+ VIRTIO_CCW_MAX_REV),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_fs_ccw_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(ccw_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+qdev_realize(vdev, BUS(&ccw_dev->bus), errp);
+}
+
+static void vhost_user_fs_ccw_instance_init(Object *obj)
+{
+VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);
+VirtioCcwDevice *ccw_dev = VIRTIO_CCW_DEVICE(obj);
+
+ccw_dev->force_revision_1 = true;
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VHOST_USER_FS);
+}
+
+static void vhost_user_fs_ccw_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_CLASS(klass);
+
+k->realize = vhost_user_fs_ccw_realize;
+device_class_set_props(dc, vhost_user_fs_ccw_properties);
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static const TypeInfo vhost_user_fs_ccw = {
+.name = TYPE_VHOST_USER_FS_CCW,
+.parent= TYPE_VIRTIO_CCW_DEVICE,
+.instance_size = sizeof(VHostUserFSCcw),
+.instance_init = vhost_user_fs_ccw_instance_init,
+.class_init= vhost_user_fs_ccw_class_init,
+};
+
+static void vhost_user_fs_ccw_register(void)
+{
+type_register_static(&vhost_user_fs_ccw);
+}
+
+type_init(vhost_user_fs_ccw_register)
--
2.25.4
[PATCH v2 0/2] Enable virtio-fs on s390x
This patch series is about enabling virtio-fs on s390x. For that we need + some shim code (first patch), and we need + libvhost-user to deal with virtio endiannes for non-legacy virtio devices as mandated by the spec. How to use? For general instructions how to use virtio-fs (on x86) please have a look at https://virtio-fs.gitlab.io/howto-qemu.html. Most of the instructions can also be applied on s390x. In short: 1. Install self-compiled QEMU with this patch series applied 2. Prepare host and guest kernel so they support virtio-fs Start virtiofsd on the host $ virtiofsd -f --socket-path=/tmp/vhostqemu -o source=/tmp/shared Now you can start QEMU in a separate shell on the host: $ qemu-system-s390x -machine type=s390-ccw-virtio,accel=kvm,memory-backend=mem \ -object memory-backend-file,id=mem,size=2G,mem-path=/dev/shm/virtiofs,share=on,prealloc=on,prealloc-threads=1 \ -chardev socket,id=char0,path=/tmp/vhostqemu -device vhost-user-fs-ccw,queue-size=1024,chardev=char0,tag=myfs \ -drive if=virtio,file=disk.qcow2 \ -m 2G -smp 2 -nographic Log into the guest and mount it $ mount -t virtiofs myfs /mnt Changelog: v1 -> v2: - rebased - patch 1: + changed to Meson build. I kept the r-b from Conny since this change seems to be straightforward to me. - patch 2: + added r-b from Michael + added comment from Conny RFC v2 -> v1: - patch 1: + Added `force_revision_1 = true` (Conny) + I didn't add the r-b from Stefan Hajnoczi as I've added the changes suggested by Conny - squashed patches 2 and 3: + removed assertion in performance critical code path (Stefan) + dropped all dead code (Stefan) + removed libvhost-access.h RFC v1 -> RFC v2: + rebased + drop patch "libvhost-user: print invalid address on vu_panic" as it's not related to this series + drop patch "[RFC 4/4] HACK: Hard-code the libvhost-user.o-cflags for s390x" + patch "virtio: add vhost-user-fs-ccw device": replace qdev_set_parent_bus and object_property_set_bool by qdev_realize + patch "libvhost-user: handle endianness as mandated by the spec": Drop support for legacy virtio devices + add patch to fence legacy virtio devices Halil Pasic (1): virtio: add vhost-user-fs-ccw device Marc Hartmayer (1): libvhost-user: handle endianness as mandated by the spec contrib/libvhost-user/libvhost-user.c | 77 +++ hw/s390x/meson.build | 1 + hw/s390x/vhost-user-fs-ccw.c | 75 ++ 3 files changed, 119 insertions(+), 34 deletions(-) create mode 100644 hw/s390x/vhost-user-fs-ccw.c -- 2.25.4
Re: [PATCH v1 0/3] pc-bios/s390-ccw: Small Makefile improvements
On Wed, Oct 02, 2024 at 11:57 AM +0200, Thomas Huth wrote: > On 01/10/2024 17.36, Marc Hartmayer wrote: >> >> Jens Remus (2): >>pc-bios/s390-ccw: Clarify alignment is in bytes >>pc-bios/s390-ccw: Don't generate TEXTRELs >> >> Marc Hartmayer (1): >>pc-bios/s390-ccw: Introduce `EXTRA_LDFLAGS` >> >> pc-bios/s390-ccw/netboot.mak | 2 +- >> pc-bios/s390-ccw/Makefile| 5 +++-- >> pc-bios/s390-ccw/start.S | 11 +++ >> 3 files changed, 11 insertions(+), 7 deletions(-) > > Series > Reviewed-by: Thomas Huth Thanks. > > I'll queue it ... but will likely wait with sending a merge request for a > while to see whether other s390-ccw bios patches will be ready within the > next weeks (so that I don't have to build the s390-ccw.img multiple times > within a release cycle) Makes sense. > > Thomas > > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH v1 3/3] pc-bios/s390-ccw: Introduce `EXTRA_LDFLAGS`
Some packaging tools want to override `LDFLAGS` when building QEMU, this will result in a build error as most likely no `-nostdlib` flag is passed. Introduce `EXTRA_LDFLAGS` so that the packager can override `LDFLAGS` without breaking the build. Signed-off-by: Marc Hartmayer --- Note: Normally, I would rather use ALL_LDFLAGS, but adding EXTRA_LDFLAGS matches with the current Makefile style. --- pc-bios/s390-ccw/netboot.mak | 2 +- pc-bios/s390-ccw/Makefile| 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak index 046aa35587ac..b1bc12603d99 100644 --- a/pc-bios/s390-ccw/netboot.mak +++ b/pc-bios/s390-ccw/netboot.mak @@ -6,7 +6,7 @@ NETOBJS := start.o sclp.o cio.o virtio.o virtio-net.o jump2ipl.o netmain.o LIBC_INC := -nostdinc -I$(SLOF_DIR)/lib/libc/include LIBNET_INC := -I$(SLOF_DIR)/lib/libnet -NETLDFLAGS := $(LDFLAGS) -Wl,-Ttext=0x780 +NETLDFLAGS := $(EXTRA_LDFLAGS) $(LDFLAGS) -Wl,-Ttext=0x780 $(NETOBJS): EXTRA_CFLAGS += $(LIBC_INC) $(LIBNET_INC) diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile index ab6bd1edf41e..56c971462992 100644 --- a/pc-bios/s390-ccw/Makefile +++ b/pc-bios/s390-ccw/Makefile @@ -4,6 +4,7 @@ all: build-all include config-host.mak CFLAGS = -O2 -g +LDFLAGS ?= MAKEFLAGS += -rR GIT_SUBMODULES = roms/SLOF @@ -40,7 +41,7 @@ EXTRA_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE EXTRA_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables EXTRA_CFLAGS += -msoft-float EXTRA_CFLAGS += -std=gnu99 -LDFLAGS += -Wl,-pie -nostdlib -z noexecstack -z text +EXTRA_LDFLAGS += -Wl,-pie -nostdlib -z noexecstack -z text cc-test = $(CC) -Werror $1 -c -o /dev/null -xc /dev/null >/dev/null 2>/dev/null cc-option = if $(call cc-test, $1); then \ @@ -58,7 +59,7 @@ config-cc.mak: Makefile build-all: s390-ccw.img s390-netboot.img s390-ccw.elf: $(OBJECTS) - $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $(OBJECTS),Linking) + $(call quiet-command,$(CC) $(EXTRA_LDFLAGS) $(LDFLAGS) -o $@ $(OBJECTS),Linking) s390-ccw.img: s390-ccw.elf $(call quiet-command,$(STRIP) --strip-unneeded $< -o $@,Stripping $< into) -- 2.43.0
[PATCH v1 1/3] pc-bios/s390-ccw: Clarify alignment is in bytes
From: Jens Remus The assembler directive .align [1] has architecture-dependent behavior, which may be ambiguous for the reader. Some architectures perform the alignment in bytes, others in power of two. s390 does in bytes. Use the directive .balign [2] instead, to clarify that the alignment request is in bytes. No functional change. [1] https://sourceware.org/binutils/docs/as/Align.html [2] https://sourceware.org/binutils/docs/as/Balign.html Signed-off-by: Jens Remus Reviewed-by: Marc Hartmayer --- pc-bios/s390-ccw/start.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S index 061b06591cff..576fc12c06e8 100644 --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -112,7 +112,7 @@ io_new_code: lctlg %c6,%c6,0(%r15) br %r14 -.align 8 +.balign 8 bss_start_literal: .quad __bss_start disabled_wait_psw: @@ -125,7 +125,7 @@ io_new_mask: .quad 0x00018000 .bss -.align 8 +.balign 8 stack: .space STACK_SIZE .size stack,STACK_SIZE -- 2.43.0
[PATCH v1 2/3] pc-bios/s390-ccw: Don't generate TEXTRELs
From: Jens Remus
Commit 7cd50cbe4ca3 ("pc-bios/s390-ccw: Don't use __bss_start with the
"larl" instruction") introduced the address constant bss_start_literal
for __bss_start in the .text section, which introduced a relocation in
code (i.e. TEXTREL). The dedicated constant is required, as __bss_start
may not necessarily be aligned on a 2-byte boundary (see subject commit
for details).
Move the constant to the .data section to get rid of the relocation in
the .text section. Add the linker option -z text to prevent TEXTRELs to
get introduced in the future.
Note that the R_390_RELATIVE relocations are taken care of by function
glue() in include/hw/elf_ops.h.inc introduced by commit 5dce07e1cb67
("elf-loader: Provide the possibility to relocate s390 ELF files").
Reported-by: Marc Hartmayer
Signed-off-by: Jens Remus
Reviewed-by: Marc Hartmayer
---
pc-bios/s390-ccw/Makefile | 2 +-
pc-bios/s390-ccw/start.S | 7 +--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 6207911b53aa..ab6bd1edf41e 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -40,7 +40,7 @@ EXTRA_CFLAGS += -ffreestanding
-fno-delete-null-pointer-checks -fno-common -fPIE
EXTRA_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
EXTRA_CFLAGS += -msoft-float
EXTRA_CFLAGS += -std=gnu99
-LDFLAGS += -Wl,-pie -nostdlib -z noexecstack
+LDFLAGS += -Wl,-pie -nostdlib -z noexecstack -z text
cc-test = $(CC) -Werror $1 -c -o /dev/null -xc /dev/null >/dev/null 2>/dev/null
cc-option = if $(call cc-test, $1); then \
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 576fc12c06e8..b70213e41246 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -113,8 +113,6 @@ io_new_code:
br %r14
.balign 8
-bss_start_literal:
-.quad __bss_start
disabled_wait_psw:
.quad 0x000200018000,0x
enabled_wait_psw:
@@ -124,6 +122,11 @@ external_new_mask:
io_new_mask:
.quad 0x00018000
+.data
+.balign 8
+bss_start_literal:
+.quad __bss_start
+
.bss
.balign 8
stack:
--
2.43.0
[PATCH v1 0/3] pc-bios/s390-ccw: Small Makefile improvements
Jens Remus (2): pc-bios/s390-ccw: Clarify alignment is in bytes pc-bios/s390-ccw: Don't generate TEXTRELs Marc Hartmayer (1): pc-bios/s390-ccw: Introduce `EXTRA_LDFLAGS` pc-bios/s390-ccw/netboot.mak | 2 +- pc-bios/s390-ccw/Makefile| 5 +++-- pc-bios/s390-ccw/start.S | 11 +++ 3 files changed, 11 insertions(+), 7 deletions(-) base-commit: 718780d20470c66a3a36d036b29148d5809dc855 -- 2.43.0
