Re: [PATCH v2 3/4] bamboo, sam460ex: Tidy up error message for unsupported RAM size

[Philippe Mathieu-Daudé]

On 4/22/20 3:48 PM, Markus Armbruster wrote:

Improve

 $ ppc-softmmu/qemu-system-ppc -M sam460ex -m 4096
 qemu-system-ppc: Max 1 banks of 2048 ,1024 ,512 ,256 ,128 ,64 ,32 MB 
DIMM/bank supported
 qemu-system-ppc: Possible valid RAM size: 2048

to

 qemu-system-ppc: at most 1 bank of 2048, 1024, 512, 256, 128, 64, 32 MiB 
each supported
 Possible valid RAM size: 1024 MiB


Thanks,

Reviewed-by: Philippe Mathieu-Daudé 



Signed-off-by: Markus Armbruster 
---
  hw/ppc/ppc4xx_devs.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 3376c43ff5..f1651e04d9 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -716,11 +716,11 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
  for (i = 0; sdram_bank_sizes[i]; i++) {
  g_string_append_printf(s, "%" PRIi64 "%s",
 sdram_bank_sizes[i] / MiB,
-   sdram_bank_sizes[i + 1] ? " ," : "");
+   sdram_bank_sizes[i + 1] ? ", " : "");
  }
-error_report("Max %d banks of %s MB DIMM/bank supported",
-nr_banks, s->str);
-error_report("Possible valid RAM size: %" PRIi64,
+error_report("at most %d bank%s of %s MiB each supported",
+ nr_banks, nr_banks == 1 ? "" : "s", s->str);
+error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",
  used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
  
  g_string_free(s, true);

Re: [PATCH] virtio-vga: fix virtio-vga bar ordering

[Michael S. Tsirkin]

On Wed, Apr 22, 2020 at 12:49:41PM +0200, Gerd Hoffmann wrote:
> On Wed, Apr 22, 2020 at 02:04:36AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Apr 21, 2020 at 11:48:53PM +0200, Anthoine Bourgeois wrote:
> > > With virtio-vga, pci bar are reordered. Bar #2 is used for compatibility
> > > with stdvga. By default, bar #2 is used by virtio modern io bar.
> > > This bar is the last one introduce in the virtio pci bar layout and it's
> > > crushed by the virtio-vga reordering. So virtio-vga and
> > > modern-pio-notify are incompatible because virtio-vga failed to
> > > initialize with this option.
> > > 
> > > This fix exchange the modern io bar with the modern memory bar,
> > > replacing the msix bar that is never impacted anyway.
> > > 
> > > Signed-off-by: Anthoine Bourgeois 
> > 
> > Such changes generally need to be tied to machine version.
> 
> Given that modern-pio-notify is off by default and
> virtio-vga,modern-pio-notify=on is broken I think we don't need to worry
> about compatibility in this specific case (assuming the patch is updated
> to not shuffle around the msix bar, see other reply).
> 
> cheers,
>   Gerd

OK, just worth documenting that this will break cross-version
migration if modern-pio-notify is enabled.

Re: [PATCH v2 08/14] virtio-net: Fix duplex=... and speed=... error handling

[Michael S. Tsirkin]

On Wed, Apr 22, 2020 at 03:07:13PM +0200, Markus Armbruster wrote:
> virtio_net_device_realize() rejects invalid duplex and speed values.
> The error handling is broken:
> 
> $ ../qemu/bld-sani/x86_64-softmmu/qemu-system-x86_64 -S -display none 
> -monitor stdio
> QEMU 4.2.93 monitor - type 'help' for more information
> (qemu) device_add virtio-net,duplex=x
> Error: 'duplex' must be 'half' or 'full'
> (qemu) c
> =
> ==15654==ERROR: AddressSanitizer: heap-use-after-free on address 
> 0x62e14590 at pc 0x560b75c8dc13 bp 0x7fffdf1a6950 sp 0x7fffdf1a6940
> READ of size 8 at 0x62e14590 thread T0
>   #0 0x560b75c8dc12 in object_dynamic_cast_assert 
> /work/armbru/qemu/qom/object.c:826
>   #1 0x560b74c38ac0 in virtio_vmstate_change 
> /work/armbru/qemu/hw/virtio/virtio.c:3210
>   #2 0x560b74d9765e in vm_state_notify /work/armbru/qemu/softmmu/vl.c:1271
>   #3 0x560b7494ba72 in vm_prepare_start /work/armbru/qemu/cpus.c:2156
>   #4 0x560b7494bacd in vm_start /work/armbru/qemu/cpus.c:2162
>   #5 0x560b75a7d890 in qmp_cont /work/armbru/qemu/monitor/qmp-cmds.c:160
>   #6 0x560b75a8d70a in hmp_cont /work/armbru/qemu/monitor/hmp-cmds.c:1043
>   #7 0x560b75a799f2 in handle_hmp_command 
> /work/armbru/qemu/monitor/hmp.c:1082
> [...]
> 
> 0x62e14590 is located 33168 bytes inside of 42288-byte region 
> [0x62e0c400,0x62e16930)
> freed by thread T1 here:
>   #0 0x7feadd39491f in __interceptor_free (/lib64/libasan.so.5+0x10d91f)
>   #1 0x7feadcebcd7c in g_free (/lib64/libglib-2.0.so.0+0x55d7c)
>   #2 0x560b75c8fd40 in object_unref /work/armbru/qemu/qom/object.c:1128
>   #3 0x560b7498a625 in memory_region_unref /work/armbru/qemu/memory.c:1762
>   #4 0x560b74999fa4 in do_address_space_destroy 
> /work/armbru/qemu/memory.c:2788
>   #5 0x560b762362fc in call_rcu_thread /work/armbru/qemu/util/rcu.c:283
>   #6 0x560b761c8884 in qemu_thread_start 
> /work/armbru/qemu/util/qemu-thread-posix.c:519
>   #7 0x7fead9be34bf in start_thread (/lib64/libpthread.so.0+0x84bf)
> 
> previously allocated by thread T0 here:
>   #0 0x7feadd394d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18)
>   #1 0x7feadcebcc88 in g_malloc (/lib64/libglib-2.0.so.0+0x55c88)
>   #2 0x560b75c8cf8a in object_new /work/armbru/qemu/qom/object.c:699
>   #3 0x560b75010ad9 in qdev_device_add 
> /work/armbru/qemu/qdev-monitor.c:654
>   #4 0x560b750120c2 in qmp_device_add /work/armbru/qemu/qdev-monitor.c:805
>   #5 0x560b75012c1b in hmp_device_add /work/armbru/qemu/qdev-monitor.c:905
> [...]
> ==15654==ABORTING
> 
> Cause: virtio_net_device_realize() neglects to bail out after setting
> the error.  Fix that.
> 
> Fixes: 9473939ed7addcaaeb8fde5c093918fb7fa0919c
> Cc: "Michael S. Tsirkin" 
> Cc: Jason Wang 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Philippe Mathieu-Daudé 

Acked-by: Michael S. Tsirkin 

Feel free to merge with the rest of the patchset.

> ---
>  hw/net/virtio-net.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a46e3b37a7..b52ff4ab63 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2947,6 +2947,7 @@ static void virtio_net_device_realize(DeviceState *dev, 
> Error **errp)
>  n->net_conf.duplex = DUPLEX_FULL;
>  } else {
>  error_setg(errp, "'duplex' must be 'half' or 'full'");
> +return;
>  }
>  n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
>  } else {
> @@ -2955,7 +2956,9 @@ static void virtio_net_device_realize(DeviceState *dev, 
> Error **errp)
>  
>  if (n->net_conf.speed < SPEED_UNKNOWN) {
>  error_setg(errp, "'speed' must be between 0 and INT_MAX");
> -} else if (n->net_conf.speed >= 0) {
> +return;
> +}
> +if (n->net_conf.speed >= 0) {
>  n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
>  }
>  
> -- 
> 2.21.1

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

On Wed, 22 Apr 2020, Markus Armbruster wrote:

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

spd_data_generate() can pass @errp to error_setg() more than once when
it adjusts both memory size and type.  Harmless, because no caller
passes anything that needs adjusting.  Until the previous commit,
sam460ex passed types that needed adjusting, but not sizes.

spd_data_generate()'s contract is rather awkward:

   If everything's fine, return non-null and don't set an error.

   Else, if memory size or type need adjusting, return non-null and
   set an error describing the adjustment.

   Else, return null and set an error reporting why no data can be
   generated.

Its callers treat the error as a warning even when null is returned.
They don't create the "smbus-eeprom" device then.  Suspicious.

Since the previous commit, only "everything's fine" can actually
happen.  Drop the unused code and simplify the callers.  This gets rid
of the error API violation.


This leaves board code no chance to recover from values given by user that 
won't fit without duplicating checks that this function does. Also this 
will abort without giving meaningful errors if an invalid value does get 
through and result in a crash which is not used friendly. So I don't like 
this but if others think this is acceptable maybe at least unit test 
should be adjusted to make sure aborts cannot be triggered by user for 
values that are not usually tested during development.


Regards,
BALATON Zoltan


Signed-off-by: Markus Armbruster 
---
include/hw/i2c/smbus_eeprom.h |  2 +-
hw/i2c/smbus_eeprom.c | 30 --
hw/mips/mips_fulong2e.c   | 10 ++
hw/ppc/sam460ex.c | 12 +++-
4 files changed, 10 insertions(+), 44 deletions(-)

diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
index 15e2151b50..68b0063ab6 100644
--- a/include/hw/i2c/smbus_eeprom.h
+++ b/include/hw/i2c/smbus_eeprom.h
@@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
   const uint8_t *eeprom_spd, int size);

enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error 
**errp);
+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);

#endif
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 5adf3b15b5..07fbbf87f1 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
}

/* Generate SDRAM SPD EEPROM data describing a module of type and size */
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
-   Error **errp)
+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
{
uint8_t *spd;
uint8_t nbanks;
@@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type, 
ram_addr_t ram_size,
g_assert_not_reached();
}
size = ram_size >> 20; /* work in terms of megabytes */
-if (size < 4) {
-error_setg(errp, "SDRAM size is too small");
-return NULL;
-}
sz_log2 = 31 - clz32(size);
size = 1U << sz_log2;
-if (ram_size > size * MiB) {
-error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, "
-   "truncating to %u MB", ram_size, size);
-}
-if (sz_log2 < min_log2) {
-error_setg(errp,
-   "Memory size is too small for SDRAM type, adjusting type");
-if (size >= 32) {
-type = DDR;
-min_log2 = 5;
-max_log2 = 12;
-} else {
-type = SDR;
-min_log2 = 2;
-max_log2 = 9;
-}
-}
+assert(ram_size == size * MiB);
+assert(sz_log2 >= min_log2);

nbanks = 1;
while (sz_log2 > max_log2 && nbanks < 8) {
@@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t 
ram_size,
nbanks++;
}

-if (size > (1ULL << sz_log2) * nbanks) {
-error_setg(errp, "Memory size is too big for SDRAM, truncating");
-}
+assert(size == (1ULL << sz_log2) * nbanks);

/* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */
if (nbanks == 1 && sz_log2 > min_log2) {
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 5040afd581..ef02d54b33 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine)
MemoryRegion *bios = g_new(MemoryRegion, 1);
long bios_size;
uint8_t *spd_data;
-Error *err = NULL;
int64_t kernel_entry;
PCIBus *pci_bus;
ISABus *isa_bus;
@@ -377,13 +376,8 @@ static void mips_fulong2e_init(MachineState *machine)
 

Re: [RFC 0/3] 64bit block-layer part I

[Vladimir Sementsov-Ogievskiy]

Any thoughts here? I need to resend to update some more functions as patchew 
said.

Is it OK in general? Or should we instead convert everything to uint64_t ?

30.03.2020 17:18, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

There is an idea to make NBD protocol extension to support 64bit
write-zero/discard/block-status commands (commands, which doesn't
transfer user data). It's needed to increase performance of zeroing
large ranges (up to the whole image). Zeroing of the whole image is used
as first step of mirror job, qemu-img convert, it should be also used at
start of backup actually..

We need to support it in block-layer, so we want 64bit write_zeros.
Currently driver handler now have int bytes parameter.

write_zeros path goes through normal pwritev, so we need 64bit write,
and then we need 64bit read for symmetry, and better, let's make all io
path work with 64bit bytes parameter.

Actually most of block-layer already have 64bit parameters: offset is
sometimes int64_t and sometimes uint64_t. bytes parameter is one of
int64_t, uint64_t, int, unsigned int...

I think we need one type for all of this, and this one type is int64_t.
Signed int64_t is a bit better than uint64_t: you can use same variable
to get some result (including error < 0) and than reuse it as an
argument without any type conversion.

So, I propose, as a first step, convert all uint64_t parameters to
int64_t.

Still, I don't have good idea of how to split this into more than 3
patches, so, this is an RFC.

What's next?

Converting write_zero and discard is not as simple: we can't just
s/int/uint64_t/, as some functions may use some int variables for
calculations and this will be broken by something larger than int.

So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and
.bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all
drivers updated - drop unused 32bit functions, and then drop "64" suffix
from API. If not - we'll live with both APIs.

Another thing to do is updating default limiting of request (currently
they are limited to INT_MAX).

Then we may move some drivers to 64bit discard/write_zero: I think about
qcow2, file-posix and nbd (as a proof-of-concept for already proposed
NBD extension).

Any ideas?

Vladimir Sementsov-Ogievskiy (3):
   block: use int64_t as bytes type in tracked requests
   block/io: convert generic io path to use int64_t parameters
   block: use int64_t instead of uint64_t in driver handlers

  include/block/block.h |  8 ++--
  include/block/block_int.h | 52 ++---
  block/backup-top.c|  5 +-
  block/blkdebug.c  |  4 +-
  block/blklogwrites.c  |  4 +-
  block/blkreplay.c |  4 +-
  block/blkverify.c |  6 +--
  block/bochs.c |  2 +-
  block/cloop.c |  2 +-
  block/commit.c|  2 +-
  block/copy-on-read.c  |  4 +-
  block/crypto.c|  4 +-
  block/curl.c  |  2 +-
  block/dmg.c   |  2 +-
  block/file-posix.c| 18 
  block/filter-compress.c   |  6 +--
  block/io.c| 97 ---
  block/iscsi.c | 12 ++---
  block/mirror.c|  4 +-
  block/nbd.c   |  8 ++--
  block/nfs.c   |  8 ++--
  block/null.c  |  8 ++--
  block/nvme.c  |  4 +-
  block/qcow.c  | 12 ++---
  block/qcow2.c | 18 
  block/quorum.c|  8 ++--
  block/raw-format.c| 28 +--
  block/rbd.c   |  4 +-
  block/throttle.c  |  4 +-
  block/vdi.c   |  4 +-
  block/vmdk.c  |  8 ++--
  block/vpc.c   |  4 +-
  block/vvfat.c |  6 +--
  tests/test-bdrv-drain.c   |  8 ++--
  block/trace-events| 14 +++---
  35 files changed, 192 insertions(+), 192 deletions(-)




--
Best regards,
Vladimir

Re: [PATCH v2 0/6] block-copy: use aio-task-pool

[Vladimir Sementsov-Ogievskiy]

ping :)

25.03.2020 16:46, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

This is the next step of improving block-copy: use aio task pool.

Async copying loop has better performance than linear, which is shown
in original series (was
"[RFC 00/24] backup performance: block_status + async", so this is
called v2)

Vladimir Sementsov-Ogievskiy (6):
   block/block-copy: rename in-flight requests to tasks
   block/block-copy: alloc task on each iteration
   block/block-copy: add state pointer to BlockCopyTask
   block/block-copy: move task size initial calculation to _task_create
   block/block-copy: move block_copy_task_create down
   block/block-copy: use aio-task-pool API

  block/block-copy.c | 250 ++---
  1 file changed, 168 insertions(+), 82 deletions(-)




--
Best regards,
Vladimir

Re: [PATCH v2 1/4] sam460ex: Suppress useless warning on -m 32 and -m 64

[Philippe Mathieu-Daudé]

On 4/22/20 3:48 PM, Markus Armbruster wrote:

Requesting 32 or 64 MiB of RAM with the sam460ex machine type produces
a useless warning:

 qemu-system-ppc: warning: Memory size is too small for SDRAM type, 
adjusting type

This is because sam460ex_init() asks spd_data_generate() for DDR2,
which is impossible, so spd_data_generate() corrects it to DDR.

The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
EEPROM creation".

Make sam460ex_init() pass the correct SDRAM type to get rid of the
warning.

Signed-off-by: Markus Armbruster 
---
  hw/ppc/sam460ex.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 898453cf30..1e3eaac0db 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -335,7 +335,8 @@ static void sam460ex_init(MachineState *machine)
  dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
  i2c = PPC4xx_I2C(dev)->bus;
  /* SPD EEPROM on RAM module */
-spd_data = spd_data_generate(DDR2, ram_sizes[0], &err);
+spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
+ ram_sizes[0], &err);


Previous to this patch question, don't we need one for each module? We 
have 4 banks... Ah, there is a comment earlier "/* put all RAM on first 
bank because board has one slot". OK...


Reviewed-by: Philippe Mathieu-Daudé 


  if (err) {
  warn_report_err(err);
  }

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Eric Blake]

On 4/22/20 8:35 AM, Philippe Mathieu-Daudé wrote:

Hi Markus,

On 4/22/20 3:07 PM, Markus Armbruster wrote:

Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
Signed-off-by: Markus Armbruster 
---
  tests/test-logging.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6387e4933f..8580b82420 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -73,10 +73,10 @@ static void test_parse_range(void)
  g_assert(qemu_log_in_addr_range(UINT64_MAX));
  g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
-    qemu_set_dfilter_ranges("0..0x", &err);
+    qemu_set_dfilter_ranges("0..0x", &error_abort);


Why sometime use this form, ...


This call must not produce an error (if it does, the test aborts, 
proving we had a bug).





  g_assert(qemu_log_in_addr_range(0));
  g_assert(qemu_log_in_addr_range(UINT64_MAX));
-
+
  qemu_set_dfilter_ranges("2..1", &err);
  error_free_or_abort(&err);


... and then this other form?


This call must produce an error (if we do not diagnose the caller's 
error, our code is buggy, and the test must fail)


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Philippe Mathieu-Daudé]

On 4/22/20 4:27 PM, BALATON Zoltan wrote:

On Wed, 22 Apr 2020, Markus Armbruster wrote:

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

spd_data_generate() can pass @errp to error_setg() more than once when
it adjusts both memory size and type.  Harmless, because no caller
passes anything that needs adjusting.  Until the previous commit,
sam460ex passed types that needed adjusting, but not sizes.

spd_data_generate()'s contract is rather awkward:

   If everything's fine, return non-null and don't set an error.

   Else, if memory size or type need adjusting, return non-null and
   set an error describing the adjustment.

   Else, return null and set an error reporting why no data can be
   generated.

Its callers treat the error as a warning even when null is returned.
They don't create the "smbus-eeprom" device then.  Suspicious.

Since the previous commit, only "everything's fine" can actually
happen.  Drop the unused code and simplify the callers.  This gets rid
of the error API violation.


This leaves board code no chance to recover from values given by user 
that won't fit without duplicating checks that this function does. Also 
this will abort without giving meaningful errors if an invalid value 
does get through and result in a crash which is not used friendly. So I 
don't like this but if others think this is acceptable maybe at least 
unit test should be adjusted to make sure aborts cannot be triggered by 
user for values that are not usually tested during development.


Agreed. Do you have an example (or more) to better show Markus this code 
use? So we can add tests.


Personally I'd use a script to generate a dumb static array of all 
possible sizes...




Regards,
BALATON Zoltan


Signed-off-by: Markus Armbruster 
---
include/hw/i2c/smbus_eeprom.h |  2 +-
hw/i2c/smbus_eeprom.c | 30 --
hw/mips/mips_fulong2e.c   | 10 ++
hw/ppc/sam460ex.c | 12 +++-
4 files changed, 10 insertions(+), 44 deletions(-)

diff --git a/include/hw/i2c/smbus_eeprom.h 
b/include/hw/i2c/smbus_eeprom.h

index 15e2151b50..68b0063ab6 100644
--- a/include/hw/i2c/smbus_eeprom.h
+++ b/include/hw/i2c/smbus_eeprom.h
@@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
   const uint8_t *eeprom_spd, int size);

enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, 
Error **errp);

+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);

#endif
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 5adf3b15b5..07fbbf87f1 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
}

/* Generate SDRAM SPD EEPROM data describing a module of type and size */
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
-   Error **errp)
+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
{
    uint8_t *spd;
    uint8_t nbanks;
@@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type, 
ram_addr_t ram_size,

    g_assert_not_reached();
    }
    size = ram_size >> 20; /* work in terms of megabytes */
-    if (size < 4) {
-    error_setg(errp, "SDRAM size is too small");
-    return NULL;
-    }
    sz_log2 = 31 - clz32(size);
    size = 1U << sz_log2;
