Re: [PATCH 2/9] hw/sd: Extract address_in_range() helper, log invalid accesses
On Wed, Jun 23, 2021 at 08:00:14PM +0200, Philippe Mathieu-Daudé wrote:
> Multiple commands have to check the address requested is valid.
check that the
> Extract this code pattern as a new address_in_range() helper, and
> log invalid accesses as guest errors.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sd.c | 32
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index d8fdf84f4db..9c8dd11bad1 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -937,6 +937,18 @@ static void sd_lock_command(SDState *sd)
> sd->card_status &= ~CARD_IS_LOCKED;
> }
>
> +static bool address_in_range(SDState *sd, const char *desc,
> + uint64_t addr, uint32_t length)
> +{
> +if (addr + length > sd->size) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s offset %lu > card %lu [%%%u]\n",
> + desc, addr, sd->size, length);
For a (fictitiously small) device with 2048 bytes and a read request
of 2k at offset 1k, this results in the odd message:
READ_BLOCK offset 1024 > card 2048 [%2048]
Would it be any better as:
"%s offset+length %lu+%lu > card size %lu\n"
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH] gluster: Align block-status tail
On Thu, Aug 05, 2021 at 04:36:03PM +0200, Max Reitz wrote: > gluster's block-status implementation is basically a copy of that in > block/file-posix.c, there is only one thing missing, and that is > aligning trailing data extents to the request alignment (as added by > commit 9c3db310ff0). > > Note that 9c3db310ff0 mentions that "there seems to be no other block > driver that sets request_alignment and [...]", but while block/gluster.c > does indeed not set request_alignment, block/io.c's > bdrv_refresh_limits() will still default to an alignment of 512 because > block/gluster.c does not provide a byte-aligned read function. > Therefore, unaligned tails can conceivably occur, and so we should apply > the change from 9c3db310ff0 to gluster's block-status implementation. > > Reported-by: Vladimir Sementsov-Ogievskiy > Signed-off-by: Max Reitz > --- > block/gluster.c | 16 > 1 file changed, 16 insertions(+) Probably not a show-stopper for 6.1, so I'm fine if it sits until 6.2. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 1/9] hw/sd: When card is in wrong state, log which state it is
On Wed, Jun 23, 2021 at 08:00:13PM +0200, Philippe Mathieu-Daudé wrote: > We report the card is in an inconsistent state, but don't precise s/don't/aren't/ > in which state it is. Add this information, as it is useful when > debugging problems. > > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/sd/sd.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 282d39a7042..d8fdf84f4db 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -1504,7 +1504,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, > SDRequest req) > return sd_illegal; > } > > -qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd); > +qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n", > + req.cmd, sd_state_name(sd->state)); > return sd_illegal; > } > > -- > 2.31.1 > > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [RFC PATCH 6/9] tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08
Hi Philippe,
Op wo 23 jun. 2021 20:00 schreef Philippe Mathieu-Daudé :
> U-Boot expects the SD card size to be at least 2GiB, so
> expand the SD card image to this size before using it.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> TODO: U-Boot reference?
> ---
> tests/acceptance/boot_linux_console.py | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/acceptance/boot_linux_console.py
> b/tests/acceptance/boot_linux_console.py
> index b10f7257503..c4c0f0b393d 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -820,11 +820,13 @@ def test_arm_orangepi_bionic_20_08(self):
> :avocado: tags=arch:arm
> :avocado: tags=machine:orangepi-pc
> :avocado: tags=device:sd
> +:avocado: tags=u-boot
> """
>
> -# This test download a 275 MiB compressed image and expand it
> -# to 1036 MiB, but the underlying filesystem is 1552 MiB...
> -# As we expand it to 2 GiB we are safe.
> +# This test download a 275 MiB compressed image and inflate it
>
Only a few typos here:
download -> downloads
inflate -> inflates
Otherwise, looks fine:
Reviewed-by: Niek Linnenbank
+# to 1036 MiB, but 1/ the underlying filesystem is 1552 MiB,
> +# 2/ U-Boot loads TFTP filenames from the last sector below
> +# 2 GiB, so we need to expand the image further more to 2 GiB.
>
> image_url = ('https://archive.armbian.com/orangepipc/archive/'
>
> 'Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img.xz')
> @@ -833,7 +835,7 @@ def test_arm_orangepi_bionic_20_08(self):
> image_path_xz = self.fetch_asset(image_url, asset_hash=image_hash,
> algorithm='sha256')
> image_path = archive.extract(image_path_xz, self.workdir)
> -image_pow2ceil_expand(image_path)
> +image_expand(image_path, 2 * 1024 * 1024 * 1024)
>
> self.vm.set_console()
> self.vm.add_args('-drive', 'file=' + image_path +
> ',if=sd,format=raw',
> --
> 2.31.1
>
>
Re: [PATCH 5/9] tests/acceptance: Use image_expand() in test_arm_orangepi_uboot_netbsd9
Hi Philippe,
Op wo 4 aug. 2021 22:55 schreef Philippe Mathieu-Daudé :
> Hi Niek,
>
> On 6/23/21 8:00 PM, Philippe Mathieu-Daudé wrote:
> > The NetBSD OrangePi image must be at least 2GiB, not less.
> > Expand the SD card image to this size before using it.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > tests/acceptance/boot_linux_console.py | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/acceptance/boot_linux_console.py
> b/tests/acceptance/boot_linux_console.py
> > index 61069f0064f..b10f7257503 100644
> > --- a/tests/acceptance/boot_linux_console.py
> > +++ b/tests/acceptance/boot_linux_console.py
> > @@ -868,7 +868,12 @@ def test_arm_orangepi_uboot_netbsd9(self):
> > :avocado: tags=device:sd
> > :avocado: tags=os:netbsd
> > """
> > -# This test download a 304MB compressed image and expand it to
> 2GB
> > +# This test download a 304MB compressed image and expand it to
> 2GB,
> > +# which is the minimum card size required by the NetBSD
> installer:
> > +# https://wiki.netbsd.org/ports/evbarm/raspberry_pi/#index7h2
> > +# "A 2 GB card is the smallest workable size that the
> installation
> > +# image will fit on."
>
> Do you agree with this comment and the one in the next patch?
>
Yes, this change looks fine for me.
Reviewed-by: Niek Linnenbank
> > +NETBSD_SDCARD_MINSIZE = 2 * 1024 * 1024 * 1024
> > deb_url = ('http://snapshot.debian.org/archive/debian/'
> > '20200108T145233Z/pool/main/u/u-boot/'
> > 'u-boot-sunxi_2020.01%2Bdfsg-1_armhf.deb')
> > @@ -886,7 +891,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
> > image_path_gz = self.fetch_asset(image_url,
> asset_hash=image_hash)
> > image_path = os.path.join(self.workdir, 'armv7.img')
> > archive.gzip_uncompress(image_path_gz, image_path)
> > -image_pow2ceil_expand(image_path)
> > +image_expand(image_path, NETBSD_SDCARD_MINSIZE)
> > image_drive_args = 'if=sd,format=raw,snapshot=on,file=' +
> image_path
> >
> > # dd if=u-boot-sunxi-with-spl.bin of=armv7.img bs=1K seek=8
> conv=notrunc
> >
>
>
[PATCH] gluster: Align block-status tail
gluster's block-status implementation is basically a copy of that in
block/file-posix.c, there is only one thing missing, and that is
aligning trailing data extents to the request alignment (as added by
commit 9c3db310ff0).
Note that 9c3db310ff0 mentions that "there seems to be no other block
driver that sets request_alignment and [...]", but while block/gluster.c
does indeed not set request_alignment, block/io.c's
bdrv_refresh_limits() will still default to an alignment of 512 because
block/gluster.c does not provide a byte-aligned read function.
Therefore, unaligned tails can conceivably occur, and so we should apply
the change from 9c3db310ff0 to gluster's block-status implementation.
Reported-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz
---
block/gluster.c | 16
1 file changed, 16 insertions(+)
diff --git a/block/gluster.c b/block/gluster.c
index e8ee14c8e9..48a04417cf 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1477,6 +1477,8 @@ static int coroutine_fn
qemu_gluster_co_block_status(BlockDriverState *bs,
off_t data = 0, hole = 0;
int ret = -EINVAL;
+assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
+
if (!s->fd) {
return ret;
}
@@ -1501,6 +1503,20 @@ static int coroutine_fn
qemu_gluster_co_block_status(BlockDriverState *bs,
/* On a data extent, compute bytes to the end of the extent,
* possibly including a partial sector at EOF. */
*pnum = MIN(bytes, hole - offset);
+
+/*
+ * We are not allowed to return partial sectors, though, so
+ * round up if necessary.
+ */
+if (!QEMU_IS_ALIGNED(*pnum, bs->bl.request_alignment)) {
+int64_t file_length = qemu_gluster_getlength(bs);
+if (file_length > 0) {
+/* Ignore errors, this is just a safeguard */
+assert(hole == file_length);
+}
+*pnum = ROUND_UP(*pnum, bs->bl.request_alignment);
+}
+
ret = BDRV_BLOCK_DATA;
} else {
/* On a hole, compute bytes to the beginning of the next extent. */
--
2.31.1
Re: [PATCH v2] block/io_uring: resubmit when result is -EAGAIN
On Wed, Aug 04, 2021 at 06:52:15PM +0200, Kevin Wolf wrote:
Am 04.08.2021 um 16:50 hat Stefano Garzarella geschrieben:
On Mon, Aug 02, 2021 at 02:40:36PM +0200, Kevin Wolf wrote:
> Am 29.07.2021 um 11:10 hat Fabian Ebner geschrieben:
> > Linux SCSI can throw spurious -EAGAIN in some corner cases in its
> > completion path, which will end up being the result in the completed
> > io_uring request.
> >
> > Resubmitting such requests should allow block jobs to complete, even
> > if such spurious errors are encountered.
> >
> > Co-authored-by: Stefan Hajnoczi
> > Reviewed-by: Stefano Garzarella
> > Signed-off-by: Fabian Ebner
> > ---
> >
> > Changes from v1:
> > * Focus on what's relevant for the patch itself in the commit
> > message.
> > * Add Stefan's comment.
> > * Add Stefano's R-b tag (I hope that's fine, since there was no
> > change code-wise).
> >
> > block/io_uring.c | 16 +++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/io_uring.c b/block/io_uring.c
> > index 00a3ee9fb8..dfa475cc87 100644
> > --- a/block/io_uring.c
> > +++ b/block/io_uring.c
> > @@ -165,7 +165,21 @@ static void luring_process_completions(LuringState *s)
> > total_bytes = ret + luringcb->total_read;
> >
> > if (ret < 0) {
> > -if (ret == -EINTR) {
> > +/*
> > + * Only writev/readv/fsync requests on regular files or host
block
> > + * devices are submitted. Therefore -EAGAIN is not expected
but it's
> > + * known to happen sometimes with Linux SCSI. Submit again and
hope
> > + * the request completes successfully.
> > + *
> > + * For more information, see:
> > + *
https://lore.kernel.org/io-uring/20210727165811.284510-3-ax...@kernel.dk/T/#u
> > + *
> > + * If the code is changed to submit other types of requests in
the
> > + * future, then this workaround may need to be extended to
deal with
> > + * genuine -EAGAIN results that should not be resubmitted
> > + * immediately.
> > + */
> > +if (ret == -EINTR || ret == -EAGAIN) {
> > luring_resubmit(s, luringcb);
> > continue;
> > }
>
> Reviewed-by: Kevin Wolf
>
> Question about the preexisting code, though: luring_resubmit() requires
> that the caller calls ioq_submit() later so that the request doesn't
> just sit in a queue without getting any attention, but actually gets
> submitted to the kernel.
>
> In the call chain ioq_submit() -> luring_process_completions() ->
> luring_resubmit(), who takes care of that?
Mmm, good point.
There should be the same problem with ioq_submit() ->
luring_process_completions() -> luring_resubmit_short_read() ->
luring_resubmit().
Should we schedule a BH for example in luring_resubmit() to make sure that
ioq_submit() is invoked after a resubmission?
Or just loop in ioq_submit() after calling luring_process_completions()
if new requests were added to the queue?
I was just concerned that we might cycle a bit if a request always
returns -EAGAIN, while scheduling a task might give room for other
devices to queue other requests.
But maybe this happens so occasionally that we might not worry about
it...
Stefano
Re: [PATCH 0/7] floppy: build as modules.
On Wed, Aug 04, 2021 at 05:19:02PM +0200, Philippe Mathieu-Daudé wrote: > +Mark > > On 8/4/21 4:27 PM, Gerd Hoffmann wrote: > > Some code shuffling needed beforehand due to floppy being part of > > several platforms. While being at it also make floppy optional > > in pc machine type. > > > floppy: move fdctrl_init_sysbus > > floppy: move sun4m_fdctrl_init > > https://www.mail-archive.com/qemu-block@nongnu.org/msg84008.html > > Mark suggested: > > You may be able to simplify this further by removing the > global legacy init functions fdctrl_init_sysbus() and > sun4m_fdctrl_init(): from what I can see fdctrl_init_sysbus() > is only used in hw/mips/jazz.c and sun4m_fdctrl_init() is only > used in hw/sparc/sun4m.c so you might as well inline them or > move the functions to the relevant files. > > I did it and plan to send during 6.2. Sounds simpler than module. > You could easily rebase your series on top (or I can include your > patches while sending). Feel free to include them. But I can also rebase when your patches landed upstream. Your choice ;) take care, Gerd
