Re: [PATCH] pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes()
09.02.2024 00:21, Stefan Hajnoczi wrote:
On Thu, 1 Feb 2024 at 06:37, Michael Tokarev wrote:
for (;;) {
-bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS);
+bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES);
Hmm. This smells like a -stable material, but you know better not
to Cc: qemu-stable@ for unrelated stuff... Is it not for stable?
This is not a user-visible bug. The code still works with the smaller
MAX_SECTORS value thanks to the loop.
Yeah, that's my thoughts exactly. Also, most of the time, the cap will
be `size' anyway, not MAX. Still thought I'd ask :)
Thank you for the confirmation!
/mjt
It doesn't hurt to include it in -stable but I also think it doesn't
help :-). It's just an inconsistency in the code.
Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it
On 8/2/24 19:33, BALATON Zoltan wrote:
On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote:
We should not wire IRQs on unrealized device.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/misc/macio/macio.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c9f22f8515..db662a2065 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s,
MACIOIDEState *ide,
Error **errp)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
+ bool success;
- sysbus_connect_irq(sbd, 0, irq0);
- sysbus_connect_irq(sbd, 1, irq1);
qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
object_property_set_link(OBJECT(ide), "dbdma", OBJECT(>dbdma),
_abort);
macio_ide_register_dma(ide);
+ success = qdev_realize(DEVICE(ide), BUS(>macio_bus), errp);
If realize is unsuccessful can you connect irqs if device may be
unrealized? So maybe either the next two lines should be in an if block
or drop the success flag and return false here if (!qdev_realize) and
true at end of func?
Doh you are right, thanks!
+ sysbus_connect_irq(sbd, 0, irq0);
+ sysbus_connect_irq(sbd, 1, irq1);
- return qdev_realize(DEVICE(ide), BUS(>macio_bus), errp);
+ return success;
}
static void macio_oldworld_realize(PCIDevice *d, Error **errp)
Re: [PATCH] pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes()
On Thu, 1 Feb 2024 at 06:37, Michael Tokarev wrote:
>
> 30.01.2024 03:27, Stefan Hajnoczi wrote:
> > The following expression is incorrect because blk_pread_nonzeroes()
> > deals in units of bytes, not sectors:
> >
> >bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS)
> >^^^
> >
> > BDRV_REQUEST_MAX_BYTES is the appropriate constant.
> >
> > Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image")
> > Cc: Xiang Zheng
> > Signed-off-by: Stefan Hajnoczi
> > ---
> > hw/block/block.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index 9f52ee6e72..ff503002aa 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
> > size, void *buf)
> > BlockDriverState *bs = blk_bs(blk);
> >
> > for (;;) {
> > -bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS);
> > +bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES);
>
> Hmm. This smells like a -stable material, but you know better not
> to Cc: qemu-stable@ for unrelated stuff... Is it not for stable?
This is not a user-visible bug. The code still works with the smaller
MAX_SECTORS value thanks to the loop.
It doesn't hurt to include it in -stable but I also think it doesn't
help :-). It's just an inconsistency in the code.
Stefan
Re: [PATCH 0/2] block: Allow concurrent BB context changes
On Wed, 7 Feb 2024 at 04:36, Hanna Czenczek wrote: > > On 06.02.24 17:53, Stefan Hajnoczi wrote: > > On Fri, Feb 02, 2024 at 03:47:53PM +0100, Hanna Czenczek wrote: > > Hi, > > Without the AioContext lock, a BB's context may kind of change at any > time (unless it has a root node, and I/O requests are pending). That > also means that its own context (BlockBackend.ctx) and that of its root > node can differ sometimes (while the context is being changed). > > blk_get_aio_context() doesn't know this yet and asserts that both are > always equal (if there is a root node). Because it's no longer true, > and because callers don't seem to really care about the root node's > context, we can and should remove the assertion and just return the BB's > context. > > Beyond that, the question is whether the callers of > blk_get_aio_context() are OK with the context potentially changing > concurrently. Honestly, it isn't entirely clear to me; most look OK, > except for the virtio-scsi code, which operates under the general > assumption that the BB's context is always equal to that of the > virtio-scsi device. I doubt that this assumption always holds (it is > definitely not obvious to me that it would), but then again, this series > will not make matters worse in that regard, and that is what counts for > me now. > > One clear point of contention is scsi_device_for_each_req_async(), which > is addressed by patch 2. Right now, it schedules a BH in the BB > context, then the BH double-checks whether the context still fits, and > if not, re-schedules itself. Because virtio-scsi's context is fixed, > this seems to indicate to me that it wants to be able to deal with a > case where BB and virtio-scsi context differ, which seems to break that > aforementioned general virtio-scsi assumption. > > I don't agree with the last sentence: virtio-scsi's context isn't fixed. > > The AioContext changes when dataplane is started/stopped. virtio-scsi > switches AioContext between the IOThread's AioContext and the main > loop's qemu_aio_context. > > However, virtio-scsi virtqueue processing only happens in the IOThread's > AioContext. Maybe this is what you meant when you said the AioContext is > fixed? > > > Specifically, I meant VirtIOSCSI.ctx, which is set only once in > virtio_scsi_dataplane_setup(). That’s at least where the virtqueue notifiers > are registered, so yes, virtqueue processing should at least be fixed to that > context. It seems like it’s always possible some things are processed in the > main thread (not just setup/teardown, but also e.g. TMF_LOGICAL_UNIT_RESET), > so to me it seems like virtio-scsi kind of runs in two contexts > simultaneously. Yes, when virtqueue processing is paused, all processing > VirtIOSCSI.ctx is stopped, but I wouldn’t say it switches contexts there. It > just stops processing some requests. > > Either way, virtio-scsi request processing doesn’t stop just because a > scsi-hd device is hot-plugged or -unplugged. If the BB changes contexts in > the hot-unplug path (while vq request processing is continuing in the I/O > thread), its context will differ from that of virtio-scsi. > > So should I just replace the “the context is fixed” and say that in this > specific instance, virtio-scsi vq processing continues in the I/O thread? > > The BH function is aware that the current AioContext might not be the > same as the AioContext at the time the BH was scheduled. That doesn't > break assumptions in the code. > > (It may be possible to rewrite virtio-blk, virtio-scsi, and core > VirtIODevice ioeventfd code to use the simpler model where the > AioContext really is fixed because things have changed significantly > over the years, but I looked a few weeks ago and it's difficult work.) > > I'm just pointing out that I think this description is incomplete. I > *do* agree with what this patch series is doing :). > > > Well, this description won’t land in any commit log, so from my side, I’m not > too worried about its correctness. O:) Okay, I think we're in agreement. What you described in your reply matches how I understand the code. No need to resend anything. Stefan
Re: [PATCH] block: allocate aligned write buffer for 'truncate -m full'
On 1/25/24 18:46, Vladimir Sementsov-Ogievskiy wrote:
> On 11.12.23 13:55, Andrey Drobyshev wrote:
>> In case we're truncating an image opened with O_DIRECT, we might get
>> -EINVAL on write with unaligned buffer. In particular, when running
>> iotests/298 with '-nocache' we get:
>>
>> qemu-io: Failed to resize underlying file: Could not write zeros for
>> preallocation: Invalid argument
>>
>> Let's just allocate the buffer using qemu_blockalign0() instead.
>>
>> Signed-off-by: Andrey Drobyshev
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
> I also suggest to use QEMU_AUTO_VFREE (keep my r-b if you do).
>
>> ---
>> block/file-posix.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index b862406c71..cee8de510b 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2354,7 +2354,7 @@ static int handle_aiocb_truncate(void *opaque)
>> goto out;
>> }
>> - buf = g_malloc0(65536);
>> + buf = qemu_blockalign0(aiocb->bs, 65536);
>> seek_result = lseek(fd, current_length, SEEK_SET);
>> if (seek_result < 0) {
>> @@ -2413,7 +2413,7 @@ out:
>> }
>> }
>> - g_free(buf);
>> + qemu_vfree(buf);
>> return result;
>> }
>>
>
Yet another ping, just checking if any of the block maintainers is
interested
Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it
On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote:
We should not wire IRQs on unrealized device.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/misc/macio/macio.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c9f22f8515..db662a2065 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s,
MACIOIDEState *ide,
Error **errp)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
+bool success;
-sysbus_connect_irq(sbd, 0, irq0);
-sysbus_connect_irq(sbd, 1, irq1);
qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
object_property_set_link(OBJECT(ide), "dbdma", OBJECT(>dbdma),
_abort);
macio_ide_register_dma(ide);
+success = qdev_realize(DEVICE(ide), BUS(>macio_bus), errp);
If realize is unsuccessful can you connect irqs if device may be
unrealized? So maybe either the next two lines should be in an if block or
drop the success flag and return false here if (!qdev_realize) and true at
end of func?
Regards,
BALATON Zoltan
+sysbus_connect_irq(sbd, 0, irq0);
+sysbus_connect_irq(sbd, 1, irq1);
-return qdev_realize(DEVICE(ide), BUS(>macio_bus), errp);
+return success;
}
static void macio_oldworld_realize(PCIDevice *d, Error **errp)
Re: [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it
On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote:
We should not wire IRQs on unrealized device.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/pc_q35.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7ca3f465e0..f67e5a55df 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -250,11 +250,11 @@ static void pc_q35_init(MachineState *machine)
TYPE_ICH9_LPC_DEVICE);
qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
x86_machine_is_smm_enabled(x86ms));
+pci_realize_and_unref(lpc, host_bus, _fatal);
lpc_dev = DEVICE(lpc);
You could also move setting lpc_dev before the qdev_prop_set_bit line and
use it for the first parameter while you're at it.
Regards,
BALATON Zoltam
for (i = 0; i < IOAPIC_NUM_PINS; i++) {
qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
}
-pci_realize_and_unref(lpc, host_bus, _fatal);
rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
[PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it
We should not wire IRQs on unrealized device. Signed-off-by: Philippe Mathieu-Daudé --- hw/sparc/sun4m.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c index e782c8ec7a..d52e6a7213 100644 --- a/hw/sparc/sun4m.c +++ b/hw/sparc/sun4m.c @@ -312,13 +312,11 @@ static void *sparc32_dma_init(hwaddr dma_base, dma = qdev_new(TYPE_SPARC32_DMA); espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component( OBJECT(dma), "espdma")); -sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq); esp = SYSBUS_ESP(object_resolve_path_component(OBJECT(espdma), "esp")); ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component( OBJECT(dma), "ledma")); -sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq); lance = SYSBUS_PCNET(object_resolve_path_component( OBJECT(ledma), "lance")); @@ -332,6 +330,11 @@ static void *sparc32_dma_init(hwaddr dma_base, } sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), _fatal); + +sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq); + +sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq); + sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base); sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base); -- 2.41.0
[PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices
Inline cpu_create() in order to call qdev_init_gpio_in_named_with_opaque() before the CPU is realized. Signed-off-by: Philippe Mathieu-Daudé --- hw/sparc/leon3.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c index 0df5fc949d..0e1d749306 100644 --- a/hw/sparc/leon3.c +++ b/hw/sparc/leon3.c @@ -234,8 +234,11 @@ static void leon3_generic_hw_init(MachineState *machine) APBPnp *apb_pnp; /* Init CPU */ -cpu = SPARC_CPU(cpu_create(machine->cpu_type)); +cpu = SPARC_CPU(object_new(machine->cpu_type)); env = >env; +qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in, +env, "pil", 1); +qdev_realize(DEVICE(cpu), NULL, _fatal); cpu_sparc_set_id(env, 0); @@ -261,8 +264,6 @@ static void leon3_generic_hw_init(MachineState *machine) /* Allocate IRQ manager */ irqmpdev = qdev_new(TYPE_GRLIB_IRQMP); -qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in, -env, "pil", 1); sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), _fatal); sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET); qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0, -- 2.41.0
[PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
Inline cpu_create() in order to call
qdev_init_gpio_in_named_with_opaque()
before the CPU is realized.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sparc64/sparc64.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index 72f0849f50..3091cde586 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -24,6 +24,7 @@
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "cpu.h"
#include "hw/boards.h"
#include "hw/sparc/sparc64.h"
@@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type,
uint64_t prom_addr)
uint32_t stick_frequency = 100 * 100;
uint32_t hstick_frequency = 100 * 100;
-cpu = SPARC_CPU(cpu_create(cpu_type));
+cpu = SPARC_CPU(object_new(cpu_type));
qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
"ivec-irq", IVEC_MAX);
+qdev_realize(DEVICE(cpu), NULL, _fatal);
env = >env;
env->tick = cpu_timer_create("tick", cpu, tick_irq,
--
2.41.0
[PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it
We should not wire IRQs on unrealized device. Signed-off-by: Philippe Mathieu-Daudé --- hw/sparc/leon3.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c index 2dfb742566..0df5fc949d 100644 --- a/hw/sparc/leon3.c +++ b/hw/sparc/leon3.c @@ -263,10 +263,10 @@ static void leon3_generic_hw_init(MachineState *machine) irqmpdev = qdev_new(TYPE_GRLIB_IRQMP); qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in, env, "pil", 1); -qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0, -qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0)); sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), _fatal); sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET); +qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0, +qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0)); env->irq_manager = irqmpdev; env->qemu_irq_ack = leon3_irq_manager; grlib_apb_pnp_add_entry(apb_pnp, LEON3_IRQMP_OFFSET, 0xFFF, -- 2.41.0
[PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'
"qemu/units.h" is not used in the "hw/rx/rx62n.h" header, include it in the source where it is. Signed-off-by: Philippe Mathieu-Daudé --- include/hw/rx/rx62n.h | 1 - hw/rx/rx-gdbsim.c | 1 + hw/rx/rx62n.c | 1 + 3 files changed, 2 insertions(+), 1 deletion(-) diff --git a/include/hw/rx/rx62n.h b/include/hw/rx/rx62n.h index 73ceeb58e5..bcda583ab3 100644 --- a/include/hw/rx/rx62n.h +++ b/include/hw/rx/rx62n.h @@ -29,7 +29,6 @@ #include "hw/timer/renesas_tmr.h" #include "hw/timer/renesas_cmt.h" #include "hw/char/renesas_sci.h" -#include "qemu/units.h" #include "qom/object.h" #define TYPE_RX62N_MCU "rx62n-mcu" diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c index 47c17026c7..bb4746c556 100644 --- a/hw/rx/rx-gdbsim.c +++ b/hw/rx/rx-gdbsim.c @@ -20,6 +20,7 @@ #include "qemu/cutils.h" #include "qemu/error-report.h" #include "qemu/guest-random.h" +#include "qemu/units.h" #include "qapi/error.h" #include "hw/loader.h" #include "hw/rx/rx62n.h" diff --git a/hw/rx/rx62n.c b/hw/rx/rx62n.c index 4dc44afd9d..d3f61a6837 100644 --- a/hw/rx/rx62n.c +++ b/hw/rx/rx62n.c @@ -23,6 +23,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qemu/error-report.h" +#include "qemu/units.h" #include "hw/rx/rx62n.h" #include "hw/loader.h" #include "hw/sysbus.h" -- 2.41.0
[PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it
We should not wire IRQs on unrealized device.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/pc_q35.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7ca3f465e0..f67e5a55df 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -250,11 +250,11 @@ static void pc_q35_init(MachineState *machine)
TYPE_ICH9_LPC_DEVICE);
qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
x86_machine_is_smm_enabled(x86ms));
+pci_realize_and_unref(lpc, host_bus, _fatal);
lpc_dev = DEVICE(lpc);
for (i = 0; i < IOAPIC_NUM_PINS; i++) {
qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
}
-pci_realize_and_unref(lpc, host_bus, _fatal);
rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
--
2.41.0
[PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it
We should not wire IRQs on unrealized device.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/misc/macio/macio.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c9f22f8515..db662a2065 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s,
MACIOIDEState *ide,
Error **errp)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
+bool success;
-sysbus_connect_irq(sbd, 0, irq0);
-sysbus_connect_irq(sbd, 1, irq1);
qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
object_property_set_link(OBJECT(ide), "dbdma", OBJECT(>dbdma),
_abort);
macio_ide_register_dma(ide);
+success = qdev_realize(DEVICE(ide), BUS(>macio_bus), errp);
+sysbus_connect_irq(sbd, 0, irq0);
+sysbus_connect_irq(sbd, 1, irq1);
-return qdev_realize(DEVICE(ide), BUS(>macio_bus), errp);
+return success;
}
static void macio_oldworld_realize(PCIDevice *d, Error **errp)
--
2.41.0
[PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
We should not wire IRQs on unrealized device.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sh4/r2d.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index e9f316a6ce..c73e8f49b8 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -285,9 +285,9 @@ static void r2d_init(MachineState *machine)
dinfo = drive_get(IF_IDE, 0, 0);
dev = qdev_new("mmio-ide");
busdev = SYS_BUS_DEVICE(dev);
-sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
qdev_prop_set_uint32(dev, "shift", 1);
sysbus_realize_and_unref(busdev, _fatal);
+sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
sysbus_mmio_map(busdev, 0, 0x14001000);
sysbus_mmio_map(busdev, 1, 0x1400080c);
mmio_ide_init_drives(dev, dinfo, NULL);
--
2.41.0
[PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge before accessing it
We should not wire IRQs on unrealized device. Signed-off-by: Philippe Mathieu-Daudé --- hw/ppc/prep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c index 1a6cd05c61..4eb5477069 100644 --- a/hw/ppc/prep.c +++ b/hw/ppc/prep.c @@ -278,9 +278,9 @@ static void ibm_40p_init(MachineState *machine) /* PCI -> ISA bridge */ i82378_dev = DEVICE(pci_new(PCI_DEVFN(11, 0), "i82378")); +qdev_realize_and_unref(i82378_dev, BUS(pci_bus), _fatal); qdev_connect_gpio_out(i82378_dev, 0, qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT)); -qdev_realize_and_unref(i82378_dev, BUS(pci_bus), _fatal); sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15)); isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0")); -- 2.41.0
[PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary
Instead of filling an array of all the possible IRQs, only call
qdev_get_gpio_in() when an IRQ is used. Remove the array from
RX62NState. Doing so we avoid calling qdev_get_gpio_in() on an
unrealized device.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/rx/rx62n.h | 1 -
hw/rx/rx62n.c | 16
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/include/hw/rx/rx62n.h b/include/hw/rx/rx62n.h
index bcda583ab3..766fe0e435 100644
--- a/include/hw/rx/rx62n.h
+++ b/include/hw/rx/rx62n.h
@@ -67,7 +67,6 @@ struct RX62NState {
MemoryRegion iomem2;
MemoryRegion iomem3;
MemoryRegion c_flash;
-qemu_irq irq[NR_IRQS];
/* Input Clock (XTAL) frequency */
uint32_t xtal_freq_hz;
diff --git a/hw/rx/rx62n.c b/hw/rx/rx62n.c
index d3f61a6837..560f53a58a 100644
--- a/hw/rx/rx62n.c
+++ b/hw/rx/rx62n.c
@@ -148,14 +148,11 @@ static void register_icu(RX62NState *s)
qlist_append_int(trigger_level, levelirq[i]);
}
qdev_prop_set_array(DEVICE(icu), "trigger-level", trigger_level);
-
-for (i = 0; i < NR_IRQS; i++) {
-s->irq[i] = qdev_get_gpio_in(DEVICE(icu), i);
-}
sysbus_realize(icu, _abort);
+
sysbus_connect_irq(icu, 0, qdev_get_gpio_in(DEVICE(>cpu), RX_CPU_IRQ));
sysbus_connect_irq(icu, 1, qdev_get_gpio_in(DEVICE(>cpu), RX_CPU_FIR));
-sysbus_connect_irq(icu, 2, s->irq[SWI]);
+sysbus_connect_irq(icu, 2, qdev_get_gpio_in(DEVICE(>icu), SWI));
sysbus_mmio_map(icu, 0, RX62N_ICU_BASE);
}
@@ -172,7 +169,8 @@ static void register_tmr(RX62NState *s, int unit)
irqbase = RX62N_TMR_IRQ + TMR_NR_IRQ * unit;
for (i = 0; i < TMR_NR_IRQ; i++) {
-sysbus_connect_irq(tmr, i, s->irq[irqbase + i]);
+sysbus_connect_irq(tmr, i,
+ qdev_get_gpio_in(DEVICE(>icu), irqbase + i));
}
sysbus_mmio_map(tmr, 0, RX62N_TMR_BASE + unit * 0x10);
}
@@ -190,7 +188,8 @@ static void register_cmt(RX62NState *s, int unit)
irqbase = RX62N_CMT_IRQ + CMT_NR_IRQ * unit;
for (i = 0; i < CMT_NR_IRQ; i++) {
-sysbus_connect_irq(cmt, i, s->irq[irqbase + i]);
+sysbus_connect_irq(cmt, i,
+ qdev_get_gpio_in(DEVICE(>icu), irqbase + i));
}
sysbus_mmio_map(cmt, 0, RX62N_CMT_BASE + unit * 0x10);
}
@@ -209,7 +208,8 @@ static void register_sci(RX62NState *s, int unit)
irqbase = RX62N_SCI_IRQ + SCI_NR_IRQ * unit;
for (i = 0; i < SCI_NR_IRQ; i++) {
-sysbus_connect_irq(sci, i, s->irq[irqbase + i]);
+sysbus_connect_irq(sci, i,
+ qdev_get_gpio_in(DEVICE(>icu), irqbase + i));
}
sysbus_mmio_map(sci, 0, RX62N_SCI_BASE + unit * 0x08);
}
--
2.41.0
[PATCH v3 00/11] hw: Strengthen SysBus & QBus API
Hi, This series ensure following is called *before* a device is realized: - qbus_new() - sysbus_init_mmio() - qdev_init_gpio_in_named_with_opaque() and these are called *after* it is: - sysbus_mmio_map() - sysbus_connect_irq(), - qdev_connect_gpio_out() - qdev_connect_gpio_out_named() Patches from v2 enforcing these checks will be posted in a separate series. Philippe Mathieu-Daudé (11): hw/ide/ich9: Use AHCIPCIState typedef hw/rx/rx62n: Reduce inclusion of 'qemu/units.h' hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary hw/i386/pc_q35: Realize LPC PCI function before accessing it hw/ppc/prep: Realize ISA bridge before accessing it hw/misc/macio: Realize IDE controller before accessing it hw/sh4/r2d: Realize IDE controller before accessing it hw/sparc/sun4m: Realize DMA controller before accessing it hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it hw/sparc/leon3: Initialize GPIO before realizing CPU devices hw/sparc64/cpu: Initialize GPIO before realizing CPU devices include/hw/rx/rx62n.h | 2 -- hw/i386/pc_q35.c | 2 +- hw/ide/ich.c | 6 +++--- hw/misc/macio/macio.c | 8 +--- hw/ppc/prep.c | 2 +- hw/rx/rx-gdbsim.c | 1 + hw/rx/rx62n.c | 17 + hw/sh4/r2d.c | 2 +- hw/sparc/leon3.c | 11 ++- hw/sparc/sun4m.c | 7 +-- hw/sparc64/sparc64.c | 4 +++- 11 files changed, 35 insertions(+), 27 deletions(-) -- 2.41.0
[PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef
QEMU coding style recommend using structure typedefs:
https://www.qemu.org/docs/master/devel/style.html#typedefs
Signed-off-by: Philippe Mathieu-Daudé
---
hw/ide/ich.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 49f8eb8a7d..048ea7e509 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -99,14 +99,14 @@ static void pci_ich9_reset(DeviceState *dev)
static void pci_ich9_ahci_init(Object *obj)
{
-struct AHCIPCIState *d = ICH9_AHCI(obj);
+AHCIPCIState *d = ICH9_AHCI(obj);
ahci_init(>ahci, DEVICE(obj));
}
static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
{
-struct AHCIPCIState *d;
+AHCIPCIState *d;
int sata_cap_offset;
uint8_t *sata_cap;
d = ICH9_AHCI(dev);
@@ -154,7 +154,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error
**errp)
static void pci_ich9_uninit(PCIDevice *dev)
{
-struct AHCIPCIState *d;
+AHCIPCIState *d;
d = ICH9_AHCI(dev);
msi_uninit(dev);
--
2.41.0
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Wed, Feb 7, 2024 at 11:18 AM Kevin Wolf wrote:
>
> Am 06.02.2024 um 17:44 hat Eugenio Perez Martin geschrieben:
> > On Mon, Feb 5, 2024 at 2:49 PM Kevin Wolf wrote:
> > >
> > > Am 05.02.2024 um 13:22 hat Eugenio Perez Martin geschrieben:
> > > > On Fri, Feb 2, 2024 at 2:25 PM Kevin Wolf wrote:
> > > > >
> > > > > VDUSE requires that virtqueues are first enabled before the DRIVER_OK
> > > > > status flag is set; with the current API of the kernel module, it is
> > > > > impossible to enable the opposite order in our block export code
> > > > > because
> > > > > userspace is not notified when a virtqueue is enabled.
> > > > >
> > > > > This requirement also mathces the normal initialisation order as done
> > > > > by
> > > > > the generic vhost code in QEMU. However, commit 6c482547 accidentally
> > > > > changed the order for vdpa-dev and broke access to VDUSE devices with
> > > > > this.
> > > > >
> > > > > This changes vdpa-dev to use the normal order again and use the
> > > > > standard
> > > > > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be
> > > > > used with vdpa-dev again after this fix.
> > > > >
> > > > > Cc: qemu-sta...@nongnu.org
> > > > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> > > > > Signed-off-by: Kevin Wolf
> > > > > ---
> > > > > hw/virtio/vdpa-dev.c | 5 +
> > > > > hw/virtio/vhost-vdpa.c | 17 +
> > > > > 2 files changed, 18 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> > > > > index eb9ecea83b..13e87f06f6 100644
> > > > > --- a/hw/virtio/vdpa-dev.c
> > > > > +++ b/hw/virtio/vdpa-dev.c
> > > > > @@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice
> > > > > *vdev, Error **errp)
> > > > >
> > > > > s->dev.acked_features = vdev->guest_features;
> > > > >
> > > > > -ret = vhost_dev_start(>dev, vdev, false);
> > > > > +ret = vhost_dev_start(>dev, vdev, true);
> > > > > if (ret < 0) {
> > > > > error_setg_errno(errp, -ret, "Error starting vhost");
> > > > > goto err_guest_notifiers;
> > > > > }
> > > > > -for (i = 0; i < s->dev.nvqs; ++i) {
> > > > > -vhost_vdpa_set_vring_ready(>vdpa, i);
> > > > > -}
> > > > > s->started = true;
> > > > >
> > > > > /*
> > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > index 3a43beb312..c4574d56c5 100644
> > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa
> > > > > *v, unsigned idx)
> > > > > return r;
> > > > > }
> > > > >
> > > > > +static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int
> > > > > enable)
> > > > > +{
> > > > > +struct vhost_vdpa *v = dev->opaque;
> > > > > +unsigned int i;
> > > > > +int ret;
> > > > > +
> > > > > +for (i = 0; i < dev->nvqs; ++i) {
> > > > > +ret = vhost_vdpa_set_vring_ready(v, i);
> > > > > +if (ret < 0) {
> > > > > +return ret;
> > > > > +}
> > > > > +}
> > > > > +
> > > > > +return 0;
> > > > > +}
> > > > > +
> > > > > static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> > > > > int fd)
> > > > > {
> > > > > @@ -1524,6 +1540,7 @@ const VhostOps vdpa_ops = {
> > > > > .vhost_set_features = vhost_vdpa_set_features,
> > > > > .vhost_reset_device = vhost_vdpa_reset_device,
> > > > > .vhost_get_vq_index = vhost_vdpa_get_vq_index,
> > > > > +.vhost_set_vring_enable = vhost_vdpa_set_vring_enable,
> > > > > .vhost_get_config = vhost_vdpa_get_config,
> > > > > .vhost_set_config = vhost_vdpa_set_config,
> > > > > .vhost_requires_shm_log = NULL,
> > > >
> > > > vhost-vdpa net enables CVQ before dataplane ones to configure all the
> > > > device in the destination of a live migration. To go back again to
> > > > this callback would cause the device to enable the dataplane before
> > > > virtqueues are configured again.
> > >
> > > Not that it makes a difference, but I don't think it would actually be
> > > going back. Even before your commit 6c482547, we were not making use of
> > > the generic callback but just called the function in a slightly
> > > different place (but less different than after commit 6c482547).
> > >
> > > But anyway... Why don't the other vhost backend need the same for
> > > vhost-net then? Do they just not support live migration?
> >
> > They don't support control virtqueue. More specifically, control
> > virtqueue is handled directly in QEMU.
>
> So the network device already has to special case vdpa instead of using
> the same code for all vhost backends? :-/
>
> > > I don't know the code well enough to say where the problem is, but if
> > > vhost-vdpa networking code relies on the usual vhost operations not
> > > being implemented and bypasses VhostOps to replace it,
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
On 8/2/24 15:28, Peter Maydell wrote:
On Thu, 8 Feb 2024 at 14:22, Kevin Wolf wrote:
Am 08.02.2024 um 11:48 hat Philippe Mathieu-Daudé geschrieben:
BTW using the same pattern:
-- >8 --
diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
index ec98456e5d..d074762a25 100644
--- a/hw/nvram/xlnx-zynqmp-efuse.c
+++ b/hw/nvram/xlnx-zynqmp-efuse.c
@@ -582,7 +582,7 @@ static uint64_t
zynqmp_efuse_cache_load_prew(RegisterInfo *reg,
static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
{
-return val == 0xDF0D ? 0 : 1;
+return val != 0xDF0D;
}
Maybe. I would have to know that device to tell if this is really meant
as boolean. Or maybe it should be written 0x0 and 0x1 to signify that
it's a register value or something.
This is a RegisterAccessinfo pre_write hook. The docs say:
* @pre_write: Pre write callback. Passed the value that's to be written,
* immediately before the actual write. The returned value is what is written,
* giving the handler a chance to modify the written value.
So it is indeed returning a register value, not a boolean flag
masquerading as a uint64_t.
diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
index 301e61d0dd..bdd73bd181 100644
--- a/tests/tcg/aarch64/sysregs.c
+++ b/tests/tcg/aarch64/sysregs.c
@@ -183,5 +183,5 @@ int main(void)
return 1;
}
-return should_fail_count == 6 ? 0 : 1;
+return should_fail_count != 6;
}
This one isn't unclear to me, though. This is EXIT_SUCCESS and
EXIT_FAILURE, just open-coded. I think making your change would make it
only more confusing.
I agree on this one.
Thanks for both analysis and sorry for my confusion,
I was just looking at the pattern without interpreting
each proper use case.
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
On Thu, 8 Feb 2024 at 14:22, Kevin Wolf wrote:
>
> Am 08.02.2024 um 11:48 hat Philippe Mathieu-Daudé geschrieben:
> > BTW using the same pattern:
> >
> > -- >8 --
> > diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
> > index ec98456e5d..d074762a25 100644
> > --- a/hw/nvram/xlnx-zynqmp-efuse.c
> > +++ b/hw/nvram/xlnx-zynqmp-efuse.c
> > @@ -582,7 +582,7 @@ static uint64_t
> > zynqmp_efuse_cache_load_prew(RegisterInfo *reg,
> >
> > static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
> > {
> > -return val == 0xDF0D ? 0 : 1;
> > +return val != 0xDF0D;
> > }
>
> Maybe. I would have to know that device to tell if this is really meant
> as boolean. Or maybe it should be written 0x0 and 0x1 to signify that
> it's a register value or something.
This is a RegisterAccessinfo pre_write hook. The docs say:
* @pre_write: Pre write callback. Passed the value that's to be written,
* immediately before the actual write. The returned value is what is written,
* giving the handler a chance to modify the written value.
So it is indeed returning a register value, not a boolean flag
masquerading as a uint64_t.
> > diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
> > index 301e61d0dd..bdd73bd181 100644
> > --- a/tests/tcg/aarch64/sysregs.c
> > +++ b/tests/tcg/aarch64/sysregs.c
> > @@ -183,5 +183,5 @@ int main(void)
> > return 1;
> > }
> >
> > -return should_fail_count == 6 ? 0 : 1;
> > +return should_fail_count != 6;
> > }
>
> This one isn't unclear to me, though. This is EXIT_SUCCESS and
> EXIT_FAILURE, just open-coded. I think making your change would make it
> only more confusing.
I agree on this one.
-- PMM
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
Am 08.02.2024 um 11:48 hat Philippe Mathieu-Daudé geschrieben:
> On 8/2/24 11:16, Kevin Wolf wrote:
> > 'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
> > Use the more obvious way to write it.
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > iothread.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/iothread.c b/iothread.c
> > index 6c1fc8c856..e1e9e04736 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
> > bool qemu_in_iothread(void)
> > {
> > -return qemu_get_current_aio_context() == qemu_get_aio_context() ?
> > -false : true;
> > +return qemu_get_current_aio_context() != qemu_get_aio_context();
> > }
>
> Reviewed-by: Philippe Mathieu-Daudé
>
> BTW using the same pattern:
>
> -- >8 --
> diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
> index ec98456e5d..d074762a25 100644
> --- a/hw/nvram/xlnx-zynqmp-efuse.c
> +++ b/hw/nvram/xlnx-zynqmp-efuse.c
> @@ -582,7 +582,7 @@ static uint64_t
> zynqmp_efuse_cache_load_prew(RegisterInfo *reg,
>
> static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
> {
> -return val == 0xDF0D ? 0 : 1;
> +return val != 0xDF0D;
> }
Maybe. I would have to know that device to tell if this is really meant
as boolean. Or maybe it should be written 0x0 and 0x1 to signify that
it's a register value or something.
> diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
> index 301e61d0dd..bdd73bd181 100644
> --- a/tests/tcg/aarch64/sysregs.c
> +++ b/tests/tcg/aarch64/sysregs.c
> @@ -183,5 +183,5 @@ int main(void)
> return 1;
> }
>
> -return should_fail_count == 6 ? 0 : 1;
> +return should_fail_count != 6;
> }
This one isn't unclear to me, though. This is EXIT_SUCCESS and
EXIT_FAILURE, just open-coded. I think making your change would make it
only more confusing.
Kevin
Re: [PATCH] hw/nvme: fix invalid check on mcl
On Feb 8 21:33, Minwoo Im wrote:
> On 24-02-08 13:22:48, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > The number of logical blocks within a source range is converted into a
> > 1s based number at the time of parsing. However, when verifying the copy
> > length we add one again, causing the check against MCL to fail in error.
> >
> > Fixes: 381ab99d8587 ("hw/nvme: check maximum copy length (MCL) for COPY")
> > Signed-off-by: Klaus Jensen
>
> Hi Klaus,
>
> Reviewed-by: Minwoo Im
>
> Thanks!
>
Thanks Minwoo, applied to nvme-next!
signature.asc
Description: PGP signature
Re: [PATCH] hw/nvme: fix invalid check on mcl
On 24-02-08 13:22:48, Klaus Jensen wrote:
> From: Klaus Jensen
>
> The number of logical blocks within a source range is converted into a
> 1s based number at the time of parsing. However, when verifying the copy
> length we add one again, causing the check against MCL to fail in error.
>
> Fixes: 381ab99d8587 ("hw/nvme: check maximum copy length (MCL) for COPY")
> Signed-off-by: Klaus Jensen
Hi Klaus,
Reviewed-by: Minwoo Im
Thanks!
> ---
> hw/nvme/ctrl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index f026245d1e9e..05c667158a3a 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -2855,7 +2855,7 @@ static inline uint16_t
> nvme_check_copy_mcl(NvmeNamespace *ns,
> uint32_t nlb;
> nvme_copy_source_range_parse(iocb->ranges, idx, iocb->format, NULL,
> , NULL, NULL, NULL);
> -copy_len += nlb + 1;
> +copy_len += nlb;
> }
>
> if (copy_len > ns->id_ns.mcl) {
>
> ---
> base-commit: 39a6e4f87e7b75a45b08d6dc8b8b7c2954c87440
> change-id: 20240208-fix-copy-mcl-check-3a6d95327154
>
> Best regards,
> --
> Klaus Jensen
>
>
[PATCH] hw/nvme: fix invalid check on mcl
From: Klaus Jensen
The number of logical blocks within a source range is converted into a
1s based number at the time of parsing. However, when verifying the copy
length we add one again, causing the check against MCL to fail in error.
Fixes: 381ab99d8587 ("hw/nvme: check maximum copy length (MCL) for COPY")
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index f026245d1e9e..05c667158a3a 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2855,7 +2855,7 @@ static inline uint16_t nvme_check_copy_mcl(NvmeNamespace
*ns,
uint32_t nlb;
nvme_copy_source_range_parse(iocb->ranges, idx, iocb->format, NULL,
, NULL, NULL, NULL);
-copy_len += nlb + 1;
+copy_len += nlb;
}
if (copy_len > ns->id_ns.mcl) {
---
base-commit: 39a6e4f87e7b75a45b08d6dc8b8b7c2954c87440
change-id: 20240208-fix-copy-mcl-check-3a6d95327154
Best regards,
--
Klaus Jensen
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
On 8/2/24 11:16, Kevin Wolf wrote:
'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
Use the more obvious way to write it.
Signed-off-by: Kevin Wolf
---
iothread.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/iothread.c b/iothread.c
index 6c1fc8c856..e1e9e04736 100644
--- a/iothread.c
+++ b/iothread.c
@@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
bool qemu_in_iothread(void)
{
-return qemu_get_current_aio_context() == qemu_get_aio_context() ?
-false : true;
+return qemu_get_current_aio_context() != qemu_get_aio_context();
}
Reviewed-by: Philippe Mathieu-Daudé
BTW using the same pattern:
-- >8 --
diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
index ec98456e5d..d074762a25 100644
--- a/hw/nvram/xlnx-zynqmp-efuse.c
+++ b/hw/nvram/xlnx-zynqmp-efuse.c
@@ -582,7 +582,7 @@ static uint64_t
zynqmp_efuse_cache_load_prew(RegisterInfo *reg,
static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
{
-return val == 0xDF0D ? 0 : 1;
+return val != 0xDF0D;
}
diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
index 301e61d0dd..bdd73bd181 100644
--- a/tests/tcg/aarch64/sysregs.c
+++ b/tests/tcg/aarch64/sysregs.c
@@ -183,5 +183,5 @@ int main(void)
return 1;
}
-return should_fail_count == 6 ? 0 : 1;
+return should_fail_count != 6;
}
---
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
Le 08/02/2024 à 11:16, Kevin Wolf a écrit :
'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
Use the more obvious way to write it.
Signed-off-by: Kevin Wolf
---
iothread.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/iothread.c b/iothread.c
index 6c1fc8c856..e1e9e04736 100644
--- a/iothread.c
+++ b/iothread.c
@@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
bool qemu_in_iothread(void)
{
-return qemu_get_current_aio_context() == qemu_get_aio_context() ?
-false : true;
+return qemu_get_current_aio_context() != qemu_get_aio_context();
}
Reviewed-by: Laurent Vivier
[PATCH] iothread: Simplify expression in qemu_in_iothread()
'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
Use the more obvious way to write it.
Signed-off-by: Kevin Wolf
---
iothread.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/iothread.c b/iothread.c
index 6c1fc8c856..e1e9e04736 100644
--- a/iothread.c
+++ b/iothread.c
@@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
bool qemu_in_iothread(void)
{
-return qemu_get_current_aio_context() == qemu_get_aio_context() ?
-false : true;
+return qemu_get_current_aio_context() != qemu_get_aio_context();
}
--
2.43.0
Re: [PULL 1/1] virtio-blk: avoid using ioeventfd state in irqfd conditional
08.02.2024 11:42, Kevin Wolf wrote: The patch email itself was CCed to qemu-stable and even contained a note for backporting to stable: https://lists.gnu.org/archive/html/qemu-block/2024-01/msg00278.html Ahh. Yes. I'm having a large(ish) queue in stable and missed the fact I already has this one in there. Was a -ENOCOFFEE issue this morning. Sorry for the noise. It's only missing in the commit message. I'll add the Cc: line to my pull request (for Stefan's pull request it seems too late because Peter is already processing it, so we'll probably end up having both versions in the git history). That's ok I guess :) Thanks, /mjt
Re: [PULL 1/1] virtio-blk: avoid using ioeventfd state in irqfd conditional
Am 08.02.2024 um 06:37 hat Michael Tokarev geschrieben:
> 06.02.2024 18:31, Stefan Hajnoczi :
> > Requests that complete in an IOThread use irqfd to notify the guest
> > while requests that complete in the main loop thread use the traditional
> > qdev irq code path. The reason for this conditional is that the irq code
> > path requires the BQL:
> >
> >if (s->ioeventfd_started && !s->ioeventfd_disabled) {
> >virtio_notify_irqfd(vdev, req->vq);
> >} else {
> >virtio_notify(vdev, req->vq);
> >}
> >
> > There is a corner case where the conditional invokes the irq code path
> > instead of the irqfd code path:
> >
> >static void virtio_blk_stop_ioeventfd(VirtIODevice *vdev)
> >{
> >...
> >/*
> > * Set ->ioeventfd_started to false before draining so that host
> > notifiers
> > * are not detached/attached anymore.
> > */
> >s->ioeventfd_started = false;
> >
> >/* Wait for virtio_blk_dma_restart_bh() and in flight I/O to
> > complete */
> >blk_drain(s->conf.conf.blk);
> >
> > During blk_drain() the conditional produces the wrong result because
> > ioeventfd_started is false.
> >
> > Use qemu_in_iothread() instead of checking the ioeventfd state.
> >
> > Buglink: https://issues.redhat.com/browse/RHEL-15394
> > Signed-off-by: Stefan Hajnoczi
> > Message-id: 20240122172625.415386-1-stefa...@redhat.com
> > Signed-off-by: Stefan Hajnoczi
>
> Cc qemu-stable? This smells like a stable material, please let me know
> if it is not.
The patch email itself was CCed to qemu-stable and even contained a note
for backporting to stable:
https://lists.gnu.org/archive/html/qemu-block/2024-01/msg00278.html
It's only missing in the commit message. I'll add the Cc: line to
my pull request (for Stefan's pull request it seems too late because
Peter is already processing it, so we'll probably end up having both
versions in the git history).
Kevin