-    if (ram_size > size * MiB) {
-    error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power 
of 2, "

-   "truncating to %u MB", ram_size, size);
-    }
-    if (sz_log2 < min_log2) {
-    error_setg(errp,
-   "Memory size is too small for SDRAM type, 
adjusting type");

-    if (size >= 32) {
-    type = DDR;
-    min_log2 = 5;
-    max_log2 = 12;
-    } else {
-    type = SDR;
-    min_log2 = 2;
-    max_log2 = 9;
-    }
-    }
+    assert(ram_size == size * MiB);
+    assert(sz_log2 >= min_log2);

    nbanks = 1;
    while (sz_log2 > max_log2 && nbanks < 8) {
@@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, 
ram_addr_t ram_size,

    nbanks++;
    }

-    if (size > (1ULL << sz_log2) * nbanks) {
-    error_setg(errp, "Memory size is too big for SDRAM, 
truncating");

-    }
+    assert(size == (1ULL << sz_log2) * nbanks);

    /* split to 2 banks if possible to avoid a bug in MIPS Malta 
firmware */

    if (nbanks == 1 && sz_log2 > min_log2) {
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 5040afd581..ef02d54b33 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine)
    MemoryRe

Re: [RFC 0/3] 64bit block-layer part I

[Eric Blake]

On 4/22/20 9:29 AM, Vladimir Sementsov-Ogievskiy wrote:
Any thoughts here? I need to resend to update some more functions as 
patchew said.


Is it OK in general? Or should we instead convert everything to uint64_t ?


I definitely prefer int64_t as our base (off_t is signed as well, making 
63 bits an effective limit everywhere).




30.03.2020 17:18, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

There is an idea to make NBD protocol extension to support 64bit
write-zero/discard/block-status commands (commands, which doesn't
transfer user data). It's needed to increase performance of zeroing
large ranges (up to the whole image). Zeroing of the whole image is used
as first step of mirror job, qemu-img convert, it should be also used at
start of backup actually..

We need to support it in block-layer, so we want 64bit write_zeros.
Currently driver handler now have int bytes parameter.

write_zeros path goes through normal pwritev, so we need 64bit write,
and then we need 64bit read for symmetry, and better, let's make all io
path work with 64bit bytes parameter.

Actually most of block-layer already have 64bit parameters: offset is
sometimes int64_t and sometimes uint64_t. bytes parameter is one of
int64_t, uint64_t, int, unsigned int...

I think we need one type for all of this, and this one type is int64_t.
Signed int64_t is a bit better than uint64_t: you can use same variable
to get some result (including error < 0) and than reuse it as an
argument without any type conversion.

So, I propose, as a first step, convert all uint64_t parameters to
int64_t.


Makes sense.  I haven't looked at the series closely in part because it 
was 5.1 material while we were still focused on getting 5.0 out the 
door, but it is now raising in my review queue again.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>> pointer to a variable containing NULL.  Passing an argument of the
>> latter kind twice without clearing it in between is wrong: if the
>> first call sets an error, it no longer points to NULL for the second
>>
>> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
>> first to check_suspend_mode(), then to acquire_privilege(), then to
>> execute_async().  Continuing after errors here can only end in tears.
>> For instance, we risk tripping error_setv()'s assertion.
>>
>> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
>> Fixes: f54603b6aa765514b2519e74114a2f417759d727
>> Cc: Michael Roth 
>> Signed-off-by: Markus Armbruster 
>> ---
>>   qga/commands-win32.c | 14 ++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 9717a8d52d..5ba56327dd 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>> *mode = GUEST_SUSPEND_MODE_DISK;
>>   check_suspend_mode(*mode, &local_err);
>> +if (local_err) {
>> +goto out;
>> +}
>>   acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>> +if (local_err) {
>> +goto out;
>> +}
>>   execute_async(do_suspend, mode, &local_err);
>>   +out:
>>   if (local_err) {
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
> slightly different by removing the if() check.

It frees @mode unconditionally (marked --> below) I believe that's
wrong.  execute_async() runs do_suspend() in a new thread, and passes it
@mode.  do_suspend() frees it.

>>   error_propagate(errp, local_err);
>>   g_free(mode);
>> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>> *mode = GUEST_SUSPEND_MODE_RAM;
>>   check_suspend_mode(*mode, &local_err);
>> +if (local_err) {
>> +goto out;
>> +}
>>   acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>> +if (local_err) {
>> +goto out;
>> +}
>>   execute_async(do_suspend, mode, &local_err);
>>   +out:
>>   if (local_err) {
>>   error_propagate(errp, local_err);
>>   g_free(mode);
>>

   diff --git a/qga/commands-win32.c b/qga/commands-win32.c
   index b49920e201..8b66098056 100644
   --- a/qga/commands-win32.c
   +++ b/qga/commands-win32.c
   @@ -1341,13 +1341,18 @@ void qmp_guest_suspend_disk(Error **errp)

*mode = GUEST_SUSPEND_MODE_DISK;
check_suspend_mode(*mode, &local_err);
   +if (local_err) {
   +goto out;
   +}
acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
   +if (local_err) {
   +goto out;
   +}
execute_async(do_suspend, mode, &local_err);

   -if (local_err) {
   -error_propagate(errp, local_err);
   -g_free(mode);
   -}
   +out:
   +error_propagate(errp, local_err);
-->+g_free(mode);
}

void qmp_guest_suspend_ram(Error **errp)

Re: [PATCH v2 11/36] tcg: Introduce TYPE_CONST temporaries

[Alex Bennée]

Richard Henderson  writes:

> These will hold a single constant for the duration of the TB.
> They are hashed, so that each value has one temp across the TB.
>
> Not used yet, this is all infrastructure.
>
> Signed-off-by: Richard Henderson 
> ---
>  include/tcg/tcg.h |  27 ++-
>  tcg/optimize.c|  40 ++---
>  tcg/tcg-op-vec.c  |  17 +++
>  tcg/tcg.c | 111 +-
>  4 files changed, 166 insertions(+), 29 deletions(-)
>
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index 27e1b509a6..f72530dfda 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -489,6 +489,8 @@ typedef enum TCGTempKind {
>  TEMP_GLOBAL,
>  /* Temp is in a fixed register. */
>  TEMP_FIXED,
> +/* Temp is a fixed constant. */
> +TEMP_CONST,
>  } TCGTempKind;
>  
>  typedef struct TCGTemp {
> @@ -664,6 +666,7 @@ struct TCGContext {
>  QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
>  #endif
>  
> +GHashTable *const_table[TCG_TYPE_COUNT];
>  TCGTempSet free_temps[TCG_TYPE_COUNT * 2];
>  TCGTemp temps[TCG_MAX_TEMPS]; /* globals first, temps after */
>  
> @@ -680,7 +683,7 @@ struct TCGContext {
>  
>  static inline bool temp_readonly(TCGTemp *ts)
>  {
> -return ts->kind == TEMP_FIXED;
> +return ts->kind >= TEMP_FIXED;

I should have clarified in the previous patch - TEMP_FIXED is a fixed
value, e.g. env or other internal pointer which we won't be overwriting
or otherwise trashing anywhere in the block?

>  }
>  
>  extern TCGContext tcg_init_ctx;
> @@ -1038,6 +1041,7 @@ TCGOp *tcg_op_insert_after(TCGContext *s, TCGOp *op, 
> TCGOpcode opc);
>  
>  void tcg_optimize(TCGContext *s);
>  
> +/* Allocate a new temporary and initialize it with a constant. */
>  TCGv_i32 tcg_const_i32(int32_t val);
>  TCGv_i64 tcg_const_i64(int64_t val);
>  TCGv_i32 tcg_const_local_i32(int32_t val);
> @@ -1047,6 +1051,27 @@ TCGv_vec tcg_const_ones_vec(TCGType);
>  TCGv_vec tcg_const_zeros_vec_matching(TCGv_vec);
>  TCGv_vec tcg_const_ones_vec_matching(TCGv_vec);
>  
> +/*
> + * Locate or create a read-only temporary that is a constant.
> + * This kind of temporary need not and should not be freed.
> + */
> +TCGTemp *tcg_constant_internal(TCGType type, tcg_target_long val);
> +
> +static inline TCGv_i32 tcg_constant_i32(int32_t val)
> +{
> +return temp_tcgv_i32(tcg_constant_internal(TCG_TYPE_I32, val));
> +}
> +
> +static inline TCGv_i64 tcg_constant_i64(int64_t val)
> +{
> +if (TCG_TARGET_REG_BITS == 32) {
> +qemu_build_not_reached();
> +}
> +return temp_tcgv_i64(tcg_constant_internal(TCG_TYPE_I64, val));
> +}
> +
> +TCGv_vec tcg_constant_vec(TCGType type, unsigned vece, int64_t val);
> +
>  #if UINTPTR_MAX == UINT32_MAX
>  # define tcg_const_ptr(x)((TCGv_ptr)tcg_const_i32((intptr_t)(x)))
>  # define tcg_const_local_ptr(x)  
> ((TCGv_ptr)tcg_const_local_i32((intptr_t)(x)))
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index afb4a9a5a9..effb47eefd 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -99,8 +99,17 @@ static void init_ts_info(struct tcg_temp_info *infos,
>  ts->state_ptr = ti;
>  ti->next_copy = ts;
>  ti->prev_copy = ts;
> -ti->is_const = false;
> -ti->mask = -1;
> +if (ts->kind == TEMP_CONST) {
> +ti->is_const = true;
> +ti->val = ti->mask = ts->val;
> +if (TCG_TARGET_REG_BITS > 32 && ts->type == TCG_TYPE_I32) {
> +/* High bits of a 32-bit quantity are garbage.  */
> +ti->mask |= ~0xull;
> +}
> +} else {
> +ti->is_const = false;
> +ti->mask = -1;
> +}
>  set_bit(idx, temps_used->l);
>  }
>  }
> @@ -113,31 +122,28 @@ static void init_arg_info(struct tcg_temp_info *infos,
>  
>  static TCGTemp *find_better_copy(TCGContext *s, TCGTemp *ts)
>  {
> -TCGTemp *i;
> +TCGTemp *i, *g, *l;
>  
> -/* If this is already a global, we can't do better. */
> -if (ts->kind >= TEMP_GLOBAL) {
> +/* If this is already readonly, we can't do better. */
> +if (temp_readonly(ts)) {
>  return ts;
>  }
>  
> -/* Search for a global first. */
> +g = l = NULL;
>  for (i = ts_info(ts)->next_copy; i != ts; i = ts_info(i)->next_copy) {
> -if (i->kind >= TEMP_GLOBAL) {
> +if (temp_readonly(i)) {
>  return i;
> -}
> -}
> -
> -/* If it is a temp, search for a temp local. */
> -if (ts->kind == TEMP_NORMAL) {
> -for (i = ts_info(ts)->next_copy; i != ts; i = ts_info(i)->next_copy) 
> {
> -if (i->kind >= TEMP_LOCAL) {
> -return i;
> +} else if (i->kind > ts->kind) {
> +if (i->kind == TEMP_GLOBAL) {
> +g = i;
> +} else if (i->kind == TEMP_LOCAL) {
> +l = i;
>  }
>  }
>  }
>  
> -/* Failure to find a better representation, r

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> Hi Markus,
>
> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>> Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
>> Signed-off-by: Markus Armbruster 
>> ---
>>   tests/test-logging.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/test-logging.c b/tests/test-logging.c
>> index 6387e4933f..8580b82420 100644
>> --- a/tests/test-logging.c
>> +++ b/tests/test-logging.c
>> @@ -73,10 +73,10 @@ static void test_parse_range(void)
>>   g_assert(qemu_log_in_addr_range(UINT64_MAX));
>>   g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
>>   -qemu_set_dfilter_ranges("0..0x", &err);
>> +qemu_set_dfilter_ranges("0..0x", &error_abort);
>
> Why sometime use this form, ...
>
>>   g_assert(qemu_log_in_addr_range(0));
>>   g_assert(qemu_log_in_addr_range(UINT64_MAX));
>> -
>> +
>>   qemu_set_dfilter_ranges("2..1", &err);
>>   error_free_or_abort(&err);
>
> ... and then this other form?

The first form crashes when the function sets an error.

The second from crashes when the function doesn't set an error, or else
frees the error.

All clear?

[PATCH v5 2/9] block: Add flags to bdrv(_co)_truncate()

[Kevin Wolf]

Now that block drivers can support flags for .bdrv_co_truncate, expose
the parameter in the node level interfaces bdrv_co_truncate() and
bdrv_truncate().

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 include/block/block.h   |  5 +++--
 block/block-backend.c   |  2 +-
 block/crypto.c  |  2 +-
 block/io.c  | 12 +++-
 block/parallels.c   |  6 +++---
 block/qcow.c|  4 ++--
 block/qcow2-refcount.c  |  2 +-
 block/qcow2.c   | 15 +--
 block/raw-format.c  |  2 +-
 block/vhdx-log.c|  2 +-
 block/vhdx.c|  2 +-
 block/vmdk.c|  2 +-
 tests/test-block-iothread.c |  6 +++---
 13 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index b05995fe9c..8b62429aa4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -339,9 +339,10 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState 
*bs,
 void bdrv_refresh_filename(BlockDriverState *bs);
 
 int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
-  PreallocMode prealloc, Error **errp);
+  PreallocMode prealloc, BdrvRequestFlags 
flags,
+  Error **errp);
 int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
-  PreallocMode prealloc, Error **errp);
+  PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
 
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
diff --git a/block/block-backend.c b/block/block-backend.c
index 38ae413826..8be20060d3 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2144,7 +2144,7 @@ int blk_truncate(BlockBackend *blk, int64_t offset, bool 
exact,
 return -ENOMEDIUM;
 }
 
-return bdrv_truncate(blk->root, offset, exact, prealloc, errp);
+return bdrv_truncate(blk->root, offset, exact, prealloc, 0, errp);
 }
 
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
diff --git a/block/crypto.c b/block/crypto.c
index 3721a8495c..ab33545c92 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -313,7 +313,7 @@ block_crypto_co_truncate(BlockDriverState *bs, int64_t 
offset, bool exact,
 
 offset += payload_offset;
 
-return bdrv_co_truncate(bs->file, offset, exact, prealloc, errp);
+return bdrv_co_truncate(bs->file, offset, exact, prealloc, 0, errp);
 }
 
 static void block_crypto_close(BlockDriverState *bs)
diff --git a/block/io.c b/block/io.c
index 04ac5cf023..795075954e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3339,12 +3339,12 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs)
  * 'offset' bytes in length.
  */
 int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
-  PreallocMode prealloc, Error **errp)
+  PreallocMode prealloc, BdrvRequestFlags 
flags,
+  Error **errp)
 {
 BlockDriverState *bs = child->bs;
 BlockDriver *drv = bs->drv;
 BdrvTrackedRequest req;
-BdrvRequestFlags flags = 0;
 int64_t old_size, new_bytes;
 int ret;
 
@@ -3402,7 +3402,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, 
int64_t offset, bool exact,
 }
 ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, flags, errp);
 } else if (bs->file && drv->is_filter) {
-ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, errp);
+ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp);
 } else {
 error_setg(errp, "Image format driver does not support resize");
 ret = -ENOTSUP;
@@ -3435,6 +3435,7 @@ typedef struct TruncateCo {
 int64_t offset;
 bool exact;
 PreallocMode prealloc;
+BdrvRequestFlags flags;
 Error **errp;
 int ret;
 } TruncateCo;
@@ -3443,12 +3444,12 @@ static void coroutine_fn bdrv_truncate_co_entry(void 
*opaque)
 {
 TruncateCo *tco = opaque;
 tco->ret = bdrv_co_truncate(tco->child, tco->offset, tco->exact,
-tco->prealloc, tco->errp);
+tco->prealloc, tco->flags, tco->errp);
 aio_wait_kick();
 }
 
 int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
-  PreallocMode prealloc, Error **errp)
+  PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
 {
 Coroutine *co;
 TruncateCo tco = {
@@ -3456,6 +3457,7 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, bool 
exact,
 .offset = offset,
 .exact  = exact,
 .prealloc   = prealloc,
+.flags  = flags,
 .errp   = errp,
 .ret= NOT_DONE,
 };
diff --git a/block/parallels.c b/block/parallels.c
index 6d4ed77f16..2be92cf417

[PATCH v5 1/9] block: Add flags to BlockDriver.bdrv_co_truncate()

[Kevin Wolf]

This adds a new BdrvRequestFlags parameter to the .bdrv_co_truncate()
driver callbacks, and a supported_truncate_flags field in
BlockDriverState that allows drivers to advertise support for request
flags in the context of truncate.

For now, we always pass 0 and no drivers declare support for any flag.

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 include/block/block_int.h   | 10 +-
 block/crypto.c  |  3 ++-
 block/file-posix.c  |  2 +-
 block/file-win32.c  |  2 +-
 block/gluster.c |  1 +
 block/io.c  |  8 +++-
 block/iscsi.c   |  2 +-
 block/nfs.c |  3 ++-
 block/qcow2.c   |  2 +-
 block/qed.c |  1 +
 block/raw-format.c  |  2 +-
 block/rbd.c |  1 +
 block/sheepdog.c|  4 ++--
 block/ssh.c |  2 +-
 tests/test-block-iothread.c |  3 ++-
 15 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4c3587ea19..92335f33c7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -355,7 +355,7 @@ struct BlockDriver {
  */
 int coroutine_fn (*bdrv_co_truncate)(BlockDriverState *bs, int64_t offset,
  bool exact, PreallocMode prealloc,
- Error **errp);
+ BdrvRequestFlags flags, Error **errp);
 
 int64_t (*bdrv_getlength)(BlockDriverState *bs);
 bool has_variable_length;
@@ -847,6 +847,14 @@ struct BlockDriverState {
 /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
  * BDRV_REQ_MAY_UNMAP, BDRV_REQ_WRITE_UNCHANGED) */
 unsigned int supported_zero_flags;
+/*
+ * Flags honoured during truncate (so far: BDRV_REQ_ZERO_WRITE).
+ *
+ * If BDRV_REQ_ZERO_WRITE is given, the truncate operation must make sure
+ * that any added space reads as all zeros. If this can't be guaranteed,
+ * the operation must fail.
+ */
+unsigned int supported_truncate_flags;
 
 /* the following member gives a name to every node on the bs graph. */
 char node_name[32];
diff --git a/block/crypto.c b/block/crypto.c
index d577f89659..3721a8495c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -299,7 +299,8 @@ static int block_crypto_co_create_generic(BlockDriverState 
*bs,
 
 static int coroutine_fn
 block_crypto_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp)
+ PreallocMode prealloc, BdrvRequestFlags flags,
+ Error **errp)
 {
 BlockCrypto *crypto = bs->opaque;
 uint64_t payload_offset =
diff --git a/block/file-posix.c b/block/file-posix.c
index 7e19bbff5f..53f475ed61 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2080,7 +2080,7 @@ raw_regular_truncate(BlockDriverState *bs, int fd, 
int64_t offset,
 
 static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
 bool exact, PreallocMode prealloc,
-Error **errp)
+BdrvRequestFlags flags, Error **errp)
 {
 BDRVRawState *s = bs->opaque;
 struct stat st;
diff --git a/block/file-win32.c b/block/file-win32.c
index 15859839a1..a6b0dda5c3 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -469,7 +469,7 @@ static void raw_close(BlockDriverState *bs)
 
 static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
 bool exact, PreallocMode prealloc,
-Error **errp)
+BdrvRequestFlags flags, Error **errp)
 {
 BDRVRawState *s = bs->opaque;
 LONG low, high;
diff --git a/block/gluster.c b/block/gluster.c
index 0aa1f2cda4..d06df900f6 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1228,6 +1228,7 @@ static coroutine_fn int 
qemu_gluster_co_truncate(BlockDriverState *bs,
  int64_t offset,
  bool exact,
  PreallocMode prealloc,
+ BdrvRequestFlags flags,
  Error **errp)
 {
 BDRVGlusterState *s = bs->opaque;
diff --git a/block/io.c b/block/io.c
index aba67f66b9..04ac5cf023 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3344,6 +3344,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, 
int64_t offset, bool exact,
 BlockDriverState *bs = child->bs;
 BlockDriver *drv = bs->drv;
 BdrvTrackedRequest req;
+BdrvRequestFlags flags = 0;
 int64_t old_size, new_bytes;
 int ret;
 
@@ -3394,7 +3395,12 @@ int coroutine_

[PATCH v5 0/9] block: Fix resize (extending) of short overlays

[Kevin Wolf]

v5:
- Fixed file-win32 [Patchew]
- Fixed zeroing in qcow2 for unaligned requests + tests [Vladimir]
- Made raw-format code more consistent [Eric]
- Leave output for existing iotests cases unchanged [Vladimir]

v4:
- Rewrote the series to move the actual zeroing to the block drivers so
  that error paths can work correctly and potentially long-running
  fallback to writing explicit zeroes is avoided.
- Fixed output filtering order in the test case [Max]

v3:
- Don't allow blocking the monitor for a zero write in block_resize
  (even though we can already blockfor other reasons there). This is
  mainly responsible for the increased complexity compared to v2.
  Personally, I think this is not an improvement over v2, but if this is
  what it takes to fix a corruption issue in 4.2... [Max]
- Don't use huge image files in the test case [Vladimir]

v2:
- Switched order of bs->total_sectors update and zero write [Vladimir]
- Fixed coding style [Vladimir]
- Changed the commit message to contain what was in the cover letter
- Test all preallocation modes
- Test allocation status with qemu-io 'map' [Vladimir]

Kevin Wolf (9):
  block: Add flags to BlockDriver.bdrv_co_truncate()
  block: Add flags to bdrv(_co)_truncate()
  block-backend: Add flags to blk_truncate()
  qcow2: Support BDRV_REQ_ZERO_WRITE for truncate
  raw-format: Support BDRV_REQ_ZERO_WRITE for truncate
  file-posix: Support BDRV_REQ_ZERO_WRITE for truncate
  block: truncate: Don't make backing file data visible
  iotests: Filter testfiles out in filter_img_info()
  iotests: Test committing to short backing file

 include/block/block.h  |   5 +-
 include/block/block_int.h  |  10 +-
 include/sysemu/block-backend.h |   2 +-
 block.c|   3 +-
 block/block-backend.c  |   4 +-
 block/commit.c |   4 +-
 block/crypto.c |   7 +-
 block/file-posix.c |   6 +-
 block/file-win32.c |   2 +-
 block/gluster.c|   1 +
 block/io.c |  32 +++-
 block/iscsi.c  |   2 +-
 block/mirror.c |   2 +-
 block/nfs.c|   3 +-
 block/parallels.c  |   6 +-
 block/qcow.c   |   4 +-
 block/qcow2-refcount.c |   2 +-
 block/qcow2.c  |  51 +--
 block/qed.c|   3 +-
 block/raw-format.c |   6 +-
 block/rbd.c|   1 +
 block/sheepdog.c   |   4 +-
 block/ssh.c|   2 +-
 block/vdi.c|   2 +-
 block/vhdx-log.c   |   2 +-
 block/vhdx.c   |   6 +-
 block/vmdk.c   |   8 +-
 block/vpc.c|   2 +-
 blockdev.c |   2 +-
 qemu-img.c |   2 +-
 qemu-io-cmds.c |   2 +-
 tests/test-block-iothread.c|   9 +-
 tests/qemu-iotests/iotests.py  |   5 +-
 tests/qemu-iotests/274 | 157 
 tests/qemu-iotests/274.out | 260 +
 tests/qemu-iotests/group   |   1 +
 36 files changed, 558 insertions(+), 62 deletions(-)
 create mode 100755 tests/qemu-iotests/274
 create mode 100644 tests/qemu-iotests/274.out

-- 
2.25.3

[PATCH v5 6/9] file-posix: Support BDRV_REQ_ZERO_WRITE for truncate

[Kevin Wolf]

For regular files, we always get BDRV_REQ_ZERO_WRITE behaviour from the
OS, so we can advertise the flag and just ignore it.

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/file-posix.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 53f475ed61..1dca220a81 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -702,6 +702,10 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 #endif
 
 bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK;
+if (S_ISREG(st.st_mode)) {
+/* When extending regular files, we get zeros from the OS */
+bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
+}
 ret = 0;
 fail:
 if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
-- 
2.25.3

[PATCH v5 3/9] block-backend: Add flags to blk_truncate()

[Kevin Wolf]

Now that node level interface bdrv_truncate() supports passing request
flags to the block driver, expose this on the BlockBackend level, too.

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 include/sysemu/block-backend.h | 2 +-
 block.c| 3 ++-
 block/block-backend.c  | 4 ++--
 block/commit.c | 4 ++--
 block/crypto.c | 2 +-
 block/mirror.c | 2 +-
 block/qcow2.c  | 4 ++--
 block/qed.c| 2 +-
 block/vdi.c| 2 +-
 block/vhdx.c   | 4 ++--
 block/vmdk.c   | 6 +++---
 block/vpc.c| 2 +-
 blockdev.c | 2 +-
 qemu-img.c | 2 +-
 qemu-io-cmds.c | 2 +-
 15 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9bbdbd63d7..34de7faa81 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -237,7 +237,7 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, 
int64_t offset,
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
   int bytes);
 int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp);
+ PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
 int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
  int64_t pos, int size);
diff --git a/block.c b/block.c
index 2e3905c99e..03cc5813a2 100644
--- a/block.c
+++ b/block.c
@@ -548,7 +548,8 @@ static int64_t create_file_fallback_truncate(BlockBackend 
*blk,
 int64_t size;
 int ret;
 
-ret = blk_truncate(blk, minimum_size, false, PREALLOC_MODE_OFF, 
&local_err);
+ret = blk_truncate(blk, minimum_size, false, PREALLOC_MODE_OFF, 0,
+   &local_err);
 if (ret < 0 && ret != -ENOTSUP) {
 error_propagate(errp, local_err);
 return ret;
diff --git a/block/block-backend.c b/block/block-backend.c
index 8be20060d3..17ed6d8c5b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2137,14 +2137,14 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t 
offset, const void *buf,
 }
 
 int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp)
+ PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
 {
 if (!blk_is_available(blk)) {
 error_setg(errp, "No medium inserted");
 return -ENOMEDIUM;
 }
 
-return bdrv_truncate(blk->root, offset, exact, prealloc, 0, errp);
+return bdrv_truncate(blk->root, offset, exact, prealloc, flags, errp);
 }
 
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
diff --git a/block/commit.c b/block/commit.c
index 8e672799af..87f6096d90 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -133,7 +133,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
 }
 
 if (base_len < len) {
-ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, NULL);
+ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, 0, NULL);
 if (ret) {
 goto out;
 }
@@ -458,7 +458,7 @@ int bdrv_commit(BlockDriverState *bs)
  * grow the backing file image if possible.  If not possible,
  * we must return an error */
 if (length > backing_length) {
-ret = blk_truncate(backing, length, false, PREALLOC_MODE_OFF,
+ret = blk_truncate(backing, length, false, PREALLOC_MODE_OFF, 0,
&local_err);
 if (ret < 0) {
 error_report_err(local_err);
diff --git a/block/crypto.c b/block/crypto.c
index ab33545c92..e02f343590 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -115,7 +115,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
  * which will be used by the crypto header
  */
 return blk_truncate(data->blk, data->size + headerlen, false,
-data->prealloc, errp);
+data->prealloc, 0, errp);
 }
 
 
diff --git a/block/mirror.c b/block/mirror.c
index c26fd9260d..aca95c9bc9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -900,7 +900,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
 
 if (s->bdev_length > base_length) {
 ret = blk_truncate(s->target, s->bdev_length, false,
-   PREALLOC_MODE_OFF, NULL);
+   PREALLOC_MODE_OFF, 0, NULL);
 if (ret < 0) {
 goto immediate_exit;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index c5b0711357..9cfbdfc939 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3511,7 +3511,7 @@ qcow2_co_create(BlockdevCreateOptions *cre

[PATCH v5 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

[Kevin Wolf]

If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling
qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't
undo any previous preallocation, but just adds the zero flag to all
relevant L2 entries. If an external data file is in use, a write_zeroes
request to the data file is made instead.

Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 9cfbdfc939..bd632405d1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1726,6 +1726,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 
 bs->supported_zero_flags = header.version >= 3 ?
BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0;
+bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
 
 /* Repair image if dirty */
 if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only &&
@@ -4214,6 +4215,35 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 g_assert_not_reached();
 }
 
+if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
+uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size);
+uint64_t zero_end = QEMU_ALIGN_UP(offset, s->cluster_size);
+
+/* Use zero clusters as much as we can */
+ret = qcow2_cluster_zeroize(bs, zero_start, zero_end - zero_start, 0);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to zero out the new area");
+goto fail;
+}
+
+/* Write explicit zeros for the unaligned head */
+if (zero_start > old_length) {
+uint8_t *buf = qemu_blockalign0(bs, s->cluster_size);
+QEMUIOVector qiov;
+qemu_iovec_init_buf(&qiov, buf, zero_start - old_length);
+
+qemu_co_mutex_unlock(&s->lock);
+ret = qcow2_co_pwritev_part(bs, old_length, qiov.size, &qiov, 0, 
0);
+qemu_co_mutex_lock(&s->lock);
+
+qemu_vfree(buf);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to zero out the new 
area");
+goto fail;
+}
+}
+}
+
 if (prealloc != PREALLOC_MODE_OFF) {
 /* Flush metadata before actually changing the image size */
 ret = qcow2_write_caches(bs);
-- 
2.25.3

[PATCH v5 5/9] raw-format: Support BDRV_REQ_ZERO_WRITE for truncate

[Kevin Wolf]

The raw format driver can simply forward the flag and let its bs->file
child take care of actually providing the zeros.

Signed-off-by: Kevin Wolf 
---
 block/raw-format.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index 3465c9a865..351f2d91c6 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -387,7 +387,7 @@ static int coroutine_fn raw_co_truncate(BlockDriverState 
*bs, int64_t offset,
 
 s->size = offset;
 offset += s->offset;
-return bdrv_co_truncate(bs->file, offset, exact, prealloc, 0, errp);
+return bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp);
 }
 
 static void raw_eject(BlockDriverState *bs, bool eject_flag)
@@ -445,6 +445,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
 ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
 bs->file->bs->supported_zero_flags);
+bs->supported_truncate_flags = bs->file->bs->supported_truncate_flags &
+   BDRV_REQ_ZERO_WRITE;
 
 if (bs->probed && !bdrv_is_read_only(bs)) {
 bdrv_refresh_filename(bs->file->bs);
-- 
2.25.3

[PATCH v5 9/9] iotests: Test committing to short backing file

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/274 | 157 ++
 tests/qemu-iotests/274.out | 260 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 418 insertions(+)
 create mode 100755 tests/qemu-iotests/274
 create mode 100644 tests/qemu-iotests/274.out

diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
new file mode 100755
index 00..8bf7ff3122
--- /dev/null
+++ b/tests/qemu-iotests/274
@@ -0,0 +1,157 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+# Creator/Owner: Kevin Wolf 
+#
+# Some tests for short backing files and short overlays
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_platform(['linux'])
+
+size_short = 1 * 1024 * 1024
+size_long = 2 * 1024 * 1024
+size_diff = size_long - size_short
+
+def create_chain():
+iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
+ str(size_long))
+iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid,
+ str(size_short))
+iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top,
+ str(size_long))
+
+iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
+
+def create_vm():
+vm = iotests.VM()
+vm.add_blockdev('file,filename=%s,node-name=base-file' % (base))
+vm.add_blockdev('%s,file=base-file,node-name=base' % (iotests.imgfmt))
+vm.add_blockdev('file,filename=%s,node-name=mid-file' % (mid))
+vm.add_blockdev('%s,file=mid-file,node-name=mid,backing=base' % 
(iotests.imgfmt))
+vm.add_drive(top, 'backing=mid,node-name=top')
+return vm
+
+with iotests.FilePath('base') as base, \
+ iotests.FilePath('mid') as mid, \
+ iotests.FilePath('top') as top:
+
+iotests.log('== Commit tests ==')
+
+create_chain()
+
+iotests.log('=== Check visible data ===')
+
+iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, top)
+iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), top)
+
+iotests.log('=== Checking allocation status ===')
+
+iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+'-c', 'alloc %d %d' % (size_short, size_diff),
+base)
+
+iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+'-c', 'alloc %d %d' % (size_short, size_diff),
+mid)
+
+iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+'-c', 'alloc %d %d' % (size_short, size_diff),
+top)
+
+iotests.log('=== Checking map ===')
+
+iotests.qemu_img_log('map', '--output=json', base)
+iotests.qemu_img_log('map', '--output=human', base)
+iotests.qemu_img_log('map', '--output=json', mid)
+iotests.qemu_img_log('map', '--output=human', mid)
+iotests.qemu_img_log('map', '--output=json', top)
+iotests.qemu_img_log('map', '--output=human', top)
+
+iotests.log('=== Testing qemu-img commit (top -> mid) ===')
+
+iotests.qemu_img_log('commit', top)
+iotests.img_info_log(mid)
+iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+iotests.log('=== Testing HMP commit (top -> mid) ===')
+
+create_chain()
+with create_vm() as vm:
+vm.launch()
+vm.qmp_log('human-monitor-command', command_line='commit drive0')
+
+iotests.img_info_log(mid)
+iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+iotests.log('=== Testing QMP active commit (top -> mid) ===')
+
+create_chain()
+with create_vm() as vm:
+vm.launch()
+vm.qmp_log('block-commit', device='top', base_node='mid',
+   job_id='job0', auto_dismiss=False)
+vm.run_job('job0', wait=5)
+
+iotests.img_info_log(mid)
+iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+
+iotests.log('== Resize tests ==')
+
+# Use different sizes for different allocation modes:
+#
+# We want to have at least one test where 32 bit tru

[PATCH v5 8/9] iotests: Filter testfiles out in filter_img_info()

[Kevin Wolf]

We want to keep TEST_IMG for the full path of the main test image, but
filter_testfiles() must be called for other test images before replacing
other things like the image format because the test directory path could
contain the format as a substring.

Insert a filter_testfiles() call between both.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7bc4934cd2..5f8c263d59 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -338,8 +338,9 @@ def filter_img_info(output, filename):
 for line in output.split('\n'):
 if 'disk size' in line or 'actual-size' in line:
 continue
-line = line.replace(filename, 'TEST_IMG') \
-   .replace(imgfmt, 'IMGFMT')
+line = line.replace(filename, 'TEST_IMG')
+line = filter_testfiles(line)
+line = line.replace(imgfmt, 'IMGFMT')
 line = re.sub('iters: [0-9]+', 'iters: XXX', line)
 line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
----', line)
 line = re.sub('cid: [0-9]+', 'cid: XX', line)
-- 
2.25.3

Re: RFC: use VFIO over a UNIX domain socket to implement device offloading

[Stefan Hajnoczi]

On Mon, Apr 20, 2020 at 11:05:25AM +, Thanos Makatos wrote:
> > In order to interoperate we'll need to maintain a protocol
> > specification.  Mayb You and JJ could put that together and CC the vfio,
> > rust-vmm, and QEMU communities for discussion?
> > 
> > It should cover the UNIX domain socket connection semantics (does a
> > listen socket only accept 1 connection at a time?  What happens when the
> > client disconnects?  What happens when the server disconnects?), how
> > VFIO structs are exchanged, any vfio-over-socket specific protocol
> > messages, etc.  Basically everything needed to write an implementation
> > (although it's not necessary to copy the VFIO struct definitions from
> > the kernel headers into the spec or even document their semantics if
> > they are identical to kernel VFIO).
> > 
> > The next step beyond the LD_PRELOAD library is a native vfio-over-socket
> > client implementation in QEMU.  There is a prototype here:
> > https://github.com/elmarco/qemu/blob/wip/vfio-user/hw/vfio/libvfio-
> > user.c
> > 
> > If there are any volunteers for working on that then this would be a
> > good time to discuss it.
> 
> Hi,
> 
> I've just shared with you the Google doc we've working on with John where 
> we've
> been drafting the protocol specification, we think it's time for some first
> comments. Please feel free to comment/edit and suggest more people to be on 
> the
> reviewers list.
> 
> You can also find the Google doc here:
> 
> https://docs.google.com/document/d/1FspkL0hVEnZqHbdoqGLUpyC38rSk_7HhY471TsVwyK8/edit?usp=sharing
> 
> If a Google doc doesn't work for you we're open to suggestions.

I can't add comments to the document so I've inlined them here:

The spec assumes the reader is already familiar with VFIO and does not
explain concepts like the device lifecycle, regions, interrupts, etc.
We don't need to duplicate detailed VFIO information, but I think the
device model should be explained so that anyone can start from the
VFIO-user spec and begin working on an implementation.  Right now I
think they would have to do some serious investigation of VFIO first in
order to be able to write code.

"only the source header files are used"
I notice the current  header is licensed "GPL-2.0 WITH
Linux-syscall-note".  I'm not a lawyer but I guess this means there are
some restrictions on using this header file.  The 
header files were explicitly licensed under the BSD license to make it
easy to use the non __KERNEL__ parts.

VFIO-user Command Types: please indicate for each request type whether
it is client->server, server->client, or both.  Also is it a "command"
or "request"?

vfio_user_req_type <-- is this an extension on top of ?
Please make it clear what is part of the base  protocol
and what is specific to vfio-user.

VFIO_USER_READ/WRITE serve completely different purposes depending on
whether they are sent client->server or server->client.  I suggest
defining separate request type constants instead of overloading them.

What is the difference between VFIO_USER_MAP_DMA and VFIO_USER_REG_MEM?
They both seem to be client->server messages for setting up memory but
I'm not sure why two request types are needed.

struct vfio_user_req->data.  Is this really a union so that every
message has the same size, regardless of how many parameters are passed
in the data field?

"a framebuffer where the guest does multiple stores to the virtual
device."  Do you mean in SMP guests?  Or even in a single CPU guest?

Also, is there any concurrency requirement on the client and server
side?  Can I implement a client/server that processes requests
sequentially and completes them before moving on to the next request or
would that deadlock for certain message types?


signature.asc
Description: PGP signature

[PATCH v5 7/9] block: truncate: Don't make backing file data visible

[Kevin Wolf]

When extending the size of an image that has a backing file larger than
its old size, make sure that the backing file data doesn't become
visible in the guest, but the added area is properly zeroed out.

Consider the following scenario where the overlay is shorter than its
backing file:

base.qcow2: 
overlay.qcow2:  

When resizing (extending) overlay.qcow2, the new blocks should not stay
unallocated and make the additional As from base.qcow2 visible like
before this patch, but zeros should be read.

A similar case happens with the various variants of a commit job when an
intermediate file is short (- for unallocated):

base.qcow2: A-A-
mid.qcow2:  BB-B
top.qcow2:  C--C--C-

After commit top.qcow2 to mid.qcow2, the following happens:

mid.qcow2:  CB-C00C0 (correct result)
mid.qcow2:  CB-C--C- (before this fix)

Without the fix, blocks that previously read as zeros on top.qcow2
suddenly turn into A.

Signed-off-by: Kevin Wolf 
Reviewed-by: Alberto Garcia 
---
 block/io.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block/io.c b/block/io.c
index 795075954e..8fbb607515 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3394,6 +3394,20 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, 
int64_t offset, bool exact,
 goto out;
 }
 
+/*
+ * If the image has a backing file that is large enough that it would
+ * provide data for the new area, we cannot leave it unallocated because
+ * then the backing file content would become visible. Instead, zero-fill
+ * the new area.
+ *
+ * Note that if the image has a backing file, but was opened without the
+ * backing file, taking care of keeping things consistent with that backing
+ * file is the user's responsibility.
+ */
+if (new_bytes && bs->backing) {
+flags |= BDRV_REQ_ZERO_WRITE;
+}
+
 if (drv->bdrv_co_truncate) {
 if (flags & ~bs->supported_truncate_flags) {
 error_setg(errp, "Block driver does not support requested flags");
-- 
2.25.3

Re: [RFC PATCH v2 1/5] crypto/secret: rename to secret_interface.

[Daniel P . Berrangé]

On Thu, Apr 16, 2020 at 01:25:21AM +0300, Alexey Krasikov wrote:
> * Rename for future division into subclasses. Most part of the interface
>   will remain in basic common class.

You don't need to put bullet points in the commit message, just
have the text.

> 
> Signed-off-by: Alexey Krasikov 
> ---
>  crypto/{secret.c => secret_interface.c} | 0
>  include/crypto/{secret.h => secret_interface.h} | 0
>  2 files changed, 0 insertions(+), 0 deletions(-)
>  rename crypto/{secret.c => secret_interface.c} (100%)
>  rename include/crypto/{secret.h => secret_interface.h} (100%)

This breaks the build because Makefile.objs doesn't reference
the new filename, and likewise other files doing #include
don't work.

I don't think renaming actually makes sense in the first place,
because you then add the original files back again in a later
patch.

You need to just have a patch which introduces secret_interface.{ch}
without killing the original secret.{c,h} entirely.  The key point is
that QEMU must successfully compile on each individual patch in the
series, otherwise it breaks "git bisect" usage.

Also, since the object is called "SecretCommon",the filenames
should match that "secret_common.{ch}"


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v5 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

[Eric Blake]

On 4/22/20 10:21 AM, Kevin Wolf wrote:

If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling
qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't
undo any previous preallocation, but just adds the zero flag to all
relevant L2 entries. If an external data file is in use, a write_zeroes
request to the data file is made instead.

Signed-off-by: Kevin Wolf 
---
  block/qcow2.c | 30 ++
  1 file changed, 30 insertions(+)




@@ -4214,6 +4215,35 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
  g_assert_not_reached();
  }
  
+if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {

+uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size);
+uint64_t zero_end = QEMU_ALIGN_UP(offset, s->cluster_size);


This rounds up beyond the new size...


+
+/* Use zero clusters as much as we can */
+ret = qcow2_cluster_zeroize(bs, zero_start, zero_end - zero_start, 0);


and then requests that the extra be zeroed.  Does that always work, even 
when it results in pdrv_co_pwrite_zeroes beyond the end of s->data_file? 
 If so,


Reviewed-by: Eric Blake 

otherwise, you may have to treat the tail specially, the same way you 
treated an unaligned head.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH RFC] target/arm: Implement SVE2 gather load insns

[Stephen Long]

Add decoding logic for SVE2 64-bit/32-bit gather non-temporal load
insns.

64-bit
* LDNT1SB
* LDNT1B (vector plus scalar)
* LDNT1SH
* LDNT1H (vector plus scalar)
* LDNT1SW
* LDNT1W (vector plus scalar)
* LDNT1D (vector plus scalar)

32-bit
* LDNT1SB
* LDNT1B (vector plus scalar)
* LDNT1SH
* LDNT1H (vector plus scalar)
* LDNT1W (vector plus scalar)

Signed-off-by: Stephen Long 

I'm not sure I'm initializing xs correctly. This also goes for the
scatter store insns in the previous patch.
---
 target/arm/sve.decode  | 11 +++
 target/arm/translate-sve.c |  8 
 2 files changed, 19 insertions(+)

diff --git a/target/arm/sve.decode b/target/arm/sve.decode
index ef5dd281a6..d7799746ab 100644
--- a/target/arm/sve.decode
+++ b/target/arm/sve.decode
@@ -1388,6 +1388,17 @@ SQRDCMLAH_  01000100 esz:2 0 rm:5 0011 rot:2 rn:5 
rd:5  ra=%reg_movprfx
 
 FMMLA   01100100 .. 1 . 111001 . .  @rda_rn_rm
 
+### SVE2 Memory Gather Load Group
+
+# SVE2 64-bit gather non-temporal load
+#   (scalar plus unpacked 32-bit unscaled offsets)
+LDNT1_zprz  1100010 msz:2 00 rm:5 1 u:1 0 pg:3 rn:5 rd:5 \
+&rprr_gather_load xs=0 esz=3 scale=0 ff=0
+
+# SVE2 32-bit gather non-temporal load (scalar plus 32-bit unscaled offsets)
+LDNT1_zprz  110 msz:2 00 rm:5 10 u:1 pg:3 rn:5 rd:5 \
+&rprr_gather_load xs=0 esz=2 scale=0 ff=0
+
 ### SVE2 Memory Store Group
 
 # SVE2 64-bit scatter non-temporal store (vector plus scalar)
diff --git a/target/arm/translate-sve.c b/target/arm/translate-sve.c
index 4873e25182..bdabb89e82 100644
--- a/target/arm/translate-sve.c
+++ b/target/arm/translate-sve.c
@@ -5886,6 +5886,14 @@ static bool trans_LD1_zpiz(DisasContext *s, arg_LD1_zpiz 
*a)
 return true;
 }
 
+static bool trans_LDNT1_zprz(DisasContext *s, arg_LD1_zprz *a)
+{
+if (!dc_isar_feature(aa64_sve2, s)) {
+return false;
+}
+return trans_LDNT1_zprz(s, a);
+}
+
 /* Indexed by [mte][be][xs][msz].  */
 static gen_helper_gvec_mem_scatter * const scatter_store_fn32[2][2][2][3] = {
 { /* MTE Inactive */
-- 
2.17.1

Re: [PATCH v5 5/9] raw-format: Support BDRV_REQ_ZERO_WRITE for truncate

[Eric Blake]

On 4/22/20 10:21 AM, Kevin Wolf wrote:

The raw format driver can simply forward the flag and let its bs->file
child take care of actually providing the zeros.

Signed-off-by: Kevin Wolf 
---
  block/raw-format.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)



Reviewed-by: Eric Blake 


diff --git a/block/raw-format.c b/block/raw-format.c
index 3465c9a865..351f2d91c6 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -387,7 +387,7 @@ static int coroutine_fn raw_co_truncate(BlockDriverState 
*bs, int64_t offset,
  
  s->size = offset;

  offset += s->offset;
-return bdrv_co_truncate(bs->file, offset, exact, prealloc, 0, errp);
+return bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp);
  }
  
  static void raw_eject(BlockDriverState *bs, bool eject_flag)

@@ -445,6 +445,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
  bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
  ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
  bs->file->bs->supported_zero_flags);
+bs->supported_truncate_flags = bs->file->bs->supported_truncate_flags &
+   BDRV_REQ_ZERO_WRITE;
  
  if (bs->probed && !bdrv_is_read_only(bs)) {

  bdrv_refresh_filename(bs->file->bs);



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/3] block: use int64_t as bytes type in tracked requests

[Stefan Hajnoczi]

On Mon, Mar 30, 2020 at 05:18:16PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes paramaters
> on all io paths. Convert tracked requests now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block_int.h |  4 ++--
>  block/io.c| 11 ++-
>  2 files changed, 8 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH v2 12/36] tcg: Use tcg_constant_i32 with icount expander

[Alex Bennée]

Richard Henderson  writes:

> We must do this before we adjust how tcg_out_movi_i32,
> lest the under-the-hood poking that we do be broken.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  include/exec/gen-icount.h | 25 +
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index 822c43cfd3..404732518a 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -34,7 +34,7 @@ static inline void gen_io_end(void)
>  
>  static inline void gen_tb_start(TranslationBlock *tb)
>  {
> -TCGv_i32 count, imm;
> +TCGv_i32 count;
>  
>  tcg_ctx->exitreq_label = gen_new_label();
>  if (tb_cflags(tb) & CF_USE_ICOUNT) {
> @@ -48,15 +48,13 @@ static inline void gen_tb_start(TranslationBlock *tb)
> offsetof(ArchCPU, env));
>  
>  if (tb_cflags(tb) & CF_USE_ICOUNT) {
> -imm = tcg_temp_new_i32();
> -/* We emit a movi with a dummy immediate argument. Keep the insn 
> index
> - * of the movi so that we later (when we know the actual insn count)
> - * can update the immediate argument with the actual insn count.  */
> -tcg_gen_movi_i32(imm, 0xdeadbeef);
> +/*
> + * We emit a sub with a dummy immediate argument. Keep the insn index
> + * of the sub so that we later (when we know the actual insn count)
> + * can update the argument with the actual insn count.
> + */
> +tcg_gen_sub_i32(count, count, tcg_constant_i32(0));
>  icount_start_insn = tcg_last_op();
> -
> -tcg_gen_sub_i32(count, count, imm);
> -tcg_temp_free_i32(imm);
>  }
>  
>  tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
> @@ -74,9 +72,12 @@ static inline void gen_tb_start(TranslationBlock *tb)
>  static inline void gen_tb_end(TranslationBlock *tb, int num_insns)
>  {
>  if (tb_cflags(tb) & CF_USE_ICOUNT) {
> -/* Update the num_insn immediate parameter now that we know
> - * the actual insn count.  */
> -tcg_set_insn_param(icount_start_insn, 1, num_insns);
> +/*
> + * Update the num_insn immediate parameter now that we know
> + * the actual insn count.
> + */
> +tcg_set_insn_param(icount_start_insn, 2,
> +   tcgv_i32_arg(tcg_constant_i32(num_insns)));
>  }
>  
>  gen_set_label(tcg_ctx->exitreq_label);


-- 
Alex Bennée

Re: [RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' secret object.

[Daniel P . Berrangé]

On Thu, Apr 16, 2020 at 01:25:24AM +0300, Alexey Krasikov wrote:
> * Add the ability for the secret object to obtain secret data from the
>   Linux in-kernel key managment and retention facility, as an extra option
>   to the existing ones: reading from a file or passing directly as a
>   string.
> 
>   The secret is identified by the key serial number.  The upper layers
>   need to instantiate the key and make sure the QEMU process has access
>   rights to read it.
> 
> Signed-off-by: Alexey Krasikov 
> ---
>  crypto/Makefile.objs   |   1 +
>  crypto/linux_keyring.c | 140 +
>  include/crypto/linux_keyring.h |  38 +
>  3 files changed, 179 insertions(+)
>  create mode 100644 crypto/linux_keyring.c
>  create mode 100644 include/crypto/linux_keyring.h

Can we call these  secret_keyring.{c,h}

> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> index 3ae0dfd1a4..7fc354a8d5 100644
> --- a/crypto/Makefile.objs
> +++ b/crypto/Makefile.objs
> @@ -19,6 +19,7 @@ crypto-obj-y += tlscredspsk.o
>  crypto-obj-y += tlscredsx509.o
>  crypto-obj-y += tlssession.o
>  crypto-obj-y += secret_interface.o
> +crypto-obj-y += linux_keyring.o

I'd expect to see a configure check for deciding whether or not
to build the keyring code, and then have $(CONFIG_SECRET_KEYRING)
variable used here.

>  crypto-obj-y += secret.o
>  crypto-obj-y += pbkdf.o
>  crypto-obj-$(CONFIG_NETTLE) += pbkdf-nettle.o
> diff --git a/crypto/linux_keyring.c b/crypto/linux_keyring.c
> new file mode 100644
> index 00..7950d4c12d
> --- /dev/null
> +++ b/crypto/linux_keyring.c
> @@ -0,0 +1,140 @@
> +#ifdef __NR_keyctl
> +
> +#include "qemu/osdep.h"
> +#include 
> +#include 
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "trace.h"
> +#include "crypto/linux_keyring.h"
> +
> +
> +static inline
> +long keyctl_read(key_serial_t key, uint8_t *buffer, size_t buflen)
> +{
> +return syscall(__NR_keyctl, KEYCTL_READ, key, buffer, buflen, 0);
> +}
> +
> +
> +static
> +long keyctl_read_alloc(key_serial_t key, uint8_t **buffer)
> +{
> +uint8_t *loc_buf;
> +long retcode = keyctl_read(key, NULL, 0);
> +if (retcode <= 0) {
> +return retcode;
> +}
> +loc_buf = g_malloc(retcode);

We generally prefer  g_new0 to guarantee zero-initialization

> +retcode = keyctl_read(key, loc_buf, retcode);
> +
> +if (retcode >= 0) {
> +*buffer = loc_buf;
> +} else {
> +g_free(loc_buf);
> +}
> +return retcode;
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_load_data(Object   *obj,
> +   uint8_t  **output,
> +   size_t   *outputlen,
> +   Error**errp)
> +{
> +QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> +uint8_t  *buffer = NULL;
> +long retcode;
> +
> +*output= NULL;
> +*outputlen = 0;
> +
> +if (secret->serial) {
> +retcode = keyctl_read_alloc(secret->serial, &buffer);
> +if (retcode < 0) {
> +  error_setg_errno(errp, errno,
> + "Unable to read serial key %08x",
> + secret->serial);
> +  return;
> +} else {
> +  *outputlen = retcode;
> +  *output= buffer;
> +}

IMHO, we should just be passing outputlen & output straight
into keyctl_read_alloc, except then I think keyctl_read_alloc
is pointless. So just inline its logic into this method.

> +} else {
> +  error_setg(errp, "Either 'serial' must be provided");

Indent is a bit off, and the error message text doesn't make sense

Just use

 "'serial' parameter must be provided"

> +}
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_set_key(Object *obj,   Visitor *v,
> +const char *name,  void*opaque,
> +Error  **errp)
> +{
> +QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> +int32_t value;
> +visit_type_int32(v, name, &value, errp);
> +if (!value) {
> +error_setg(errp, "The 'serial' should be not equal 0");
> +}
> +secret->serial = value;
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_get_key(Object *obj,   Visitor *v,
> +const char *name,  void*opaque,
> +Error  **errp)
> +{
> +QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> +int32_t value = secret->serial;
> +visit_type_int32(v, name, &value, errp);
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_complete(UserCreatable *uc, Error **errp)
> +{
> +object_property_set_bool(OBJECT(uc), true, "loaded", errp);
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_class_init(ObjectClass *oc, void *data)
> +{
> +QCryptoSecretCommonClass *sic = QCRYPTO_SECRET_COMMON_CLASS(oc);
> +sic->load_data = qcrypto_secret_linux_load_data;

Re: [RFC PATCH v2 5/5] test-crypto-secret: add 'syskey' object tests.

[Daniel P . Berrangé]

On Thu, Apr 16, 2020 at 01:25:25AM +0300, Alexey Krasikov wrote:
> * test_secret_seckey_bad_key_access_right() is not working yet.
>   We don't know yet if this due a bag in the Linux kernel or
>   whether it's normal syscall behavior.
>   We've requested information from kernel maintainer.
> 
> Signed-off-by: Alexey Krasikov 
> ---
>  tests/test-crypto-secret.c | 138 +
>  1 file changed, 138 insertions(+)
> 
> diff --git a/tests/test-crypto-secret.c b/tests/test-crypto-secret.c
> index 13fc6c4c75..6b17fe3a81 100644
> --- a/tests/test-crypto-secret.c
> +++ b/tests/test-crypto-secret.c
> @@ -22,8 +22,10 @@
>  
>  #include "crypto/init.h"
>  #include "crypto/secret.h"
> +#include "crypto/linux_keyring.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include 
>  
>  static void test_secret_direct(void)
>  {
> @@ -125,6 +127,132 @@ static void test_secret_indirect_emptyfile(void)
>  }
>  
>  
> +#define DESCRIPTION "qemu_test_secret"
> +#define PAYLOAD "Test Payload"
> +
> +
> +static void test_secret_seckey_good(void)
> +{
> +char key_str[16];
> +Object *sec;
> +key_serial_t key = add_key("user", DESCRIPTION, PAYLOAD,
> +   strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
> +
> +g_assert(key >= 0);
> +
> +snprintf(key_str, sizeof(key_str), "0x%08x", key);
> +sec = object_new_with_props(
> +TYPE_QCRYPTO_SECRET_LINUX_KEYRING,
> +object_get_objects_root(),
> +"sec0",
> +&error_abort,
> +"serial", key_str,
> +NULL);
> +
> +assert(0 <= keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING));
> +char *pw = qcrypto_secret_lookup_as_utf8("sec0",
> + &error_abort);
> +g_assert_cmpstr(pw, ==, PAYLOAD);
> +
> +object_unparent(sec);
> +g_free(pw);
> +}
> +
> +
> +static void test_secret_seckey_revoked_key(void)
> +{
> +char key_str[16];
> +Object *sec;
> +key_serial_t key = add_key("user", DESCRIPTION, PAYLOAD,
> +   strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
> +g_assert(key >= 0);
> +g_assert_false(keyctl_revoke(key));
> +
> +snprintf(key_str, sizeof(key_str), "0x%08x", key);
> +sec = object_new_with_props(
> +TYPE_QCRYPTO_SECRET_LINUX_KEYRING,
> +object_get_objects_root(),
> +"sec0",
> +NULL,
> +"serial", key_str,
> +NULL);
> +
> +g_assert(errno == EKEYREVOKED);
> +g_assert(sec == NULL);
> +
> +keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING);
> +}
> +
> +
> +static void test_secret_seckey_expired_key(void)
> +{
> +char key_str[16];
> +Object *sec;
> +key_serial_t key = add_key("user", DESCRIPTION, PAYLOAD,
> +   strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
> +g_assert(key >= 0);
> +g_assert_false(keyctl_set_timeout(key, 1));
> +sleep(1);
> +
> +snprintf(key_str, sizeof(key_str), "0x%08x", key);
> +sec = object_new_with_props(
> +TYPE_QCRYPTO_SECRET_LINUX_KEYRING,
> +object_get_objects_root(),
> +"sec0",
> +NULL,
> +"serial", key_str,
> +NULL);
> +
> +g_assert(errno == EKEYEXPIRED);
> +g_assert(sec == NULL);
> +
> +keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING);
> +}
> +
> +
> +static void test_secret_seckey_bad_serial_key(void)
> +{
> +Object *sec;
> +
> +sec = object_new_with_props(
> +TYPE_QCRYPTO_SECRET,
> +object_get_objects_root(),
> +"sec0",
> +NULL,
> +"serial", "1",
> +NULL);
> +
> +g_assert(errno == ENOKEY);
> +g_assert(sec == NULL);
> +}
> +
> +
> +static void test_secret_seckey_bad_key_access_right(void)
> +{
> +char key_str[16];
> +Object *sec;
> +key_serial_t key = add_key("user", DESCRIPTION, PAYLOAD,
> +   strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
> +g_assert(key >= 0);
> +g_assert_false(keyctl_setperm(key, KEY_POS_ALL & (~KEY_POS_READ)));
> +
> +snprintf(key_str, sizeof(key_str), "0x%08x", key);
> +
> +sec = object_new_with_props(
> +TYPE_QCRYPTO_SECRET_LINUX_KEYRING,
> +object_get_objects_root(),
> +"sec0",
> +NULL,
> +"serial", key_str,
> +NULL);
> +
> +g_assert(errno == EACCES);
> +g_assert(sec == NULL);
> +
> +keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING);
> +}
> +
> +
>  static void test_secret_noconv_base64_good(void)
>  {
>  Object *sec = object_new_with_props(
> @@ -425,6 +553,16 @@ int main(int argc, char **argv)
>  test_secret_indirect_badfile);
>  g_test_add_func("/crypto/secret/indirect/emptyfile",
>  test_secret_indirect_emptyfile);
> +g_test_add_func("/crypto/secret/seckey/good",
> +test_secret_seckey_good);
> +g_test_add_func("/crypto/secret/seckey/revoked_key",
> +test_secret_seckey_revoked_key)

Re: [PATCH 2/2] crypto/secret: fix return logic of crypto_secret_prop_get_loaded()

[Daniel P . Berrangé]

On Wed, Apr 15, 2020 at 11:13:36PM +0300, Alexey Krasikov wrote:
> * Get function returned value of properties 'data' insteed of returning
>   value of raw data internal field. This error did not affect anyone,
>   because no one called the get function.
> 
> Signed-off-by: Alexey Krasikov 
> ---
>  crypto/secret.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/crypto/secret.c b/crypto/secret.c
> index 546b965afe..79b9b4ce0c 100644
> --- a/crypto/secret.c
> +++ b/crypto/secret.c
> @@ -231,7 +231,7 @@ qcrypto_secret_prop_get_loaded(Object *obj,
> Error **errp G_GNUC_UNUSED)
>  {
>  QCryptoSecret *secret = QCRYPTO_SECRET(obj);
> -return secret->data != NULL;
> +return secret->rawdata != NULL;
>  }

I already have this change queued from another user, so I'll drop this
dupe.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 2/3] block/io: convert generic io path to use int64_t parameters

[Stefan Hajnoczi]

On Mon, Mar 30, 2020 at 05:18:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> @@ -875,9 +875,9 @@ static bool coroutine_fn 
> bdrv_wait_serialising_requests(BdrvTrackedRequest *self
>  }
>  
>  static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
> -   size_t size)
> +   int64_t bytes)
>  {
> -if (size > BDRV_REQUEST_MAX_BYTES) {
> +if (offset < 0 || bytes < 0 || bytes > BDRV_REQUEST_MAX_BYTES) {
>  return -EIO;
>  }
>  
> @@ -885,10 +885,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, 
> int64_t offset,
>  return -ENOMEDIUM;
>  }
>  
> -if (offset < 0) {
> -return -EIO;
> -}
> -
>  return 0;
>  }

Moving this if statement was unnecessary.  I'm not sure if there are
cases where callers will now see EIO instead of ENOMEDIUM and change
their behavior :(.

More conservative code changes are easier to review and merge because
they are less risky.

> @@ -1743,7 +1740,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
>  }
>  
>  static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> -int64_t offset, int bytes, BdrvRequestFlags flags)
> +int64_t offset, int64_t bytes, BdrvRequestFlags flags)
>  {
>  BlockDriver *drv = bs->drv;
>  QEMUIOVector qiov;
> @@ -1773,7 +1770,7 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>  assert(max_write_zeroes >= bs->bl.request_alignment);
>  
>  while (bytes > 0 && !ret) {
> -int num = bytes;
> +int64_t num = bytes;
>  
>  /* Align request.  Block drivers can expect the "bulk" of the request
>   * to be aligned, and that unaligned requests do not cross cluster
> @@ -1799,6 +1796,7 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>  ret = -ENOTSUP;
>  /* First try the efficient write zeroes operation */
>  if (drv->bdrv_co_pwrite_zeroes) {
> +assert(num <= INT_MAX);

max_write_zeroes already enforces this, so the assertion is always
false:

  int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
  ...
/* limit request size */
if (num > max_write_zeroes) {
num = max_write_zeroes;
}


signature.asc
Description: PGP signature

Re: [PATCH 1/2] crypto/secret: fix inconsequential errors.

[Daniel P . Berrangé]

On Wed, Apr 15, 2020 at 11:13:35PM +0300, Alexey Krasikov wrote:
> * change condition from QCRYPTO_SECRET_FORMAT_RAW
>   to QCRYPTO_SECRET_FORMAT_BASE64 in if-operator, because
>   this is potencial error if you add another format value.

I'll drop the bullet point.

> 
> Signed-off-by: Alexey Krasikov 
> ---
>  crypto/secret.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 

and queued.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [RFC 0/3] 64bit block-layer part I

[Stefan Hajnoczi]

On Mon, Mar 30, 2020 at 05:18:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> There is an idea to make NBD protocol extension to support 64bit
> write-zero/discard/block-status commands (commands, which doesn't
> transfer user data). It's needed to increase performance of zeroing
> large ranges (up to the whole image). Zeroing of the whole image is used
> as first step of mirror job, qemu-img convert, it should be also used at
> start of backup actually..
> 
> We need to support it in block-layer, so we want 64bit write_zeros.
> Currently driver handler now have int bytes parameter.
> 
> write_zeros path goes through normal pwritev, so we need 64bit write,
> and then we need 64bit read for symmetry, and better, let's make all io
> path work with 64bit bytes parameter.
> 
> Actually most of block-layer already have 64bit parameters: offset is
> sometimes int64_t and sometimes uint64_t. bytes parameter is one of
> int64_t, uint64_t, int, unsigned int...
> 
> I think we need one type for all of this, and this one type is int64_t.
> Signed int64_t is a bit better than uint64_t: you can use same variable
> to get some result (including error < 0) and than reuse it as an
> argument without any type conversion.
> 
> So, I propose, as a first step, convert all uint64_t parameters to
> int64_t.
> 
> Still, I don't have good idea of how to split this into more than 3
> patches, so, this is an RFC.
> 
> What's next?
> 
> Converting write_zero and discard is not as simple: we can't just
> s/int/uint64_t/, as some functions may use some int variables for
> calculations and this will be broken by something larger than int.
> 
> So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and
> .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all
> drivers updated - drop unused 32bit functions, and then drop "64" suffix
> from API. If not - we'll live with both APIs.
> 
> Another thing to do is updating default limiting of request (currently
> they are limited to INT_MAX).
> 
> Then we may move some drivers to 64bit discard/write_zero: I think about
> qcow2, file-posix and nbd (as a proof-of-concept for already proposed
> NBD extension).

Sounds good to me.  This is a good series to merge early in the QEMU 5.1
release cycle.  That way we'll have time to catch any issues.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v5 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

[Kevin Wolf]

Am 22.04.2020 um 17:33 hat Eric Blake geschrieben:
> On 4/22/20 10:21 AM, Kevin Wolf wrote:
> > If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling
> > qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't
> > undo any previous preallocation, but just adds the zero flag to all
> > relevant L2 entries. If an external data file is in use, a write_zeroes
> > request to the data file is made instead.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >   block/qcow2.c | 30 ++
> >   1 file changed, 30 insertions(+)
> > 
> 
> > @@ -4214,6 +4215,35 @@ static int coroutine_fn 
> > qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> >   g_assert_not_reached();
> >   }
> > +if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
> > +uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size);
> > +uint64_t zero_end = QEMU_ALIGN_UP(offset, s->cluster_size);
> 
> This rounds up beyond the new size...
> 
> > +
> > +/* Use zero clusters as much as we can */
> > +ret = qcow2_cluster_zeroize(bs, zero_start, zero_end - zero_start, 
> > 0);
> 
> and then requests that the extra be zeroed.  Does that always work, even
> when it results in pdrv_co_pwrite_zeroes beyond the end of s->data_file?

You mean the data_file_is_raw() path in qcow2_cluster_zeroize()? It's
currently not a code path that is run because we only set
BDRV_REQ_ZERO_WRITE for truncate if the image has a backing file, and
data_file_is_raw() doesn't work with backing files.

But hypothetically, if someone called truncate with BDRV_REQ_ZERO_WRITE
for such a file, I think it would fail.

> If so,
> 
> Reviewed-by: Eric Blake 
> 
> otherwise, you may have to treat the tail specially, the same way you
> treated an unaligned head.

Actually, do I even need to round the tail?

/* Caller must pass aligned values, except at image end */
assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
   end_offset == bs->total_sectors << BDRV_SECTOR_BITS);

So qcow2_cluster_zeroize() seems to accept the unaligned tail. It would
still set the zero flag for the partial last cluster and for the
external data file, bdrv_co_pwrite_zeroes() would have the correct size.

Kevin

Re: FYI GitHub pull request / issue tracker lockdown bot

[Stefan Hajnoczi]

On Fri, Apr 03, 2020 at 03:22:13PM +0100, Daniel P. Berrangé wrote:
> QEMU, like libvirt, has a github.com project which contains automated
> read-only mirrors of QEMU repositories.
> 
>   https://github.com/qemu/
> 
> An unfortunate side effect of this is that some users will try to open
> pull requests against these mirrors. These get ignored until eventually
> someone notices and closes the request. QEMU has had about 90 prs opened
> over the years.
> 
>   https://github.com/qemu/qemu/pulls
> 
> The same applies to the issue tracker, but fortunately github lets
> projects disable this feature, which QEMU has done.
> 
> I have recently discovered that there is a nice 3rd party bot for github
> which can autorespond to pull requests with a friendly comment, close the
> request, and then lock it to prevent further comments.
> 
>   https://github.com/apps/repo-lockdown
> 
> I'm setting this up for libvirt and it was suggested QEMU can probably
> benefit from it too as an example see:
> 
>   https://github.com/libvirt/test/issues/2
>   https://github.com/libvirt/test/pull/3
> 
> 
> Configuration just requires creation of a ".github/lockdown.yml" file
> which provides the friendly message to add to the merge requests. This
> can be either done per-repository, or a special repo can be created
> called ".github" and this will apply to all repos within the project.
> 
> Ideally each repo would have a CONTRIBUTING.md file created too, since
> both GitHub and GitLab will direct users to this file for guidelines
> on how to contribute.
> 
> I don't have time right now to do this for QEMU, so consider this email
> a friendly suggestion for some other interested person to do for QEMU...

Ping Alex and Paolo, who have access to github.com/qemu.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH 1/2] crypto/secret: fix inconsequential errors.

[Philippe Mathieu-Daudé]

On 4/15/20 10:13 PM, Alexey Krasikov wrote:

* change condition from QCRYPTO_SECRET_FORMAT_RAW
   to QCRYPTO_SECRET_FORMAT_BASE64 in if-operator, because
   this is potencial error if you add another format value.

Signed-off-by: Alexey Krasikov 
---
  crypto/secret.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/secret.c b/crypto/secret.c
index 1cf0ad0ce8..546b965afe 100644
--- a/crypto/secret.c
+++ b/crypto/secret.c
@@ -204,7 +204,7 @@ qcrypto_secret_prop_set_loaded(Object *obj,
  input = output;
  inputlen = outputlen;
  } else {
-if (secret->format != QCRYPTO_SECRET_FORMAT_RAW) {
+if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
  qcrypto_secret_decode(input, inputlen,
&output, &outputlen, &local_err);
  g_free(input);



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Philippe Mathieu-Daudé]

On 4/22/20 5:17 PM, Markus Armbruster wrote:

Philippe Mathieu-Daudé  writes:


On 4/22/20 3:07 PM, Markus Armbruster wrote:

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second

qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
first to check_suspend_mode(), then to acquire_privilege(), then to
execute_async().  Continuing after errors here can only end in tears.
For instance, we risk tripping error_setv()'s assertion.

Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
Fixes: f54603b6aa765514b2519e74114a2f417759d727
Cc: Michael Roth 
Signed-off-by: Markus Armbruster 
---
   qga/commands-win32.c | 14 ++
   1 file changed, 14 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9717a8d52d..5ba56327dd 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
 *mode = GUEST_SUSPEND_MODE_DISK;
   check_suspend_mode(*mode, &local_err);
+if (local_err) {
+goto out;
+}
   acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+if (local_err) {
+goto out;
+}
   execute_async(do_suspend, mode, &local_err);
   +out:
   if (local_err) {


https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
slightly different by removing the if() check.


It frees @mode unconditionally (marked --> below) I believe that's
wrong.  execute_async() runs do_suspend() in a new thread, and passes it
@mode.  do_suspend() frees it.


Oops I missed that, good catch!

Reviewed-by: Philippe Mathieu-Daudé 




   error_propagate(errp, local_err);
   g_free(mode);
@@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
 *mode = GUEST_SUSPEND_MODE_RAM;
   check_suspend_mode(*mode, &local_err);
+if (local_err) {
+goto out;
+}
   acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+if (local_err) {
+goto out;
+}
   execute_async(do_suspend, mode, &local_err);
   +out:
   if (local_err) {
   error_propagate(errp, local_err);
   g_free(mode);



diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b49920e201..8b66098056 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1341,13 +1341,18 @@ void qmp_guest_suspend_disk(Error **errp)

 *mode = GUEST_SUSPEND_MODE_DISK;
 check_suspend_mode(*mode, &local_err);
+if (local_err) {
+goto out;
+}
 acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+if (local_err) {
+goto out;
+}
 execute_async(do_suspend, mode, &local_err);

-if (local_err) {
-error_propagate(errp, local_err);
-g_free(mode);
-}
+out:
+error_propagate(errp, local_err);
-->+g_free(mode);
 }

 void qmp_guest_suspend_ram(Error **errp)

Re: [edk2-discuss] Load Option passing. Either bugs or my confusion.

[Laszlo Ersek]

On 04/22/20 09:42, Hou Qiming wrote:
> A little off topic thing: isn't the default resolution supposed to be
> 1024x768?

No.

> This is the Microsoft regulation which all my physical devices
> seem to follow:
> 
> https://docs.microsoft.com/en-us/windows-hardware/test/hlk/testref/6afc8979-df62-4d86-8f6a-99f05bbdc7f3

Key term being "Microsoft regulation".

The UEFI spec requires discrete ("plug-in") graphics devices to support
at least either 800x600x32 or 640x480x32.

And the edk2 (not just OVMF) default for the console resolution is
800x600. (See PcdVideoHorizontalResolution and
PcdVideoVerticalResolution in "MdeModulePkg/MdeModulePkg.dec".)

> And when the user provides an EDID one should parse it and set the default
> resolution to match it. But that's a less important feature.

It's more complex than you might think, and (to me personally) it seems
to require more time than its importance justifies.

https://bugzilla.redhat.com/show_bug.cgi?id=1749250

Laszlo

Re: [PATCH 5/9] block/io: expand in_flight inc/dec section: simple cases

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 04:47:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2020 19:22, Stefan Hajnoczi wrote:
> > On Wed, Apr 08, 2020 at 12:30:47PM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> > > It's safer to expand in_flight request to start before enter to
> > 
> > Please explain what exeactly "safer" means.  If I understand correctly
> > this is just a refactoring and does not fix bugs that have been hit in
> > the real world.
> > 
> > Is this just a generate attempt to avoid accidentally performing
> > operations that need to happen as part of the request after the dec
> > call?
> 
> Consider write.
> 
> It's possible, that qemu_coroutine_enter only schedules execution, assume 
> such case.
> Then we may possibly have the following:
> 
> 1. Somehow check that we are not in drained section in outer code
> 
> 2. call bdrv_pwritev(), assuming that it will increse in_flight, which will 
> protect us from starting drained section
> 
> 3. it calls bdrv_prwv_co -> bdrv_coroutine_enter (not yet increased in_flight)
> 
> 4. assume coroutine not yet actually entered, only scheduled, and we go to 
> some code, which starts drained section (as in_flight is zero)
> 
> 5. scheduled coroutine starts, and blindly increases in_flight, and we are in 
> drained section with in_flight request.
> 
> The series does the same thing for block/io.c like Kevin's "block: Fix 
> blk->in_flight during blk_wait_while_drained()" for blk layer.

Please include this in the commit description.  Thanks!

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v5 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

[Eric Blake]

On 4/22/20 10:58 AM, Kevin Wolf wrote:


@@ -4214,6 +4215,35 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
   g_assert_not_reached();
   }
+if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
+uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size);
+uint64_t zero_end = QEMU_ALIGN_UP(offset, s->cluster_size);


This rounds up beyond the new size...


+
+/* Use zero clusters as much as we can */
+ret = qcow2_cluster_zeroize(bs, zero_start, zero_end - zero_start, 0);


and then requests that the extra be zeroed.  Does that always work, even
when it results in pdrv_co_pwrite_zeroes beyond the end of s->data_file?


You mean the data_file_is_raw() path in qcow2_cluster_zeroize()? It's
currently not a code path that is run because we only set
BDRV_REQ_ZERO_WRITE for truncate if the image has a backing file, and
data_file_is_raw() doesn't work with backing files.


Good point.



But hypothetically, if someone called truncate with BDRV_REQ_ZERO_WRITE
for such a file, I think it would fail.


If so,

Reviewed-by: Eric Blake 

otherwise, you may have to treat the tail specially, the same way you
treated an unaligned head.


Actually, do I even need to round the tail?

 /* Caller must pass aligned values, except at image end */
 assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
 assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
end_offset == bs->total_sectors << BDRV_SECTOR_BITS);

So qcow2_cluster_zeroize() seems to accept the unaligned tail. It would
still set the zero flag for the partial last cluster and for the
external data file, bdrv_co_pwrite_zeroes() would have the correct size.


Then I'm in favor of NOT rounding the tail.  That's an easy enough 
change and we've now justified that it does what we want, so R-b stands 
with that one-line tweak.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/5] Revert "hw/display/ramfb: initialize fw-config space with xres/ yres"

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> This reverts commit f79081b4b71b72640bedd40a7cd76f864c8287f1.
> 
> Patch has broken byteorder handling: RAMFBCfg fields are in bigendian
> byteorder, the reset function doesn't care so native byteorder is used
> instead.  Given this went unnoticed so far the feature is obviously
> unused, so just revert the patch.
> 
> Cc: Hou Qiming 
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/hw/display/ramfb.h|  2 +-
>  hw/display/ramfb-standalone.c | 12 +---
>  hw/display/ramfb.c| 16 +---
>  hw/vfio/display.c |  4 ++--
>  stubs/ramfb.c |  2 +-
>  5 files changed, 6 insertions(+), 30 deletions(-)

Acked-by: Laszlo Ersek 


> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index f6c2de93b222..b33a2c467b28 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -4,7 +4,7 @@
>  /* ramfb.c */
>  typedef struct RAMFBState RAMFBState;
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> -RAMFBState *ramfb_setup(DeviceState *dev, Error **errp);
> +RAMFBState *ramfb_setup(Error **errp);
>  
>  /* ramfb-standalone.c */
>  #define TYPE_RAMFB_DEVICE "ramfb"
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index d76a9d0fe2c9..b18db97eeb1b 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -3,7 +3,6 @@
>  #include "qemu/module.h"
>  #include "hw/loader.h"
>  #include "hw/qdev-properties.h"
> -#include "hw/isa/isa.h"
>  #include "hw/display/ramfb.h"
>  #include "ui/console.h"
>  
> @@ -13,8 +12,6 @@ typedef struct RAMFBStandaloneState {
>  SysBusDevice parent_obj;
>  QemuConsole *con;
>  RAMFBState *state;
> -uint32_t xres;
> -uint32_t yres;
>  } RAMFBStandaloneState;
>  
>  static void display_update_wrapper(void *dev)
> @@ -37,22 +34,15 @@ static void ramfb_realizefn(DeviceState *dev, Error 
> **errp)
>  RAMFBStandaloneState *ramfb = RAMFB(dev);
>  
>  ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
> -ramfb->state = ramfb_setup(dev, errp);
> +ramfb->state = ramfb_setup(errp);
>  }
>  
> -static Property ramfb_properties[] = {
> -DEFINE_PROP_UINT32("xres", RAMFBStandaloneState, xres, 0),
> -DEFINE_PROP_UINT32("yres", RAMFBStandaloneState, yres, 0),
> -DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void ramfb_class_initfn(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  
>  set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>  dc->realize = ramfb_realizefn;
> -device_class_set_props(dc, ramfb_properties);
>  dc->desc = "ram framebuffer standalone device";
>  dc->user_creatable = true;
>  }
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 7ba07c80f6e1..bd4746dc1768 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -13,7 +13,6 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> -#include "qemu/option.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
>  #include "ui/console.h"
> @@ -31,7 +30,6 @@ struct QEMU_PACKED RAMFBCfg {
>  struct RAMFBState {
>  DisplaySurface *ds;
>  uint32_t width, height;
> -uint32_t starting_width, starting_height;
>  struct RAMFBCfg cfg;
>  bool locked;
>  };
> @@ -117,11 +115,9 @@ static void ramfb_reset(void *opaque)
>  RAMFBState *s = (RAMFBState *)opaque;
>  s->locked = false;
>  memset(&s->cfg, 0, sizeof(s->cfg));
> -s->cfg.width = s->starting_width;
> -s->cfg.height = s->starting_height;
>  }
>  
> -RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
> +RAMFBState *ramfb_setup(Error **errp)
>  {
>  FWCfgState *fw_cfg = fw_cfg_find();
>  RAMFBState *s;
> @@ -133,16 +129,6 @@ RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
>  
>  s = g_new0(RAMFBState, 1);
>  
> -const char *s_fb_width = qemu_opt_get(dev->opts, "xres");
> -const char *s_fb_height = qemu_opt_get(dev->opts, "yres");
> -if (s_fb_width) {
> -s->cfg.width = atoi(s_fb_width);
> -s->starting_width = s->cfg.width;
> -}
> -if (s_fb_height) {
> -s->cfg.height = atoi(s_fb_height);
> -s->starting_height = s->cfg.height;
> -}
>  s->locked = false;
>  
>  rom_add_vga("vgabios-ramfb.bin");
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index f4977c66e1b5..a57a22674d62 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -353,7 +353,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, 
> Error **errp)
>&vfio_display_dmabuf_ops,
>vdev);
>  if (vdev->enable_ramfb) {
> -vdev->dpy->ramfb = ramfb_setup(DEVICE(vdev), errp);
> +vdev->dpy->ramfb = ramfb_setup(errp);
>  }
>  vfio_display_edid_init(vdev);
>  return 0;
> @@ -479,7 +479,7 @@ static int vfio_display_region_init(VFIOPCIDevice *

Re: [PATCH v2 13/36] tcg: Use tcg_constant_{i32,i64} with tcg int expanders

[Alex Bennée]

Richard Henderson  writes:

> Signed-off-by: Richard Henderson 
> ---
>  include/tcg/tcg-op.h |  13 +--
>  tcg/tcg-op.c | 216 ---
>  2 files changed, 100 insertions(+), 129 deletions(-)
>
> diff --git a/include/tcg/tcg-op.h b/include/tcg/tcg-op.h
> index 230db6e022..11ed9192f7 100644
> --- a/include/tcg/tcg-op.h
> +++ b/include/tcg/tcg-op.h

> @@ -1468,12 +1441,17 @@ void tcg_gen_brcond_i64(TCGCond cond, TCGv_i64 arg1, 
> TCGv_i64 arg2, TCGLabel *l)
>  
>  void tcg_gen_brcondi_i64(TCGCond cond, TCGv_i64 arg1, int64_t arg2, TCGLabel 
> *l)
>  {
> -if (cond == TCG_COND_ALWAYS) {
> +if (TCG_TARGET_REG_BITS == 64) {
> +tcg_gen_brcond_i64(cond, arg1, tcg_constant_i64(arg2), l);
> +} else if (cond == TCG_COND_ALWAYS) {
>  tcg_gen_br(l);
>  } else if (cond != TCG_COND_NEVER) {
> -TCGv_i64 t0 = tcg_const_i64(arg2);
> -tcg_gen_brcond_i64(cond, arg1, t0, l);
> -tcg_temp_free_i64(t0);
> +l->refs++;

Hmm is this a separate fix?

> +tcg_gen_op6ii_i32(INDEX_op_brcond2_i32,
> +  TCGV_LOW(arg1), TCGV_HIGH(arg1),
> +  tcg_constant_i32(arg2),
> +  tcg_constant_i32(arg2 >> 32),
> +  cond, label_arg(l));
>  }
>  }


otherwise lgtm:

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: Integration of qemu-img

[Stefan Hajnoczi]

On Thu, Apr 16, 2020 at 09:50:48AM +0200, Markus Armbruster wrote:
> Cc: qemu-block
> 
>  writes:
> 
> > Dear Sir or Madam,
> >
> >  
> >
> > I am a PhD student at the Friedrich-Alexander-University Erlangen-Nürnberg
> > in Bavaria Germany and I am currently working on an open-source forensic
> > analysis tool. I would like to use qemu-img for converting virtual discs to
> > raw files and to get virtual disc information. By now I tried to create a
> > qemu-img DLL with the qemu source code you provide on your website, but I am
> > unable to compile it properly. Therefore, I would like to ask you if there
> > is a simple solution to integrate qemu-img to other C++ projects? Or is
> > there a precompiled qemu-img DLL which I could use? Thank you very much for
> > your support.

Can you simply spawn a qemu-img process from your application?  That
would be much easier than trying to build it as a library and link it
into your application.

qemu-img has --output=json exactly for this case.  It will let you parse
the output as JSON.

Stefan


signature.asc
Description: PGP signature

Re: qemu 4.2.0 audiodev soundhw

[Philippe Mathieu-Daudé]

Hi Jakob,

On 4/21/20 12:06 AM, Jakob Bohm wrote:
[...]


In fact, over the years, I have found it excruciatingly difficult to find
valid qemu documentation, as each feature effort tends to leave behind
half-updated pages and a bunch of uncoordinated messages about what may or
may not have been implemented in unspecified versions.

I feel your pain and agree.

How can this get improved?

Keeping the command line backward compatible is not an easy task.

There is a quite important effort in progress to improve the documentation.

Users reporting bad/incomplete/outdated documentation would help and 
motivate developers to fix it. That would reduce the gap between 
developers implementing features and users.


Do you other have suggestions about what should be improved?

Thanks,

Phil.



On 20/04/2020 19:54, Idar Lund wrote:

Hi,

Thanks for your response!

Yes, I agree with you on the options. If you guys decide on (3), I 
would suggest to make it dynamically like this; "-soundhw 
hda,audiodev=sound1". This would then copy the 'audiodev' (and 
possible other) parameter(s) to the '-device' option.


My personal preference would be to recommend option number 1.
The reason for this is that maintaining a shortcut like this makes it 
hard to maintain for developers when adding features and fixes bugs on 
other options. And of course documentation maintainers :)
The second reason as I see it is that people tend to create a .sh 
script or similar to start their qemu virtual machines if they don't 
use libvirt/xml schema. And for that, a more verbose command would 
actually be easier to maintain for users since we then know where to 
put parameters like this.


-Idar

On Mon, Apr 20, 2020 at 4:44 PM Gerd Hoffmann > wrote:


    On Fri, Apr 17, 2020 at 12:15:30PM +0100, Peter Maydell wrote:
    > On Fri, 17 Apr 2020 at 12:08, Idar Lund mailto:idarl...@gmail.com>> wrote:
    > > I'm using qemu-system-x86_64 with the following options:
    > > -audiodev pa,id=sound1,server=/run/user/1000/pulse/native \
    > > -soundhw hda
    > >
    > > After upgrade to 4.2.0 (qemu-4.2.0-7.fc32) I get the following
    warning:
    > > (qemu) audio: Device hda: audiodev default parameter is
    deprecated, please specify audiodev=sound1
    > >
    > > The documentation `man qemu-system-x86_64` seems to not
    reflect this.
    > > How am I supposed to use audiodev and soundhw?
    >
    > This looks like another question for you, Gerd...

    Hmm, good question how to proceed here best ...

    "-soundhw hda" is a shortcut for "-device intel-hda -device
    hda-duplex"

    You can use "-device intel-hda -device hda-duplex,audiodev=sound1" to
    make the warning go away.  That is pretty verbose when compared to
    "-soundhw hda" though ...

    So the options I see are:

      (1) deprecate the -soundhw shortcut, expect users to use -device
          instead.
      (2) have -soundhw lookup the audiodev and add it automatically.
    Works
          only with a single audiodev, but that isn't different from what
          we have today.  If you want do more complicated things you
          already have to use the more verbose -device command line.
      (3) add audiodev option to -soundhw.

    I don't like (3) much, our command line is already messy enough.
    So my
    personal preference would be (1) or (2) ...



Enjoy

Jakob

Re: AF_UNIX Monitor Device

[Stefan Hajnoczi]

On Tue, Apr 14, 2020 at 09:46:00PM -0400, Joshua Abraham wrote:
> Hello Stefan,
> 
> This blog post [0] talks about the AF_VSOCK monitoring device
> (vsockmon) Stefan upstreamed into Linux a few years ago. It seems to
> me the same rationale for enabling packet captures for AF_VSOCK
> traffic applies to UNIX domain sockets as well. What do you think? I
> have a proof of concept patch for Linux for a unixmon capture device
> if you think this is a good idea.
> 
> [0] https://blog.vmsplice.net/2017/07/packet-capture-coming-to-afvsock.html

Sorry, I didn't see your email until now.

Unlike AF_VSOCK, AF_UNIX has no control packets so the capture would
only consist of the data which you can already see using strace(1).
So while you really need this feature in order to inspect AF_VSOCK
traffic you don't strictly need it for AF_UNIX.  Maybe that is why this
feature has never been implemented.

I suggest asking the Linux AF_UNIX and networking maintainers if this
feature could be merged.  I've CCed them.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH 2/5] Revert "hw/display/ramfb: lock guest resolution after it's set"

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> This reverts commit a9e0cb67b7f4c485755659f9b764c38b5f970de4.
> 
> This breaks OVMF.  Reproducer: Just hit 'ESC' at early boot to enter
> firmware setup.  OVMF wants switch from (default) 800x600 to 640x480 for
> that, and this patch blocks it.
> 
> Cc: Hou Qiming 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/display/ramfb.c | 26 --
>  1 file changed, 4 insertions(+), 22 deletions(-)

(1) I've verified that, after applying patches #1 and #2 in this series,
the resultant state of "hw/display/ramfb.c" matches the one at
a9e0cb67b7f4^ (note the caret), modulo:

- commit 71e8a9158558 ("Include sysemu/reset.h a lot less", 2019-08-16), and

- commit 85eb7c18ee39 ("Let cpu_[physical]_memory() calls pass a boolean
'is_write' argument", 2020-02-20).

So:

Reviewed-by: Laszlo Ersek 

(2) I've also tested this patch (i.e., built QEMU with patches #1 and #2
applied -- patch #1 is needed for context --, and entered the OVMF Setup
TUI while using ramfb).

Tested-by: Laszlo Ersek 

Thanks!
Laszlo

> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index bd4746dc1768..9d41c2ad2868 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -31,7 +31,6 @@ struct RAMFBState {
>  DisplaySurface *ds;
>  uint32_t width, height;
>  struct RAMFBCfg cfg;
> -bool locked;
>  };
>  
>  static void ramfb_unmap_display_surface(pixman_image_t *image, void *unused)
> @@ -72,25 +71,18 @@ static DisplaySurface *ramfb_create_display_surface(int 
> width, int height,
>  static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
>  {
>  RAMFBState *s = dev;
> -uint32_t fourcc, format, width, height;
> +uint32_t fourcc, format;
>  hwaddr stride, addr;
>  
> -width = be32_to_cpu(s->cfg.width);
> -height= be32_to_cpu(s->cfg.height);
> +s->width  = be32_to_cpu(s->cfg.width);
> +s->height = be32_to_cpu(s->cfg.height);
>  stride= be32_to_cpu(s->cfg.stride);
>  fourcc= be32_to_cpu(s->cfg.fourcc);
>  addr  = be64_to_cpu(s->cfg.addr);
>  format= qemu_drm_format_to_pixman(fourcc);
>  
>  fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
> -width, height, addr);
> -if (s->locked) {
> -fprintf(stderr, "%s: resolution locked, change rejected\n", 
> __func__);
> -return;
> -}
> -s->locked = true;
> -s->width = width;
> -s->height = height;
> +s->width, s->height, addr);
>  s->ds = ramfb_create_display_surface(s->width, s->height,
>   format, stride, addr);
>  }
> @@ -110,13 +102,6 @@ void ramfb_display_update(QemuConsole *con, RAMFBState 
> *s)
>  dpy_gfx_update_full(con);
>  }
>  
> -static void ramfb_reset(void *opaque)
> -{
> -RAMFBState *s = (RAMFBState *)opaque;
> -s->locked = false;
> -memset(&s->cfg, 0, sizeof(s->cfg));
> -}
> -
>  RAMFBState *ramfb_setup(Error **errp)
>  {
>  FWCfgState *fw_cfg = fw_cfg_find();
> @@ -129,12 +114,9 @@ RAMFBState *ramfb_setup(Error **errp)
>  
>  s = g_new0(RAMFBState, 1);
>  
> -s->locked = false;
> -
>  rom_add_vga("vgabios-ramfb.bin");
>  fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>   NULL, ramfb_fw_cfg_write, s,
>   &s->cfg, sizeof(s->cfg), false);
> -qemu_register_reset(ramfb_reset, s);
>  return s;
>  }
> 

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Philippe Mathieu-Daudé]

On 4/22/20 4:49 PM, Eric Blake wrote:

On 4/22/20 8:35 AM, Philippe Mathieu-Daudé wrote:

Hi Markus,

On 4/22/20 3:07 PM, Markus Armbruster wrote:

Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
Signed-off-by: Markus Armbruster 
---
  tests/test-logging.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6387e4933f..8580b82420 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -73,10 +73,10 @@ static void test_parse_range(void)
  g_assert(qemu_log_in_addr_range(UINT64_MAX));
  g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
-    qemu_set_dfilter_ranges("0..0x", &err);
+    qemu_set_dfilter_ranges("0..0x", &error_abort);


Why sometime use this form, ...


This call must not produce an error (if it does, the test aborts, 
proving we had a bug).





  g_assert(qemu_log_in_addr_range(0));
  g_assert(qemu_log_in_addr_range(UINT64_MAX));
-
+
  qemu_set_dfilter_ranges("2..1", &err);
  error_free_or_abort(&err);


... and then this other form?


This call must produce an error (if we do not diagnose the caller's 
error, our code is buggy, and the test must fail)


Ah OK it makes sense, thanks!

Re: [PATCH 3/5] ramfb: don't update RAMFBState on errors

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> Store width & height & surface in local variables.  Update RAMFBState
> with the new values only in case the ramfb_create_display_surface() call
> succeeds.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/display/ramfb.c | 25 -
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 9d41c2ad2868..fbe959147dc9 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -71,20 +71,27 @@ static DisplaySurface *ramfb_create_display_surface(int 
> width, int height,
>  static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
>  {
>  RAMFBState *s = dev;
> -uint32_t fourcc, format;
> +DisplaySurface *surface;
> +uint32_t fourcc, format, width, height;
>  hwaddr stride, addr;
>  
> -s->width  = be32_to_cpu(s->cfg.width);
> -s->height = be32_to_cpu(s->cfg.height);
> -stride= be32_to_cpu(s->cfg.stride);
> -fourcc= be32_to_cpu(s->cfg.fourcc);
> -addr  = be64_to_cpu(s->cfg.addr);
> -format= qemu_drm_format_to_pixman(fourcc);
> +width  = be32_to_cpu(s->cfg.width);
> +height = be32_to_cpu(s->cfg.height);
> +stride = be32_to_cpu(s->cfg.stride);
> +fourcc = be32_to_cpu(s->cfg.fourcc);
> +addr   = be64_to_cpu(s->cfg.addr);
> +format = qemu_drm_format_to_pixman(fourcc);
>  
>  fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
>  s->width, s->height, addr);
> -s->ds = ramfb_create_display_surface(s->width, s->height,
> - format, stride, addr);
> +surface = ramfb_create_display_surface(width, height,
> +   format, stride, addr);
> +if (!surface)
> +return;

A warning message here, or inside ramfb_create_display_surface(), could
be nice; but I agree it's not required at all.

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

> +
> +s->width = width;
> +s->height = height;
> +s->ds = surface;
>  }
>  
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s)
> 

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Philippe Mathieu-Daudé]

On 4/22/20 5:19 PM, Markus Armbruster wrote:

Philippe Mathieu-Daudé  writes:


Hi Markus,

On 4/22/20 3:07 PM, Markus Armbruster wrote:

Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
Signed-off-by: Markus Armbruster 
---
   tests/test-logging.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6387e4933f..8580b82420 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -73,10 +73,10 @@ static void test_parse_range(void)
   g_assert(qemu_log_in_addr_range(UINT64_MAX));
   g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
   -qemu_set_dfilter_ranges("0..0x", &err);
+qemu_set_dfilter_ranges("0..0x", &error_abort);


Why sometime use this form, ...


   g_assert(qemu_log_in_addr_range(0));
   g_assert(qemu_log_in_addr_range(UINT64_MAX));
-
+
   qemu_set_dfilter_ranges("2..1", &err);
   error_free_or_abort(&err);


... and then this other form?


The first form crashes when the function sets an error.

The second from crashes when the function doesn't set an error, or else
frees the error.

All clear?


Yes =) It is even documented!

 * Assert that an expected error occurred, but clean it up without
 * reporting it (primarily useful in testsuites):
 * error_free_or_abort(&err);

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] fuzz: select fuzz target using executable name

[Stefan Hajnoczi]

On Tue, Apr 21, 2020 at 02:22:30PM -0400, Alexander Bulekov wrote:
> The fuzzers are built into a binary (e.g. qemu-fuzz-i386). To select the
> device to fuzz/fuzz target, we usually use the --fuzz-target= argument.
> This commit allows the fuzz-target to be specified using the name of the
> executable. If the executable name ends with -target-FUZZ_TARGET, then
> we select the fuzz target based on this name, rather than the
> --fuzz-target argument. This is useful for systems such as oss-fuzz
> where we don't have control of the arguments passed to the fuzzer.
> 
> Signed-off-by: Alexander Bulekov 
> ---
>  tests/qtest/fuzz/fuzz.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)

I fixed the indentation when merging.

Thanks, applied to my master tree:
https://github.com/stefanha/qemu/commits/master

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v2] target/arm: Implement SVE2 FMMLA

[Richard Henderson]

On 4/22/20 7:15 AM, Stephen Long wrote:
> Signed-off-by: Stephen Long 
> 
> I'm guessing endianness doesn't matter because we are writing to the
> corresponding 32-bit/64-bit in the destination register.
> ---
>  target/arm/cpu.h   | 10 +
>  target/arm/helper-sve.h|  3 +++
>  target/arm/sve.decode  |  4 
>  target/arm/sve_helper.c| 44 ++
>  target/arm/translate-sve.c | 29 +
>  5 files changed, 90 insertions(+)

Endianness does matter for 32-bit, as we are writing into a host-endian 64-bit
quantity.  I was being over-brief in my earlier reply.


> +TYPE p0, p1, results[4];\
> +\
> +/* i = 0, j = 0 */  \
> +p0 = MUL(n00, m00, status); \
> +p1 = MUL(n01, m01, status); \
> +results[0] = ADD(a[0], ADD(p0, p1, status), status);\
> +\
> +/* i = 0, j = 1 */  \
> +p0 = MUL(n00, m10, status); \
> +p1 = MUL(n01, m11, status); \
> +results[1] = ADD(a[1], ADD(p0, p1, status), status);\
> +\
> +/* i = 1, j = 0 */  \
> +p0 = MUL(n10, m00, status); \
> +p1 = MUL(n11, m01, status); \
> +results[2] = ADD(a[2], ADD(p0, p1, status), status);\
> +\
> +/* i = 1, j = 1 */  \
> +p0 = MUL(n10, m10, status); \
> +p1 = MUL(n11, m11, status); \
> +results[3] = ADD(a[3], ADD(p0, p1, status), status);\
> +\
> +memcpy(d, results, sizeof(TYPE) * 4);   \

There's no need for the result array -- we have already read the inputs, so we
can write back the result straight away.


r~

Re: [PATCH] MAINTAINERS: Update Keith Busch's email address

[Keith Busch]

On Tue, Apr 21, 2020 at 02:22:36PM +0200, Philippe Mathieu-Daudé wrote:
> keith.bu...@intel.com address is being rejected.
> Replace by the email address Keith is actively using.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Thank you for sending the correction.

Acked-by: Keith Busch 

Re: [PATCH 4/5] ramfb: add sanity checks to ramfb_create_display_surface

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/display/ramfb.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index fbe959147dc9..d1b1cb9bb294 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -15,6 +15,7 @@
>  #include "qapi/error.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
> +#include "hw/display/bochs-vbe.h" /* for limits */
>  #include "ui/console.h"
>  #include "sysemu/reset.h"
>  
> @@ -49,6 +50,11 @@ static DisplaySurface *ramfb_create_display_surface(int 
> width, int height,
>  hwaddr size;
>  void *data;
>  
> +if (width < 16 || width > VBE_DISPI_MAX_XRES ||
> +height < 16 || height > VBE_DISPI_MAX_YRES ||

Seems to make sense (I've checked the constants).

> +format == 0 /* unknown format */)

OK, this is from qemu_drm_format_to_pixman().

> +return NULL;
> +
>  if (linesize == 0) {
>  linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
>  }
> 

I would suggest four more sanity checks:

- if "linesize" is nonzero, make sure it is a whole multiple of the
required word size (?)

- if "linesize" is nonzero, make sure it is not bogus with relation to
"width". I'm thinking something like:

if (linesize > 0) {
min_linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
if (linesize < min_linesize) {
return NULL;
}
}

May not be the best way to put it, but you get the idea.

- We might want to put an upper bound on "linesize" too. I realize
"(hwaddr)linesize * height" should be safe in this function, as the
multiplication is done in uint64_t. But we also pass "linesize" to
qemu_create_displaysurface_from(), and who knows how it is used for
multiplication there.

- in the ramfb_create_display_surface() function, we should change the
type of the parameters "width", "height", and "linesize", from "int" to
"uint32_t". In ramfb_fw_cfg_write(), we do take them as uint32_t from
the guest, but then pass them as "int"s. And so the current state can
produce negative values for any of "width", "height", and "linesize" --
and I'd rather not investigate where those lead. (The new checks catch a
negative "width" and "height" already, but not "linesize".) Casting a
negative "linesize" to (hwaddr) produces a big, ugly value, FWIW.

Thanks
Laszlo

Re: [PATCH 5/5] ramfb: drop leftover debug message

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/display/ramfb.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index d1b1cb9bb294..be884c9ea837 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -88,8 +88,6 @@ static void ramfb_fw_cfg_write(void *dev, off_t offset, 
> size_t len)
>  addr   = be64_to_cpu(s->cfg.addr);
>  format = qemu_drm_format_to_pixman(fourcc);
>  
> -fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
> -s->width, s->height, addr);
>  surface = ramfb_create_display_surface(width, height,
> format, stride, addr);
>  if (!surface)
> 

Reviewed-by: Laszlo Ersek 

[PATCH v3] target/arm: Implement SVE2 FMMLA

[Stephen Long]

Signed-off-by: Stephen Long 

Fixed the errors Richard pointed out.
---
 target/arm/cpu.h   | 10 +
 target/arm/helper-sve.h|  3 +++
 target/arm/sve.decode  |  4 
 target/arm/sve_helper.c| 42 ++
 target/arm/translate-sve.c | 29 ++
 5 files changed, 88 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b7c7946771..d41c4a08c0 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3870,6 +3870,16 @@ static inline bool isar_feature_aa64_sve2_bitperm(const 
ARMISARegisters *id)
 return FIELD_EX64(id->id_aa64zfr0, ID_AA64ZFR0, BITPERM) != 0;
 }
 
+static inline bool isar_feature_aa64_sve2_f32mm(const ARMISARegisters *id)
+{
+return FIELD_EX64(id->id_aa64zfr0, ID_AA64ZFR0, F32MM) != 0;
+}
+
+static inline bool isar_feature_aa64_sve2_f64mm(const ARMISARegisters *id)
+{
+return FIELD_EX64(id->id_aa64zfr0, ID_AA64ZFR0, F64MM) != 0;
+}
+
 /*
  * Feature tests for "does this exist in either 32-bit or 64-bit?"
  */
diff --git a/target/arm/helper-sve.h b/target/arm/helper-sve.h
index ea53750141..8104d23c5f 100644
--- a/target/arm/helper-sve.h
+++ b/target/arm/helper-sve.h
@@ -2683,3 +2683,6 @@ DEF_HELPER_FLAGS_5(sve2_sqrdcmlah__s, TCG_CALL_NO_RWG,
void, ptr, ptr, ptr, ptr, i32)
 DEF_HELPER_FLAGS_5(sve2_sqrdcmlah__d, TCG_CALL_NO_RWG,
void, ptr, ptr, ptr, ptr, i32)
+
+DEF_HELPER_FLAGS_6(fmmla_s, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr, ptr, 
i32)
+DEF_HELPER_FLAGS_6(fmmla_d, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr, ptr, 
i32)
diff --git a/target/arm/sve.decode b/target/arm/sve.decode
index 95c73c665a..dd987da648 100644
--- a/target/arm/sve.decode
+++ b/target/arm/sve.decode
@@ -1383,3 +1383,7 @@ UMLSLT_zzzw 01000100 .. 0 . 010 111 . .  
@rda_rn_rm
 
 CMLA_   01000100 esz:2 0 rm:5 0010 rot:2 rn:5 rd:5  ra=%reg_movprfx
 SQRDCMLAH_  01000100 esz:2 0 rm:5 0011 rot:2 rn:5 rd:5  ra=%reg_movprfx
+
+### SVE2 floating point matrix multiply accumulate
+
+FMMLA   01100100 .. 1 . 111001 . .  @rda_rn_rm
diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
index b392a87aef..9c6709d6df 100644
--- a/target/arm/sve_helper.c
+++ b/target/arm/sve_helper.c
@@ -7389,3 +7389,45 @@ void HELPER(sve2_histseg)(void *vd, void *vn, void *vm, 
uint32_t desc)
 *(uint64_t *)(vd + i + 8) = out1;
 }
 }
+
+#define DO_FP_MATRIX_MUL(NAME, TYPE, MUL, ADD, H)   \
+void HELPER(NAME)(void *vd, void *va, void *vn, void *vm,   \
+ void *status, uint32_t desc)   \
+{   \
+intptr_t s; \
+intptr_t opr_sz = simd_oprsz(desc) / (sizeof(TYPE) >> 2);   \
+\
+for (s = 0; s < opr_sz; ++s) {  \
+TYPE *n = vn + s * (sizeof(TYPE) >> 2); \
+TYPE *m = vm + s * (sizeof(TYPE) >> 2); \
+TYPE *a = va + s * (sizeof(TYPE) >> 2); \
+TYPE *d = vd + s * (sizeof(TYPE) >> 2); \
+\
+TYPE n00 = n[H(0)], n01 = n[H(1)], n10 = n[H(2)], n11 = n[H(3)];\
+TYPE m00 = m[H(0)], m01 = m[H(1)], m10 = m[H(2)], m11 = m[H(3)];\
+TYPE p0, p1;\
+\
+/* i = 0, j = 0 */  \
+p0 = MUL(n00, m00, status); \
+p1 = MUL(n01, m01, status); \
+d[H(0)] = ADD(a[H(0)], ADD(p0, p1, status), status);\
+\
+/* i = 0, j = 1 */  \
+p0 = MUL(n00, m10, status); \
+p1 = MUL(n01, m11, status); \
+d[H(1)] = ADD(a[H(1)], ADD(p0, p1, status), status);\
+\
+/* i = 1, j = 0 */  \
+p0 = MUL(n10, m00, status); \
+p1 = MUL(n11, m01, status); \
+d[H(2)] = ADD(a[H(2)], ADD(p0, p1, status), status);\
+\
+/* i = 1, j = 1 */   

Re: [PATCH v2 11/36] tcg: Introduce TYPE_CONST temporaries

[Richard Henderson]

On 4/22/20 8:17 AM, Alex Bennée wrote:
>>  static inline bool temp_readonly(TCGTemp *ts)
>>  {
>> -return ts->kind == TEMP_FIXED;
>> +return ts->kind >= TEMP_FIXED;
> 
> I should have clarified in the previous patch - TEMP_FIXED is a fixed
> value, e.g. env or other internal pointer which we won't be overwriting
> or otherwise trashing anywhere in the block?

Correct.  Only env, actually.  There are (currently) no other internal pointers
that are fixed.


r~

Re: [PATCH v2 14/36] tcg: Use tcg_constant_{i32,vec} with tcg vec expanders

[Alex Bennée]

Richard Henderson  writes:

> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  tcg/tcg-op-vec.c | 63 ++--
>  1 file changed, 34 insertions(+), 29 deletions(-)
>
> diff --git a/tcg/tcg-op-vec.c b/tcg/tcg-op-vec.c
> index f3927089a7..655b3ae32d 100644
> --- a/tcg/tcg-op-vec.c
> +++ b/tcg/tcg-op-vec.c
> @@ -233,25 +233,17 @@ void tcg_gen_mov_vec(TCGv_vec r, TCGv_vec a)
>  }
>  }
>  
> -#define MO_REG  (TCG_TARGET_REG_BITS == 64 ? MO_64 : MO_32)
> -
> -static void do_dupi_vec(TCGv_vec r, unsigned vece, TCGArg a)
> -{
> -TCGTemp *rt = tcgv_vec_temp(r);
> -vec_gen_2(INDEX_op_dupi_vec, rt->base_type, vece, temp_arg(rt), a);
> -}
> -
>  TCGv_vec tcg_const_zeros_vec(TCGType type)
>  {
>  TCGv_vec ret = tcg_temp_new_vec(type);
> -do_dupi_vec(ret, MO_REG, 0);
> +tcg_gen_mov_vec(ret, tcg_constant_vec(type, MO_8, 0));
>  return ret;
>  }
>  
>  TCGv_vec tcg_const_ones_vec(TCGType type)
>  {
>  TCGv_vec ret = tcg_temp_new_vec(type);
> -do_dupi_vec(ret, MO_REG, -1);
> +tcg_gen_mov_vec(ret, tcg_constant_vec(type, MO_8, -1));
>  return ret;
>  }
>  
> @@ -267,37 +259,50 @@ TCGv_vec tcg_const_ones_vec_matching(TCGv_vec m)
>  return tcg_const_ones_vec(t->base_type);
>  }
>  
> -void tcg_gen_dup64i_vec(TCGv_vec r, uint64_t a)
> +void tcg_gen_dupi_vec(unsigned vece, TCGv_vec dest, uint64_t val)
>  {
> -if (TCG_TARGET_REG_BITS == 32 && a == deposit64(a, 32, 32, a)) {
> -do_dupi_vec(r, MO_32, a);
> -} else if (TCG_TARGET_REG_BITS == 64 || a == (uint64_t)(int32_t)a) {
> -do_dupi_vec(r, MO_64, a);
> -} else {
> -TCGv_i64 c = tcg_const_i64(a);
> -tcg_gen_dup_i64_vec(MO_64, r, c);
> -tcg_temp_free_i64(c);
> +TCGType type = tcgv_vec_temp(dest)->base_type;
> +
> +/*
> + * For MO_64 constants that can't be represented in tcg_target_long,
> + * we must use INDEX_op_dup2_vec.
> + */
> +if (TCG_TARGET_REG_BITS == 32) {
> +val = dup_const(vece, val);
> +if (val != deposit64(val, 32, 32, val) &&
> +val != (uint64_t)(int32_t)val) {
> +uint32_t vl = extract64(val, 0, 32);
> +uint32_t vh = extract64(val, 32, 32);
> +TCGArg al = tcgv_i32_arg(tcg_constant_i32(vl));
> +TCGArg ah = tcgv_i32_arg(tcg_constant_i32(vh));
> +TCGArg di = tcgv_vec_arg(dest);
> +
> +vec_gen_3(INDEX_op_dup2_vec, type, MO_64, di, al, ah);
> +return;
> +}
>  }
> +
> +tcg_gen_mov_vec(dest, tcg_constant_vec(type, vece, val));
>  }
>  
> -void tcg_gen_dup32i_vec(TCGv_vec r, uint32_t a)
> +void tcg_gen_dup64i_vec(TCGv_vec dest, uint64_t val)
>  {
> -do_dupi_vec(r, MO_REG, dup_const(MO_32, a));
> +tcg_gen_dupi_vec(MO_64, dest, val);
>  }
>  
> -void tcg_gen_dup16i_vec(TCGv_vec r, uint32_t a)
> +void tcg_gen_dup32i_vec(TCGv_vec dest, uint32_t val)
>  {
> -do_dupi_vec(r, MO_REG, dup_const(MO_16, a));
> +tcg_gen_dupi_vec(MO_32, dest, val);
>  }
>  
> -void tcg_gen_dup8i_vec(TCGv_vec r, uint32_t a)
> +void tcg_gen_dup16i_vec(TCGv_vec dest, uint32_t val)
>  {
> -do_dupi_vec(r, MO_REG, dup_const(MO_8, a));
> +tcg_gen_dupi_vec(MO_16, dest, val);
>  }
>  
> -void tcg_gen_dupi_vec(unsigned vece, TCGv_vec r, uint64_t a)
> +void tcg_gen_dup8i_vec(TCGv_vec dest, uint32_t val)
>  {
> -do_dupi_vec(r, MO_REG, dup_const(vece, a));
> +tcg_gen_dupi_vec(MO_8, dest, val);
>  }
>  
>  void tcg_gen_dup_i64_vec(unsigned vece, TCGv_vec r, TCGv_i64 a)
> @@ -502,8 +507,8 @@ void tcg_gen_abs_vec(unsigned vece, TCGv_vec r, TCGv_vec 
> a)
>  if (tcg_can_emit_vec_op(INDEX_op_sari_vec, type, vece) > 0) {
>  tcg_gen_sari_vec(vece, t, a, (8 << vece) - 1);
>  } else {
> -do_dupi_vec(t, MO_REG, 0);
> -tcg_gen_cmp_vec(TCG_COND_LT, vece, t, a, t);
> +tcg_gen_cmp_vec(TCG_COND_LT, vece, t, a,
> +tcg_constant_vec(type, vece, 0));
>  }
>  tcg_gen_xor_vec(vece, r, a, t);
>  tcg_gen_sub_vec(vece, r, r, t);


-- 
Alex Bennée

Re: [PATCH v2 13/36] tcg: Use tcg_constant_{i32,i64} with tcg int expanders

[Richard Henderson]

On 4/22/20 9:18 AM, Alex Bennée wrote:
>>  void tcg_gen_brcondi_i64(TCGCond cond, TCGv_i64 arg1, int64_t arg2, 
>> TCGLabel *l)
>>  {
>> -if (cond == TCG_COND_ALWAYS) {
>> +if (TCG_TARGET_REG_BITS == 64) {
>> +tcg_gen_brcond_i64(cond, arg1, tcg_constant_i64(arg2), l);
>> +} else if (cond == TCG_COND_ALWAYS) {
>>  tcg_gen_br(l);
>>  } else if (cond != TCG_COND_NEVER) {
>> -TCGv_i64 t0 = tcg_const_i64(arg2);
>> -tcg_gen_brcond_i64(cond, arg1, t0, l);
>> -tcg_temp_free_i64(t0);
>> +l->refs++;
> 
> Hmm is this a separate fix?

No, it's expanding what tcg_gen_brcond_i64 would do for TCG_TARGET_REG_BITS == 
32.

>> +tcg_gen_op6ii_i32(INDEX_op_brcond2_i32,
>> +  TCGV_LOW(arg1), TCGV_HIGH(arg1),
>> +  tcg_constant_i32(arg2),
>> +  tcg_constant_i32(arg2 >> 32),
>> +  cond, label_arg(l));

Because we have two separate TCGv_i32, from tcg_constant_i32(), which cannot be
packaged up with TCGV_HIGH/LOW.


r~

Re: [PATCH v2 15/36] tcg: Use tcg_constant_{i32, i64} with tcg plugins

[Alex Bennée]

Richard Henderson  writes:

> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  accel/tcg/plugin-gen.c | 49 +++---
>  1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 51580d51a0..e5dc9d0ca9 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -284,8 +284,8 @@ static TCGOp *copy_extu_i32_i64(TCGOp **begin_op, TCGOp 
> *op)
>  if (TCG_TARGET_REG_BITS == 32) {
>  /* mov_i32 */
>  op = copy_op(begin_op, op, INDEX_op_mov_i32);
> -/* movi_i32 */
> -op = copy_op(begin_op, op, INDEX_op_movi_i32);
> +/* mov_i32 w/ $0 */
> +op = copy_op(begin_op, op, INDEX_op_mov_i32);
>  } else {
>  /* extu_i32_i64 */
>  op = copy_op(begin_op, op, INDEX_op_extu_i32_i64);
> @@ -306,39 +306,34 @@ static TCGOp *copy_mov_i64(TCGOp **begin_op, TCGOp *op)
>  return op;
>  }
>  
> -static TCGOp *copy_movi_i64(TCGOp **begin_op, TCGOp *op, uint64_t v)
> -{
> -if (TCG_TARGET_REG_BITS == 32) {
> -/* 2x movi_i32 */
> -op = copy_op(begin_op, op, INDEX_op_movi_i32);
> -op->args[1] = v;
> -
> -op = copy_op(begin_op, op, INDEX_op_movi_i32);
> -op->args[1] = v >> 32;
> -} else {
> -/* movi_i64 */
> -op = copy_op(begin_op, op, INDEX_op_movi_i64);
> -op->args[1] = v;
> -}
> -return op;
> -}
> -
>  static TCGOp *copy_const_ptr(TCGOp **begin_op, TCGOp *op, void *ptr)
>  {
>  if (UINTPTR_MAX == UINT32_MAX) {
> -/* movi_i32 */
> -op = copy_op(begin_op, op, INDEX_op_movi_i32);
> -op->args[1] = (uintptr_t)ptr;
> +/* mov_i32 */
> +op = copy_op(begin_op, op, INDEX_op_mov_i32);
> +op->args[1] = tcgv_i32_arg(tcg_constant_i32((uintptr_t)ptr));
>  } else {
> -/* movi_i64 */
> -op = copy_movi_i64(begin_op, op, (uint64_t)(uintptr_t)ptr);
> +/* mov_i64 */
> +op = copy_op(begin_op, op, INDEX_op_mov_i64);
> +op->args[1] = tcgv_i64_arg(tcg_constant_i64((uintptr_t)ptr));
>  }
>  return op;
>  }
>  
>  static TCGOp *copy_const_i64(TCGOp **begin_op, TCGOp *op, uint64_t v)
>  {
> -return copy_movi_i64(begin_op, op, v);
> +if (TCG_TARGET_REG_BITS == 32) {
> +/* 2x mov_i32 */
> +op = copy_op(begin_op, op, INDEX_op_mov_i32);
> +op->args[1] = tcgv_i32_arg(tcg_constant_i32(v));
> +op = copy_op(begin_op, op, INDEX_op_mov_i32);
> +op->args[1] = tcgv_i32_arg(tcg_constant_i32(v >> 32));
> +} else {
> +/* mov_i64 */
> +op = copy_op(begin_op, op, INDEX_op_mov_i64);
> +op->args[1] = tcgv_i64_arg(tcg_constant_i64(v));
> +}
> +return op;
>  }
>  
>  static TCGOp *copy_extu_tl_i64(TCGOp **begin_op, TCGOp *op)
> @@ -486,8 +481,8 @@ static TCGOp *append_mem_cb(const struct 
> qemu_plugin_dyn_cb *cb,
>  
>  tcg_debug_assert(type == PLUGIN_GEN_CB_MEM);
>  
> -/* const_i32 == movi_i32 ("info", so it remains as is) */
> -op = copy_op(&begin_op, op, INDEX_op_movi_i32);
> +/* const_i32 == mov_i32 ("info", so it remains as is) */
> +op = copy_op(&begin_op, op, INDEX_op_mov_i32);
>  
>  /* const_ptr */
>  op = copy_const_ptr(&begin_op, op, cb->userp);


-- 
Alex Bennée

Re: [PATCH v2 16/36] tcg: Rename struct tcg_temp_info to TempOptInfo

[Alex Bennée]

Richard Henderson  writes:

> Fix this name vs our coding style.
>
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  tcg/optimize.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index effb47eefd..b86bf3d707 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -35,20 +35,20 @@
>  glue(glue(case INDEX_op_, x), _i64):\
>  glue(glue(case INDEX_op_, x), _vec)
>  
> -struct tcg_temp_info {
> +typedef struct TempOptInfo {
>  bool is_const;
>  TCGTemp *prev_copy;
>  TCGTemp *next_copy;
>  tcg_target_ulong val;
>  tcg_target_ulong mask;
> -};
> +} TempOptInfo;
>  
> -static inline struct tcg_temp_info *ts_info(TCGTemp *ts)
> +static inline TempOptInfo *ts_info(TCGTemp *ts)
>  {
>  return ts->state_ptr;
>  }
>  
> -static inline struct tcg_temp_info *arg_info(TCGArg arg)
> +static inline TempOptInfo *arg_info(TCGArg arg)
>  {
>  return ts_info(arg_temp(arg));
>  }
> @@ -71,9 +71,9 @@ static inline bool ts_is_copy(TCGTemp *ts)
>  /* Reset TEMP's state, possibly removing the temp for the list of copies.  */
>  static void reset_ts(TCGTemp *ts)
>  {
> -struct tcg_temp_info *ti = ts_info(ts);
> -struct tcg_temp_info *pi = ts_info(ti->prev_copy);
> -struct tcg_temp_info *ni = ts_info(ti->next_copy);
> +TempOptInfo *ti = ts_info(ts);
> +TempOptInfo *pi = ts_info(ti->prev_copy);
> +TempOptInfo *ni = ts_info(ti->next_copy);
>  
>  ni->prev_copy = ti->prev_copy;
>  pi->next_copy = ti->next_copy;
> @@ -89,12 +89,12 @@ static void reset_temp(TCGArg arg)
>  }
>  
>  /* Initialize and activate a temporary.  */
> -static void init_ts_info(struct tcg_temp_info *infos,
> +static void init_ts_info(TempOptInfo *infos,
>   TCGTempSet *temps_used, TCGTemp *ts)
>  {
>  size_t idx = temp_idx(ts);
>  if (!test_bit(idx, temps_used->l)) {
> -struct tcg_temp_info *ti = &infos[idx];
> +TempOptInfo *ti = &infos[idx];
>  
>  ts->state_ptr = ti;
>  ti->next_copy = ts;
> @@ -114,7 +114,7 @@ static void init_ts_info(struct tcg_temp_info *infos,
>  }
>  }
>  
> -static void init_arg_info(struct tcg_temp_info *infos,
> +static void init_arg_info(TempOptInfo *infos,
>TCGTempSet *temps_used, TCGArg arg)
>  {
>  init_ts_info(infos, temps_used, arg_temp(arg));
> @@ -177,7 +177,7 @@ static void tcg_opt_gen_movi(TCGContext *s, TCGOp *op, 
> TCGArg dst, TCGArg val)
>  const TCGOpDef *def;
>  TCGOpcode new_op;
>  tcg_target_ulong mask;
> -struct tcg_temp_info *di = arg_info(dst);
> +TempOptInfo *di = arg_info(dst);
>  
>  def = &tcg_op_defs[op->opc];
>  if (def->flags & TCG_OPF_VECTOR) {
> @@ -208,8 +208,8 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, 
> TCGArg dst, TCGArg src)
>  TCGTemp *dst_ts = arg_temp(dst);
>  TCGTemp *src_ts = arg_temp(src);
>  const TCGOpDef *def;
> -struct tcg_temp_info *di;
> -struct tcg_temp_info *si;
> +TempOptInfo *di;
> +TempOptInfo *si;
>  tcg_target_ulong mask;
>  TCGOpcode new_op;
>  
> @@ -242,7 +242,7 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, 
> TCGArg dst, TCGArg src)
>  di->mask = mask;
>  
>  if (src_ts->type == dst_ts->type) {
> -struct tcg_temp_info *ni = ts_info(si->next_copy);
> +TempOptInfo *ni = ts_info(si->next_copy);
>  
>  di->next_copy = si->next_copy;
>  di->prev_copy = src_ts;
> @@ -605,7 +605,7 @@ void tcg_optimize(TCGContext *s)
>  {
>  int nb_temps, nb_globals;
>  TCGOp *op, *op_next, *prev_mb = NULL;
> -struct tcg_temp_info *infos;
> +TempOptInfo *infos;
>  TCGTempSet temps_used;
>  
>  /* Array VALS has an element for each temp.
> @@ -616,7 +616,7 @@ void tcg_optimize(TCGContext *s)
>  nb_temps = s->nb_temps;
>  nb_globals = s->nb_globals;
>  bitmap_zero(temps_used.l, nb_temps);
> -infos = tcg_malloc(sizeof(struct tcg_temp_info) * nb_temps);
> +infos = tcg_malloc(sizeof(TempOptInfo) * nb_temps);
>  
>  QTAILQ_FOREACH_SAFE(op, &s->ops, link, op_next) {
>  tcg_target_ulong mask, partmask, affected;


-- 
Alex Bennée

[PATCH 2/8] docker.py/build: support binary files in --extra-files

[Paolo Bonzini]

Read the --extra-files in binary mode to avoid encoding errors.

Signed-off-by: Paolo Bonzini 
---
 tests/docker/docker.py | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index ad61747bae..85e1dda10f 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -56,15 +56,19 @@ class EngineEnum(enum.IntEnum):
 
 USE_ENGINE = EngineEnum.AUTO
 
+def _bytes_checksum(bytes):
+"""Calculate a digest string unique to the text content"""
+return hashlib.sha1(bytes).hexdigest()
+
 def _text_checksum(text):
 """Calculate a digest string unique to the text content"""
-return hashlib.sha1(text.encode('utf-8')).hexdigest()
+return _bytes_checksum(text.encode('utf-8'))
 
 def _read_dockerfile(path):
 return open(path, 'rt', encoding='utf-8').read()
 
 def _file_checksum(filename):
-return _text_checksum(_read_dockerfile(filename))
+return _bytes_checksum(open(filename, 'rb').read())
 
 
 def _guess_engine_command():
-- 
2.18.2

[PATCH 1/8] docker.py/build: support -t and -f arguments

[Paolo Bonzini]

The docker.py command line is subtly different from docker and podman's,
in that the tag and Dockerfile are passed via positional arguments.
Remove this gratuitous difference and just parse -f and -t.

-f was previously used by --extra-files, only keep the long option.

Signed-off-by: Paolo Bonzini 
---
 tests/docker/Makefile.include | 2 +-
 tests/docker/docker.py| 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 43a8678688..262704663f 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -55,7 +55,7 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
 else
 docker-image-%: $(DOCKER_FILES_DIR)/%.docker
$(call quiet-command,\
-   $(DOCKER_SCRIPT) build qemu:$* $< \
+   $(DOCKER_SCRIPT) build -t qemu:$* -f $< \
$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
$(if $(NOUSER),,--add-current-user) \
$(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))\
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index d8268c..ad61747bae 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -391,16 +391,16 @@ class BuildCommand(SubCommand):
 help="""Specify a binary that will be copied to the
 container together with all its dependent
 libraries""")
-parser.add_argument("--extra-files", "-f", nargs='*',
+parser.add_argument("--extra-files", nargs='*',
 help="""Specify files that will be copied in the
 Docker image, fulfilling the ADD directive from the
 Dockerfile""")
 parser.add_argument("--add-current-user", "-u", dest="user",
 action="store_true",
 help="Add the current user to image's passwd")
-parser.add_argument("tag",
+parser.add_argument("-t", dest="tag",
 help="Image Tag")
-parser.add_argument("dockerfile",
+parser.add_argument("-f", dest="dockerfile",
 help="Dockerfile name")
 
 def run(self, args, argv):
-- 
2.18.2

[PATCH 8/8] run-coverity-scan: support --update-tools-only --docker

[Paolo Bonzini]

Just build the container when run-coverity-scan is invoked with
--update-tools-only --docker.  This requires moving the "docker build"
logic into the update_coverity_tools function.

The only snag is that --update-tools-only --docker requires access to
the dockerfile.  For now just report an error for --src-tarball, and
"docker build" will fail if not in a source tree.  Another possibility
could be to host our container images on a public registry, and use
"FROM qemu:fedora" to make the Dockerfile small enough that it can be
included directly in the run-coverity-scan script.

Signed-off-by: Paolo Bonzini 
---
 scripts/coverity-scan/run-coverity-scan | 39 +++--
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan 
b/scripts/coverity-scan/run-coverity-scan
index 49df8dcc06..900ce9dd14 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -93,6 +93,18 @@ check_upload_permissions() {
 }
 
 
+build_docker_image() {
+# build docker container including the coverity-scan tools
+echo "Building docker container..."
+# TODO: This re-unpacks the tools every time, rather than caching
+# and reusing the image produced by the COPY of the .tgz file.
+# Not sure why.
+tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
+   -t coverity-scanner -f 
scripts/coverity-scan/coverity-scan.docker \
+   --extra-files scripts/coverity-scan/run-coverity-scan \
+ "$COVERITY_TOOL_BASE"/coverity_tool.tgz
+}
+
 update_coverity_tools () {
 # Check for whether we need to download the Coverity tools
 # (either because we don't have a copy, or because it's out of date)
@@ -126,6 +138,11 @@ update_coverity_tools () {
 fi
 fi
 rm -f coverity_tool.md5.new
+cd "$SRCDIR"
+
+if [ "$DOCKER" = yes ]; then
+build_docker_image
+fi
 }
 
 
@@ -245,15 +262,16 @@ fi
 PROJNAME=QEMU
 TARBALL=cov-int.tar.xz
 
-if [ "$UPDATE" = only ] && [ "$DOCKER" = yes ]; then
-echo "Combining --docker and --update-only is not supported"
-exit 1
-fi
-
 if [ "$UPDATE" = only ]; then
 # Just do the tools update; we don't need to check whether
 # we are in a source tree or have upload rights for this,
 # so do it before some of the command line and source tree checks.
+
+if [ "$DOCKER" = yes ] && [ ! -z "$SRCTARBALL" ]; then
+echo --update-tools-only --docker is incompatible with --src-tarball.
+exit 1
+fi
+
 update_coverity_tools
 exit 0
 fi
@@ -315,17 +333,6 @@ if [ "$DOCKER" = yes ]; then
 echo "Created temporary directory $SECRETDIR"
 SECRET="$SECRETDIR/token"
 echo "$COVERITY_TOKEN" > "$SECRET"
-if [ "$UPDATE" != no ]; then
-# build docker container including the coverity-scan tools
-echo "Building docker container..."
-# TODO: This re-unpacks the tools every time, rather than caching
-# and reusing the image produced by the COPY of the .tgz file.
-# Not sure why.
-tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
-   -t coverity-scanner -f 
scripts/coverity-scan/coverity-scan.docker \
-   --extra-files scripts/coverity-scan/run-coverity-scan \
- "$COVERITY_TOOL_BASE"/coverity_tool.tgz
-fi
 echo "Archiving sources to be analyzed..."
 ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
 ARGS="--no-update-tools"
-- 
2.18.2

[PATCH 7/8] run-coverity-scan: download tools outside the container

[Paolo Bonzini]

This lets us look at coverity_tool.md5 across executions of run-coverity-scan
and skip the download.

Signed-off-by: Paolo Bonzini 
---
 scripts/coverity-scan/coverity-scan.docker |  3 +-
 scripts/coverity-scan/run-coverity-scan| 42 +++---
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/scripts/coverity-scan/coverity-scan.docker 
b/scripts/coverity-scan/coverity-scan.docker
index 6f0460b66c..efcf63224d 100644
--- a/scripts/coverity-scan/coverity-scan.docker
+++ b/scripts/coverity-scan/coverity-scan.docker
@@ -127,5 +127,6 @@ RUN dnf install -y $PACKAGES
 RUN rpm -q $PACKAGES | sort > /packages.txt
 ENV PATH $PATH:/usr/libexec/python3-sphinx/
 ENV COVERITY_TOOL_BASE=/coverity-tools
+COPY coverity_tool.tgz coverity_tool.tgz
+RUN mkdir -p /coverity-tools/coverity_tool && cd /coverity-tools/coverity_tool 
&& tar xf /coverity_tool.tgz
 COPY run-coverity-scan run-coverity-scan
-RUN ./run-coverity-scan --update-tools-only --tokenfile /work/token
diff --git a/scripts/coverity-scan/run-coverity-scan 
b/scripts/coverity-scan/run-coverity-scan
index 0c2c0e4087..49df8dcc06 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -114,15 +114,17 @@ update_coverity_tools () {
 echo "Downloaded tarball didn't match md5sum!"
 exit 1
 fi
-# extract the new one, keeping it corralled in a 'coverity_tool' 
directory
-echo "Unpacking coverity build tools..."
-mkdir -p coverity_tool
-cd coverity_tool
-tar xf ../coverity_tool.tgz
-cd ..
-mv coverity_tool.md5.new coverity_tool.md5
-fi
 
+if [ "$DOCKER" != yes ]; then
+# extract the new one, keeping it corralled in a 'coverity_tool' 
directory
+echo "Unpacking coverity build tools..."
+mkdir -p coverity_tool
+cd coverity_tool
+tar xf ../coverity_tool.tgz
+cd ..
+mv coverity_tool.md5.new coverity_tool.md5
+fi
+fi
 rm -f coverity_tool.md5.new
 }
 
@@ -289,6 +291,14 @@ if [ -z "$COVERITY_EMAIL" ]; then
 COVERITY_EMAIL="$(git config user.email)"
 fi
 
+# Otherwise, continue with the full build and upload process.
+
+check_upload_permissions
+
+if [ "$UPDATE" != no ]; then
+update_coverity_tools
+fi
+
 # Run ourselves inside docker if that's what the user wants
 if [ "$DOCKER" = yes ]; then
 # Put the Coverity token into a temporary file that only
@@ -308,13 +318,13 @@ if [ "$DOCKER" = yes ]; then
 if [ "$UPDATE" != no ]; then
 # build docker container including the coverity-scan tools
 echo "Building docker container..."
-# TODO: This re-downloads the tools every time, rather than
-# caching and reusing the image produced with the downloaded tools.
+# TODO: This re-unpacks the tools every time, rather than caching
+# and reusing the image produced by the COPY of the .tgz file.
 # Not sure why.
 tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
-t coverity-scanner -f 
scripts/coverity-scan/coverity-scan.docker \
-   -v "$SECRETDIR:/work" \
-   --extra-files scripts/coverity-scan/run-coverity-scan
+   --extra-files scripts/coverity-scan/run-coverity-scan \
+ "$COVERITY_TOOL_BASE"/coverity_tool.tgz
 fi
 echo "Archiving sources to be analyzed..."
 ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
@@ -345,14 +355,6 @@ if [ "$DOCKER" = yes ]; then
 exit 0
 fi
 
-# Otherwise, continue with the full build and upload process.
-
-check_upload_permissions
-
-if [ "$UPDATE" != no ]; then
-update_coverity_tools
-fi
-
 TOOLBIN="$(cd "$COVERITY_TOOL_BASE" && echo 
$PWD/coverity_tool/cov-analysis-*/bin)"
 
 if ! test -x "$TOOLBIN/cov-build"; then
-- 
2.18.2

[PATCH 4/8] run-coverity-scan: use docker.py

[Paolo Bonzini]

Our trusted docker wrapper allows run-coverity-scan to run with both
docker and podman.

For the "run" phase this is transparent; for the "build" phase however
scripts are replaced with a bind mount (-v).  This is not an issue
because the secret option is meant for secrets stored globally in the
system and bind mounts are a valid substitute for secrets that are known
to whoever builds the container.

This also removes the need for DOCKER_BUILDKIT=1.

Signed-off-by: Paolo Bonzini 
---
Later in the series, the secret will not be used in "docker build"
at all.

 scripts/coverity-scan/coverity-scan.docker |  2 +-
 scripts/coverity-scan/run-coverity-scan| 23 --
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/scripts/coverity-scan/coverity-scan.docker 
b/scripts/coverity-scan/coverity-scan.docker
index a4f64d1283..6f0460b66c 100644
--- a/scripts/coverity-scan/coverity-scan.docker
+++ b/scripts/coverity-scan/coverity-scan.docker
@@ -128,4 +128,4 @@ RUN rpm -q $PACKAGES | sort > /packages.txt
 ENV PATH $PATH:/usr/libexec/python3-sphinx/
 ENV COVERITY_TOOL_BASE=/coverity-tools
 COPY run-coverity-scan run-coverity-scan
-RUN --mount=type=secret,id=coverity.token,required ./run-coverity-scan 
--update-tools-only --tokenfile /run/secrets/coverity.token
+RUN ./run-coverity-scan --update-tools-only --tokenfile /work/token
diff --git a/scripts/coverity-scan/run-coverity-scan 
b/scripts/coverity-scan/run-coverity-scan
index f7325b570c..ae1fc7ae76 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -197,6 +197,12 @@ while [ "$#" -ge 1 ]; do
 ;;
 --docker)
 DOCKER=yes
+DOCKER_ENGINE=auto
+shift
+;;
+--docker=*)
+DOCKER=yes
+DOCKER_ENGINE=${1#--docker=}
 shift
 ;;
 *)
@@ -283,9 +289,8 @@ if [ "$DOCKER" = yes ]; then
 # build docker container including the coverity-scan tools
 # Put the Coverity token into a temporary file that only
 # we have read access to, and then pass it to docker build
-# using --secret. This requires at least Docker 18.09.
-# Mostly what we are trying to do here is ensure we don't leak
-# the token into the Docker image.
+# using a volume.  A volume is enough for the token not to
+# leak into the Docker image.
 umask 077
 SECRETDIR=$(mktemp -d)
 if [ -z "$SECRETDIR" ]; then
@@ -300,12 +305,10 @@ if [ "$DOCKER" = yes ]; then
 # TODO: This re-downloads the tools every time, rather than
 # caching and reusing the image produced with the downloaded tools.
 # Not sure why.
-# TODO: how do you get 'docker build' to print the output of the
-# commands it is running to its stdout? This would be useful for debug.
-DOCKER_BUILDKIT=1 docker build -t coverity-scanner \
-   --secret id=coverity.token,src="$SECRET" \
-   -f scripts/coverity-scan/coverity-scan.docker \
-   scripts/coverity-scan
+tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
+   -t coverity-scanner -f 
scripts/coverity-scan/coverity-scan.docker \
+   -v "$SECRETDIR:/work" \
+   --extra-files scripts/coverity-scan/run-coverity-scan
 echo "Archiving sources to be analyzed..."
 ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
 if [ "$DRYRUN" = yes ]; then
@@ -323,7 +326,7 @@ if [ "$DOCKER" = yes ]; then
 # Arrange for this docker run to get access to the sources with -v.
 # We pass through all the configuration from the outer script to the inner.
 export COVERITY_EMAIL COVERITY_BUILD_CMD
-docker run -it --env COVERITY_EMAIL --env COVERITY_BUILD_CMD \
+tests/docker/docker.py run -it --env COVERITY_EMAIL --env 
COVERITY_BUILD_CMD \
-v "$SECRETDIR:/work" coverity-scanner \
./run-coverity-scan --version "$VERSION" \
--description "$DESCRIPTION" $DRYRUNARG --tokenfile /work/token \
-- 
2.18.2

[PATCH 0/8] run-coverity-scan: misc improvements, especially for docker mode

[Paolo Bonzini]

These include:

1) podman support through tests/docker/docker.py

2) avoiding repeated downloading of the tools in the container, by
   sharing the cache with the host

3) support for --update-tools-only --docker (though unlike regular
   --update-tools-only it must be run from within a QEMU source tree)

4) not related to docker mode, but used by it, a new --no-update-tools
   option that does not check for tool updates

5) the ability to get the Coverity token from git configuration, and
   also to have email from git configuration if it is not equal to
   user.email.

Patches 1 and 2 are tweaks to tests/docker/docker.py, while the others
are for run-coverity-scan.

Paolo Bonzini (8):
  docker.py/build: support -t and -f arguments
  docker.py/build: support binary files in --extra-files
  run-coverity-scan: get Coverity token and email from special git
config section
  run-coverity-scan: use docker.py
  run-coverity-scan: add --no-update-tools option
  run-coverity-scan: use --no-update-tools in docker run
  run-coverity-scan: download tools outside the container
  run-coverity-scan: support --update-tools-only --docker

 scripts/coverity-scan/coverity-scan.docker |   3 +-
 scripts/coverity-scan/run-coverity-scan| 126 +
 tests/docker/Makefile.include  |   2 +-
 tests/docker/docker.py |  14 ++-
 4 files changed, 88 insertions(+), 57 deletions(-)

-- 
2.18.2

[PATCH 6/8] run-coverity-scan: use --no-update-tools in docker run

[Paolo Bonzini]

Tools are already updated via the docker build.

Signed-off-by: Paolo Bonzini 
---
 scripts/coverity-scan/run-coverity-scan | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan 
b/scripts/coverity-scan/run-coverity-scan
index 9403429849..0c2c0e4087 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -318,17 +318,16 @@ if [ "$DOCKER" = yes ]; then
 fi
 echo "Archiving sources to be analyzed..."
 ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
+ARGS="--no-update-tools"
 if [ "$DRYRUN" = yes ]; then
-DRYRUNARG=--dry-run
+ARGS="$ARGS --dry-run"
 fi
 echo "Running scanner..."
 # If we need to capture the output tarball, get the inner run to
 # save it to the secrets directory so we can copy it out before the
 # directory is cleaned up.
 if [ ! -z "$RESULTSTARBALL" ]; then
-RTARGS="--results-tarball /work/cov-int.tar.xz"
-else
-RTARGS=""
+ARGS="$ARGS --results-tarball /work/cov-int.tar.xz"
 fi
 # Arrange for this docker run to get access to the sources with -v.
 # We pass through all the configuration from the outer script to the inner.
@@ -336,8 +335,8 @@ if [ "$DOCKER" = yes ]; then
 tests/docker/docker.py run -it --env COVERITY_EMAIL --env 
COVERITY_BUILD_CMD \
-v "$SECRETDIR:/work" coverity-scanner \
./run-coverity-scan --version "$VERSION" \
-   --description "$DESCRIPTION" $DRYRUNARG --tokenfile /work/token \
-   --srcdir /qemu --src-tarball /work/qemu-sources.tgz $RTARGS
+   --description "$DESCRIPTION" $ARGS --tokenfile /work/token \
+   --srcdir /qemu --src-tarball /work/qemu-sources.tgz
 if [ ! -z "$RESULTSTARBALL" ]; then
 echo "Copying results tarball to $RESULTSTARBALL..."
 cp "$SECRETDIR/cov-int.tar.xz" "$RESULTSTARBALL"
-- 
2.18.2

[PATCH 3/8] run-coverity-scan: get Coverity token and email from special git config section

[Paolo Bonzini]

Support a [coverity] section in .git/config.  It can be used to retrieve the
token and also, if it is different from user.email, the username of the
submitter.

Signed-off-by: Paolo Bonzini 
---
 scripts/coverity-scan/run-coverity-scan | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan 
b/scripts/coverity-scan/run-coverity-scan
index 2e067ef5cf..f7325b570c 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -41,9 +41,10 @@
 #   is intended mainly for internal use by the Docker support
 #
 # User-specifiable environment variables:
-#  COVERITY_TOKEN -- Coverity token
+#  COVERITY_TOKEN -- Coverity token (default: looks at your
+#coverity.token config)
 #  COVERITY_EMAIL -- the email address to use for uploads (default:
-#looks at your git user.email config)
+#looks at your git coverity.email or user.email config)
 #  COVERITY_BUILD_CMD -- make command (default: 'make -jN' where N is
 #number of CPUs as determined by 'nproc')
 #  COVERITY_TOOL_BASE -- set to directory to put coverity tools
@@ -62,7 +63,7 @@ check_upload_permissions() {
 
 echo "Checking upload permissions..."
 
-if ! up_perm="$(wget https://scan.coverity.com/api/upload_permitted 
--post-data "token=$PROJTOKEN&project=$PROJNAME" -q -O -)"; then
+if ! up_perm="$(wget https://scan.coverity.com/api/upload_permitted 
--post-data "token=$COVERITY_TOKEN&project=$PROJNAME" -q -O -)"; then
 echo "Coverity Scan API access denied: bad token?"
 exit 1
 fi
@@ -100,14 +101,14 @@ update_coverity_tools () {
 cd "$COVERITY_TOOL_BASE"
 
 echo "Checking for new version of coverity build tools..."
-wget https://scan.coverity.com/download/linux64 --post-data 
"token=$PROJTOKEN&project=$PROJNAME&md5=1" -O coverity_tool.md5.new
+wget https://scan.coverity.com/download/linux64 --post-data 
"token=$COVERITY_TOKEN&project=$PROJNAME&md5=1" -O coverity_tool.md5.new
 
 if ! cmp -s coverity_tool.md5 coverity_tool.md5.new; then
 # out of date md5 or no md5: download new build tool
 # blow away the old build tool
 echo "Downloading coverity build tools..."
 rm -rf coverity_tool coverity_tool.tgz
-wget https://scan.coverity.com/download/linux64 --post-data 
"token=$PROJTOKEN&project=$PROJNAME" -O coverity_tool.tgz
+wget https://scan.coverity.com/download/linux64 --post-data 
"token=$COVERITY_TOKEN&project=$PROJNAME" -O coverity_tool.tgz
 if ! (cat coverity_tool.md5.new; echo "  coverity_tool.tgz") | md5sum 
-c --status; then
 echo "Downloaded tarball didn't match md5sum!"
 exit 1
@@ -205,6 +206,9 @@ while [ "$#" -ge 1 ]; do
 esac
 done
 
+if [ -z "$COVERITY_TOKEN" ]; then
+COVERITY_TOKEN="$(git config coverity.token)"
+fi
 if [ -z "$COVERITY_TOKEN" ]; then
 echo "COVERITY_TOKEN environment variable not set"
 exit 1
@@ -225,7 +229,6 @@ if [ -z "$SRCDIR" ]; then
 SRCDIR="$PWD"
 fi
 
-PROJTOKEN="$COVERITY_TOKEN"
 PROJNAME=QEMU
 TARBALL=cov-int.tar.xz
 
@@ -268,6 +271,9 @@ if [ -z "$DESCRIPTION" ]; then
 DESCRIPTION="$(git rev-parse HEAD)"
 fi
 
+if [ -z "$COVERITY_EMAIL" ]; then
+COVERITY_EMAIL="$(git config coverity.email)"
+fi
 if [ -z "$COVERITY_EMAIL" ]; then
 COVERITY_EMAIL="$(git config user.email)"
 fi
@@ -393,7 +399,7 @@ if [ "$DRYRUN" = yes ]; then
 exit 0
 fi
 
-curl --form token="$PROJTOKEN" --form email="$COVERITY_EMAIL" \
+curl --form token="$COVERITY_TOKEN" --form email="$COVERITY_EMAIL" \
  --form file=@"$TARBALL" --form version="$VERSION" \
  --form description="$DESCRIPTION" \
  https://scan.coverity.com/builds?project="$PROJNAME";
-- 
2.18.2

[PATCH 5/8] run-coverity-scan: add --no-update-tools option

[Paolo Bonzini]

Provide a quick way to skip building the container while we figure out how
to get caching right.

Signed-off-by: Paolo Bonzini 
---
 scripts/coverity-scan/run-coverity-scan | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan 
b/scripts/coverity-scan/run-coverity-scan
index ae1fc7ae76..9403429849 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -31,6 +31,7 @@
 #   --dry-run : run the tools, but don't actually do the upload
 #   --docker : create and work inside a docker container
 #   --update-tools-only : update the cached copy of the tools, but don't run 
them
+#   --no-update-tools : do not update the cached copy of the tools
 #   --tokenfile : file to read Coverity token from
 #   --version ver : specify version being analyzed (default: ask git)
 #   --description desc : specify description of this version (default: ask git)
@@ -128,7 +129,7 @@ update_coverity_tools () {
 
 # Check user-provided environment variables and arguments
 DRYRUN=no
-UPDATE_ONLY=no
+UPDATE=yes
 DOCKER=no
 
 while [ "$#" -ge 1 ]; do
@@ -137,9 +138,13 @@ while [ "$#" -ge 1 ]; do
 shift
 DRYRUN=yes
 ;;
+--no-update-tools)
+shift
+UPDATE=no
+;;
 --update-tools-only)
 shift
-UPDATE_ONLY=yes
+UPDATE=only
 ;;
 --version)
 shift
@@ -238,12 +243,12 @@ fi
 PROJNAME=QEMU
 TARBALL=cov-int.tar.xz
 
-if [ "$UPDATE_ONLY" = yes ] && [ "$DOCKER" = yes ]; then
+if [ "$UPDATE" = only ] && [ "$DOCKER" = yes ]; then
 echo "Combining --docker and --update-only is not supported"
 exit 1
 fi
 
-if [ "$UPDATE_ONLY" = yes ]; then
+if [ "$UPDATE" = only ]; then
 # Just do the tools update; we don't need to check whether
 # we are in a source tree or have upload rights for this,
 # so do it before some of the command line and source tree checks.
@@ -286,7 +291,6 @@ fi
 
 # Run ourselves inside docker if that's what the user wants
 if [ "$DOCKER" = yes ]; then
-# build docker container including the coverity-scan tools
 # Put the Coverity token into a temporary file that only
 # we have read access to, and then pass it to docker build
 # using a volume.  A volume is enough for the token not to
@@ -301,14 +305,17 @@ if [ "$DOCKER" = yes ]; then
 echo "Created temporary directory $SECRETDIR"
 SECRET="$SECRETDIR/token"
 echo "$COVERITY_TOKEN" > "$SECRET"
-echo "Building docker container..."
-# TODO: This re-downloads the tools every time, rather than
-# caching and reusing the image produced with the downloaded tools.
-# Not sure why.
-tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
-   -t coverity-scanner -f 
scripts/coverity-scan/coverity-scan.docker \
-   -v "$SECRETDIR:/work" \
-   --extra-files scripts/coverity-scan/run-coverity-scan
+if [ "$UPDATE" != no ]; then
+# build docker container including the coverity-scan tools
+echo "Building docker container..."
+# TODO: This re-downloads the tools every time, rather than
+# caching and reusing the image produced with the downloaded tools.
+# Not sure why.
+tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
+   -t coverity-scanner -f 
scripts/coverity-scan/coverity-scan.docker \
+   -v "$SECRETDIR:/work" \
+   --extra-files scripts/coverity-scan/run-coverity-scan
+fi
 echo "Archiving sources to be analyzed..."
 ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
 if [ "$DRYRUN" = yes ]; then
@@ -343,7 +350,9 @@ fi
 
 check_upload_permissions
 
-update_coverity_tools
+if [ "$UPDATE" != no ]; then
+update_coverity_tools
+fi
 
 TOOLBIN="$(cd "$COVERITY_TOOL_BASE" && echo 
$PWD/coverity_tool/cov-analysis-*/bin)"
 
-- 
2.18.2

Re: FYI GitHub pull request / issue tracker lockdown bot

[Paolo Bonzini]

On 22/04/20 18:01, Stefan Hajnoczi wrote:
> On Fri, Apr 03, 2020 at 03:22:13PM +0100, Daniel P. Berrangé wrote:
>> QEMU, like libvirt, has a github.com project which contains automated
>> read-only mirrors of QEMU repositories.
>>
>>   https://github.com/qemu/
>>
>> An unfortunate side effect of this is that some users will try to open
>> pull requests against these mirrors. These get ignored until eventually
>> someone notices and closes the request. QEMU has had about 90 prs opened
>> over the years.
>>
>>   https://github.com/qemu/qemu/pulls
>>
>> The same applies to the issue tracker, but fortunately github lets
>> projects disable this feature, which QEMU has done.
>>
>> I have recently discovered that there is a nice 3rd party bot for github
>> which can autorespond to pull requests with a friendly comment, close the
>> request, and then lock it to prevent further comments.
>>
>>   https://github.com/apps/repo-lockdown
>>
>> I'm setting this up for libvirt and it was suggested QEMU can probably
>> benefit from it too as an example see:
>>
>>   https://github.com/libvirt/test/issues/2
>>   https://github.com/libvirt/test/pull/3
>>
>>
>> Configuration just requires creation of a ".github/lockdown.yml" file
>> which provides the friendly message to add to the merge requests. This
>> can be either done per-repository, or a special repo can be created
>> called ".github" and this will apply to all repos within the project.
>>
>> Ideally each repo would have a CONTRIBUTING.md file created too, since
>> both GitHub and GitLab will direct users to this file for guidelines
>> on how to contribute.
>>
>> I don't have time right now to do this for QEMU, so consider this email
>> a friendly suggestion for some other interested person to do for QEMU...

Philippe did it on April 6!

Thanks,

Paolo




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/8] docker.py/build: support binary files in --extra-files

[Philippe Mathieu-Daudé]

On 4/22/20 7:23 PM, Paolo Bonzini wrote:

Read the --extra-files in binary mode to avoid encoding errors.

Signed-off-by: Paolo Bonzini 
---
  tests/docker/docker.py | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index ad61747bae..85e1dda10f 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -56,15 +56,19 @@ class EngineEnum(enum.IntEnum):
  
  USE_ENGINE = EngineEnum.AUTO
  
+def _bytes_checksum(bytes):

+"""Calculate a digest string unique to the text content"""
+return hashlib.sha1(bytes).hexdigest()
+
  def _text_checksum(text):
  """Calculate a digest string unique to the text content"""
-return hashlib.sha1(text.encode('utf-8')).hexdigest()
+return _bytes_checksum(text.encode('utf-8'))
  
  def _read_dockerfile(path):

  return open(path, 'rt', encoding='utf-8').read()
  
  def _file_checksum(filename):

-return _text_checksum(_read_dockerfile(filename))
+return _bytes_checksum(open(filename, 'rb').read())
  
  
  def _guess_engine_command():




Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 1/8] docker.py/build: support -t and -f arguments

[Philippe Mathieu-Daudé]

On 4/22/20 7:23 PM, Paolo Bonzini wrote:

The docker.py command line is subtly different from docker and podman's,
in that the tag and Dockerfile are passed via positional arguments.
Remove this gratuitous difference and just parse -f and -t.

-f was previously used by --extra-files, only keep the long option.

Signed-off-by: Paolo Bonzini 
---
  tests/docker/Makefile.include | 2 +-
  tests/docker/docker.py| 6 +++---
  2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 43a8678688..262704663f 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -55,7 +55,7 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
  else
  docker-image-%: $(DOCKER_FILES_DIR)/%.docker
$(call quiet-command,\
-   $(DOCKER_SCRIPT) build qemu:$* $< \
+   $(DOCKER_SCRIPT) build -t qemu:$* -f $< \
$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
$(if $(NOUSER),,--add-current-user) \
$(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))\
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index d8268c..ad61747bae 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -391,16 +391,16 @@ class BuildCommand(SubCommand):
  help="""Specify a binary that will be copied to 
the
  container together with all its dependent
  libraries""")
-parser.add_argument("--extra-files", "-f", nargs='*',
+parser.add_argument("--extra-files", nargs='*',
  help="""Specify files that will be copied in the
  Docker image, fulfilling the ADD directive from 
the
  Dockerfile""")
  parser.add_argument("--add-current-user", "-u", dest="user",
  action="store_true",
  help="Add the current user to image's passwd")
-parser.add_argument("tag",
+parser.add_argument("-t", dest="tag",
  help="Image Tag")
-parser.add_argument("dockerfile",
+parser.add_argument("-f", dest="dockerfile",
  help="Dockerfile name")
  
  def run(self, args, argv):




Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH for-5.0?] target/arm: Fix ID_MMFR4 value on AArch64 'max' CPU

[Philippe Mathieu-Daudé]

On 4/22/20 2:45 PM, Peter Maydell wrote:

In commit 41a4bf1feab098da4cd the added code to set the CNP
field in ID_MMFR4 for the AArch64 'max' CPU had a typo
where it used the wrong variable name, resulting in ID_MMFR4
fields AC2, XNX and LSM being wrong. Fix the typo.

Fixes: 41a4bf1feab098da4cd
Reported-by: Laurent Desnogues 


Nice testing/catch Laurent!

Reviewed-by: Philippe Mathieu-Daudé 


Signed-off-by: Peter Maydell 
---
maybe 5.0 just because it's so trivial. I dunno...

There's also an error where we use the uint32_t u variable
to update the 64-bit ID_AA64DFR0 register, but that's harmless
because as it happens the top 32 bits of that register are
all zeroes anyway, so we can just fix that in 5.1.

  target/arm/cpu64.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 62d36f9e8d3..95d0c8c101a 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -705,7 +705,7 @@ static void aarch64_max_initfn(Object *obj)
  u = cpu->isar.id_mmfr4;
  u = FIELD_DP32(u, ID_MMFR4, HPDS, 1); /* AA32HPD */
  u = FIELD_DP32(u, ID_MMFR4, AC2, 1); /* ACTLR2, HACTLR2 */
-u = FIELD_DP32(t, ID_MMFR4, CNP, 1); /* TTCNP */
+u = FIELD_DP32(u, ID_MMFR4, CNP, 1); /* TTCNP */
  cpu->isar.id_mmfr4 = u;
  
  u = cpu->isar.id_aa64dfr0;

Re: [PATCH v4 20/30] qcow2: Add subcluster support to discard_in_l2_slice()

[Alberto Garcia]

On Wed 22 Apr 2020 01:35:25 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 17.03.2020 21:16, Alberto Garcia wrote:
>> Two changes are needed in this function:
>> 
>> 1) A full discard deallocates a cluster so we can skip the operation if
>> it is already unallocated. With extended L2 entries however if any
>> of the subclusters has the 'all zeroes' bit set then we have to
>> clear it.
>> 
>> 2) Setting the QCOW_OFLAG_ZERO bit of the L2 entry is forbidden if an
>> image has extended L2 entries. Instead, the individual 'all zeroes'
>> bits must be used.
>> 
>> Signed-off-by: Alberto Garcia 
>> ---
>>   block/qcow2-cluster.c | 18 +++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>> 
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 746006a117..824c710760 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -1790,12 +1790,20 @@ static int discard_in_l2_slice(BlockDriverState *bs, 
>> uint64_t offset,
>>* TODO We might want to use bdrv_block_status(bs) here, but we're
>>* holding s->lock, so that doesn't work today.
>>*
>> - * If full_discard is true, the sector should not read back as 
>> zeroes,
>> + * If full_discard is true, the cluster should not read back as 
>> zeroes,
>>* but rather fall through to the backing file.
>>*/
>>   switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
>>   case QCOW2_CLUSTER_UNALLOCATED:
>> -if (full_discard || !bs->backing) {
>> +if (full_discard) {
>> +/* If the image has extended L2 entries we can only
>> + * skip this operation if the L2 bitmap is zero. */
>> +uint64_t bitmap = has_subclusters(s) ?
>> +get_l2_bitmap(s, l2_slice, l2_index + i) : 0;
>> +if (bitmap == 0) {
>> +continue;
>> +}
>> +} else if (!bs->backing) {
>>   continue;
>>   }
>
> Hmm, so you do continue if full_discard is false AND bitmap != 0 &
> !bs->backing,

> but you do not continue if full_discard is true AND bitmap != 0 &
> !bs->backing (as you will not go to "else") branch.

1. If full_discard is true it means that the entry and the bitmap should
   always be set to 0, regardless of whether there's a backing file or
   any other consideration.

   This is used e.g when shrinking an image, or by qcow2_make_empty().

   We can only skip this operation if both the entry and the bitmap are
   already 0 (the former we know because of QCOW2_CLUSTER_UNALLOCATED).

2. If full_discard is false it means that we must ensure that the
   cluster reads back as zeroes, but there's no need to clear the bitmap
   (in fact we must set QCOW_OFLAG_ZERO or QCOW_L2_BITMAP_ALL_ZEROES
   depending on the type of image).

   We can skip this operation if there's no backing file and the cluster
   is already unallocated (because then we know that it already reads as
   zeroes).

   One optimization would be to skip the operation also if the image has
   subclusters and the bitmap is QCOW_L2_BITMAP_ALL_ZEROES, I can do
   that for the next version.

> In case QCOW2_CLUSTER_ZERO_PLAIN, worth assert !has_subclusters(s) or
> mark image corrupted ?

I think that should be handled directly in qcow2_get_cluster_type().

There's currently an inconsistency now that I think of it: if an image
has subclusters and QCOW_OFLAG_ZERO set then qcow2_get_cluster_type()
returns QCOW2_CLUSTER_ZERO_* but qcow2_get_subcluster_type() returns
QCOW2_SUBCLUSTER_INVALID.

Two alternatives:

  - We add QCOW2_CLUSTER_INVALID so we get an error in both
cases. Problem: any function that calls qcow2_get_cluster_type()
should be modified to handle that.

  - We ignore QCOW_OFLAG_ZERO. Simpler, and it would allow us to use
that bit in the future if we wanted.

Berto

Re: [PATCH for-5.0?] target/arm: Fix ID_MMFR4 value on AArch64 'max' CPU

[Philippe Mathieu-Daudé]

On Wed, Apr 22, 2020 at 7:41 PM Philippe Mathieu-Daudé
 wrote:
>
> On 4/22/20 2:45 PM, Peter Maydell wrote:
> > In commit 41a4bf1feab098da4cd the added code to set the CNP
> > field in ID_MMFR4 for the AArch64 'max' CPU had a typo
> > where it used the wrong variable name, resulting in ID_MMFR4
> > fields AC2, XNX and LSM being wrong. Fix the typo.
> >
> > Fixes: 41a4bf1feab098da4cd
> > Reported-by: Laurent Desnogues 
>
> Nice testing/catch Laurent!
>
> Reviewed-by: Philippe Mathieu-Daudé 
>
> > Signed-off-by: Peter Maydell 
> > ---
> > maybe 5.0 just because it's so trivial. I dunno...

BTW FWIW LGTM...

> >
> > There's also an error where we use the uint32_t u variable
> > to update the 64-bit ID_AA64DFR0 register, but that's harmless
> > because as it happens the top 32 bits of that register are
> > all zeroes anyway, so we can just fix that in 5.1.
> >
> >   target/arm/cpu64.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 62d36f9e8d3..95d0c8c101a 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -705,7 +705,7 @@ static void aarch64_max_initfn(Object *obj)
> >   u = cpu->isar.id_mmfr4;
> >   u = FIELD_DP32(u, ID_MMFR4, HPDS, 1); /* AA32HPD */
> >   u = FIELD_DP32(u, ID_MMFR4, AC2, 1); /* ACTLR2, HACTLR2 */
> > -u = FIELD_DP32(t, ID_MMFR4, CNP, 1); /* TTCNP */
> > +u = FIELD_DP32(u, ID_MMFR4, CNP, 1); /* TTCNP */
> >   cpu->isar.id_mmfr4 = u;
> >
> >   u = cpu->isar.id_aa64dfr0;
> >

Re: [PATCH] roms: opensbi: Upgrade from v0.6 to v0.7

[Alistair Francis]

On Mon, Apr 20, 2020 at 6:34 PM Bin Meng  wrote:
>
> Hi Alistair,
>
> On Tue, Apr 21, 2020 at 2:41 AM Alistair Francis  wrote:
> >
> > On Mon, Apr 20, 2020 at 6:25 AM Bin Meng  wrote:
> > >
> > > Upgrade OpenSBI from v0.6 to v0.7 and the pre-built bios images.
> > >
> > > The v0.7 release includes the following commits:
> > >
> > > f64f4b9 lib: Add a new platform feature to bringup secondary harts
> > > b677a9b lib: Implement hart hotplug
> > > 5b48240 lib: Add possible hart status values
> > > e3f69fc lib: Implement Hart State Management (HSM) SBI extension
> > > 6704216 lib: Check MSIP bit after returning from WFI
> > > 82ae8e8 makefile: Do setup of the install target more flexible
> > > e1a5b73 platform: sifive: fu540: allow sv32 as an mmu-type
> > > 8c83fb2 lib: Fix return type of sbi_hsm_hart_started()
> > > 00d332b include: Move bits related defines and macros to sbi_bitops.h
> > > a148996 include: sbi_bitops: More useful bit operations
> > > 4a603eb platform: kendryte/k210: Set per-HART stack size to 8KB
> > > 678c3c3 include: sbi_scratch: Set per-HART scratch size to 4KB
> > > 2abc55b lib: Sort build objects in alphabetical order
> > > 6e87507 platform: ae350: Sort build objects in alphabetical order
> > > 650c0e5 lib: sbi: Fix coding style issues
> > > 078686d lib: serial: Fix coding style issues
> > > 3226bd9 lib: Simple bitmap library
> > > c741abc include: Simple hartmask library
> > > d6d7e18 lib: sbi_init: Don't allow HARTID greater than 
> > > SBI_HARTMASK_MAX_BITS
> > > a4a6a81 lib: Introduce SBI_TLB_INFO_INIT() helper macro
> > > d963164 lib: sbi_tlb: Use sbi_hartmask in sbi_tlb_info
> > > 71d2b83 lib: Move all coldboot wait APIs to sbi_init.c
> > > 2b945fc lib: sbi_init: Use hartmask for coldboot wait
> > > 44ce5b9 include: Remove disabled_hart_mask from sbi_platform
> > > 2db381f lib: Introduce sbi_hsm_hart_started_mask() API
> > > 61f7768 lib: sbi_ecall_legacy: Use sbi_hsm_hart_started_mask() API
> > > 466fecb lib: sbi_system: Use sbi_hsm_hart_started_mask() API
> > > 9aad831 lib: sbi_ipi: Use sbi_hsm_hart_started_mask() API
> > > eede1aa lib: sbi_hart: Remove HART available mask and related APIs
> > > 757bb44 docs: Remove out-of-date documentation
> > > 86d37bb lib: sbi: Fix misaligned trap handling
> > > ffdc858 platform: ariane-fpga: Change license for ariane-fpga from 
> > > GPL-2.0 to BSD-2
> > > 4b2f594 sbi: Add definitions for true/false
> > > 0cfe49a libfdt: Add INT32_MAX and UINT32_MAX in libfdt_env.h
> > > baac7e0 libfdt: Upgrade to v1.5.1 release
> > > f92147c include: Make sbi_hart_id_to_scratch() as macro
> > > eeae3d9 firmware: fw_base: Optimize _hartid_to_scratch() implementation
> > > 16e7071 lib: sbi_hsm: Optimize sbi_hsm_hart_get_state() implementation
> > > 823345e include: Make sbi_current_hartid() as macro in riscv_asm.h
> > > 9aabba2 Makefile: Fix distclean make target
> > > 9275ed3 platform: ariane-fpga: Set per-HART stack size to 8KB
> > > 2343efd platform: Set per-HART stack size to 8KB in the template platform 
> > > codes
> > > 72a0628 platform: Use one unified per-HART stack size macro for all 
> > > platforms
> > > 327ba36 scripts: Cover sifive/fu540 in the 32-bit build
> > > 5fbcd62 lib: sbi: Update pmp_get() to return decoded size directly
> > > dce8846 libfdt: Compile fdt_addresses.c
> > > fcb1ded lib: utils: Add a fdt_reserved_memory_fixup() helper
> > > 666be6d platform: Clean up include header files
> > > 6af5576 lib: utils: Move PLIC DT fix up codes to fdt_helper.c
> > > e846ce1 platform: andes/ae350: Fix up DT for reserved memory
> > > 8135520 platform: ariane-fpga: Fix up DT for reserved memory
> > > c9a5268 platform: qemu/virt: Fix up DT for reserved memory
> > > 6f9bb83 platform: sifive/fu540: Fix up DT for reserved memory
> > > 1071f05 platform: sifive/fu540: Remove "stdout-path" fix-up
> > > dd9439f lib: utils: Add a fdt_cpu_fixup() helper
> > > 3f1c847 platform: sifive/fu540: Replace cpu0 node fix-up with the new 
> > > helper
> > > db6a2b5 lib: utils: Add a general device tree fix-up helper
> > > 3f8d754 platform: Update to call general DT fix-up helper
> > > 87a7ef7 lib: sbi_scratch: Introduce HART id to scratch table
> > > e23d3ba include: Simplify HART id to scratch macro
> > > 19bd531 lib: sbi_hsm: Simplify hart_get_state() and hart_started() APIs
> > > 3ebfe0e lib: sbi_tlb: Simplify sbi_tlb_entry_process() function
> > > 209134d lib: Handle failure of sbi_hartid_to_scratch() API
> > > bd6ef02 include: sbi_platform: Improve sbi_platform_hart_disabled() API
> > > c9f60fc lib: sbi_scratch: Don't set hartid_to_scratch table for disabled 
> > > HART
> > > 680b098 lib: sbi_hsm: Don't use sbi_platform_hart_count() API
> > > db187d6 lib: sbi_hsm: Remove scratch parameter from hart_started_mask() 
> > > API
> > > 814f38d lib: sbi_hsm: Don't use sbi_platform_hart_disabled() API
> > > 75eec9d lib: Don't use sbi_platform_hart_count() API
> > > c51f02c include: sbi_platform: Introduce HART index to HART id table
> > > 315a877 platform: sifive/fu540: R

Re: [PATCH] roms: opensbi: Upgrade from v0.6 to v0.7

[Alistair Francis]

On Tue, Apr 21, 2020 at 6:30 PM Bin Meng  wrote:
>
> Hi Alistair,
>
> On Tue, Apr 21, 2020 at 9:34 AM Bin Meng  wrote:
> >
> > Hi Alistair,
> >
> > On Tue, Apr 21, 2020 at 2:41 AM Alistair Francis  
> > wrote:
> > >
> > > On Mon, Apr 20, 2020 at 6:25 AM Bin Meng  wrote:
> > > >
> > > > Upgrade OpenSBI from v0.6 to v0.7 and the pre-built bios images.
> > > >
> > > > The v0.7 release includes the following commits:
> > > >
> > > > f64f4b9 lib: Add a new platform feature to bringup secondary harts
> > > > b677a9b lib: Implement hart hotplug
> > > > 5b48240 lib: Add possible hart status values
> > > > e3f69fc lib: Implement Hart State Management (HSM) SBI extension
> > > > 6704216 lib: Check MSIP bit after returning from WFI
> > > > 82ae8e8 makefile: Do setup of the install target more flexible
> > > > e1a5b73 platform: sifive: fu540: allow sv32 as an mmu-type
> > > > 8c83fb2 lib: Fix return type of sbi_hsm_hart_started()
> > > > 00d332b include: Move bits related defines and macros to sbi_bitops.h
> > > > a148996 include: sbi_bitops: More useful bit operations
> > > > 4a603eb platform: kendryte/k210: Set per-HART stack size to 8KB
> > > > 678c3c3 include: sbi_scratch: Set per-HART scratch size to 4KB
> > > > 2abc55b lib: Sort build objects in alphabetical order
> > > > 6e87507 platform: ae350: Sort build objects in alphabetical order
> > > > 650c0e5 lib: sbi: Fix coding style issues
> > > > 078686d lib: serial: Fix coding style issues
> > > > 3226bd9 lib: Simple bitmap library
> > > > c741abc include: Simple hartmask library
> > > > d6d7e18 lib: sbi_init: Don't allow HARTID greater than 
> > > > SBI_HARTMASK_MAX_BITS
> > > > a4a6a81 lib: Introduce SBI_TLB_INFO_INIT() helper macro
> > > > d963164 lib: sbi_tlb: Use sbi_hartmask in sbi_tlb_info
> > > > 71d2b83 lib: Move all coldboot wait APIs to sbi_init.c
> > > > 2b945fc lib: sbi_init: Use hartmask for coldboot wait
> > > > 44ce5b9 include: Remove disabled_hart_mask from sbi_platform
> > > > 2db381f lib: Introduce sbi_hsm_hart_started_mask() API
> > > > 61f7768 lib: sbi_ecall_legacy: Use sbi_hsm_hart_started_mask() API
> > > > 466fecb lib: sbi_system: Use sbi_hsm_hart_started_mask() API
> > > > 9aad831 lib: sbi_ipi: Use sbi_hsm_hart_started_mask() API
> > > > eede1aa lib: sbi_hart: Remove HART available mask and related APIs
> > > > 757bb44 docs: Remove out-of-date documentation
> > > > 86d37bb lib: sbi: Fix misaligned trap handling
> > > > ffdc858 platform: ariane-fpga: Change license for ariane-fpga from 
> > > > GPL-2.0 to BSD-2
> > > > 4b2f594 sbi: Add definitions for true/false
> > > > 0cfe49a libfdt: Add INT32_MAX and UINT32_MAX in libfdt_env.h
> > > > baac7e0 libfdt: Upgrade to v1.5.1 release
> > > > f92147c include: Make sbi_hart_id_to_scratch() as macro
> > > > eeae3d9 firmware: fw_base: Optimize _hartid_to_scratch() implementation
> > > > 16e7071 lib: sbi_hsm: Optimize sbi_hsm_hart_get_state() implementation
> > > > 823345e include: Make sbi_current_hartid() as macro in riscv_asm.h
> > > > 9aabba2 Makefile: Fix distclean make target
> > > > 9275ed3 platform: ariane-fpga: Set per-HART stack size to 8KB
> > > > 2343efd platform: Set per-HART stack size to 8KB in the template 
> > > > platform codes
> > > > 72a0628 platform: Use one unified per-HART stack size macro for all 
> > > > platforms
> > > > 327ba36 scripts: Cover sifive/fu540 in the 32-bit build
> > > > 5fbcd62 lib: sbi: Update pmp_get() to return decoded size directly
> > > > dce8846 libfdt: Compile fdt_addresses.c
> > > > fcb1ded lib: utils: Add a fdt_reserved_memory_fixup() helper
> > > > 666be6d platform: Clean up include header files
> > > > 6af5576 lib: utils: Move PLIC DT fix up codes to fdt_helper.c
> > > > e846ce1 platform: andes/ae350: Fix up DT for reserved memory
> > > > 8135520 platform: ariane-fpga: Fix up DT for reserved memory
> > > > c9a5268 platform: qemu/virt: Fix up DT for reserved memory
> > > > 6f9bb83 platform: sifive/fu540: Fix up DT for reserved memory
> > > > 1071f05 platform: sifive/fu540: Remove "stdout-path" fix-up
> > > > dd9439f lib: utils: Add a fdt_cpu_fixup() helper
> > > > 3f1c847 platform: sifive/fu540: Replace cpu0 node fix-up with the new 
> > > > helper
> > > > db6a2b5 lib: utils: Add a general device tree fix-up helper
> > > > 3f8d754 platform: Update to call general DT fix-up helper
> > > > 87a7ef7 lib: sbi_scratch: Introduce HART id to scratch table
> > > > e23d3ba include: Simplify HART id to scratch macro
> > > > 19bd531 lib: sbi_hsm: Simplify hart_get_state() and hart_started() APIs
> > > > 3ebfe0e lib: sbi_tlb: Simplify sbi_tlb_entry_process() function
> > > > 209134d lib: Handle failure of sbi_hartid_to_scratch() API
> > > > bd6ef02 include: sbi_platform: Improve sbi_platform_hart_disabled() API
> > > > c9f60fc lib: sbi_scratch: Don't set hartid_to_scratch table for 
> > > > disabled HART
> > > > 680b098 lib: sbi_hsm: Don't use sbi_platform_hart_count() API
> > > > db187d6 lib: sbi_hsm: Remove scratch parameter from hart_started_mask() 
> > > > API

[PATCH 1/1] pci: vmd: Use Shadow MEMBAR registers for QEMU/KVM guests

[Jon Derrick]

VMD device 28C0 provides native guest passthrough of the VMD endpoint
through the use of shadow registers that provide Host Physical Addresses
to correctly assign bridge windows. A quirk has been added to QEMU's
VFIO quirks to emulate the shadow registers for VMD devices which don't
support this mode natively in hardware.

The VFIO quirk assigns the VMD a subsystem vendor/device ID using the
standard QEMU vendor/device, which are typically only used for emulation
and not VFIO. There are no plans for an emulated VMD controller, but if
one is developed in the future, support for this mode can be added by
emulating the VMD VMLOCK and Shadow MEMBAR registers

Signed-off-by: Jon Derrick 
---
 drivers/pci/controller/vmd.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index dac91d60701d..764404b45ebb 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -598,6 +598,7 @@ static irqreturn_t vmd_irq(int irq, void *data)
 static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
struct vmd_dev *vmd;
+   unsigned long features = id->driver_data;
int i, err;
 
if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20))
@@ -648,9 +649,14 @@ static int vmd_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
return err;
}
 
+   /* VFIO-quirked VMD controllers emulate the Shadow MEMBAR feature */
+   if (dev->subsystem_vendor == PCI_SUBVENDOR_ID_REDHAT_QUMRANET &&
+   dev->subsystem_device == PCI_SUBDEVICE_ID_QEMU)
+   features |= VMD_FEAT_HAS_MEMBAR_SHADOW;
+
spin_lock_init(&vmd->cfg_lock);
pci_set_drvdata(dev, vmd);
-   err = vmd_enable_domain(vmd, (unsigned long) id->driver_data);
+   err = vmd_enable_domain(vmd, features);
if (err)
return err;
 
-- 
2.18.1

Re: [PATCH 2/3] block/io: convert generic io path to use int64_t parameters

[Vladimir Sementsov-Ogievskiy]

22.04.2020 18:50, Stefan Hajnoczi wrote:

On Mon, Mar 30, 2020 at 05:18:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:

@@ -875,9 +875,9 @@ static bool coroutine_fn 
bdrv_wait_serialising_requests(BdrvTrackedRequest *self
  }
  
  static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,

-   size_t size)
+   int64_t bytes)
  {
-if (size > BDRV_REQUEST_MAX_BYTES) {
+if (offset < 0 || bytes < 0 || bytes > BDRV_REQUEST_MAX_BYTES) {
  return -EIO;
  }
  
@@ -885,10 +885,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,

  return -ENOMEDIUM;
  }
  
-if (offset < 0) {

-return -EIO;
-}
-
  return 0;
  }


Moving this if statement was unnecessary.  I'm not sure if there are
cases where callers will now see EIO instead of ENOMEDIUM and change
their behavior :(.

More conservative code changes are easier to review and merge because
they are less risky.


Hmm, would be a bit strange to just add "bytes < 0" to the first "if" keeping "offset 
< 0" in the third.
And strange to keep "bytes > BDRV_REQUEST_MAX_BYTES" in the first, if add "bytes < 
0" to the third..




@@ -1743,7 +1740,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
  }
  
  static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,

-int64_t offset, int bytes, BdrvRequestFlags flags)
+int64_t offset, int64_t bytes, BdrvRequestFlags flags)
  {
  BlockDriver *drv = bs->drv;
  QEMUIOVector qiov;
@@ -1773,7 +1770,7 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  assert(max_write_zeroes >= bs->bl.request_alignment);
  
  while (bytes > 0 && !ret) {

-int num = bytes;
+int64_t num = bytes;
  
  /* Align request.  Block drivers can expect the "bulk" of the request

   * to be aligned, and that unaligned requests do not cross cluster
@@ -1799,6 +1796,7 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  ret = -ENOTSUP;
  /* First try the efficient write zeroes operation */
  if (drv->bdrv_co_pwrite_zeroes) {
+assert(num <= INT_MAX);


max_write_zeroes already enforces this, so the assertion is always
false:

   int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
   ...
 /* limit request size */
 if (num > max_write_zeroes) {
 num = max_write_zeroes;
 }



You are right, I'll drop it.

--
Best regards,
Vladimir

[PATCH 0/1] KVM support for VMD devices

[Jon Derrick]

The two patches (Linux & QEMU) add support for passthrough VMD devices
in QEMU/KVM. VMD device 28C0 already supports passthrough natively by
providing the Host Physical Address in a shadow register to the guest
for correct bridge programming.

The QEMU patch emulates the 28C0 mode by creating a shadow register and
advertising its support by using QEMU's subsystem vendor/id.
The Linux patch matches the QEMU subsystem vendor/id to use the shadow
register.

Jon Derrick (1):
  pci: vmd: Use Shadow MEMBAR registers for QEMU/KVM guests

 drivers/pci/controller/vmd.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.18.1

[PATCH for QEMU] hw/vfio: Add VMD Passthrough Quirk

[Jon Derrick]

The VMD endpoint provides a real PCIe domain to the guest, including
bridges and endpoints. The IOMMU performs Host Physical Address to Guest
Physical Address translation when assigning downstream endpoint BARs and
when translating MMIO addresses.

This translation is not desired when assigning bridge windows. When MMIO
goes to an endpoint after being translated to HPA, the bridge will
reject the HPA transaction because the bridge window has been programmed
with translated GPAs.

VMD device 28C0 natively supports passthrough by providing the Host
Physical Address in shadow registers accessible to the guest for bridge
window assignment. The shadow registers are valid if bit 1 is set in VMD
VMLOCK config register 0x70.

This quirk emulates the VMLOCK and HPA shadow registers for all VMD
device ids which don't natively offer this feature. The Linux VMD driver
is updated to match the QEMU subsystem id to enable this feature.

Signed-off-by: Jon Derrick 
---
 hw/vfio/pci-quirks.c | 119 +++
 hw/vfio/pci.c|   7 +++
 hw/vfio/pci.h|   2 +
 hw/vfio/trace-events |   4 ++
 4 files changed, 132 insertions(+)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 2d348f8237..2fd27cc8f6 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1709,3 +1709,122 @@ free_exit:
 
 return ret;
 }
+
+/*
+ * The VMD endpoint provides a real PCIe domain to the guest. The IOMMU
+ * performs Host Physical Address to Guest Physical Address translation when
+ * assigning downstream endpoint BARs and when translating MMIO addresses.
+ * However this translation is not desired when assigning bridge windows. When
+ * MMIO goes to an endpoint after being translated to HPA, the bridge rejects
+ * the transaction because the window has been programmed with translated GPAs.
+ *
+ * VMD uses the Host Physical Address in order to correctly program the bridge
+ * windows in its PCIe domain. VMD device 28C0 has HPA shadow registers located
+ * at offset 0x2000 in MEMBAR2 (BAR 4). The shadow registers are valid if bit 1
+ * is set in the VMD VMLOCK config register 0x70.
+ *
+ * This quirk emulates the VMLOCK and HPA shadow registers for all VMD device
+ * ids which don't natively offer this feature. The subsystem vendor/device
+ * id is set to the QEMU subsystem vendor/device id, where the driver matches
+ * the id to enable this feature.
+ */
+typedef struct VFIOVMDQuirk {
+VFIOPCIDevice *vdev;
+uint64_t membar_phys[2];
+} VFIOVMDQuirk;
+
+static uint64_t vfio_vmd_quirk_read(void *opaque, hwaddr addr, unsigned size)
+{
+VFIOVMDQuirk *data = opaque;
+uint64_t val = 0;
+
+memcpy(&val, (void *)data->membar_phys + addr, size);
+return val;
+}
+
+static const MemoryRegionOps vfio_vmd_quirk = {
+.read = vfio_vmd_quirk_read,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+#define VMD_VMLOCK  0x70
+#define VMD_SHADOW  0x2000
+#define VMD_MEMBAR2 4
+
+static int vfio_vmd_emulate_shadow_registers(VFIOPCIDevice *vdev)
+{
+VFIOQuirk *quirk;
+VFIOVMDQuirk *data;
+PCIDevice *pdev = &vdev->pdev;
+int ret;
+
+data = g_malloc0(sizeof(*data));
+ret = pread(vdev->vbasedev.fd, data->membar_phys, 16,
+vdev->config_offset + PCI_BASE_ADDRESS_2);
+if (ret != 16) {
+error_report("VMD %s cannot read MEMBARs (%d)",
+ vdev->vbasedev.name, ret);
+g_free(data);
+return -EFAULT;
+}
+
+quirk = vfio_quirk_alloc(1);
+quirk->data = data;
+data->vdev = vdev;
+
+/* Emulate Shadow Registers */
+memory_region_init_io(quirk->mem, OBJECT(vdev), &vfio_vmd_quirk, data,
+  "vfio-vmd-quirk", sizeof(data->membar_phys));
+memory_region_add_subregion_overlap(vdev->bars[VMD_MEMBAR2].region.mem,
+VMD_SHADOW, quirk->mem, 1);
+memory_region_set_readonly(quirk->mem, true);
+memory_region_set_enabled(quirk->mem, true);
+
+QLIST_INSERT_HEAD(&vdev->bars[VMD_MEMBAR2].quirks, quirk, next);
+
+trace_vfio_pci_vmd_quirk_shadow_regs(vdev->vbasedev.name,
+ data->membar_phys[0],
+ data->membar_phys[1]);
+
+/* Advertise Shadow Register support */
+pci_byte_test_and_set_mask(pdev->config + VMD_VMLOCK, 0x2);
+pci_set_byte(pdev->wmask + VMD_VMLOCK, 0);
+pci_set_byte(vdev->emulated_config_bits + VMD_VMLOCK, 0x2);
+
+trace_vfio_pci_vmd_quirk_vmlock(vdev->vbasedev.name,
+pci_get_byte(pdev->config + VMD_VMLOCK));
+
+/* Drivers can match the subsystem vendor/device id */
+pci_set_word(pdev->config + PCI_SUBSYSTEM_VENDOR_ID,
+ PCI_SUBVENDOR_ID_REDHAT_QUMRANET);
+pci_set_word(vdev->emulated_config_bits + PCI_SUBSYSTEM_VENDOR_ID, ~0);
+
+pci_set_word(pdev->config + PCI_SUBSYSTEM_ID, PCI_SUBDEVICE_ID_QEMU);
+pci_set_word(vdev->emulated_config_bits + PCI_SU

Re: [PATCH v2 17/36] tcg/optimize: Adjust TempOptInfo allocation

[Alex Bennée]

Richard Henderson  writes:

> Do not allocate a large block for indexing.  Instead, allocate
> for each temporary as they are seen.
>
> In general, this will use less memory, if we consider that most
> TBs do not touch every target register.  This also allows us to
> allocate TempOptInfo for new temps created during optimization.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  tcg/optimize.c | 60 --
>  1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index b86bf3d707..d36d7e1d7f 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -89,35 +89,41 @@ static void reset_temp(TCGArg arg)
>  }
>  
>  /* Initialize and activate a temporary.  */
> -static void init_ts_info(TempOptInfo *infos,
> - TCGTempSet *temps_used, TCGTemp *ts)
> +static void init_ts_info(TCGTempSet *temps_used, TCGTemp *ts)
>  {
>  size_t idx = temp_idx(ts);
> -if (!test_bit(idx, temps_used->l)) {
> -TempOptInfo *ti = &infos[idx];
> +TempOptInfo *ti;
>  
> +if (test_bit(idx, temps_used->l)) {
> +return;
> +}
> +set_bit(idx, temps_used->l);
> +
> +ti = ts->state_ptr;
> +if (ti == NULL) {
> +ti = tcg_malloc(sizeof(TempOptInfo));
>  ts->state_ptr = ti;
> -ti->next_copy = ts;
> -ti->prev_copy = ts;
> -if (ts->kind == TEMP_CONST) {
> -ti->is_const = true;
> -ti->val = ti->mask = ts->val;
> -if (TCG_TARGET_REG_BITS > 32 && ts->type == TCG_TYPE_I32) {
> -/* High bits of a 32-bit quantity are garbage.  */
> -ti->mask |= ~0xull;
> -}
> -} else {
> -ti->is_const = false;
> -ti->mask = -1;
> +}
> +
> +ti->next_copy = ts;
> +ti->prev_copy = ts;
> +if (ts->kind == TEMP_CONST) {
> +ti->is_const = true;
> +ti->val = ts->val;
> +ti->mask = ts->val;
> +if (TCG_TARGET_REG_BITS > 32 && ts->type == TCG_TYPE_I32) {
> +/* High bits of a 32-bit quantity are garbage.  */
> +ti->mask |= ~0xull;
>  }
> -set_bit(idx, temps_used->l);
> +} else {
> +ti->is_const = false;
> +ti->mask = -1;
>  }
>  }
>  
> -static void init_arg_info(TempOptInfo *infos,
> -  TCGTempSet *temps_used, TCGArg arg)
> +static void init_arg_info(TCGTempSet *temps_used, TCGArg arg)
>  {
> -init_ts_info(infos, temps_used, arg_temp(arg));
> +init_ts_info(temps_used, arg_temp(arg));
>  }
>  
>  static TCGTemp *find_better_copy(TCGContext *s, TCGTemp *ts)
> @@ -603,9 +609,8 @@ static bool swap_commutative2(TCGArg *p1, TCGArg *p2)
>  /* Propagate constants and copies, fold constant expressions. */
>  void tcg_optimize(TCGContext *s)
>  {
> -int nb_temps, nb_globals;
> +int nb_temps, nb_globals, i;
>  TCGOp *op, *op_next, *prev_mb = NULL;
> -TempOptInfo *infos;
>  TCGTempSet temps_used;
>  
>  /* Array VALS has an element for each temp.
> @@ -615,12 +620,15 @@ void tcg_optimize(TCGContext *s)
>  
>  nb_temps = s->nb_temps;
>  nb_globals = s->nb_globals;
> +
>  bitmap_zero(temps_used.l, nb_temps);
> -infos = tcg_malloc(sizeof(TempOptInfo) * nb_temps);
> +for (i = 0; i < nb_temps; ++i) {
> +s->temps[i].state_ptr = NULL;
> +}
>  
>  QTAILQ_FOREACH_SAFE(op, &s->ops, link, op_next) {
>  tcg_target_ulong mask, partmask, affected;
> -int nb_oargs, nb_iargs, i;
> +int nb_oargs, nb_iargs;
>  TCGArg tmp;
>  TCGOpcode opc = op->opc;
>  const TCGOpDef *def = &tcg_op_defs[opc];
> @@ -633,14 +641,14 @@ void tcg_optimize(TCGContext *s)
>  for (i = 0; i < nb_oargs + nb_iargs; i++) {
>  TCGTemp *ts = arg_temp(op->args[i]);
>  if (ts) {
> -init_ts_info(infos, &temps_used, ts);
> +init_ts_info(&temps_used, ts);
>  }
>  }
>  } else {
>  nb_oargs = def->nb_oargs;
>  nb_iargs = def->nb_iargs;
>  for (i = 0; i < nb_oargs + nb_iargs; i++) {
> -init_arg_info(infos, &temps_used, op->args[i]);
> +init_arg_info(&temps_used, op->args[i]);
>  }
>  }


-- 
Alex Bennée

Re: [PATCH v2 13/36] tcg: Use tcg_constant_{i32,i64} with tcg int expanders

[Alex Bennée]

Richard Henderson  writes:

> On 4/22/20 9:18 AM, Alex Bennée wrote:
>>>  void tcg_gen_brcondi_i64(TCGCond cond, TCGv_i64 arg1, int64_t arg2, 
>>> TCGLabel *l)
>>>  {
>>> -if (cond == TCG_COND_ALWAYS) {
>>> +if (TCG_TARGET_REG_BITS == 64) {
>>> +tcg_gen_brcond_i64(cond, arg1, tcg_constant_i64(arg2), l);
>>> +} else if (cond == TCG_COND_ALWAYS) {
>>>  tcg_gen_br(l);
>>>  } else if (cond != TCG_COND_NEVER) {
>>> -TCGv_i64 t0 = tcg_const_i64(arg2);
>>> -tcg_gen_brcond_i64(cond, arg1, t0, l);
>>> -tcg_temp_free_i64(t0);
>>> +l->refs++;
>> 
>> Hmm is this a separate fix?
>
> No, it's expanding what tcg_gen_brcond_i64 would do for TCG_TARGET_REG_BITS 
> == 32.
>
>>> +tcg_gen_op6ii_i32(INDEX_op_brcond2_i32,
>>> +  TCGV_LOW(arg1), TCGV_HIGH(arg1),
>>> +  tcg_constant_i32(arg2),
>>> +  tcg_constant_i32(arg2 >> 32),
>>> +  cond, label_arg(l));
>
> Because we have two separate TCGv_i32, from tcg_constant_i32(), which cannot 
> be
> packaged up with TCGV_HIGH/LOW.
>
>
> r~

OK I see that now - the r-b stands ;-)

-- 
Alex Bennée

Re: [PATCH v4 20/30] qcow2: Add subcluster support to discard_in_l2_slice()

[Vladimir Sementsov-Ogievskiy]

22.04.2020 20:42, Alberto Garcia wrote:

On Wed 22 Apr 2020 01:35:25 PM CEST, Vladimir Sementsov-Ogievskiy wrote:

17.03.2020 21:16, Alberto Garcia wrote:

Two changes are needed in this function:

1) A full discard deallocates a cluster so we can skip the operation if
 it is already unallocated. With extended L2 entries however if any
 of the subclusters has the 'all zeroes' bit set then we have to
 clear it.

2) Setting the QCOW_OFLAG_ZERO bit of the L2 entry is forbidden if an
 image has extended L2 entries. Instead, the individual 'all zeroes'
 bits must be used.

Signed-off-by: Alberto Garcia 
---
   block/qcow2-cluster.c | 18 +++---
   1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 746006a117..824c710760 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1790,12 +1790,20 @@ static int discard_in_l2_slice(BlockDriverState *bs, 
uint64_t offset,
* TODO We might want to use bdrv_block_status(bs) here, but we're
* holding s->lock, so that doesn't work today.
*
- * If full_discard is true, the sector should not read back as zeroes,
+ * If full_discard is true, the cluster should not read back as zeroes,
* but rather fall through to the backing file.
*/
   switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
   case QCOW2_CLUSTER_UNALLOCATED:
-if (full_discard || !bs->backing) {
+if (full_discard) {
+/* If the image has extended L2 entries we can only
+ * skip this operation if the L2 bitmap is zero. */
+uint64_t bitmap = has_subclusters(s) ?
+get_l2_bitmap(s, l2_slice, l2_index + i) : 0;
+if (bitmap == 0) {
+continue;
+}
+} else if (!bs->backing) {
   continue;
   }


Hmm, so you do continue if full_discard is false AND bitmap != 0 &
!bs->backing,



but you do not continue if full_discard is true AND bitmap != 0 &
!bs->backing (as you will not go to "else") branch.


1. If full_discard is true it means that the entry and the bitmap should
always be set to 0, regardless of whether there's a backing file or
any other consideration.

This is used e.g when shrinking an image, or by qcow2_make_empty().

We can only skip this operation if both the entry and the bitmap are
already 0 (the former we know because of QCOW2_CLUSTER_UNALLOCATED).


Ah, understand, sorry. I thought that behavior was changed accidentally, but it 
is for purpose. With old code cluster is already unallocated, but with 
subclusters we may have some ZERO_PLAIN subclusters.



2. If full_discard is false it means that we must ensure that the
cluster reads back as zeroes, but there's no need to clear the bitmap
(in fact we must set QCOW_OFLAG_ZERO or QCOW_L2_BITMAP_ALL_ZEROES
depending on the type of image).

We can skip this operation if there's no backing file and the cluster
is already unallocated (because then we know that it already reads as
zeroes).

One optimization would be to skip the operation also if the image has
subclusters and the bitmap is QCOW_L2_BITMAP_ALL_ZEROES, I can do
that for the next version.


In case QCOW2_CLUSTER_ZERO_PLAIN, worth assert !has_subclusters(s) or
mark image corrupted ?


I think that should be handled directly in qcow2_get_cluster_type().

There's currently an inconsistency now that I think of it: if an image
has subclusters and QCOW_OFLAG_ZERO set then qcow2_get_cluster_type()
returns QCOW2_CLUSTER_ZERO_* but qcow2_get_subcluster_type() returns
QCOW2_SUBCLUSTER_INVALID.

Two alternatives:

   - We add QCOW2_CLUSTER_INVALID so we get an error in both
 cases. Problem: any function that calls qcow2_get_cluster_type()
 should be modified to handle that.

   - We ignore QCOW_OFLAG_ZERO. Simpler, and it would allow us to use
 that bit in the future if we wanted.



Hmm. Actually we don't check other reserved bits. But ZERO bit is risky, we may 
miss data corruptions during transmission to the qcow2-subclusters world. So 
I'm for the first variant if it's not too huge.




--
Best regards,
Vladimir

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Alistair Francis]

On Tue, Apr 21, 2020 at 9:10 PM Richard Henderson
 wrote:
>
> On 4/21/20 7:34 PM, LIU Zhiwei wrote:
> > Ping.
> >
> > When I port RISU, I find this bug. I can't get the correct registers from 
> > the
> > struct ucontext_t parameter in the signal handler.
>
> The RISC-V Linux ABI will need to be extended to handle RVV state.
>
> There is room in your sigcontext structure:
>
> > struct __riscv_q_ext_state {
> > __u64 f[64] __attribute__((aligned(16)));
> > __u32 fcsr;
> > /*
> >  * Reserved for expansion of sigcontext structure.  Currently zeroed
> >  * upon signal, and must be zero upon sigreturn.
> >  */
> > __u32 reserved[3];
> > };
>
> in uc->uc_mcontext.sc_fpregs.q.
>
> That reserved field is going to have to be used in some way.

Just to clarify, this patch is still correct right?

It looks good to me.

Alistair

>
> My suggestion is to use some sort of extendable record list, akin to AArch64:
>
> struct _aarch64_ctx {
> __u32 magic;
> __u32 size;
> };
>
> One of the 3 zeros could be the total size of the extensions, so that it's 
> easy
> to validate the size or memcpy the lot without parsing each individual record.
>  The other two zeros could be the first header of the next record.  Which in
> this case also allows the payload of that first record to be aligned mod 16,
> which could come in handy.
>
> Talk to the risc-v kernel engineers and come up with a plan that includes room
> for the next architecture extension as well.  They may have already done so,
> but I'm not monitoring the correct mailing list to know.
>
>
> r~
>

[PATCH v21 QEMU 0/5] virtio-balloon: add support for free page reporting

[Alexander Duyck]

This series provides an asynchronous means of reporting free guest pages
to QEMU through virtio-balloon so that the memory associated with those
pages can be dropped and reused by other processes and/or guests on the
host. Using this it is possible to avoid unnecessary I/O to disk and
greatly improve performance in the case of memory overcommit on the host.

As of April 7th this functionality has been enabled in Linus's kernel
tree[1] and so I am submitting the QEMU pieces for inclusion.

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c504f154718904ae49349147e3b7e6ae91ffdc

Changes from v17:
Fixed typo in patch 1 title
Addressed white-space issues reported via checkpatch
Added braces {} for two if statements to match expected coding style

Changes from v18:
Updated patches 2 and 3 based on input from dhildenb
Added comment to patch 2 describing what keeps us from reporting a bad page
Added patch to address issue with ROM devices being directly writable

Changes from v19:
Added std-headers change to match changes pushed for linux kernel headers
Added patch to remove "report" from page hinting code paths
Updated comment to better explain why we disable hints w/ page poisoning
Removed code that was modifying config size for poison vs hinting
Dropped x-page-poison property
Added code to bounds check the reported region vs the RAM block
Dropped patch for ROM devices as that was already pulled in by Paolo

Changes from v20:
Rearranged patches to push Linux header sync patches to front
Removed association between free page hinting and VIRTIO_BALLOON_F_PAGE_POISON
Added code to enable VIRTIO_BALLOON_F_PAGE_POISON if page reporting is enabled
Fixed possible resource leak if poison or qemu_balloon_is_inhibited return true
Updated cover page comments

---

Alexander Duyck (5):
  linux-headers: Update to allow renaming of free_page_report_cmd_id
  linux-headers: update to contain virito-balloon free page reporting
  virtio-balloon: Replace free page hinting references to 'report' with 
'hint'
  virtio-balloon: Implement support for page poison tracking feature
  virtio-balloon: Provide an interface for free page reporting


 hw/virtio/virtio-balloon.c  |  151 +--
 include/hw/virtio/virtio-balloon.h  |   23 ++--
 include/standard-headers/linux/virtio_balloon.h |   12 ++
 3 files changed, 136 insertions(+), 50 deletions(-)

--

[PATCH v21 QEMU 3/5] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[Alexander Duyck]

From: Alexander Duyck 

In an upcoming patch a feature named Free Page Reporting is about to be
added. In order to avoid any confusion we should drop the use of the word
'report' when referring to Free Page Hinting. So what this patch does is go
through and replace all instances of 'report' with 'hint" when we are
referring to free page hinting.

Signed-off-by: Alexander Duyck 
---
 hw/virtio/virtio-balloon.c |   74 ++--
 include/hw/virtio/virtio-balloon.h |   20 +-
 2 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7fc930..a1d6fb52c876 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -466,21 +466,21 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
 ret = false;
 goto out;
 }
-if (id == dev->free_page_report_cmd_id) {
-dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+if (id == dev->free_page_hint_cmd_id) {
+dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
 } else {
 /*
  * Stop the optimization only when it has started. This
  * avoids a stale stop sign for the previous command.
  */
-if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
-dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
+dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
 }
 }
 }
 
 if (elem->in_num) {
-if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
 qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
   elem->in_sg[0].iov_len);
 }
@@ -506,11 +506,11 @@ static void virtio_ballloon_get_free_page_hints(void 
*opaque)
 qemu_mutex_unlock(&dev->free_page_lock);
 virtio_notify(vdev, vq);
   /*
-   * Start to poll the vq once the reporting started. Otherwise, continue
+   * Start to poll the vq once the hinting started. Otherwise, continue
* only when there are entries on the vq, which need to be given back.
*/
 } while (continue_to_get_hints ||
- dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
+ dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
 virtio_queue_set_notification(vq, 1);
 }
 
@@ -531,14 +531,14 @@ static void virtio_balloon_free_page_start(VirtIOBalloon 
*s)
 return;
 }
 
-if (s->free_page_report_cmd_id == UINT_MAX) {
-s->free_page_report_cmd_id =
-   VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+if (s->free_page_hint_cmd_id == UINT_MAX) {
+s->free_page_hint_cmd_id =
+   VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
 } else {
-s->free_page_report_cmd_id++;
+s->free_page_hint_cmd_id++;
 }
 
-s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
+s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
 virtio_notify_config(vdev);
 }
 
@@ -546,18 +546,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon 
*s)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
 /*
  * The lock also guarantees us that the
  * virtio_ballloon_get_free_page_hints exits after the
- * free_page_report_status is set to S_STOP.
+ * free_page_hint_status is set to S_STOP.
  */
 qemu_mutex_lock(&s->free_page_lock);
 /*
  * The guest hasn't done the reporting, so host sends a notification
  * to the guest to actively stop the reporting.
  */
-s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
 qemu_mutex_unlock(&s->free_page_lock);
 virtio_notify_config(vdev);
 }
@@ -567,15 +567,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon 
*s)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
+s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
 virtio_notify_config(vdev);
 }
 
 static int
-virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
+virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
 {
 VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
-  free_page_report_notify);
+  free_page_hint_notify);
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 PrecopyNotifyData *pnd = data;
 
@@ -624,7 +624,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
 if (vi

[PATCH v21 QEMU 1/5] linux-headers: Update to allow renaming of free_page_report_cmd_id

[Alexander Duyck]

From: Alexander Duyck 

Sync to the latest upstream changes for free page hinting. To be
replaced by a full linux header sync.

Signed-off-by: Alexander Duyck 
---
 include/standard-headers/linux/virtio_balloon.h |   11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/standard-headers/linux/virtio_balloon.h 
b/include/standard-headers/linux/virtio_balloon.h
index 9375ca2a70de..af0a6b59dab2 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -47,8 +47,15 @@ struct virtio_balloon_config {
uint32_t num_pages;
/* Number of pages we've actually got in balloon. */
uint32_t actual;
-   /* Free page report command id, readonly by guest */
-   uint32_t free_page_report_cmd_id;
+   /*
+* Free page hint command id, readonly by guest.
+* Was previously name free_page_report_cmd_id so we
+* need to carry that name for legacy support.
+*/
+   union {
+   uint32_t free_page_hint_cmd_id;
+   uint32_t free_page_report_cmd_id;   /* deprecated */
+   };
/* Stores PAGE_POISON if page poisoning is in use */
uint32_t poison_val;
 };

[PATCH v21 QEMU 4/5] virtio-balloon: Implement support for page poison tracking feature

[Alexander Duyck]

From: Alexander Duyck 

We need to make certain to advertise support for page poison tracking if
we want to actually get data on if the guest will be poisoning pages.

Add a value for tracking the poison value being used if page poisoning is
enabled. With this we can determine if we will need to skip page reporting
when it is enabled in the future.

Signed-off-by: Alexander Duyck 
---
 hw/virtio/virtio-balloon.c |7 +++
 include/hw/virtio/virtio-balloon.h |1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a1d6fb52c876..5effc8b4653b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, 
uint8_t *config_data)
 
 config.num_pages = cpu_to_le32(dev->num_pages);
 config.actual = cpu_to_le32(dev->actual);
+config.poison_val = cpu_to_le32(dev->poison_val);
 
 if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
 config.free_page_hint_cmd_id =
@@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
 qapi_event_send_balloon_change(vm_ram_size -
 ((ram_addr_t) dev->actual << 
VIRTIO_BALLOON_PFN_SHIFT));
 }
+dev->poison_val = 0;
+if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
+dev->poison_val = le32_to_cpu(config.poison_val);
+}
 trace_virtio_balloon_set_config(dev->actual, oldactual);
 }
 
@@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
 g_free(s->stats_vq_elem);
 s->stats_vq_elem = NULL;
 }
+
+s->poison_val = 0;
 }
 
 static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
diff --git a/include/hw/virtio/virtio-balloon.h 
b/include/hw/virtio/virtio-balloon.h
index 108cff97e71a..3ca2a78e1aca 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -70,6 +70,7 @@ typedef struct VirtIOBalloon {
 uint32_t host_features;
 
 bool qemu_4_0_config_size;
+uint32_t poison_val;
 } VirtIOBalloon;
 
 #endif

[PATCH v21 QEMU 5/5] virtio-balloon: Provide an interface for free page reporting

[Alexander Duyck]

From: Alexander Duyck 

Add support for free page reporting. The idea is to function very similar
to how the balloon works in that we basically end up madvising the page as
not being used. However we don't really need to bother with any deflate
type logic since the page will be faulted back into the guest when it is
read or written to.

This provides a new way of letting the guest proactively report free
pages to the hypervisor, so the hypervisor can reuse them. In contrast to
inflate/deflate that is triggered via the hypervisor explicitly.

Signed-off-by: Alexander Duyck 
---
 hw/virtio/virtio-balloon.c |   70 
 include/hw/virtio/virtio-balloon.h |2 +
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5effc8b4653b..b473ff7f4b88 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -321,6 +321,60 @@ static void balloon_stats_set_poll_interval(Object *obj, 
Visitor *v,
 balloon_stats_change_timer(s, 0);
 }
 
+static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
+{
+VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+VirtQueueElement *elem;
+
+while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement {
+unsigned int i;
+
+if (qemu_balloon_is_inhibited() || dev->poison_val) {
+goto skip_element;
+}
+
+for (i = 0; i < elem->in_num; i++) {
+void *addr = elem->in_sg[i].iov_base;
+size_t size = elem->in_sg[i].iov_len;
+ram_addr_t ram_offset;
+RAMBlock *rb;
+
+/*
+ * There is no need to check the memory section to see if
+ * it is ram/readonly/romd like there is for handle_output
+ * below. If the region is not meant to be written to then
+ * address_space_map will have allocated a bounce buffer
+ * and it will be freed in address_space_unmap and trigger
+ * and unassigned_mem_write before failing to copy over the
+ * buffer. If more than one bad descriptor is provided it
+ * will return NULL after the first bounce buffer and fail
+ * to map any resources.
+ */
+rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+if (!rb) {
+trace_virtio_balloon_bad_addr(elem->in_addr[i]);
+continue;
+}
+
+/*
+ * For now we will simply ignore unaligned memory regions, or
+ * regions that overrun the end of the RAMBlock.
+ */
+if (!QEMU_IS_ALIGNED(ram_offset | size, qemu_ram_pagesize(rb)) ||
+(ram_offset + size) > qemu_ram_get_used_length(rb)) {
+continue;
+}
+
+ram_block_discard_range(rb, ram_offset, size);
+}
+
+skip_element:
+virtqueue_push(vq, elem, 0);
+virtio_notify(vdev, vq);
+g_free(elem);
+}
+}
+
 static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
 VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
@@ -782,6 +836,16 @@ static void virtio_balloon_device_realize(DeviceState 
*dev, Error **errp)
 VirtIOBalloon *s = VIRTIO_BALLOON(dev);
 int ret;
 
+/*
+ * Page reporting is dependant on page poison to make sure we can
+ * report a page without changing the state of the internal data.
+ * We need to set the flag before we call virtio_init as it will
+ * affect the config size of the vdev.
+ */
+if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
+s->host_features |= 1 << VIRTIO_BALLOON_F_PAGE_POISON;
+}
+
 virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
 virtio_balloon_config_size(s));
 
@@ -798,6 +862,10 @@ static void virtio_balloon_device_realize(DeviceState 
*dev, Error **errp)
 s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
 s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
 
+if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
+s->rvq = virtio_add_queue(vdev, 32, virtio_balloon_handle_report);
+}
+
 if (virtio_has_feature(s->host_features,
VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
 s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
@@ -923,6 +991,8 @@ static Property virtio_balloon_properties[] = {
 VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
 DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
 VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
+DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
+VIRTIO_BALLOON_F_REPORTING, true),
 /* QEMU 4.0 accidentally changed the config size even when free-page-hint
  * is disabled, resulting in QEMU 3.1 migration incompatibility.  Thi

[PATCH v21 QEMU 2/5] linux-headers: update to contain virito-balloon free page reporting

[Alexander Duyck]

From: Alexander Duyck 

Sync the latest upstream changes for free page reporting. To be
replaced by a full linux header sync.

Signed-off-by: Alexander Duyck 
---
 include/standard-headers/linux/virtio_balloon.h |1 +
 1 file changed, 1 insertion(+)

diff --git a/include/standard-headers/linux/virtio_balloon.h 
b/include/standard-headers/linux/virtio_balloon.h
index af0a6b59dab2..af3b7a1fa263 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -36,6 +36,7 @@
 #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM2 /* Deflate balloon on OOM */
 #define VIRTIO_BALLOON_F_FREE_PAGE_HINT3 /* VQ to report free pages */
 #define VIRTIO_BALLOON_F_PAGE_POISON   4 /* Guest is using page poisoning */
+#define VIRTIO_BALLOON_F_REPORTING 5 /* Page reporting virtqueue */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12

Re: [RFC 0/3] 64bit block-layer part I

[Vladimir Sementsov-Ogievskiy]

30.03.2020 17:18, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

There is an idea to make NBD protocol extension to support 64bit
write-zero/discard/block-status commands (commands, which doesn't
transfer user data). It's needed to increase performance of zeroing
large ranges (up to the whole image). Zeroing of the whole image is used
as first step of mirror job, qemu-img convert, it should be also used at
start of backup actually..

We need to support it in block-layer, so we want 64bit write_zeros.
Currently driver handler now have int bytes parameter.

write_zeros path goes through normal pwritev, so we need 64bit write,
and then we need 64bit read for symmetry, and better, let's make all io
path work with 64bit bytes parameter.

Actually most of block-layer already have 64bit parameters: offset is
sometimes int64_t and sometimes uint64_t. bytes parameter is one of
int64_t, uint64_t, int, unsigned int...

I think we need one type for all of this, and this one type is int64_t.
Signed int64_t is a bit better than uint64_t: you can use same variable
to get some result (including error < 0) and than reuse it as an
argument without any type conversion.

So, I propose, as a first step, convert all uint64_t parameters to
int64_t.

Still, I don't have good idea of how to split this into more than 3
patches, so, this is an RFC.

What's next?

Converting write_zero and discard is not as simple: we can't just
s/int/uint64_t/, as some functions may use some int variables for
calculations and this will be broken by something larger than int.

So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and
.bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all
drivers updated - drop unused 32bit functions, and then drop "64" suffix
from API. If not - we'll live with both APIs.


Hmm. Or, maybe nothing dangerous if we convert it to 64bit, and add comment,
that function is not actually prepared for 64bit bytes and depends on
max_transfer/max_zeroes being <= INT_MAX ?

Or, maybe better, keep old functions as is and add 64bit wrappers, which
assert that bytes < INT_MAX, and call old function? This is clean, driver
maybe updated later on demand, and we don't need extra API. If no objections,
I'll try this way in the next version.



Another thing to do is updating default limiting of request (currently
they are limited to INT_MAX).

Then we may move some drivers to 64bit discard/write_zero: I think about
qcow2, file-posix and nbd (as a proof-of-concept for already proposed
NBD extension).

Any ideas?

Vladimir Sementsov-Ogievskiy (3):
   block: use int64_t as bytes type in tracked requests
   block/io: convert generic io path to use int64_t parameters
   block: use int64_t instead of uint64_t in driver handlers

  include/block/block.h |  8 ++--
  include/block/block_int.h | 52 ++---
  block/backup-top.c|  5 +-
  block/blkdebug.c  |  4 +-
  block/blklogwrites.c  |  4 +-
  block/blkreplay.c |  4 +-
  block/blkverify.c |  6 +--
  block/bochs.c |  2 +-
  block/cloop.c |  2 +-
  block/commit.c|  2 +-
  block/copy-on-read.c  |  4 +-
  block/crypto.c|  4 +-
  block/curl.c  |  2 +-
  block/dmg.c   |  2 +-
  block/file-posix.c| 18 
  block/filter-compress.c   |  6 +--
  block/io.c| 97 ---
  block/iscsi.c | 12 ++---
  block/mirror.c|  4 +-
  block/nbd.c   |  8 ++--
  block/nfs.c   |  8 ++--
  block/null.c  |  8 ++--
  block/nvme.c  |  4 +-
  block/qcow.c  | 12 ++---
  block/qcow2.c | 18 
  block/quorum.c|  8 ++--
  block/raw-format.c| 28 +--
  block/rbd.c   |  4 +-
  block/throttle.c  |  4 +-
  block/vdi.c   |  4 +-
  block/vmdk.c  |  8 ++--
  block/vpc.c   |  4 +-
  block/vvfat.c |  6 +--
  tests/test-bdrv-drain.c   |  8 ++--
  block/trace-events| 14 +++---
  35 files changed, 192 insertions(+), 192 deletions(-)




--
Best regards,
Vladimir

Re: [PATCH v2 0/4] smbus: SPD fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200422134815.1584-1-arm...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v2 0/4] smbus: SPD fixes
Message-id: 20200422134815.1584-1-arm...@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   7769c23..ee573f5  master -> master
 - [tag update]  patchew/20200421122236.24867-1-f4...@amsat.org -> 
patchew/20200421122236.24867-1-f4...@amsat.org
 - [tag update]  
patchew/20200422011722.13287-1-richard.hender...@linaro.org -> 
patchew/20200422011722.13287-1-richard.hender...@linaro.org
 - [tag update]  patchew/20200422100211.30614-1-kra...@redhat.com -> 
patchew/20200422100211.30614-1-kra...@redhat.com
 - [tag update]  patchew/20200422124501.28015-1-peter.mayd...@linaro.org -> 
patchew/20200422124501.28015-1-peter.mayd...@linaro.org
 * [new tag] patchew/20200422171305.10923-1-jonathan.derr...@intel.com 
-> patchew/20200422171305.10923-1-jonathan.derr...@intel.com
 * [new tag] patchew/20200422172351.26583-1-pbonz...@redhat.com -> 
patchew/20200422172351.26583-1-pbonz...@redhat.com
Switched to a new branch 'test'
d2614d2 smbus: Fix spd_data_generate() for number of banks > 2
1156589 bamboo, sam460ex: Tidy up error message for unsupported RAM size
fcdd256 smbus: Fix spd_data_generate() error API violation
4de6998 sam460ex: Suppress useless warning on -m 32 and -m 64

=== OUTPUT BEGIN ===
1/4 Checking commit 4de6998f69d4 (sam460ex: Suppress useless warning on -m 32 
and -m 64)
2/4 Checking commit fcdd25692f49 (smbus: Fix spd_data_generate() error API 
violation)
3/4 Checking commit 1156589650a7 (bamboo, sam460ex: Tidy up error message for 
unsupported RAM size)
ERROR: unnecessary whitespace before a quoted newline
#37: FILE: hw/ppc/ppc4xx_devs.c:723:
+error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",

total: 1 errors, 0 warnings, 15 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/4 Checking commit d2614d2e379d (smbus: Fix spd_data_generate() for number of 
banks > 2)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200422134815.1584-1-arm...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v2 18/36] tcg/optimize: Use tcg_constant_internal with constant folding

[Alex Bennée]

Richard Henderson  writes:

> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v2 17/36] tcg/optimize: Adjust TempOptInfo allocation

[Alex Bennée]

Alex Bennée  writes:

> Richard Henderson  writes:
>
>> Do not allocate a large block for indexing.  Instead, allocate
>> for each temporary as they are seen.
>>
>> In general, this will use less memory, if we consider that most
>> TBs do not touch every target register.  This also allows us to
>> allocate TempOptInfo for new temps created during optimization.
>>
>> Signed-off-by: Richard Henderson 
>
> Reviewed-by: Alex Bennée 

>>  
>> -static void init_arg_info(TempOptInfo *infos,
>> -  TCGTempSet *temps_used, TCGArg arg)
>> +static void init_arg_info(TCGTempSet *temps_used, TCGArg arg)
>>  {
>> -init_ts_info(infos, temps_used, arg_temp(arg));
>> +init_ts_info(temps_used, arg_temp(arg));
>>  }

Although I've noticed this function which is only called once where as
others call init_ts_info directly. Any reason to keep it around?

-- 
Alex Bennée

Re: [PATCH v2 19/36] tcg/tci: Add special tci_movi_{i32,i64} opcodes

[Alex Bennée]

Richard Henderson  writes:

> The normal movi opcodes are going away.  We need something
> for TCI to use internally.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  include/tcg/tcg-opc.h| 8 
>  tcg/tci.c| 4 ++--
>  tcg/tci/tcg-target.inc.c | 4 ++--
>  3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h
> index 9288a04946..7dee9b38f7 100644
> --- a/include/tcg/tcg-opc.h
> +++ b/include/tcg/tcg-opc.h
> @@ -268,6 +268,14 @@ DEF(last_generic, 0, 0, 0, TCG_OPF_NOT_PRESENT)
>  #include "tcg-target.opc.h"
>  #endif
>  
> +#ifdef TCG_TARGET_INTERPRETER
> +/* These opcodes are only for use between the tci generator and interpreter. 
> */
> +DEF(tci_movi_i32, 1, 0, 1, TCG_OPF_NOT_PRESENT)
> +#if TCG_TARGET_REG_BITS == 64
> +DEF(tci_movi_i64, 1, 0, 1, TCG_OPF_64BIT | TCG_OPF_NOT_PRESENT)
> +#endif
> +#endif
> +
>  #undef TLADDR_ARGS
>  #undef DATA64_ARGS
>  #undef IMPL
> diff --git a/tcg/tci.c b/tcg/tci.c
> index 46fe9ce63f..a6c1aaf5af 100644
> --- a/tcg/tci.c
> +++ b/tcg/tci.c
> @@ -576,7 +576,7 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t 
> *tb_ptr)
>  t1 = tci_read_r32(regs, &tb_ptr);
>  tci_write_reg32(regs, t0, t1);
>  break;
> -case INDEX_op_movi_i32:
> +case INDEX_op_tci_movi_i32:
>  t0 = *tb_ptr++;
>  t1 = tci_read_i32(&tb_ptr);
>  tci_write_reg32(regs, t0, t1);
> @@ -847,7 +847,7 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t 
> *tb_ptr)
>  t1 = tci_read_r64(regs, &tb_ptr);
>  tci_write_reg64(regs, t0, t1);
>  break;
> -case INDEX_op_movi_i64:
> +case INDEX_op_tci_movi_i64:
>  t0 = *tb_ptr++;
>  t1 = tci_read_i64(&tb_ptr);
>  tci_write_reg64(regs, t0, t1);
> diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
> index 992d50cb1e..1f1639df0d 100644
> --- a/tcg/tci/tcg-target.inc.c
> +++ b/tcg/tci/tcg-target.inc.c
> @@ -530,13 +530,13 @@ static void tcg_out_movi(TCGContext *s, TCGType type,
>  uint8_t *old_code_ptr = s->code_ptr;
>  uint32_t arg32 = arg;
>  if (type == TCG_TYPE_I32 || arg == arg32) {
> -tcg_out_op_t(s, INDEX_op_movi_i32);
> +tcg_out_op_t(s, INDEX_op_tci_movi_i32);
>  tcg_out_r(s, t0);
>  tcg_out32(s, arg32);
>  } else {
>  tcg_debug_assert(type == TCG_TYPE_I64);
>  #if TCG_TARGET_REG_BITS == 64
> -tcg_out_op_t(s, INDEX_op_movi_i64);
> +tcg_out_op_t(s, INDEX_op_tci_movi_i64);
>  tcg_out_r(s, t0);
>  tcg_out64(s, arg);
>  #else


-- 
Alex Bennée

Re: [PATCH v2 20/36] tcg: Remove movi and dupi opcodes

[Alex Bennée]

Richard Henderson  writes:

> These are now completely covered by mov from a
> TYPE_CONST temporary.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  include/tcg/tcg-opc.h|  3 ---
>  tcg/aarch64/tcg-target.inc.c |  3 ---
>  tcg/arm/tcg-target.inc.c |  1 -
>  tcg/i386/tcg-target.inc.c|  3 ---
>  tcg/mips/tcg-target.inc.c|  2 --
>  tcg/optimize.c   |  4 
>  tcg/ppc/tcg-target.inc.c |  3 ---
>  tcg/riscv/tcg-target.inc.c   |  2 --
>  tcg/s390/tcg-target.inc.c|  2 --
>  tcg/sparc/tcg-target.inc.c   |  2 --
>  tcg/tcg-op-vec.c |  1 -
>  tcg/tcg.c| 18 +-
>  tcg/tci/tcg-target.inc.c |  2 --
>  13 files changed, 1 insertion(+), 45 deletions(-)
>
> diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h
> index 7dee9b38f7..4a9cbf5426 100644
> --- a/include/tcg/tcg-opc.h
> +++ b/include/tcg/tcg-opc.h
> @@ -45,7 +45,6 @@ DEF(br, 0, 0, 1, TCG_OPF_BB_END)
>  DEF(mb, 0, 0, 1, 0)
>  
>  DEF(mov_i32, 1, 1, 0, TCG_OPF_NOT_PRESENT)
> -DEF(movi_i32, 1, 0, 1, TCG_OPF_NOT_PRESENT)
>  DEF(setcond_i32, 1, 2, 1, 0)
>  DEF(movcond_i32, 1, 4, 1, IMPL(TCG_TARGET_HAS_movcond_i32))
>  /* load/store */
> @@ -110,7 +109,6 @@ DEF(ctz_i32, 1, 2, 0, IMPL(TCG_TARGET_HAS_ctz_i32))
>  DEF(ctpop_i32, 1, 1, 0, IMPL(TCG_TARGET_HAS_ctpop_i32))
>  
>  DEF(mov_i64, 1, 1, 0, TCG_OPF_64BIT | TCG_OPF_NOT_PRESENT)
> -DEF(movi_i64, 1, 0, 1, TCG_OPF_64BIT | TCG_OPF_NOT_PRESENT)
>  DEF(setcond_i64, 1, 2, 1, IMPL64)
>  DEF(movcond_i64, 1, 4, 1, IMPL64 | IMPL(TCG_TARGET_HAS_movcond_i64))
>  /* load/store */
> @@ -215,7 +213,6 @@ DEF(qemu_st_i64, 0, TLADDR_ARGS + DATA64_ARGS, 1,
>  #define IMPLVEC  TCG_OPF_VECTOR | IMPL(TCG_TARGET_MAYBE_vec)
>  
>  DEF(mov_vec, 1, 1, 0, TCG_OPF_VECTOR | TCG_OPF_NOT_PRESENT)
> -DEF(dupi_vec, 1, 0, 1, TCG_OPF_VECTOR | TCG_OPF_NOT_PRESENT)
>  
>  DEF(dup_vec, 1, 1, 0, IMPLVEC)
>  DEF(dup2_vec, 1, 2, 0, IMPLVEC | IMPL(TCG_TARGET_REG_BITS == 32))
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index 843fd0ca69..7918aeb9d5 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -2261,8 +2261,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>  
>  case INDEX_op_mov_i32:  /* Always emitted via tcg_out_mov.  */
>  case INDEX_op_mov_i64:
> -case INDEX_op_movi_i32: /* Always emitted via tcg_out_movi.  */
> -case INDEX_op_movi_i64:
>  case INDEX_op_call: /* Always emitted via tcg_out_call.  */
>  default:
>  g_assert_not_reached();
> @@ -2467,7 +2465,6 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
>  break;
>  
>  case INDEX_op_mov_vec:  /* Always emitted via tcg_out_mov.  */
> -case INDEX_op_dupi_vec: /* Always emitted via tcg_out_movi.  */
>  case INDEX_op_dup_vec:  /* Always emitted via tcg_out_dup_vec.  */
>  default:
>  g_assert_not_reached();
> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
> index 6aa7757aac..b967499fa4 100644
> --- a/tcg/arm/tcg-target.inc.c
> +++ b/tcg/arm/tcg-target.inc.c
> @@ -2068,7 +2068,6 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode 
> opc,
>  break;
>  
>  case INDEX_op_mov_i32:  /* Always emitted via tcg_out_mov.  */
> -case INDEX_op_movi_i32: /* Always emitted via tcg_out_movi.  */
>  case INDEX_op_call: /* Always emitted via tcg_out_call.  */
>  default:
>  tcg_abort();
> diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
> index ec083bddcf..320a4bddd1 100644
> --- a/tcg/i386/tcg-target.inc.c
> +++ b/tcg/i386/tcg-target.inc.c
> @@ -2678,8 +2678,6 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode 
> opc,
>  break;
>  case INDEX_op_mov_i32:  /* Always emitted via tcg_out_mov.  */
>  case INDEX_op_mov_i64:
> -case INDEX_op_movi_i32: /* Always emitted via tcg_out_movi.  */
> -case INDEX_op_movi_i64:
>  case INDEX_op_call: /* Always emitted via tcg_out_call.  */
>  default:
>  tcg_abort();
> @@ -2965,7 +2963,6 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
>  break;
>  
>  case INDEX_op_mov_vec:  /* Always emitted via tcg_out_mov.  */
> -case INDEX_op_dupi_vec: /* Always emitted via tcg_out_movi.  */
>  case INDEX_op_dup_vec:  /* Always emitted via tcg_out_dup_vec.  */
>  default:
>  g_assert_not_reached();
> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
> index 4d32ebc1df..09dc5a94fa 100644
> --- a/tcg/mips/tcg-target.inc.c
> +++ b/tcg/mips/tcg-target.inc.c
> @@ -2155,8 +2155,6 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode 
> opc,
>  break;
>  case INDEX_op_mov_i32:  /* Always emitted via tcg_out_mov.  */
>  case INDEX_op_mov_i64:
> -case INDEX_op_movi_i32: /* Always emitted via tcg_out_movi.  */
> -case INDEX_op_movi_i64:
>  case INDEX_op_call: /* Always emitted via tcg_out_call.  */
>  de