Re: [PATCH 1/2] configure: avx2 and avx512f detection for clang

[Thomas Huth]

On 23/07/2020 08.04, Shu-Chun Weng wrote:
> Do we have the flexibility to do that for util/bufferiszero.c as well?
> Otherwise, we are using different mechanisms to detect (compile test.c
> with -mavx2) and actually use (GCC pragma & __attribute__((target(*)
> the feature in production.

That's true ... so it's likely better to keep the pragmas in the
configure script, indeed!

 Thanks,
  Thomas


> Shu-Chun
> 
> On Wed, Jul 22, 2020 at 9:55 PM Thomas Huth  > wrote:
> 
> On 23/07/2020 02.27, Shu-Chun Weng wrote:
> > Since clang does not support "#pragma GCC", the instruction sets are
> > always disabled. In this change, we
> >
> >  1. wrap "#pragma GCC" inside "#ifndef __clang__",
> >  2. only retain them around "#include <{e,i,s}mmintrin.h>" to work
> >     around gcc bug,
> >  3. and annotate each function with `__attribute__((target(*)))` which
> >     is recognized by both gcc and clang.
> >
> > Signed-off-by: Shu-Chun Weng mailto:s...@google.com>>
> > ---
> >  configure           | 16 ++--
> >  util/bufferiszero.c | 33 +++--
> >  2 files changed, 37 insertions(+), 12 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 4bd80ed507..d9ce3aa5db 100755
> > --- a/configure
> > +++ b/configure
> > @@ -5808,10 +5808,16 @@ fi
> > 
> >  if test "$cpuid_h" = "yes" && test "$avx2_opt" != "no"; then
> >    cat > $TMPC << EOF
> > +#include 
> > +#ifndef __clang__
> >  #pragma GCC push_options
> >  #pragma GCC target("avx2")
> > -#include 
> > +#endif
> >  #include 
> > +#ifndef __clang__
> > +#pragma GCC pop_options
> > +#endif
> > +__attribute__((target("avx2")))
> >  static int bar(void *a) {
> >      __m256i x = *(__m256i *)a;
> >      return _mm256_testz_si256(x, x);
> 
> I wonder whether it would make more sense to pass "-mavx2" to the
> compile_object call afterwards and simply remove the #pragmas here?
> Did you try that already?
> 
>  Thomas
> 

Re: [PATCH 1/2] configure: avx2 and avx512f detection for clang

[Shu-Chun Weng]

Do we have the flexibility to do that for util/bufferiszero.c as well?
Otherwise, we are using different mechanisms to detect (compile test.c with
-mavx2) and actually use (GCC pragma & __attribute__((target(*) the
feature in production.

Shu-Chun

On Wed, Jul 22, 2020 at 9:55 PM Thomas Huth  wrote:

> On 23/07/2020 02.27, Shu-Chun Weng wrote:
> > Since clang does not support "#pragma GCC", the instruction sets are
> > always disabled. In this change, we
> >
> >  1. wrap "#pragma GCC" inside "#ifndef __clang__",
> >  2. only retain them around "#include <{e,i,s}mmintrin.h>" to work
> > around gcc bug,
> >  3. and annotate each function with `__attribute__((target(*)))` which
> > is recognized by both gcc and clang.
> >
> > Signed-off-by: Shu-Chun Weng 
> > ---
> >  configure   | 16 ++--
> >  util/bufferiszero.c | 33 +++--
> >  2 files changed, 37 insertions(+), 12 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 4bd80ed507..d9ce3aa5db 100755
> > --- a/configure
> > +++ b/configure
> > @@ -5808,10 +5808,16 @@ fi
> >
> >  if test "$cpuid_h" = "yes" && test "$avx2_opt" != "no"; then
> >cat > $TMPC << EOF
> > +#include 
> > +#ifndef __clang__
> >  #pragma GCC push_options
> >  #pragma GCC target("avx2")
> > -#include 
> > +#endif
> >  #include 
> > +#ifndef __clang__
> > +#pragma GCC pop_options
> > +#endif
> > +__attribute__((target("avx2")))
> >  static int bar(void *a) {
> >  __m256i x = *(__m256i *)a;
> >  return _mm256_testz_si256(x, x);
>
> I wonder whether it would make more sense to pass "-mavx2" to the
> compile_object call afterwards and simply remove the #pragmas here?
> Did you try that already?
>
>  Thomas
>
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Cornelia Huck]

On Tue, 21 Jul 2020 14:40:14 -0400
Collin Walling  wrote:

> On 7/21/20 4:41 AM, David Hildenbrand wrote:

> > The options I would support are
> > 
> > 1. "sccb_boundary_is_valid" which returns "true" if valid
> > 2. "sccb_boundary_is_invalid" which returns "true" if invalid
> > 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
> > 
> > Which makes reading this code a bit easier.
> >   

Of these, I like option 1 best.

> 
> Sounds good. I'll takes this into consideration for the next round. (I
> may wait just a little longer for that to allow more reviews to come in
> from whoever has the time, if that's okay.)

We have to wait for (a) QEMU to do a release and (b) the Linux changes
to merge upstream anyway, so we're not in a hurry :)

As said before, it already looked good from my side, but the suggested
changes are fine with me as well.

Re: [PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

On Mon, 20 Jul 2020 11:07:51 +0200
David Hildenbrand  wrote:

> On 20.07.20 11:03, Michael S. Tsirkin wrote:
> > On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:  
> >> On 07.07.20 12:54, Cornelia Huck wrote:  
> >>> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> >>> a good idea to make sure that any new virtio device (which does not
> >>> support legacy virtio) is indeed a non-transitional device, just to
> >>> catch accidental misconfigurations. We can easily compile a list
> >>> of virtio devices with legacy support and have transports verify
> >>> in their plugged callbacks that legacy support is off for any device
> >>> not in that list.
> >>>
> >>> Most new virtio devices force non-transitional already, so nothing
> >>> changes for them. vhost-user-fs-pci even does not allow to configure
> >>> a non-transitional device, so it is fine as well.
> >>>
> >>> One problematic device, however, is virtio-iommu-pci. It currently
> >>> offers both the transitional and the non-transitional variety of the
> >>> device, and does not force anything. I'm unsure whether we should
> >>> consider transitional virtio-iommu unsupported, or if we should add
> >>> some compat handling. (The support for legacy or not generally may
> >>> change based upon the bus, IIUC, so I'm unsure how to come up with
> >>> something generic.)
> >>>
> >>> Cornelia Huck (2):
> >>>   virtio: list legacy-capable devices
> >>>   virtio: verify that legacy support is not accidentally on  
> >>
> >> I'd squash both patches. Looking at patch #1, I wonder why we don't
> >> store that information along with the device implementation? What was
> >> the motivation to define this information separately?  
> > 
> > Because people seem to cut and paste code, so when one
> > enables it in an old device, it gets pasted into a new one.
> > With a list in a central place, it's easier to figure out
> > what's going on.  
> 
> Makes sense, I suggest adding that to the patch description.

"The list of devices supporting legacy is supposed to be static. We
keep it in a central place to make sure that new devices do not enable
legacy by accident."

?

> 
> Both patches look sane to me (- squashing them).
> 

Patch 1 does not change behaviour, while patch 2 does (for
virtio-iommu-pci). Still would like an opinion whether changing the
behaviour for virtio-iommu-pci with no compat handling is ok.

(I could be persuaded to squash them.)

Re: [PATCH-for-5.1] hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value

[Markus Armbruster]

Cc: Vladimir

Laszlo Ersek  writes:

> On 07/21/20 10:33, Markus Armbruster wrote:
>> Laszlo Ersek  writes:
>>
>>> On 07/20/20 14:35, Philippe Mathieu-Daudé wrote:
 Commits b6d7e9b66f..a43770df5d simplified the error propagation.
 Similarly to commit 6fd5bef10b "qom: Make functions taking Error**
 return bool, not void", let fw_cfg_add_from_generator() return a
 boolean value, not void.
 This allow to simplify parse_fw_cfg() and fixes the error handling
 issue reported by Coverity (CID 1430396):

   In parse_fw_cfg():

 Variable assigned once to a constant guards dead code.

 Local variable local_err is assigned only once, to a constant
 value, making it effectively constant throughout its scope.
 If this is not the intent, examine the logic to see if there
 is a missing assignment that would make local_err not remain
 constant.
>>
>> It's the call of fw_cfg_add_from_generator():
>>
>> Error *local_err = NULL;
>>
>> fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return -1;
>> }
>> return 0;
>>
>> If it fails, parse_fw_cfg() sets an error and returns 0, which is wrong.
>> Harmless, because the only caller passes _fatal.
>>
>> Please work this impact assessment into the commit message.
>>

 Reported-by: Peter Maydell 
 Fixes: Coverity CID 1430396: 'Constant' variable guards dead code 
 (DEADCODE)
 Fixes: 6552d87c48 ("softmmu/vl: Let -fw_cfg option take a 'gen_id' 
 argument")
 Signed-off-by: Philippe Mathieu-Daudé 
 ---
  include/hw/nvram/fw_cfg.h |  4 +++-
  hw/nvram/fw_cfg.c | 10 ++
  softmmu/vl.c  |  6 +-
  3 files changed, 10 insertions(+), 10 deletions(-)

 diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
 index 11feae3177..d90857f092 100644
 --- a/include/hw/nvram/fw_cfg.h
 +++ b/include/hw/nvram/fw_cfg.h
 @@ -302,8 +302,10 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
 *filename, void *data,
   * will be used; also, a new entry will be added to the file directory
   * structure residing at key value FW_CFG_FILE_DIR, containing the item 
 name,
   * data size, and assigned selector key value.
 + *
 + * Returns: %true on success, %false on error.
   */
 -void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
 +bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
 const char *gen_id, Error **errp);

  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
 diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
 index 3b1811d3bf..c88aec4341 100644
 --- a/hw/nvram/fw_cfg.c
 +++ b/hw/nvram/fw_cfg.c
 @@ -1032,7 +1032,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
 *filename,
  return NULL;
  }

 -void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
 +bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
 const char *gen_id, Error **errp)
  {
  ERRP_GUARD();
 @@ -1044,20 +1044,22 @@ void fw_cfg_add_from_generator(FWCfgState *s, 
 const char *filename,
  obj = object_resolve_path_component(object_get_objects_root(), 
 gen_id);
  if (!obj) {
  error_setg(errp, "Cannot find object ID '%s'", gen_id);
 -return;
 +return false;
  }
  if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
  error_setg(errp, "Object ID '%s' is not a '%s' subclass",
 gen_id, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE);
 -return;
 +return false;
  }
  klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
  array = klass->get_data(obj, errp);
  if (*errp) {
 -return;
 +return false;
  }
  size = array->len;
  fw_cfg_add_file(s, filename, g_byte_array_free(array, TRUE), size);
 +
 +return true;
  }

  static void fw_cfg_machine_reset(void *opaque)
 diff --git a/softmmu/vl.c b/softmmu/vl.c
 index f476ef89ed..3416241557 100644
 --- a/softmmu/vl.c
 +++ b/softmmu/vl.c
 @@ -2070,11 +2070,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts 
 *opts, Error **errp)
  size = strlen(str); /* NUL terminator NOT included in fw_cfg blob 
 */
  buf = g_memdup(str, size);
  } else if (nonempty_str(gen_id)) {
 -Error *local_err = NULL;
 -
 -fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
 -if (local_err) {
 -error_propagate(errp, local_err);
 +if (!fw_cfg_add_from_generator(fw_cfg, 

Re: [PATCH v2 15/20] iotests: 219: prepare for backup over block-copy

[Max Reitz]

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> The further change of moving backup to be a on block-copy call will

-on?

> make copying chunk-size and cluster-size a separate things. So, even

s/a/two/

> with 64k cluster sized qcow2 image, default chunk would be 1M.
> Test 219 depends on specified chunk-size. Update it for explicit
> chunk-size for backup as for mirror.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/219 | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
> index db272c5249..2bbed28f39 100755
> --- a/tests/qemu-iotests/219
> +++ b/tests/qemu-iotests/219
> @@ -203,13 +203,13 @@ with iotests.FilePath('disk.img') as disk_path, \
>  # but related to this also automatic state transitions like job
>  # completion), but still get pause points often enough to avoid making 
> this
>  # test very slow, it's important to have the right ratio between speed 
> and
> -# buf_size.
> +# copy-chunk-size.
>  #
> -# For backup, buf_size is hard-coded to the source image cluster size 
> (64k),
> -# so we'll pick the same for mirror. The slice time, i.e. the granularity
> -# of the rate limiting is 100ms. With a speed of 256k per second, we can
> -# get four pause points per second. This gives us 250ms per iteration,
> -# which should be enough to stay deterministic.
> +# Chose 64k copy-chunk-size both for mirror (by buf_size) and backup (by
> +# x-max-chunk). The slice time, i.e. the granularity of the rate limiting
> +# is 100ms. With a speed of 256k per second, we can get four pause points
> +# per second. This gives us 250ms per iteration, which should be enough 
> to
> +# stay deterministic.

Don’t we also have to limit the number of workers to 1 so we actually
keep 250 ms per iteration instead of just finishing four requests
immediately, then pausing for a second?

>  test_job_lifecycle(vm, 'drive-mirror', has_ready=True, job_args={
>  'device': 'drive0-node',
> @@ -226,6 +226,7 @@ with iotests.FilePath('disk.img') as disk_path, \
>  'target': copy_path,
>  'sync': 'full',
>  'speed': 262144,
> +'x-max-chunk': 65536,
>  'auto-finalize': auto_finalize,
>  'auto-dismiss': auto_dismiss,
>  })
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 16/20] iotests: 257: prepare for backup over block-copy

[Max Reitz]

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> Iotest 257 dumps a lot of in-progress information of backup job, such
> as offset and bitmap dirtiness. Further commit will move backup to be
> one block-copy call, which will introduce async parallel requests
> instead of plain cluster-by-cluster copying. To keep things
> deterministic, allow only one worker (only one copy request at a time)
> for this test.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/257 |   1 +
>  tests/qemu-iotests/257.out | 306 ++---
>  2 files changed, 154 insertions(+), 153 deletions(-)

It’s a shame that we don’t run this test with the default configuration
then, but I suppose that’s how it is.  For now, at least.

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device)

[Markus Armbruster]

Stefan Berger  writes:

> On 7/22/20 1:55 AM, Markus Armbruster wrote:
>> pm socket --tpmstate dir=tpm --ctrl type=unixio,path=tpm/swtpm-soc
>> running in another terminal.
>>
 3/ no machine plug it using isa_register_ioport()
 (it is not registered to the ISA memory space)
>>> There's no requirement for an ISA device to have IO ports...
>>>
>>> thanks
>>> -- PMM
>> Thread hijack!  Since I didn't have swtpm installed, I tried to take a
>> shortcut:
>>
>>  $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio 
>> -chardev null,id=tpm0 -tpmdev emulator,id=tpm0,chardev=chrtpm -device 
>> tpm-tis,tpmdev=tpm0
>>  qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: 
>> tpm-emulator: tpm chardev 'chrtpm' not found.
>>  qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: 
>> tpm-emulator: Could not cleanly shutdown the TPM: No such file or directory
>>  QEMU 5.0.90 monitor - type 'help' for more information
>>  (qemu) qemu-system-x86_64: -device tpm-tis,tpmdev=tpm0: Property 
>> 'tpm-tis.tpmdev' can't find value 'tpm0'
>>  $ echo $?
>>  1
>>
>> That a null chardev doesn't work is fine.  But the error handling looks
>> broken: QEMU diagnoses and reports the problem, then continues.  The
>> final error message indicates that it continued without creating the
>> backend "tpm0".  That's wrong.
>
>
> This issue can be solve via the following change that then displays
> this error:
>
> $ x86_64-softmmu/qemu-system-x86_64 -nodefaults -S -display none
> -monitor stdio -chardev null,id=tpm0 -tpmdev
> emulator,id=tpm0,chardev=chrtpm -device tpm-tis,tpmdev=tpm0
> qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm:
> tpm-emulator: tpm chardev 'chrtpm' not found.
> qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm:
> tpm-emulator: Could not cleanly shutdown the TPM: No such file or
> directory
>
>
> diff --git a/tpm.c b/tpm.c
> index 358566cb10..857a861e69 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -170,8 +170,10 @@ void tpm_cleanup(void)
>   */
>  void tpm_init(void)
>  {
> -    qemu_opts_foreach(qemu_find_opts("tpmdev"),
> -  tpm_init_tpmdev, NULL, _fatal);
> +    if (qemu_opts_foreach(qemu_find_opts("tpmdev"),
> +  tpm_init_tpmdev, NULL, _fatal)) {
> +    exit(1);
> +    }
>  }
>
>  /*

Interesting.

> We had something like this before this patch here was applied:
> https://github.com/qemu/qemu/commit/d10e05f15d5c3dd5e5cc59c5dfff460d89d48580#diff-0ec5df49c6751cb2dc9fa18ed5cf9f0e
>
>
> Do we now want to partially revert this patch or call the exit(1) as
> shown here?

Let's have a closer look.

qemu_opts_foreach()'s contract:

 * For each member of @list, call @func(@opaque, member, @errp).
 * Call it with the current location temporarily set to the member's.
 * @func() may store an Error through @errp, but must return non-zero then.
 * When @func() returns non-zero, break the loop and return that value.
 * Return zero when the loop completes.

When qemu_opts_foreach(list, func, opaque, _fatal) returns, then
func() did not set an error (If it did, we'd have died due to
_fatal).

Therefore, func() must have returned non-zero without setting an error.
That's wrong.  Let's look for this in tpm_init_tpmdev():

static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
{
[...]
drv = be->create(opts);
if (!drv) {
return 1;

Bingo!

When I did commit d10e05f15d5, I missed this error path.

}

drv->id = g_strdup(id);
QLIST_INSERT_HEAD(_backends, drv, list);

return 0;
}

Two possible fixes:

1. Revert d10e05f15d5, live with the "error_report() in a function that
takes an Error ** argument" code smell.  Bearable, because it's confined
to tpm.c.  I'd recommend a comment explaining the non-use of @errp in
tpm_init_tpmdev().

2. Convert the ->create() to Error: tpm_passthrough_create(),
tpm_emulator_create(), and their helpers.  I think this would leave us
in a better state, but I'm not sure the improvement is worth the effort
right now.

Spotted while writing this: ->tpm_startup() methods can fail.  They
appear to run in DeviceClass method reset(), which can't fail.
Awkward.  Could some failures be moved to realize() somehow?

Re: [PATCH for-5.1] sd/milkymist-memcard: Fix format string

[Markus Armbruster]

Stefan Weil  writes:

Let's add

  Fixes: b98e8d1230ff7023bb34ddeb7194424dfcbaf789

> Signed-off-by: Stefan Weil 
> ---
>  hw/sd/milkymist-memcard.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index afdb8aa0c0..11f61294fc 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -281,7 +281,7 @@ static void milkymist_memcard_realize(DeviceState *dev, 
> Error **errp)
>  carddev = qdev_new(TYPE_SD_CARD);
>  qdev_prop_set_drive(carddev, "drive", blk);
>  if (!qdev_realize_and_unref(carddev, BUS(>sdbus), )) {
> -error_propagate_prepend(errp, err, "failed to init SD card: %s");
> +error_propagate_prepend(errp, err, "failed to init SD card");
>  return;
>  }
>  s->enabled = blk && blk_is_inserted(blk);

Reviewed-by: Markus Armbruster 

Missed because error_propagate_prepend() lacks GCC_FMT_ATTR().  I'll fix
that.  Since the fix needs to be on top of this patch, me taking both
patches through my tree would be easier for me, assuming Michael doesn't
mind.

Thanks for the fix, Stefan!

Re: Replacing existing kernel with new on qemuriscv64

[Pankaj Vinadrao Joshi]

Hi,
yes am building it natively for riscv (sifive's u540 hardware ) and in /boot  i 
am able to find System.map-5.7.2 and vmlinux-5.7.2

root@exaleapsemi:~/linux-stable# make install
make: Warning: File '.vmlinux.cmd' has modification time 1221719 s in the future
make[1]: Warning: File 'arch/riscv/boot/.Image.cmd' has modification time 
1221720 s in the future
sh ./arch/riscv/boot/install.sh 5.7.2 \
arch/riscv/boot/Image System.map "/boot"
Installing normal kernel

But now i want to boot with this kernel but i am not able to do so..its booting 
from the earlier kernel only..i am not able to figure out why this is happening 
,while building kernel on x86 with $make install and after reboot i am able to 
get the new kernel but the same doesnt seems to be working on my hardware.
How i should do that??Kindly suggest.

Thanks

From: Alistair Francis 
Sent: Thursday, July 23, 2020 6:07 AM
To: Pankaj Vinadrao Joshi 
Subject: Re: Replacing existing kernel with new on qemuriscv64

On Mon, Jul 20, 2020 at 10:31 PM Pankaj Vinadrao Joshi
 wrote:
>
> Hi,
> i will explain what i mean is, i am using sifive's u540 hardware for which we 
> have built our own custom kernel using openembedded-core lets say 5.5.6 and 
> now i want to build the other kernel lets say 5.7, natively

Natively for which platform? You want to do build a new kernel on the
hardware and then boot into that? The kernel's are kept in a boot
partition, you should just have to copy them to that location.

> and replace the old kernel with new one,and for this i really did not 
> understood where and what exactly i should modify to boot from the new 
> kernel??

Yep, just replace the old one.

>
> The steps what i followed are following
>
> 1) i have cloned the kernel source
> 2)make menuconfig
> 3)make -j4
> 4)make modules_install
> 5make install

make install is probably not going to understand what to do. I think
you will need to manually do this. I can't remember where the
partition is, but it should be easy to find.

Alistair

>
> Hope now its clear what i was trying to ask for??
>
> Thanks & regards
> Pankaj
>
>
>
> 
> From: Alistair Francis 
> Sent: Tuesday, July 21, 2020 12:10 AM
> To: Pankaj Vinadrao Joshi 
> Cc: qemu-devel@nongnu.org 
> Subject: Re: Replacing existing kernel with new on qemuriscv64
>
> On Mon, Jul 20, 2020 at 11:19 AM Pankaj Vinadrao Joshi
>  wrote:
> >
> > Hi Alistair Francis,
> >
> > Thanks for your response.I have same concern for other riscv hardware with 
> > custom kernel will same be applicable for it?If no from where i should 
> > change my kernel image since there also i am not able to find grub and the 
> > clarification given on web are seems to be very specific to x86 arch where 
> > i should look in case of riscv ??
>
> Hello,
>
> I'm not really sure what you are asking.
>
> The way to update the kernel will depend on how you are booting it and
> where it is stored. GRUB supports RISC-V, but as there is no UEFI
> support in the kernel (yet) it probably isn't the best boot method.
> Most hardware will probably use u-boot instead.
>
> Alistair
>
> >
> > Thanks
> >
> > 
> > From: Alistair Francis 
> > Sent: Monday, July 20, 2020 10:29 PM
> > To: Pankaj Vinadrao Joshi 
> > Cc: qemu-devel@nongnu.org 
> > Subject: Re: Replacing existing kernel with new on qemuriscv64
> >
> > On Mon, Jul 20, 2020 at 2:46 AM Pankaj Vinadrao Joshi
> >  wrote:
> > >
> > > Hi ,
> > > I am trying to replace my existing kernel image which is 5.5.6 to 5.5.7 
> > > on qemuriscv64 i would like to mention i have built my qemu image with 
> > > openembedded-core.
> > >
> > > i have tried to build the kernel 5.5.7 natively,i was able to build the 
> > > kernel successfully and in my /boot folder i am able to see vmlinux-5.5.7 
> > >  but now i want to install this image..i have tried to do it by make 
> > > install i didnt got any error with it but when i rebooted my system i am 
> > > getting
> > >
> > > i have followed the following steps to build kernel
> > > $root@qemuriscv64-exaleapsemi-r2:/usr/src/kernel#  make menuconfig 
> > > arch=riscv64
> > > $root@qemuriscv64-exaleapsemi-r2:/usr/src/kernel# make -j4
> > > $root@qemuriscv64-exaleapsemi-r2:/usr/src/kernel# make modules_install
> > > $root@qemuriscv64-exaleapsemi-r2:/usr/src/kernel# make install
> > > sh ./arch/riscv/boot/install.sh 5.5.7 \
> > > arch/riscv/boot/Image System.map "/boot"
> > > Installing normal kernel
> > >
> > > $reboot
> > > 13024.451157] printk: systemd-shutdow: 37 output lines suppressed due to 
> > > ratelimiting
> > > [13024.527282] systemd-shutdown[1]: Syncing filesystems and block devices.
> > > [13024.668538] systemd-shutdown[1]: Sending SIGTERM to remaining 
> > > processes...
> > > [13024.719496] systemd-journald[87]: Received SIGTERM from PID 1 
> > > (systemd-shutdow).
> > > [13024.769405] systemd-shutdown[1]: Sending SIGKILL to remaining 
> > > processes...
> > > 

Re: 5.1.0-rc1 regression: reset fails with kvm and -cpu host

[Philippe Mathieu-Daudé]

+Vitaly

On 7/23/20 10:40 AM, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabk...@redhat.com) wrote:
>> On Wed, Jul 22, 2020 at 04:47:32PM -0400, Eduardo Habkost wrote:
>>> On Wed, Jul 22, 2020 at 08:05:01PM +0200, Jan Kiszka wrote:
 On 22.07.20 19:35, Eduardo Habkost wrote:
> Hi Jan,
>
> What was the last version where it worked for you?  Does using
> "-cpu host,-vmx" help?

 Yeah, -vmx does indeed help.

 I didn't have the time to bisect yet. Just check my reflog, picked
 eb6490f544, and that works.
>>>
>>> Thanks!
>>>
>>> I could reproduce it locally[1], I will bisect it.
>>>
>>> The good news is that "-cpu host,+vmx" still works, on commit
>>> eb6490f544.
>>>
>>> [1] Linux 5.6.19-300.fc32.x86_64, Intel Core i7-8665U CPU.
>>
>> Bisected to:
>>
>> commit b16c0e20c74218f2d69710cedad11da7dd4d2190
>> Author: Paolo Bonzini 
>> Date:   Wed May 20 10:49:22 2020 -0400
>>
>> KVM: add support for AMD nested live migration
>>
>> Support for nested guest live migration is part of Linux 5.8, add the
>> corresponding code to QEMU.  The migration format consists of a few
>> flags, is an opaque 4k blob.
>>
>> The blob is in VMCB format (the control area represents the L1 VMCB
>> control fields, the save area represents the pre-vmentry state; KVM does
>> not use the host save area since the AMD manual allows that) but QEMU
>> does not really care about that.  However, the flags need to be
>> copied to hflags/hflags2 and back.
>>
>> In addition, support for retrieving and setting the AMD nested 
>> virtualization
>> states allows the L1 guest to be reset while running a nested guest, but
>> a small bug in CPU reset needs to be fixed for that to work.
>>
>> Signed-off-by: Paolo Bonzini 
> 
> Guesswork led me to try reverting the chunk in kvm_put_nested_state;
> without it the reset seems to work; I can't explain that code though.
> 
> Dave
> 
>>
>> -- 
>> Eduardo
>>
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 
> 

Re: [PULL 0/2] fw_cfg patches for 2020-07-21

[Peter Maydell]

On Tue, 21 Jul 2020 at 18:54, Philippe Mathieu-Daudé  wrote:
>
> The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:
>
>   Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07=
> -21' into staging (2020-07-21 10:24:38 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/fw_cfg-20200721
>
> for you to fetch changes up to 077195187b47d83418e5fb521c89d7881fab3049:
>
>   hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value (2020=
> -07-21 16:47:54 +0200)
>
> 
> fw_cfg patches
>
> Fixes the DEADCODE issue reported by Coverity (CID 1430396).
>
> CI jobs result:
> . https://gitlab.com/philmd/qemu/-/pipelines/169086301
>
> 
>
> Philippe Mathieu-Daud=C3=A9 (2):

Something in your cover-letter creation is not handling UTF-8 right :-)

>   hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error
> propagation
>   hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

Re: [PATCH v2 12/20] iotests: 56: prepare for backup over block-copy

[Max Reitz]

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> After introducing parallel async copy requests instead of plain
> cluster-by-cluster copying loop, we'll have to wait for paused status,
> as we need to wait for several parallel request. So, let's gently wait
> instead of just asserting that job already paused.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/056 | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
> index f73fc74457..2ced356a43 100755
> --- a/tests/qemu-iotests/056
> +++ b/tests/qemu-iotests/056
> @@ -306,8 +306,12 @@ class BackupTest(iotests.QMPTestCase):
>  event = self.vm.event_wait(name="BLOCK_JOB_ERROR",
> match={'data': {'device': 'drive0'}})
>  self.assertNotEqual(event, None)
> -# OK, job should be wedged
> -res = self.vm.qmp('query-block-jobs')
> +# OK, job should pause, but it can't do it immediately, as it can't
> +# cancel other parallel requests (which didn't fail)
> +while True:
> +res = self.vm.qmp('query-block-jobs')
> +if res['return'][0]['status'] == 'paused':
> +break

A timeout around this would be nice, I think.

>  self.assert_qmp(res, 'return[0]/status', 'paused')
>  res = self.vm.qmp('block-job-dismiss', id='drive0')
>  self.assert_qmp(res, 'error/desc',
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH for-5.1] sd/milkymist-memcard: Fix format string

[Philippe Mathieu-Daudé]

On 7/22/20 10:40 PM, Stefan Weil wrote:

Fixes: b98e8d1230 ("sd/milkymist-memcard: Plug minor memory leak in
realize")

Reviewed-by: Philippe Mathieu-Daudé 

> Signed-off-by: Stefan Weil 
> ---
>  hw/sd/milkymist-memcard.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index afdb8aa0c0..11f61294fc 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -281,7 +281,7 @@ static void milkymist_memcard_realize(DeviceState *dev, 
> Error **errp)
>  carddev = qdev_new(TYPE_SD_CARD);
>  qdev_prop_set_drive(carddev, "drive", blk);
>  if (!qdev_realize_and_unref(carddev, BUS(>sdbus), )) {
> -error_propagate_prepend(errp, err, "failed to init SD card: %s");
> +error_propagate_prepend(errp, err, "failed to init SD card");

Oops... b98e8d1230 was to fix 3d0369ba49 ("hw/sd/milkymist-memcard:
expose a SDBus and connect the SDCard to it").

>  return;
>  }
>  s->enabled = blk && blk_is_inserted(blk);
> 

Re: [PATCH for-5.1] sd/milkymist-memcard: Fix format string

[Li Qiang]

Stefan Weil  于2020年7月23日周四 上午4:41写道：
>
> Signed-off-by: Stefan Weil 

Reviewed-by: Li Qiang 

> ---
>  hw/sd/milkymist-memcard.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index afdb8aa0c0..11f61294fc 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -281,7 +281,7 @@ static void milkymist_memcard_realize(DeviceState *dev, 
> Error **errp)
>  carddev = qdev_new(TYPE_SD_CARD);
>  qdev_prop_set_drive(carddev, "drive", blk);
>  if (!qdev_realize_and_unref(carddev, BUS(>sdbus), )) {
> -error_propagate_prepend(errp, err, "failed to init SD card: %s");
> +error_propagate_prepend(errp, err, "failed to init SD card");
>  return;
>  }
>  s->enabled = blk && blk_is_inserted(blk);
> --
> 2.27.0
>
>

Re: [PATCH v2] hw/misc/edu: support pci device state migration

[Peter Maydell]

On Thu, 23 Jul 2020 at 10:01, Zeng Guang  wrote:
>
> Currently edu device doesn't support live migration. Part of PCI
> configuration information would be lost after migration.
>
> PCI device state in source VM:
>  Bus  0, device   3, function 0:
>  Class 0255: PCI device 1234:11e8
>  PCI subsystem 1af4:1100
>  IRQ 11, pin A
>  BAR0: 32 bit memory at 0xfea0 [0xfeaf].
>  id ""
>
> PCI device state in destination VM:
>  Bus  0, device   3, function 0:
>  Class 0255: PCI device 1234:11e8
>  PCI subsystem 1af4:1100
>  IRQ 0, pin A
>  BAR0: 32 bit memory at 0x [0x000e].
>  id ""
>
> Add VMState for edu device to support migration.
>
> Signed-off-by: Gao Chao 
> Signed-off-by: Zeng Guang 
> Reviewed-by: Wei Wang 


Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [RFC v2 19/76] target/riscv: rvv-0.9: add narrower_nanbox_fpr helper

[Frank Chang]

On Thu, Jul 23, 2020 at 3:15 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> > From: Frank Chang 
> >
> > For floating-point operations, the scalar can be taken from a scalar
> > f register. If FLEN > SEW, the value in the f registers is checked for
> > a valid NaN-boxed value, in which case the least-significant SEW bits
> > of the f register are used, else the canonical NaN value is used.
> >
> > Add helper to generate the correspond NaN-boxed value or the SEW-bit
> > canonical NaN for floating-point operations.
> >
> > Signed-off-by: Frank Chang 
> > ---
> >  target/riscv/helper.h|  2 ++
> >  target/riscv/vector_helper.c | 32 
> >  2 files changed, 34 insertions(+)
>
> The helper can be done inline in two tcg ops.
>
> Though, really, we need to coordinate with Liu Zhiwei's other patch set
> that
> also deals with nan-boxing.
>
>
So, it's better to leverage the codes at:
https://patchew.org/QEMU/20200626205917.4545-1-zhiwei_...@c-sky.com/
but has to extend for the case of SEW=16?

Frank Chang


>
> r~
>

[PATCH] linux-user: Use getcwd syscall directly

[Andreas Schwab]

The glibc getcwd function returns different errors than the getcwd
syscall, which triggers an assertion failure in the glibc getcwd function
when running under the emulation.

Signed-off-by: Andreas Schwab 
---
 linux-user/syscall.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b9144b18fc..e4e46867e8 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -388,14 +388,7 @@ static bitmask_transtbl fcntl_flags_tbl[] = {
   { 0, 0, 0, 0 }
 };
 
-static int sys_getcwd1(char *buf, size_t size)
-{
-  if (getcwd(buf, size) == NULL) {
-  /* getcwd() sets errno */
-  return (-1);
-  }
-  return strlen(buf)+1;
-}
+_syscall2(int, sys_getcwd1, char *, buf, size_t, size)
 
 #ifdef TARGET_NR_utimensat
 #if defined(__NR_utimensat)
-- 
2.26.2


-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: please try to avoid sending pullreqs late on release-candidate day

[Philippe Mathieu-Daudé]

On 7/23/20 8:28 AM, Markus Armbruster wrote:
> Alex Bennée  writes:
> 
>> Kevin Wolf  writes:
>>
>>> Am 21.07.2020 um 17:56 hat Peter Maydell geschrieben:
 It is not helpful if everybody sends their pullrequests late
 on the Tuesday afternoon, as there just isn't enough time in the
 day to merge test and apply them all before I have to cut the tag.
 Please, if you can, try to send pullrequests earlier, eg Monday.
>>>
>> 
>>>
>>> So given that we _will_ have some late patches, what can we do to
>>> improve the situation?
>>>
>>> Maybe I could send the pull request before testing it to save some time.
>>> Your tests will take a while anyway, so if my own testing fails (e.g.
>>> for the parts of iotests that you don't test), I would still have time
>>> to NACK my own pull request. This wouldn't buy us more than an hour at
>>> most and could lead to wasted testing effort on your side (which is
>>> exactly the resource we want to save).
>>>
>>> Can you test multiple pull requests at once? The Tuesday ones tend to be
>>> small (between 1 and 3 patches was what I saw yesterday), so they should
>>> be much less likely to fail than large pull requests. If you test two
>>> pull requests together and it fails so you have to retest one of them in
>>> isolation, you still haven't really lost time compared to testing both
>>> individually. And if it succeeds, you cut the testing time in half.
>>
>> I've taken to just stacking up patches from my multiple trees to avoid
>> sending more than one PR a week. Of course sometimes the stack grows a
>> bit too tall and becomes unwieldy :-/
> 
> You're right, stacking unrelated smaller pull requests makes sense when
> pulling all the pull requests in flight races with a deadline.

I tend to disagree, since few patches from the "candidate fixes for
5.1-rc1" series are still being discussed, and we are past rc1. Half
of them could have been merged in for rc1.

Re: [PATCH-for-5.2] qapi/error: Make error_vprepend() static

[Li Qiang]

Philippe Mathieu-Daudé  于2020年7月23日周四 下午6:15写道：
>
> error_vprepend() is only used by util/error.c where it is
> defined. Make it static to reduce its scope.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Li Qiang 

> ---
>  include/qapi/error.h | 6 --
>  util/error.c | 6 +-
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 7932594dce..fa2308dedd 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -384,12 +384,6 @@ void error_propagate(Error **dst_errp, Error *local_err);
>  void error_propagate_prepend(Error **dst_errp, Error *local_err,
>   const char *fmt, ...);
>
> -/*
> - * Prepend some text to @errp's human-readable error message.
> - * The text is made by formatting @fmt, @ap like vprintf().
> - */
> -void error_vprepend(Error *const *errp, const char *fmt, va_list ap);
> -
>  /*
>   * Prepend some text to @errp's human-readable error message.
>   * The text is made by formatting @fmt, ... like printf().
> diff --git a/util/error.c b/util/error.c
> index b6c89d1412..3990b741ff 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -121,7 +121,11 @@ void error_setg_file_open_internal(Error **errp,
>"Could not open '%s'", filename);
>  }
>
> -void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
> +/*
> + * Prepend some text to @errp's human-readable error message.
> + * The text is made by formatting @fmt, @ap like vprintf().
> + */
> +static void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
>  {
>  GString *newmsg;
>
> --
> 2.21.3
>
>

[PATCH] hw/pci-host: save/restore pci host config register

[Wang King]

From: Hogan Wang 

The pci host config register is used to save PCI address for
read/write config data. If guest write a value to config register,
and then pause the vcpu to migrate, After the migration, the guest
continue to write pci config data, and the write data will be ignored
because of new qemu process lost the config register state.

Example:
1. guest booting in seabios.
2. guest enabled the SMM memory window in piix4_apmc_smm_setup, and
then try to close the SMM memory window.
3. pasued vcpu to finish migration.
4. guest close the SMM memory window fail becasue of config register
state lost.
5. guest continue to boot and crash in ipxe option ROM (SMM memory
window is enabled).

Due to the complex guest, the negative effect is unpredictable.
---
 hw/pci-host/i440fx.c   | 11 +++
 hw/pci-host/q35.c  | 11 +++
 hw/pci/pci_host.c  | 11 +++
 hw/pci/pcie_host.c | 11 +++
 include/hw/pci/pci_host.h  | 10 ++
 include/hw/pci/pcie_host.h | 10 ++
 6 files changed, 64 insertions(+)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 8ed2417f0c..17705bb025 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
 }
 };
 
+static const VMStateDescription vmstate_i440fx_pcihost = {
+.name = "I440FX_PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_HOST(parent_obj, I440FXState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
   const char *name, void *opaque,
   Error **errp)
@@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
void *data)
 hc->root_bus_path = i440fx_pcihost_root_bus_path;
 dc->realize = i440fx_pcihost_realize;
 dc->fw_name = "pci";
+dc->vmsd = _i440fx_pcihost;
 device_class_set_props(dc, i440fx_props);
 /* Reason: needs to be wired up by pc_init1 */
 dc->user_creatable = false;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b67cb9c29f..5e323be2e3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
Visitor *v,
 visit_type_uint64(v, name, , errp);
 }
 
+static const VMStateDescription vmstate_q35_pcihost = {
+.name = "Q35_PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
+VMSTATE_END_OF_LIST()
+}
+};
+
 /*
  * NOTE: setting defaults for the mch.* fields in this table
  * doesn't work, because mch is a separate QOM object that is
@@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, void 
*data)
 
 hc->root_bus_path = q35_host_root_bus_path;
 dc->realize = q35_host_realize;
+dc->vmsd = _q35_pcihost;
 device_class_set_props(dc, q35_host_props);
 /* Reason: needs to be wired up by pc_q35_init */
 dc->user_creatable = false;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index ce7bcdb1d5..7cdd5a3ea3 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pci_host.h"
 #include "qemu/module.h"
 #include "hw/pci/pci_bus.h"
+#include "migration/vmstate.h"
 #include "trace.h"
 
 /* debug PCI */
@@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
 .endianness = DEVICE_BIG_ENDIAN,
 };
 
+const VMStateDescription vmstate_pcihost = {
+.name = "PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(config_reg, PCIHostState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static const TypeInfo pci_host_type_info = {
 .name = TYPE_PCI_HOST_BRIDGE,
 .parent = TYPE_SYS_BUS_DEVICE,
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index 3534006f99..a653c39bb7 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pcie_host.h"
 #include "qemu/module.h"
 #include "exec/address-spaces.h"
+#include "migration/vmstate.h"
 
 /* a helper function to get a PCIDevice for a given mmconfig address */
 static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
@@ -121,6 +122,16 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
 memory_region_transaction_commit();
 }
 
+const VMStateDescription vmstate_pciehost = {
+.name = "PCIEHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_HOST(pci, PCIExpressHost),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static const TypeInfo pcie_host_type_info = {
 .name = TYPE_PCIE_HOST_BRIDGE,
 .parent = TYPE_PCI_HOST_BRIDGE,
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 9ce088bd13..fc88305e04 100644
--- a/include/hw/pci/pci_host.h

Re: [PATCH-for-5.1 v2 2/2] tpm: List the available TPM backends

[Philippe Mathieu-Daudé]

On 7/23/20 12:39 PM, Philippe Mathieu-Daudé wrote:
> When an incorrect backend is selected, tpm_display_backend_drivers()
> is supposed to list the available backends. However the error is
> directly propagated, and we never display the list. The user only
> gets "Parameter 'type' expects a TPM backend type" error.
> 
> Convert the fprintf(stderr,) calls to error hints propagated with
> the error.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Since v1:
> - Use g_assert_not_reached after processing 'help' in tpm_config_parse
> ---
>  tpm.c | 19 +--
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/tpm.c b/tpm.c
> index e36803a64d..f883340d1a 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -58,23 +58,21 @@ static int tpm_backend_drivers_count(void)
>  }
>  
>  /*
> - * Walk the list of available TPM backend drivers and display them on the
> - * screen.
> + * Walk the list of available TPM backend drivers and list them as Error 
> hint.
>   */
> -static void tpm_display_backend_drivers(void)
> +static void tpm_list_backend_drivers_hint(Error **errp)
>  {
>  int i;
>  
> -fprintf(stderr, "Supported TPM types (choose only one):\n");
> +error_append_hint(errp, "Supported TPM types (choose only one):\n");
>  
>  for (i = 0; i < TPM_TYPE__MAX; i++) {
>  const TPMBackendClass *bc = tpm_be_find_by_type(i);
>  if (!bc) {
>  continue;
>  }
> -fprintf(stderr, "%12s   %s\n", TpmType_str(i), bc->desc);
> +error_append_hint(errp, "%12s   %s\n", TpmType_str(i), bc->desc);
>  }
> -fprintf(stderr, "\n");
>  }
>  
>  /*
> @@ -97,6 +95,7 @@ TPMBackend *qemu_find_tpm_be(const char *id)
>  
>  static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>  {
> +ERRP_GUARD();
>  const char *value;
>  const char *id;
>  const TPMBackendClass *be;
> @@ -122,7 +121,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>  value = qemu_opt_get(opts, "type");
>  if (!value) {
>  error_setg(errp, QERR_MISSING_PARAMETER, "type");
> -tpm_display_backend_drivers();
> +tpm_list_backend_drivers_hint(errp);
>  return 1;
>  }
>  
> @@ -131,7 +130,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>  if (be == NULL) {
>  error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
> "a TPM backend type");
> -tpm_display_backend_drivers();
> +tpm_list_backend_drivers_hint(errp);
>  return 1;
>  }
>  
> @@ -184,8 +183,8 @@ int tpm_config_parse(QemuOptsList *opts_list, const char 
> *optarg)
>  QemuOpts *opts;
>  
>  if (!strcmp(optarg, "help")) {
> -tpm_display_backend_drivers();
> -return -1;
> +tpm_list_backend_drivers_hint(_fatal);
> +g_assert_not_reached(); /* Using _fatal triggers exit(1). */

Maybe tpm_config_parse() should take an Error** parameter instead?
And in vl.c:

-- >8 --
 #ifdef CONFIG_TPM
 case QEMU_OPTION_tpmdev:
-if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg)
< 0) {
-exit(1);
-}
+tpm_config_parse(qemu_find_opts("tpmdev"), optarg,
+ _fatal);
 break;
 #endif
---

>  }
>  opts = qemu_opts_parse_noisily(opts_list, optarg, true);
>  if (!opts) {
> 

[PATCH] qapi: enable use of g_autoptr with QAPI types

[Daniel P . Berrangé]

Currently QAPI generates a type and function for free'ing it:

  typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
  void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions *obj);

This is used in the traditional manner:

  QCryptoBlockCreateOptions *opts = NULL;

  opts = g_new0(QCryptoBlockCreateOptions, 1);

  do stuff with opts...

  qapi_free_QCryptoBlockCreateOptions(opts);

Since bumping the min glib to 2.48, QEMU has incrementally adopted the
use of g_auto/g_autoptr. This allows the compiler to run a function to
free a variable when it goes out of scope, the benefit being the
compiler can guarantee it is freed in all possible code ptahs.

This benefit is applicable to QAPI types too, and given the seriously
long method names for some qapi_free_() functions, is much less
typing. This change thus makes the code generator emit:

 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlockCreateOptions,
  qapi_free_QCryptoBlockCreateOptions)

The above code example now becomes

  g_autoptr(QCryptoBlockCreateOptions) opts = NULL;

  opts = g_new0(QCryptoBlockCreateOptions, 1);

  do stuff with opts...

Note, if the local pointer needs to live beyond the scope holding the
variable, then g_steal_pointer can be used. This is useful to return the
pointer to the caller in the success codepath, while letting it be freed
in all error codepaths.

  return g_steal_pointer();

Signed-off-by: Daniel P. Berrangé 
---
 include/crypto/block.h | 2 --
 scripts/qapi/types.py  | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/crypto/block.h b/include/crypto/block.h
index d274819791..7a65e8e402 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -311,7 +311,5 @@ uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block);
 void qcrypto_block_free(QCryptoBlock *block);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlock, qcrypto_block_free)
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlockCreateOptions,
-  qapi_free_QCryptoBlockCreateOptions)
 
 #endif /* QCRYPTO_BLOCK_H */
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 3ad33af4ee..3640f17cd6 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -213,6 +213,7 @@ def gen_type_cleanup_decl(name):
 ret = mcgen('''
 
 void qapi_free_%(c_name)s(%(c_name)s *obj);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(%(c_name)s, qapi_free_%(c_name)s)
 ''',
 c_name=c_name(name))
 return ret
-- 
2.26.2

Re: [virtio-comment] [RFC] ivshmem v2: Shared memory device specification

[Stefan Hajnoczi]

On Fri, Jul 17, 2020 at 06:15:58PM +0200, Jan Kiszka wrote:
> On 15.07.20 15:27, Stefan Hajnoczi wrote:
> > On Mon, May 25, 2020 at 09:58:28AM +0200, Jan Kiszka wrote:

Thanks for the responses. It would be great to update the spec with
these clarifications.

> > > If BAR 2 is not present, the shared memory region is not relocatable
> > > by the user. In that case, the hypervisor has to implement the Base
> > > Address register in the vendor-specific capability.
> > 
> > What does relocatable mean in this context?
> 
> That the guest can decide (via BAR) where the resource should show up in the
> physical guest address space. We do not want to support this in setups like
> for static partitioning hypervisors, and then we use that side-channel
> read-only configuration.

I see. I'm not sure what is vendor-specific about non-relocatable shared
memory. I guess it could be added to the spec too?

In any case, since "relocatable" hasn't been fully defined, I suggest
making the statement more general:

  If BAR 2 is not present the hypervisor has to implement the Base
  Address Register in the vendor-specific capability. This can be used
  for vendor-specific shared memory functionality.


signature.asc
Description: PGP signature

Re: [PATCH] virtio: Drop broken and superfluous object_property_set_link()

[Cornelia Huck]

On Tue, 21 Jul 2020 14:11:53 +0200
Markus Armbruster  wrote:

> virtio_crypto_pci_realize() and copies the value of vcrypto->vdev's
> property "cryptodev" to vcrypto's property:
> 
> object_property_set_link(OBJECT(vrng), "rng", OBJECT(vrng->vdev.conf.rng),
>  NULL);
> 
> Since it does so only after realize, this always fails, but the error
> is ignored.
> 
> It's actually superfluous: vcrypto's property is an alias of
> vcrypto->vdev's property, created by virtio_instance_init_common().
> 
> Drop the call.
> 
> Same for virtio_ccw_crypto_realize(), virtio_rng_pci_realize(),
> virtio_ccw_rng_realize().
> 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/s390x/virtio-ccw-crypto.c  | 3 ---
>  hw/s390x/virtio-ccw-rng.c | 3 ---
>  hw/virtio/virtio-crypto-pci.c | 2 --
>  hw/virtio/virtio-rng-pci.c| 3 ---
>  4 files changed, 11 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH v0 0/4] background snapshot

[Denis Plotnikov]

On 22.07.2020 19:30, Peter Xu wrote:

On Wed, Jul 22, 2020 at 06:47:44PM +0300, Denis Plotnikov wrote:


On 22.07.2020 18:42, Denis Plotnikov wrote:


On 22.07.2020 17:50, Peter Xu wrote:

Hi, Denis,

Hi, Peter

...

How to use:
1. enable background snapshot capability
     virsh qemu-monitor-command vm --hmp migrate_set_capability
background-snapshot on

2. stop the vm
     virsh qemu-monitor-command vm --hmp stop

3. Start the external migration to a file
     virsh qemu-monitor-command cent78-bs --hmp migrate exec:'cat

./vm_state'

4. Wait for the migration finish and check that the migration
has completed state.

Thanks for continued working on this project! I have two high level
questions
before dig into the patches.

Firstly, is step 2 required?  Can we use a single QMP command to
take snapshots
(which can still be a "migrate" command)?

With this series it is required, but steps 2 and 3 should be merged into
a single one.

I'm not sure whether you're talking about the disk snapshot operations, anyway
yeah it'll be definitely good if we merge them into one in the next version.


After thinking for a while, I remembered why I split these two steps.
The vm snapshot consists of two parts: disk(s) snapshot(s) and vmstate.
With migrate command we save the vmstate only. So, the steps to save
the whole vm snapshot is the following:

2. stop the vm
    virsh qemu-monitor-command vm --hmp stop

2.1. Make a disk snapshot, something like
virsh qemu-monitor-command vm --hmp snapshot_blkdev drive-scsi0-0-0-0 
./new_data
   
3. Start the external migration to a file

    virsh qemu-monitor-command vm --hmp migrate exec:'cat ./vm_state'

In this example, vm snapshot consists of two files: vm_state and the disk file. 
new_data will contain all new disk data written since [2.1.] executing.




Meanwhile, we might also want to check around the type of backend
RAM.  E.g.,
shmem and hugetlbfs are still not supported for uffd-wp (which I'm still
working on).  I didn't check explicitly whether we'll simply fail
the migration
for those cases since the uffd ioctls will fail for those kinds of
RAM.  It
would be okay if we handle all the ioctl failures gracefully,

The ioctl's result is processed but the patch has a flaw - it ignores
the result of ioctl check. Need to fix it.

It happens here:

+int ram_write_tracking_start(void)
+{
+if (page_fault_thread_start()) {
+return -1;
+}
+
+ram_block_list_create();
+ram_block_list_set_readonly(); << this returns -1 in case of failure but I 
just ignore it here
+
+return 0;
+}


or it would be
even better if we directly fail when we want to enable live snapshot
capability
for a guest who contains other types of ram besides private
anonymous memories.

I agree, but to know whether shmem or hugetlbfs are supported by the
current kernel we should
execute the ioctl for all memory regions on the capability enabling.

Yes, that seems to be a better solution, so we don't care about the type of ram
backend anymore but check directly with the uffd ioctls.  With these checks,
it'll be even fine to ignore the above retcode, or just assert, because we've
already checked that before that point.

Thanks,

Re: 5.1.0-rc1 regression: reset fails with kvm and -cpu host

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabk...@redhat.com) wrote:
> On Wed, Jul 22, 2020 at 04:47:32PM -0400, Eduardo Habkost wrote:
> > On Wed, Jul 22, 2020 at 08:05:01PM +0200, Jan Kiszka wrote:
> > > On 22.07.20 19:35, Eduardo Habkost wrote:
> > > > Hi Jan,
> > > > 
> > > > What was the last version where it worked for you?  Does using
> > > > "-cpu host,-vmx" help?
> > > 
> > > Yeah, -vmx does indeed help.
> > > 
> > > I didn't have the time to bisect yet. Just check my reflog, picked
> > > eb6490f544, and that works.
> > 
> > Thanks!
> > 
> > I could reproduce it locally[1], I will bisect it.
> > 
> > The good news is that "-cpu host,+vmx" still works, on commit
> > eb6490f544.
> > 
> > [1] Linux 5.6.19-300.fc32.x86_64, Intel Core i7-8665U CPU.
> 
> Bisected to:
> 
> commit b16c0e20c74218f2d69710cedad11da7dd4d2190
> Author: Paolo Bonzini 
> Date:   Wed May 20 10:49:22 2020 -0400
> 
> KVM: add support for AMD nested live migration
> 
> Support for nested guest live migration is part of Linux 5.8, add the
> corresponding code to QEMU.  The migration format consists of a few
> flags, is an opaque 4k blob.
> 
> The blob is in VMCB format (the control area represents the L1 VMCB
> control fields, the save area represents the pre-vmentry state; KVM does
> not use the host save area since the AMD manual allows that) but QEMU
> does not really care about that.  However, the flags need to be
> copied to hflags/hflags2 and back.
> 
> In addition, support for retrieving and setting the AMD nested 
> virtualization
> states allows the L1 guest to be reset while running a nested guest, but
> a small bug in CPU reset needs to be fixed for that to work.
> 
> Signed-off-by: Paolo Bonzini 

Guesswork led me to try reverting the chunk in kvm_put_nested_state;
without it the reset seems to work; I can't explain that code though.

Dave

> 
> -- 
> Eduardo
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[PATCH v2] hw/misc/edu: support pci device state migration

[Zeng Guang]

Currently edu device doesn't support live migration. Part of PCI
configuration information would be lost after migration.

PCI device state in source VM:
 Bus  0, device   3, function 0:
 Class 0255: PCI device 1234:11e8
 PCI subsystem 1af4:1100
 IRQ 11, pin A
 BAR0: 32 bit memory at 0xfea0 [0xfeaf].
 id ""

PCI device state in destination VM:
 Bus  0, device   3, function 0:
 Class 0255: PCI device 1234:11e8
 PCI subsystem 1af4:1100
 IRQ 0, pin A
 BAR0: 32 bit memory at 0x [0x000e].
 id ""

Add VMState for edu device to support migration.

Signed-off-by: Gao Chao 
Signed-off-by: Zeng Guang 
Reviewed-by: Wei Wang 
---
 hw/misc/edu.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index ec617e63f3..5f3fecac41 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -27,6 +27,7 @@
 #include "hw/pci/pci.h"
 #include "hw/hw.h"
 #include "hw/pci/msi.h"
+#include "migration/vmstate.h"
 #include "qemu/timer.h"
 #include "qemu/main-loop.h" /* iothread mutex */
 #include "qemu/module.h"
@@ -70,7 +71,7 @@ typedef struct {
 dma_addr_t cmd;
 } dma;
 QEMUTimer dma_timer;
-char dma_buf[DMA_SIZE];
+uint8_t dma_buf[DMA_SIZE];
 uint64_t dma_mask;
 } EduState;
 
@@ -405,6 +406,28 @@ static void edu_instance_init(Object *obj)
>dma_mask, OBJ_PROP_FLAG_READWRITE);
 }
 
+static const VMStateDescription vmstate_edu = {
+.name = "edu",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_DEVICE(pdev, EduState),
+VMSTATE_BOOL(stopping, EduState),
+VMSTATE_UINT32(addr4, EduState),
+VMSTATE_UINT32(fact, EduState),
+VMSTATE_UINT32(status, EduState),
+VMSTATE_UINT32(irq_status, EduState),
+VMSTATE_UINT64(dma.src, EduState),
+VMSTATE_UINT64(dma.dst, EduState),
+VMSTATE_UINT64(dma.cnt, EduState),
+VMSTATE_UINT64(dma.cmd, EduState),
+VMSTATE_TIMER(dma_timer, EduState),
+VMSTATE_BUFFER(dma_buf, EduState),
+VMSTATE_UINT64(dma_mask, EduState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void edu_class_init(ObjectClass *class, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(class);
@@ -415,6 +438,7 @@ static void edu_class_init(ObjectClass *class, void *data)
 k->vendor_id = PCI_VENDOR_ID_QEMU;
 k->device_id = 0x11e8;
 k->revision = 0x10;
+dc->vmsd = _edu;
 k->class_id = PCI_CLASS_OTHERS;
 set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
-- 
2.17.1

Re: [PATCH v2 06/12] accel/tcg: better handle memory constrained systems

[Daniel P . Berrangé]

On Wed, Jul 22, 2020 at 12:02:59PM -0700, Richard Henderson wrote:
> On 7/22/20 9:44 AM, Daniel P. Berrangé wrote:
> > OpenStack uses TCG in alot of their CI infrastructure for example
> > and runs multiple VMs. If there's 4 VMs, that's another 4 GB of
> > RAM usage just silently added on top of the explicit -m value.
> > 
> > I wouldn't be surprised if this pushes CI into OOM, even without
> > containers or cgroups being involved, as they have plenty of other
> > services consuming RAM in the CI VMs.
> 
> I would hope that CI would also supply a -tb_size to go along with that -m
> value.  Because we really can't guess on their behalf.

I've never even seen mention of -tb_size argument before myself, nor
seen anyone else using it and libvirt doesn't set it, so I think
this is not a valid assumption.


> > The commit 600e17b261555c56a048781b8dd5ba3985650013 talks about this
> > minimizing codegen cache flushes, but doesn't mention the real world
> > performance impact of eliminating those flushes ?
> 
> Somewhere on the mailing list was this info.  It was so dreadfully slow it was
> *really* noticable.  Timeouts everywhere.
> 
> > Presumably this makes the guest OS boot faster, but what's the before
> > and after time ?  And what's the time like for values in between the
> > original 32mb and the new 1 GB ?
> 
> But it wasn't "the original 32MB".
> It was the original "ram_size / 4", until that broke due to argument parsing
> ordering.

Hmm, 600e17b261555c56a048781b8dd5ba3985650013 says it was 32 MB as the
default in its commit message, which seems to match the code doing

 #define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (32 * MiB)


> I don't know what CI usually uses, but I usually use at least -m 4G, sometimes
> more.  What's the libvirt default?

There's no default memory size - its up to whomever/whatever creates the
VMs to choose how much RAM is given.

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 3/4] error: Remove NULL checks on error_propagate() calls (again)

[Markus Armbruster]

Eric Blake  writes:

> On 7/22/20 3:40 AM, Markus Armbruster wrote:
>> Patch created mechanically by rerunning:
>>
>>  $ spatch --sp-file scripts/coccinelle/error_propagate_null.cocci \
>>   --macro-file scripts/cocci-macro-file.h \
>>   --use-gitgrep .
>>
>> Cc: Jens Freimann 
>> Cc: Hailiang Zhang 
>> Cc: Juan Quintela 
>> Signed-off-by: Markus Armbruster 
>> ---
>
>> +++ b/migration/colo.c
>> @@ -798,9 +798,7 @@ static void 
>> colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>> colo_send_message(mis->to_src_file,
>> COLO_MESSAGE_VMSTATE_LOADED,
>>_err);
>> -if (local_err) {
>> -error_propagate(errp, local_err);
>> -}
>> +error_propagate(errp, local_err);
>>   }
>
> As this is mechanical, it is fine. But there is now a further cleanup
> possible of passing errp directly to colo_send_message, and possibly
> dropping local_err altogether.

True.

The patch is small and simple enough for squashing in further manual
cleanups.  I'd like to first check whether a followup patch created with
the machinery I used for eliminating error_propagate() comes out better.

> Reviewed-by: Eric Blake 

Thanks!

Re: [RFC PATCH-for-5.1 2/2] tpm: List the available TPM backends

[Philippe Mathieu-Daudé]

On 7/22/20 11:44 PM, Stefan Berger wrote:
> On 7/22/20 7:23 AM, Philippe Mathieu-Daudé wrote:
>> When an incorrect backend is selected, tpm_display_backend_drivers()
>> is supposed to list the available backends. However the error is
>> directly propagated, and we never display the list. The user only
>> gets "Parameter 'type' expects a TPM backend type" error.
>>
>> Convert the fprintf(stderr,) calls to error hints propagated with
>> the error.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> RFC because this is now odd in tpm_config_parse():
> 
> because it's not using the fprintf anymore ?
> 
> 

Because when using _fatal you don't return:

if (!strcmp(optarg, "help")) {
tpm_list_backend_drivers_hint(_fatal);
/* not reached */
return -1;
}

I should probably use that instead:

if (!strcmp(optarg, "help")) {
tpm_list_backend_drivers_hint(_fatal);
g_assert_not_reached();
}

Re: [PATCH 2/2] e1000e: make TX reentrant

[Peter Maydell]

On Wed, 22 Jul 2020 at 10:00, Jason Wang  wrote:
>
> In loopback mode, e1000e RX can DMA into TX doorbell which requires
> TX to be reentrant. This patch make e1000e's TX routine reentrant by
> introducing a per device boolean for recording whether or not a TX
> rountine is being called and return early.
>
> Signed-off-by: Jason Wang 
> ---

This feels like a sticking-plaster fix that's not really in the
right place... It stops us from calling back into
e1000e_start_xmit(), but it doesn't prevent a DMA request
from touching other device registers that update state in
the E100ECore struct that the transmit code is not expecting
to change.

thanks
-- PMM

Re: [PATCH 1/2] ppc: Rename current DAWR macros

[Cornelia Huck]

On Thu, 23 Jul 2020 16:12:19 +0530
Ravi Bangoria  wrote:

> Power10 is introducing second DAWR. Use real register names (with
> suffix 0) from ISA for current macros.
> 
> Signed-off-by: Ravi Bangoria 
> ---
>  include/hw/ppc/spapr.h  | 2 +-
>  linux-headers/asm-powerpc/kvm.h | 4 ++--
>  target/ppc/cpu.h| 4 ++--
>  target/ppc/translate_init.inc.c | 8 
>  4 files changed, 9 insertions(+), 9 deletions(-)
> 

(...)

> diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
> index 264e266a85..38d61b73f5 100644
> --- a/linux-headers/asm-powerpc/kvm.h
> +++ b/linux-headers/asm-powerpc/kvm.h
> @@ -608,8 +608,8 @@ struct kvm_ppc_cpu_char {
>  #define KVM_REG_PPC_BESCR(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa7)
>  #define KVM_REG_PPC_TAR  (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa8)
>  #define KVM_REG_PPC_DPDES(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa9)
> -#define KVM_REG_PPC_DAWR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xaa)
> -#define KVM_REG_PPC_DAWRX(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xab)
> +#define KVM_REG_PPC_DAWR0(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xaa)
> +#define KVM_REG_PPC_DAWRX0   (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xab)
>  #define KVM_REG_PPC_CIABR(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xac)
>  #define KVM_REG_PPC_IC   (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xad)
>  #define KVM_REG_PPC_VTB  (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xae)

Changes here need to come in via a proper headers sync, so this needs
to be split out into a separate patch (either one doing a headers sync,
or a placeholder if the Linux changes are not upstream yet.)

Re: please try to avoid sending pullreqs late on release-candidate day

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 7/23/20 8:28 AM, Markus Armbruster wrote:
>> Alex Bennée  writes:
>> 
>>> Kevin Wolf  writes:
>>>
 Am 21.07.2020 um 17:56 hat Peter Maydell geschrieben:
> It is not helpful if everybody sends their pullrequests late
> on the Tuesday afternoon, as there just isn't enough time in the
> day to merge test and apply them all before I have to cut the tag.
> Please, if you can, try to send pullrequests earlier, eg Monday.

>>> 

 So given that we _will_ have some late patches, what can we do to
 improve the situation?

 Maybe I could send the pull request before testing it to save some time.
 Your tests will take a while anyway, so if my own testing fails (e.g.
 for the parts of iotests that you don't test), I would still have time
 to NACK my own pull request. This wouldn't buy us more than an hour at
 most and could lead to wasted testing effort on your side (which is
 exactly the resource we want to save).

 Can you test multiple pull requests at once? The Tuesday ones tend to be
 small (between 1 and 3 patches was what I saw yesterday), so they should
 be much less likely to fail than large pull requests. If you test two
 pull requests together and it fails so you have to retest one of them in
 isolation, you still haven't really lost time compared to testing both
 individually. And if it succeeds, you cut the testing time in half.
>>>
>>> I've taken to just stacking up patches from my multiple trees to avoid
>>> sending more than one PR a week. Of course sometimes the stack grows a
>>> bit too tall and becomes unwieldy :-/
>> 
>> You're right, stacking unrelated smaller pull requests makes sense when
>> pulling all the pull requests in flight races with a deadline.
>
> I tend to disagree, since few patches from the "candidate fixes for
> 5.1-rc1" series are still being discussed, and we are past rc1. Half
> of them could have been merged in for rc1.

That's a different issue, I think.

Picking patches that are ready and independent when the complete series
isn't often makes sense.  More so when a deadline is involved.

Re: [PATCH v2 13/20] iotests: 129: prepare for backup over block-copy

[Max Reitz]

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> After introducing parallel async copy requests instead of plain
> cluster-by-cluster copying loop, backup job may finish earlier than
> final assertion in do_test_stop. Let's require slow backup explicitly
> by specifying speed parameter.

Isn’t the problem really that block_set_io_throttle does absolutely
nothing?  (Which is a long-standing problem with 129.  I personally just
never run it, honestly.)

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/129 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 4db5eca441..bca56b589d 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -76,7 +76,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>  def test_drive_backup(self):
>  self.do_test_stop("drive-backup", device="drive0",
>target=self.target_img,
> -  sync="full")
> +  sync="full", speed=1024)
>  
>  def test_block_commit(self):
>  self.do_test_stop("block-commit", device="drive0")
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 14/20] iotests: 185: prepare for backup over block-copy

[Max Reitz]

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> The further change of moving backup to be a on block-copy call will

-on?

> make copying chunk-size and cluster-size a separate things. So, even

s/a/two/

> with 64k cluster sized qcow2 image, default chunk would be 1M.
> 185 test however assumes, that with speed limited to 64K, one iteration
> would result in offset=64K. It will change, as first iteration would
> result in offset=1M independently of speed.
> 
> So, let's explicitly specify, what test wants: set max-chunk to 64K, so
> that one iteration is 64K. Note, that we don't need to limit
> max-workers, as block-copy rate limitator will handle the situation and

*limitator

> wouldn't start new workers when speed limit is obviously reached.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/185 | 3 ++-
>  tests/qemu-iotests/185.out | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
> index fd5e6ebe11..6afb3fc82f 100755
> --- a/tests/qemu-iotests/185
> +++ b/tests/qemu-iotests/185
> @@ -182,7 +182,8 @@ _send_qemu_cmd $h \
>'target': '$TEST_IMG.copy',
>'format': '$IMGFMT',
>'sync': 'full',
> -  'speed': 65536 } }" \
> +  'speed': 65536,
> +  'x-max-chunk': 65536 } }" \

Out of curiosity, would it also suffice to disable copy offloading?

But anyway:

Reviewed-by: Max Reitz 

>  "return"
>  
>  # If we don't sleep here 'quit' command races with disk I/O
> diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
> index ac5ab16bc8..5232647972 100644
> --- a/tests/qemu-iotests/185.out
> +++ b/tests/qemu-iotests/185.out
> @@ -61,7 +61,7 @@ Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 
> cluster_size=65536 l
>  
>  { 'execute': 'qmp_capabilities' }
>  {"return": {}}
> -{ 'execute': 'drive-backup', 'arguments': { 'device': 'disk', 'target': 
> 'TEST_DIR/t.IMGFMT.copy', 'format': 'IMGFMT', 'sync': 'full', 'speed': 65536 
> } }
> +{ 'execute': 'drive-backup', 'arguments': { 'device': 'disk', 'target': 
> 'TEST_DIR/t.IMGFMT.copy', 'format': 'IMGFMT', 'sync': 'full', 'speed': 65536, 
> 'x-max-chunk': 65536 } }
>  Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 
> cluster_size=65536 lazy_refcounts=off refcount_bits=16 compression_type=zlib
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "disk"}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "disk"}}
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] spice: simplify chardev setup

[Christophe de Dinechin]

On 2020-07-22 at 13:18 CEST, Gerd Hoffmann wrote...
> On Wed, Jul 22, 2020 at 12:19:43PM +0200, Christophe de Dinechin wrote:
>>
>> On 2020-07-22 at 11:20 CEST, Christophe de Dinechin wrote...
>> > On 2020-07-22 at 10:49 CEST, Gerd Hoffmann wrote...
>> >> Initialize spice before chardevs.  That allows to register the spice
>> >> chardevs directly in the init function and removes the need to maintain
>> >> a linked list of chardevs just for registration.
>> >>
>> >> Signed-off-by: Gerd Hoffmann 
>> >
>> > Looks good to me, but I still need to test how this integrates with my work
>> > on putting SPICE in a module.
>>
>> That part does not seem to work so well. It looks like with this move, the
>> init happens before the module is loaded:
>>
>>qemu-system-x86_64 -display spice-app
>>Unexpected error in qemu_chr_open_spice_port() at 
>> ui/../chardev/spice.c:307:
>>qemu-system-x86_64: spice not enabled
>>
>> I'll try to find the correct fix. Any idea how to address this?
>
> move chardev init from early to normal:

Works for me.

Reviewed-by: Christophe de Dinechin 
Tested-by: Christophe de Dinechin 

>
> commit 77297a71e6be60997caf2c55c09ce01a8c492bc2
> Author: Gerd Hoffmann 
> Date:   Wed Jul 22 13:17:28 2020 +0200
>
> [fixup] spice app
>
> diff --git a/ui/spice-app.c b/ui/spice-app.c
> index 40fb2ef57399..03d971b15f0c 100644
> --- a/ui/spice-app.c
> +++ b/ui/spice-app.c
> @@ -117,7 +117,6 @@ static void spice_app_atexit(void)
>  static void spice_app_display_early_init(DisplayOptions *opts)
>  {
>  QemuOpts *qopts;
> -ChardevBackend *be = chr_spice_backend_new();
>  GError *err = NULL;
>
>  if (opts->has_full_screen) {
> @@ -162,6 +161,15 @@ static void spice_app_display_early_init(DisplayOptions 
> *opts)
>  qemu_opt_set(qopts, "gl", opts->has_gl ? "on" : "off", _abort);
>  display_opengl = opts->has_gl;
>  #endif
> +}
> +
> +static void spice_app_display_init(DisplayState *ds, DisplayOptions *opts)
> +{
> +ChardevBackend *be = chr_spice_backend_new();
> +QemuOpts *qopts;
> +GError *err = NULL;
> +gchar *uri;
> +
>  be->u.spiceport.data->fqdn = g_strdup("org.qemu.monitor.qmp.0");
>  qemu_chardev_new("org.qemu.monitor.qmp", TYPE_CHARDEV_SPICEPORT,
>   be, NULL, _abort);
> @@ -171,13 +179,6 @@ static void spice_app_display_early_init(DisplayOptions 
> *opts)
>  qemu_opt_set(qopts, "mode", "control", _abort);
>
>  qapi_free_ChardevBackend(be);
> -}
> -
> -static void spice_app_display_init(DisplayState *ds, DisplayOptions *opts)
> -{
> -GError *err = NULL;
> -gchar *uri;
> -
>  uri = g_strjoin("", "spice+unix://", app_dir, "/", "spice.sock", NULL);
>  info_report("Launching display with URI: %s", uri);
>  g_app_info_launch_default_for_uri(uri, NULL, );


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 06/12] accel/tcg: better handle memory constrained systems

[Alex Bennée]

Daniel P. Berrangé  writes:

> On Wed, Jul 22, 2020 at 12:02:59PM -0700, Richard Henderson wrote:
>> On 7/22/20 9:44 AM, Daniel P. Berrangé wrote:
>> > OpenStack uses TCG in alot of their CI infrastructure for example
>> > and runs multiple VMs. If there's 4 VMs, that's another 4 GB of
>> > RAM usage just silently added on top of the explicit -m value.
>> > 
>> > I wouldn't be surprised if this pushes CI into OOM, even without
>> > containers or cgroups being involved, as they have plenty of other
>> > services consuming RAM in the CI VMs.
>> 
>> I would hope that CI would also supply a -tb_size to go along with that -m
>> value.  Because we really can't guess on their behalf.
>
> I've never even seen mention of -tb_size argument before myself, nor
> seen anyone else using it and libvirt doesn't set it, so I think
> this is not a valid assumption.
>
>
>> > The commit 600e17b261555c56a048781b8dd5ba3985650013 talks about this
>> > minimizing codegen cache flushes, but doesn't mention the real world
>> > performance impact of eliminating those flushes ?
>> 
>> Somewhere on the mailing list was this info.  It was so dreadfully slow it 
>> was
>> *really* noticable.  Timeouts everywhere.
>> 
>> > Presumably this makes the guest OS boot faster, but what's the before
>> > and after time ?  And what's the time like for values in between the
>> > original 32mb and the new 1 GB ?
>> 
>> But it wasn't "the original 32MB".
>> It was the original "ram_size / 4", until that broke due to argument parsing
>> ordering.
>
> Hmm, 600e17b261555c56a048781b8dd5ba3985650013 says it was 32 MB as the
> default in its commit message, which seems to match the code doing
>
>  #define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (32 * MiB)

You need to look earlier in the sequence (see the tag pull-tcg-20200228):

  47a2def4533a2807e48954abd50b32ecb1aaf29a

so when the argument ordering broke the guest ram_size heuristic we
started getting reports of performance regressions because we fell back
to that size. Before then it was always based on guest ram size within
the min/max bounds set by those defines.

>> I don't know what CI usually uses, but I usually use at least -m 4G, 
>> sometimes
>> more.  What's the libvirt default?
>
> There's no default memory size - its up to whomever/whatever creates the
> VMs to choose how much RAM is given.
>
> Regards,
> Daniel


-- 
Alex Bennée

[PATCH-for-5.2] qapi/error: Make error_vprepend() static

[Philippe Mathieu-Daudé]

error_vprepend() is only used by util/error.c where it is
defined. Make it static to reduce its scope.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/qapi/error.h | 6 --
 util/error.c | 6 +-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7932594dce..fa2308dedd 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -384,12 +384,6 @@ void error_propagate(Error **dst_errp, Error *local_err);
 void error_propagate_prepend(Error **dst_errp, Error *local_err,
  const char *fmt, ...);
 
-/*
- * Prepend some text to @errp's human-readable error message.
- * The text is made by formatting @fmt, @ap like vprintf().
- */
-void error_vprepend(Error *const *errp, const char *fmt, va_list ap);
-
 /*
  * Prepend some text to @errp's human-readable error message.
  * The text is made by formatting @fmt, ... like printf().
diff --git a/util/error.c b/util/error.c
index b6c89d1412..3990b741ff 100644
--- a/util/error.c
+++ b/util/error.c
@@ -121,7 +121,11 @@ void error_setg_file_open_internal(Error **errp,
   "Could not open '%s'", filename);
 }
 
-void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
+/*
+ * Prepend some text to @errp's human-readable error message.
+ * The text is made by formatting @fmt, @ap like vprintf().
+ */
+static void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
 {
 GString *newmsg;
 
-- 
2.21.3

Re: [PATCH for-5.1] Fix grammar in documentation

[Peter Maydell]

On Wed, 22 Jul 2020 at 22:06, Stefan Weil  wrote:
>
> Signed-off-by: Stefan Weil 
> ---
>  docs/system/build-platforms.rst | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/docs/system/build-platforms.rst b/docs/system/build-platforms.rst
> index c2b92a9698..9734eba2f1 100644
> --- a/docs/system/build-platforms.rst
> +++ b/docs/system/build-platforms.rst
> @@ -57,12 +57,12 @@ macOS

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

On Mon, 20 Jul 2020 11:54:06 +0200
Halil Pasic  wrote:

> On Tue,  7 Jul 2020 12:54:44 +0200
> Cornelia Huck  wrote:
> 
> > As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> > a good idea to make sure that any new virtio device (which does not
> > support legacy virtio) is indeed a non-transitional device, just to
> > catch accidental misconfigurations. We can easily compile a list
> > of virtio devices with legacy support and have transports verify
> > in their plugged callbacks that legacy support is off for any device
> > not in that list.
> > 
> > Most new virtio devices force non-transitional already, so nothing
> > changes for them. vhost-user-fs-pci even does not allow to configure
> > a non-transitional device, so it is fine as well.
> > 
> > One problematic device, however, is virtio-iommu-pci. It currently
> > offers both the transitional and the non-transitional variety of the
> > device, and does not force anything. I'm unsure whether we should
> > consider transitional virtio-iommu unsupported, or if we should add
> > some compat handling. (The support for legacy or not generally may
> > change based upon the bus, IIUC, so I'm unsure how to come up with
> > something generic.)
> >   
> 
> Both patches look good to me (Acked-by: Halil Pasic
> ). I tend to agree with Davids comment on how
> this information is coded: the more object oriented way would have
> been to store this at the something like VirtioDeviceClass, but
> Michael's argument stands.
> 
> Another OO option would be to expose this as a virtio property. Would
> enable introspection, and would also give the host admin means to
> force non-legacy for transitional devices. But the juice is probably not
> worth the squeeze.

I agree, that would be a lot of hassle for exposing what is basically
static information.

Re: [virtio-comment] [RFC] ivshmem v2: Shared memory device specification

[Jan Kiszka]

On 23.07.20 08:54, Stefan Hajnoczi wrote:

On Fri, Jul 17, 2020 at 06:15:58PM +0200, Jan Kiszka wrote:

On 15.07.20 15:27, Stefan Hajnoczi wrote:

On Mon, May 25, 2020 at 09:58:28AM +0200, Jan Kiszka wrote:


Thanks for the responses. It would be great to update the spec with
these clarifications.


If BAR 2 is not present, the shared memory region is not relocatable
by the user. In that case, the hypervisor has to implement the Base
Address register in the vendor-specific capability.


What does relocatable mean in this context?


That the guest can decide (via BAR) where the resource should show up in the
physical guest address space. We do not want to support this in setups like
for static partitioning hypervisors, and then we use that side-channel
read-only configuration.


I see. I'm not sure what is vendor-specific about non-relocatable shared
memory. I guess it could be added to the spec too?


That "vendor-specific" comes from the PCI spec which - to my 
understanding - provides us no other means to introduce registers to the 
config space that are outside of the PCI spec. I could introduce a name 
for the ivshmem vendor cap and use that name here - would that be better?




In any case, since "relocatable" hasn't been fully defined, I suggest
making the statement more general:

   If BAR 2 is not present the hypervisor has to implement the Base
   Address Register in the vendor-specific capability. This can be used
   for vendor-specific shared memory functionality.



Will integrate this.

Thanks,
Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

roms/seabios-hppa can't be built with gcc-10: -fno-ipa-sra

[Michael Tokarev]

Switching to gcc-10 makes seabios-hppa unbuildable.
It fails at the final linking step with a lot of
missing references to memcpy & memcmp all over the
places.

The notable difference between gcc-10 and previous
gcc is that ccode32flat.o does _not_ have the text
for these two functions but have two .isra.0:

$ hppa-linux-gnu-nm ccode32flat.o | grep mem[sc]
03e0 t memcmp
 U memcpy
2f38 t memcpy.isra.0
 U memset
3a84 t memset.isra.0


while previous version of the compiler did have them:

$ hppa-linux-gnu-nm ccode32flat.o | grep mem[sc]
02fc t memcmp
370c t memcpy
036c t memset

After adding -fno-ipa-sra to the gcc flags, the firmware
is built successfully.

I don't know what to make out of this. Previous versions
of gcc apparently accepts -fno-ipa-sra too, for quite some
time.  So maybe add this to the flags unconditionally?

Thanks,

/mjt

Re: [PATCH for-5.1] nbd: Fix large trim/zero requests

[Vladimir Sementsov-Ogievskiy]

23.07.2020 00:22, Eric Blake wrote:

Although qemu as NBD client limits requests to <2G, the NBD protocol
allows clients to send requests almost all the way up to 4G.  But
because our block layer is not yet 64-bit clean, we accidentally wrap
such requests into a negative size, and fail with EIO instead of
performing the intended operation.

The bug is visible in modern systems with something as simple as:

$ qemu-img create -f qcow2 /tmp/image.img 5G
$ sudo qemu-nbd --connect=/dev/nbd0 /tmp/image.img
$ sudo blkdiscard /dev/nbd0

or with user-space only:

$ truncate --size=3G file
$ qemu-nbd -f raw file
$ nbdsh -u nbd://localhost:10809 -c 'h.trim(3*1024*1024*1024,0)'

Alas, our iotests do not currently make it easy to add external
dependencies on blkdiscard or nbdsh, so we have to rely on manual
testing for now.

This patch can be reverted when we later improve the overall block
layer to be 64-bit clean, but for now, a minimal fix was deemed less
risky prior to release.

CC: qemu-sta...@nongnu.org
Fixes: 1f4d6d18ed
Fixes: 1c6c4bb7f0
Fixes: https://github.com/systemd/systemd/issues/16242
Signed-off-by: Eric Blake 
---
  nbd/server.c | 26 ++
  1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4752a6c8bc07..029618017c90 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2378,8 +2378,17 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
  if (request->flags & NBD_CMD_FLAG_FAST_ZERO) {
  flags |= BDRV_REQ_NO_FALLBACK;
  }
-ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
-request->len, flags);
+ret = 0;
+/* FIXME simplify this when blk_pwrite_zeroes switches to 64-bit */
+while (ret == 0 && request->len) {
+int align = client->check_align ?: 1;
+int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
+align));
+ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
+len, flags);
+request->len -= len;
+request->from += len;
+}
  return nbd_send_generic_reply(client, request->handle, ret,
"writing to file failed", errp);

@@ -2393,8 +2402,17 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
"flush failed", errp);

  case NBD_CMD_TRIM:
-ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
-  request->len);
+ret = 0;
+/* FIXME simplify this when blk_co_pdiscard switches to 64-bit */
+while (ret == 0 && request->len) {


Did you check all the paths not to return positive ret on success? I'd prefer to 
compare ret >= 0 for this temporary code to not care of it


+int align = client->check_align ?: 1;
+int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
+align));
+ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
+  len);
+request->len -= len;
+request->from += len;


Hmm.. Modifying the function parameter. Safe now, as nbd_handle_request() call 
is the last usage of request in nbd_trip().


+}
  if (ret == 0 && request->flags & NBD_CMD_FLAG_FUA) {
  ret = blk_co_flush(exp->blk);
  }




--
Best regards,
Vladimir

Re: [PATCH v2 06/12] accel/tcg: better handle memory constrained systems

[Daniel P . Berrangé]

On Thu, Jul 23, 2020 at 10:22:25AM +0100, Alex Bennée wrote:
> 
> Daniel P. Berrangé  writes:
> 
> > On Wed, Jul 22, 2020 at 12:02:59PM -0700, Richard Henderson wrote:
> >> On 7/22/20 9:44 AM, Daniel P. Berrangé wrote:
> >> > OpenStack uses TCG in alot of their CI infrastructure for example
> >> > and runs multiple VMs. If there's 4 VMs, that's another 4 GB of
> >> > RAM usage just silently added on top of the explicit -m value.
> >> > 
> >> > I wouldn't be surprised if this pushes CI into OOM, even without
> >> > containers or cgroups being involved, as they have plenty of other
> >> > services consuming RAM in the CI VMs.
> >> 
> >> I would hope that CI would also supply a -tb_size to go along with that -m
> >> value.  Because we really can't guess on their behalf.
> >
> > I've never even seen mention of -tb_size argument before myself, nor
> > seen anyone else using it and libvirt doesn't set it, so I think
> > this is not a valid assumption.
> >
> >
> >> > The commit 600e17b261555c56a048781b8dd5ba3985650013 talks about this
> >> > minimizing codegen cache flushes, but doesn't mention the real world
> >> > performance impact of eliminating those flushes ?
> >> 
> >> Somewhere on the mailing list was this info.  It was so dreadfully slow it 
> >> was
> >> *really* noticable.  Timeouts everywhere.
> >> 
> >> > Presumably this makes the guest OS boot faster, but what's the before
> >> > and after time ?  And what's the time like for values in between the
> >> > original 32mb and the new 1 GB ?
> >> 
> >> But it wasn't "the original 32MB".
> >> It was the original "ram_size / 4", until that broke due to argument 
> >> parsing
> >> ordering.
> >
> > Hmm, 600e17b261555c56a048781b8dd5ba3985650013 says it was 32 MB as the
> > default in its commit message, which seems to match the code doing
> >
> >  #define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (32 * MiB)
> 
> You need to look earlier in the sequence (see the tag pull-tcg-20200228):
> 
>   47a2def4533a2807e48954abd50b32ecb1aaf29a
> 
> so when the argument ordering broke the guest ram_size heuristic we
> started getting reports of performance regressions because we fell back
> to that size. Before then it was always based on guest ram size within
> the min/max bounds set by those defines.

Ah I see. That's a shame, as something based on guest RAM size feels like
a much safer bet for a default heuristic than basing it on host RAM size.

I'd probably say that the original commit which changed the argument
processing is flawed, and could/should be fixed.

The problem that commit was trying to solve was to do validation of the
value passed to -m. In fixing that it also moving the parsing. The key
problem here is that we need to do parsing and validating at different
points in the startup procedure.  IOW, we need to split the logic, not
simply moving the CLI parsing to the place that makes validation work.

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-for-5.1?] qapi/error: Check format string argument in error_propagate_prepend()

[Stefan Weil]

Am 23.07.20 um 11:13 schrieb Philippe Mathieu-Daudé:

> error_propagate_prepend() "behaves like error_prepend()", and
> error_prepend() uses "formatting @fmt, ... like printf()".
> error_prepend() checks its format string argument, but
> error_propagate_prepend() does not. Fix that.
>
> This would have catched the invalid format introduced in commit
> b98e8d1230f:
>
> CC  hw/sd/milkymist-memcard.o
>   hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’:
>   hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching 
> ‘char *’ argument [-Werror=format=]
> 284 | error_propagate_prepend(errp, err, "failed to init SD card: 
> %s");
> | 
> ~^
> | 
>  |
> | 
>  char *
>
> Fixes: 4b5766488f ("Fix use of error_prepend() with _fatal, 
> _abort")
> Inspired-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/qapi/error.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 7932594dce..f1a34d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err);
>   * error_propagate(dst_errp, local_err);
>   * Please use ERRP_GUARD() and error_prepend() instead when possible.
>   */
> +GCC_FMT_ATTR(3, 4)
>  void error_propagate_prepend(Error **dst_errp, Error *local_err,
>   const char *fmt, ...);
>  


Reviewed-by: Stefan Weil 

error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add
that, too.

Regards,

Stefan

Re: [PATCH-for-5.1 1/2] tpm: Display when no backend is available

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> Display "No TPM backend available in this binary." error when
> no backend is available.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tpm.c | 21 +
>  1 file changed, 21 insertions(+)
>
> diff --git a/tpm.c b/tpm.c
> index fe03b24858..e36803a64d 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -41,6 +41,22 @@ tpm_be_find_by_type(enum TpmType type)
>  return TPM_BACKEND_CLASS(oc);
>  }
>  
> +/*
> + * Walk the list of available TPM backend drivers and count them.
> + */
> +static int tpm_backend_drivers_count(void)
> +{
> +int count = 0, i;
> +
> +for (i = 0; i < TPM_TYPE__MAX; i++) {
> +const TPMBackendClass *bc = tpm_be_find_by_type(i);
> +if (bc) {
> +count++;
> +}
> +}
> +return count;
> +}
> +
>  /*
>   * Walk the list of available TPM backend drivers and display them on the
>   * screen.
> @@ -87,6 +103,11 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>  TPMBackend *drv;
>  int i;
>  
> +if (!tpm_backend_drivers_count()) {
> +error_setg(errp, "No TPM backend available in this binary.");

Scratch the '.', please.  From error_setg()'s contract:

 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.

> +return 1;
> +}
> +
>  if (!QLIST_EMPTY(_backends)) {
>  error_setg(errp, "Only one TPM is allowed.");
>  return 1;

This works, but it's more code than I'd like to see for the purpose.

Moreover, it's tied to the error handling issue discussed in

Subject: Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device)
Message-ID: <87tuxyoauy@dusky.pond.sub.org>

If we revert flawed commit d10e05f15d5, then your patch needs a v2.
Your PATCH 2 might become unnecessary.  I'll give it a quick try to see
how it comes out.

Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters

[Max Reitz]

On 22.07.20 14:22, Max Reitz wrote:
> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
>> Add new parameters to configure future backup features. The patch
>> doesn't introduce aio backup requests (so we actually have only one
>> worker) neither requests larger than one cluster. Still, formally we
>> satisfy these maximums anyway, so add the parameters now, to facilitate
>> further patch which will really change backup job behavior.
>>
>> Options are added with x- prefixes, as the only use for them are some
>> very conservative iotests which will be updated soon.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>  qapi/block-core.json  |  9 -
>>  include/block/block_int.h |  7 +++
>>  block/backup.c| 21 +
>>  block/replication.c   |  2 +-
>>  blockdev.c|  5 +
>>  5 files changed, 42 insertions(+), 2 deletions(-)

[...]

>> diff --git a/block/replication.c b/block/replication.c
>> index 25987eab0f..a9ee82db80 100644
>> --- a/block/replication.c
>> +++ b/block/replication.c
>> @@ -563,7 +563,7 @@ static void replication_start(ReplicationState *rs, 
>> ReplicationMode mode,
>>  s->backup_job = backup_job_create(
>>  NULL, s->secondary_disk->bs, 
>> s->hidden_disk->bs,
>>  0, MIRROR_SYNC_MODE_NONE, NULL, 0, false, 
>> NULL,
>> -true,
>> +true, 0, 0,
> 
> Passing 0 for max_workers will error out immediately, won’t it?

Ah, oops.  Saw your own reply only now.  Yep, 1 worker would be nice. :)



signature.asc
Description: OpenPGP digital signature

[PATCH-for-5.1?] qapi/error: Check format string argument in error_propagate_prepend()

[Philippe Mathieu-Daudé]

error_propagate_prepend() "behaves like error_prepend()", and
error_prepend() uses "formatting @fmt, ... like printf()".
error_prepend() checks its format string argument, but
error_propagate_prepend() does not. Fix that.

This would have catched the invalid format introduced in commit
b98e8d1230f:

CC  hw/sd/milkymist-memcard.o
  hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’:
  hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching ‘char 
*’ argument [-Werror=format=]
284 | error_propagate_prepend(errp, err, "failed to init SD card: 
%s");
| ~^
|  |
|  
char *

Fixes: 4b5766488f ("Fix use of error_prepend() with _fatal, _abort")
Inspired-by: Stefan Weil 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/qapi/error.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7932594dce..f1a34d 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err);
  * error_propagate(dst_errp, local_err);
  * Please use ERRP_GUARD() and error_prepend() instead when possible.
  */
+GCC_FMT_ATTR(3, 4)
 void error_propagate_prepend(Error **dst_errp, Error *local_err,
  const char *fmt, ...);
 
-- 
2.21.3

Re: [PATCH v2 06/12] accel/tcg: better handle memory constrained systems

[Alex Bennée]

Daniel P. Berrangé  writes:

> On Thu, Jul 23, 2020 at 10:22:25AM +0100, Alex Bennée wrote:
>> 
>> Daniel P. Berrangé  writes:
>> 
>> > On Wed, Jul 22, 2020 at 12:02:59PM -0700, Richard Henderson wrote:
>> >> On 7/22/20 9:44 AM, Daniel P. Berrangé wrote:
>> >> > OpenStack uses TCG in alot of their CI infrastructure for example
>> >> > and runs multiple VMs. If there's 4 VMs, that's another 4 GB of
>> >> > RAM usage just silently added on top of the explicit -m value.
>> >> > 
>> >> > I wouldn't be surprised if this pushes CI into OOM, even without
>> >> > containers or cgroups being involved, as they have plenty of other
>> >> > services consuming RAM in the CI VMs.
>> >> 
>> >> I would hope that CI would also supply a -tb_size to go along with that -m
>> >> value.  Because we really can't guess on their behalf.
>> >
>> > I've never even seen mention of -tb_size argument before myself, nor
>> > seen anyone else using it and libvirt doesn't set it, so I think
>> > this is not a valid assumption.
>> >
>> >
>> >> > The commit 600e17b261555c56a048781b8dd5ba3985650013 talks about this
>> >> > minimizing codegen cache flushes, but doesn't mention the real world
>> >> > performance impact of eliminating those flushes ?
>> >> 
>> >> Somewhere on the mailing list was this info.  It was so dreadfully slow 
>> >> it was
>> >> *really* noticable.  Timeouts everywhere.
>> >> 
>> >> > Presumably this makes the guest OS boot faster, but what's the before
>> >> > and after time ?  And what's the time like for values in between the
>> >> > original 32mb and the new 1 GB ?
>> >> 
>> >> But it wasn't "the original 32MB".
>> >> It was the original "ram_size / 4", until that broke due to argument 
>> >> parsing
>> >> ordering.
>> >
>> > Hmm, 600e17b261555c56a048781b8dd5ba3985650013 says it was 32 MB as the
>> > default in its commit message, which seems to match the code doing
>> >
>> >  #define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (32 * MiB)
>> 
>> You need to look earlier in the sequence (see the tag pull-tcg-20200228):
>> 
>>   47a2def4533a2807e48954abd50b32ecb1aaf29a
>> 
>> so when the argument ordering broke the guest ram_size heuristic we
>> started getting reports of performance regressions because we fell back
>> to that size. Before then it was always based on guest ram size within
>> the min/max bounds set by those defines.
>
> Ah I see. That's a shame, as something based on guest RAM size feels like
> a much safer bet for a default heuristic than basing it on host RAM
> size.

It was a poor heuristic because the amount of code generation space you
need really depends on the amount of code being executed and that is
more determined by workload than RAM size. You may have 4gb of RAM
running a single program with a large block cache or 128Mb of RAM but
constantly swapping code from a block store which triggers a
re-translation every time.

Also as the translation cache is mmap'ed it doesn't all have to get
used. Having spare cache isn't too wasteful.

> I'd probably say that the original commit which changed the argument
> processing is flawed, and could/should be fixed.

I'd say not - we are not trying to replace/fix the original heuristic
but introduce a new one to finesse behaviour in relatively resource
constrained machines. Nothing we do can cope with all the potential
range of invocations of QEMU people might do. For that the user will
have to look at workload and tweak the tb-size control. The default was
chosen to make the "common" case of running a single guest on a users
desktop work at a reasonable performance level. You'll see we make that
distinction in the comments between system emulation and for example
linux-user where it's much more reasonable to expect multiple QEMU
invocations.

> The problem that commit was trying to solve was to do validation of the
> value passed to -m. In fixing that it also moving the parsing. The key
> problem here is that we need to do parsing and validating at different
> points in the startup procedure.  IOW, we need to split the logic, not
> simply moving the CLI parsing to the place that makes validation work.
>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [PATCH-for-5.1] hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value

[Laszlo Ersek]

+Igor, and question below

On 07/23/20 09:37, Markus Armbruster wrote:

> You must use ERRP_GUARD() in functions that dereference their @errp
> parameter (so that works even when the argument is null) or pass it to
> error_prepend() or error_append_hint() (so they get reached even when
> the argumentis _fatal).
>
> You should use Use ERRP_GUARD() to avoid clumsy error propagation.
>
> You should not use ERRP_GUARD() when propagation is not actually
> needed.

Thank you for the explanation. :)

Two patches from a series (work in progress) that I'd like to raise:

- [PATCH 2/6] x86: cphp: prevent guest crash on CPU hotplug when broadcast SMI 
is in use
  20200720141610.574308-3-imammedo@redhat.com">http://mid.mail-archive.com/20200720141610.574308-3-imammedo@redhat.com
  https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg05852.html

- [PATCH 3/6] x86: cpuhp: refuse cpu hot-unplug request earlier if not supported
  20200720141610.574308-4-imammedo@redhat.com">http://mid.mail-archive.com/20200720141610.574308-4-imammedo@redhat.com
  https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg05851.html

Both of these call error_append_hint(errp, ...). I think these functions
are never called against "error_fatal" (they are reached in "device_add"
and "device_del" monitor commands). But just for consistency with the
new rules, should these functions -- ich9_pm_device_pre_plug_cb() and
ich9_pm_device_unplug_request_cb() -- adopt ERRP_GUARD() in those
patches?

(If the answer is "yes", then could you please state that right under
those patches, so the feedback is easier for Igor to collect?

Plus I think commit e3fe3988d78 should be mentioned frequently, because
it's really helpful, and at least I wouldn't have remembered to check
"include/qapi/error.h" for the new rules; mea culpa :/)

Thanks!
Laszlo

[Bug 1886793] Re: "go install" command fails while running inside s390x docker container on x86_64 host using qemu

[Nirman Narang]

I ran the following commands:

#apt install debootstrap
#debootstrap_dir=debootstrap
#debootstrap --arch=s390x --foreign sid "$debootstrap_dir"
#sudo mkdir -p "${debootstrap_dir}/usr/bin"
#sudo cp "$(which qemu-s390x-static)" "${debootstrap_dir}/usr/bin"
#sudo cp "$(which qemu-s390x)" "${debootstrap_dir}/usr/bin"

All commands above were successful except below one:

#sudo chroot "$debootstrap_dir" /debootstrap/debootstrap --second-stage
Gave following error:
W: Failure trying to run:  dpkg --force-depends --install 
/var/cache/apt/archives/base-passwd_3.5.47_s390x.deb
W: See //debootstrap/debootstrap.log for details (possibly the package libgcc1 
is at fault)

Anyway, I proceeded:
# uname -m
s390x

# apt install golang-go
Reading package lists... Done
Building dependency tree... Done
You might want to run 'apt --fix-broken install' to correct these.
The following packages have unmet dependencies:
 dpkg : Conflicts: dpkg:none
 dpkg:none : Conflicts: dpkg but 1.20.5 is to be installed
 golang-go : Depends: golang-1.14-go but it is not going to be installed
 Depends: golang-src (>= 2:1.14~2) but it is not going to be 
installed
 libgcc1 : Depends: gcc-10-base (= 10.1.0-1) but 10.1.0-6+b1 is to be installed
E: Unmet dependencies. Try 'apt --fix-broken install' with no packages (or 
specify a solution).

# apt --fix-broken install
Reading package lists... Done
Building dependency tree... Done
Correcting dependencies... Done
The following packages will be REMOVED:
  dpkg:none libgcc1
0 upgraded, 0 newly installed, 2 to remove and 0 not upgraded.
1 not fully installed or removed.
After this operation, 85.0 kB disk space will be freed.
Do you want to continue? [Y/n] y
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
LANGUAGE = (unset),
LC_ALL = (unset),
LANG = "en_US.UTF-8"
are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
/usr/bin/locale: Cannot set LC_CTYPE to default locale: No such file or 
directory
/usr/bin/locale: Cannot set LC_MESSAGES to default locale: No such file or 
directory
/usr/bin/locale: Cannot set LC_ALL to default locale: No such file or directory
dpkg: error: parsing file '/var/lib/dpkg/status' near line 3918 package 'dpkg':
 duplicate value for 'Package' field
E: Sub-process dpkg --set-selections returned an error code (2)
E: Couldn't record the approved state changes as dpkg selection states

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1886793

Title:
  "go install" command fails while running inside s390x docker container
  on x86_64 host using qemu

Status in QEMU:
  New

Bug description:
  Steps to reproduce the issue:

  Register x86_64 host with the latest qemu-user-static.
  docker run --rm --privileged multiarch/qemu-user-static --reset -p yes

  Build the following Docker Image using following Dockerfile.s390x
  using command docker build -t test/crossbuild:latest-s390x -f
  Dockerfile.s390x .

  Dockerfile.s390x

  ##
  FROM alpine:3.11 as qemu
  ARG QEMU_VERSION=5.0.0-2
  ARG QEMU_ARCHS="s390x"
  RUN apk --update add curl
  #Enable non-native runs on amd64 architecture hosts
  RUN for i in ${QEMU_ARCHS}; do curl -L 
https://github.com/multiarch/qemu-user-static/releases/download/v${QEMU_VERSION}/qemu-${i}-static.tar.gz
 | tar zxvf - -C /usr/bin; done
  RUN chmod +x /usr/bin/qemu-*

  FROM s390x/golang:1.14.2-alpine3.11
  MAINTAINER LoZ Open Source Ecosystem 
(https://www.ibm.com/developerworks/community/groups/community/lozopensource)

  ARG MANIFEST_TOOL_VERSION=v1.0.2

  #Enable non-native builds of this image on an amd64 hosts.
  #This must be the first RUN command in this file!
  COPY --from=qemu /usr/bin/qemu-*-static /usr/bin/

  #Install su-exec for use in the entrypoint.sh (so processes run as the right 
user)
  #Install bash for the entry script (and because it's generally useful)
  #Install curl to download glide
  #Install git for fetching Go dependencies
  #Install ssh for fetching Go dependencies
  #Install mercurial for fetching go dependencies
  #Install wget since it's useful for fetching
  #Install make for building things
  #Install util-linux for column command (used for output formatting).
  #Install grep and sed for use in some Makefiles (e.g. pulling versions out of 
glide.yaml)
  #Install shadow for useradd (it allows to use big UID)
  RUN apk update && apk add --no-cache su-exec curl bash git openssh mercurial 
make wget util-linux tini file grep sed shadow
  RUN apk upgrade --no-cache

  #Disable ssh host key checking
  RUN echo 'Host *' >> /etc/ssh/ssh_config \
    && echo 'StrictHostKeyChecking no' >> /etc/ssh/ssh_config

  #Disable cgo so that binaries we build will be fully static.
  ENV CGO_ENABLED=0

  #Recompile the standard library with cgo disabled.  This prevents the 

Re: [PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

On Thu, 23 Jul 2020 13:57:08 +0200
David Hildenbrand  wrote:

> On 23.07.20 08:33, Cornelia Huck wrote:
> > On Mon, 20 Jul 2020 11:07:51 +0200
> > David Hildenbrand  wrote:
> >   
> >> On 20.07.20 11:03, Michael S. Tsirkin wrote:  
> >>> On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:
>  On 07.07.20 12:54, Cornelia Huck wrote:
> > As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> > a good idea to make sure that any new virtio device (which does not
> > support legacy virtio) is indeed a non-transitional device, just to
> > catch accidental misconfigurations. We can easily compile a list
> > of virtio devices with legacy support and have transports verify
> > in their plugged callbacks that legacy support is off for any device
> > not in that list.
> >
> > Most new virtio devices force non-transitional already, so nothing
> > changes for them. vhost-user-fs-pci even does not allow to configure
> > a non-transitional device, so it is fine as well.
> >
> > One problematic device, however, is virtio-iommu-pci. It currently
> > offers both the transitional and the non-transitional variety of the
> > device, and does not force anything. I'm unsure whether we should
> > consider transitional virtio-iommu unsupported, or if we should add
> > some compat handling. (The support for legacy or not generally may
> > change based upon the bus, IIUC, so I'm unsure how to come up with
> > something generic.)
> >
> > Cornelia Huck (2):
> >   virtio: list legacy-capable devices
> >   virtio: verify that legacy support is not accidentally on
> 
>  I'd squash both patches. Looking at patch #1, I wonder why we don't
>  store that information along with the device implementation? What was
>  the motivation to define this information separately?
> >>>
> >>> Because people seem to cut and paste code, so when one
> >>> enables it in an old device, it gets pasted into a new one.
> >>> With a list in a central place, it's easier to figure out
> >>> what's going on.
> >>
> >> Makes sense, I suggest adding that to the patch description.  
> > 
> > "The list of devices supporting legacy is supposed to be static. We
> > keep it in a central place to make sure that new devices do not enable
> > legacy by accident."
> > 
> > ?  
> 
> Ack!
> 
> >   
> >>
> >> Both patches look sane to me (- squashing them).
> >>  
> > 
> > Patch 1 does not change behaviour, while patch 2 does (for
> > virtio-iommu-pci). Still would like an opinion whether changing the
> > behaviour for virtio-iommu-pci with no compat handling is ok.
> > 
> > (I could be persuaded to squash them.)  
> 
> I'm a friend of introducing helper functions along with code that
> actually uses it. But I agree that the change in behavior might be
> hairy. Maybe we can split that out somehow to give it more attention?

It should not really be noticeable for anything but virtio-iommu.

However, I see these are already in a pull request...

Re: 5.1.0-rc1 regression: reset fails with kvm and -cpu host

[Vitaly Kuznetsov]

Philippe Mathieu-Daudé  writes:

> +Vitaly
>
> On 7/23/20 10:40 AM, Dr. David Alan Gilbert wrote:
>> * Eduardo Habkost (ehabk...@redhat.com) wrote:
>>> On Wed, Jul 22, 2020 at 04:47:32PM -0400, Eduardo Habkost wrote:
 On Wed, Jul 22, 2020 at 08:05:01PM +0200, Jan Kiszka wrote:
> On 22.07.20 19:35, Eduardo Habkost wrote:
>> Hi Jan,
>>
>> What was the last version where it worked for you?  Does using
>> "-cpu host,-vmx" help?
>
> Yeah, -vmx does indeed help.
>
> I didn't have the time to bisect yet. Just check my reflog, picked
> eb6490f544, and that works.

 Thanks!

 I could reproduce it locally[1], I will bisect it.

 The good news is that "-cpu host,+vmx" still works, on commit
 eb6490f544.

 [1] Linux 5.6.19-300.fc32.x86_64, Intel Core i7-8665U CPU.
>>>
>>> Bisected to:
>>>
>>> commit b16c0e20c74218f2d69710cedad11da7dd4d2190
>>> Author: Paolo Bonzini 
>>> Date:   Wed May 20 10:49:22 2020 -0400
>>>
>>> KVM: add support for AMD nested live migration
>>>
>>> Support for nested guest live migration is part of Linux 5.8, add the
>>> corresponding code to QEMU.  The migration format consists of a few
>>> flags, is an opaque 4k blob.
>>>
>>> The blob is in VMCB format (the control area represents the L1 VMCB
>>> control fields, the save area represents the pre-vmentry state; KVM does
>>> not use the host save area since the AMD manual allows that) but QEMU
>>> does not really care about that.  However, the flags need to be
>>> copied to hflags/hflags2 and back.
>>>
>>> In addition, support for retrieving and setting the AMD nested 
>>> virtualization
>>> states allows the L1 guest to be reset while running a nested guest, but
>>> a small bug in CPU reset needs to be fixed for that to work.
>>>
>>> Signed-off-by: Paolo Bonzini 
>> 
>> Guesswork led me to try reverting the chunk in kvm_put_nested_state;
>> without it the reset seems to work; I can't explain that code though.
>> 

(sorry, missed the beginning of the discussion)

So one does:

(qemu) system_reset 

on Intel wiht '-cpu host' and the result is:

(qemu) KVM: entry failed, hardware error 0x8021

If you're running a guest on an Intel machine without unrestricted mode
support, the failure can be most likely due to the guest entering an invalid
state for Intel VT. For example, the guest maybe running in big real mode
which is not supported on less recent Intel processors.

EAX=0064 EBX=91df5efe ECX= EDX=03f8
ESI= EDI=91ee32c0 EBP=90643260 ESP=00013c68
EIP=906428e6 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =   9300
CS =f000   9b00
SS =   9300
DS =   9300
FS =   9300
GS =   9300
LDT=   8200
TR =   8b00
GDT=  
IDT=  
CR0=6010 CR2= CR3= CR4=
DR0= DR1= DR2= 
DR3= 
DR6=0ff0 DR7=0400
EFER=
Code=?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??  ?? ?? ?? 
?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??

I can take a look (if no one beats me to it).

-- 
Vitaly

[PATCH 4/4] gitlab-ci.yml: Add build-system-debian and build-system-centos jobs

[Thomas Huth]

We were missing the two new targets avr-softmmu and rx-softmmu in the
gitlab-CI so far, and did not add some of the "other endianess" targets
like sh4eb-softmmu yet.
Since the current build-system-* jobs run already for a very long time,
let's do not add these missing targets there, but introduce two new
additional build jobs, one running with Debian and one running with
CentOS, and add the new targets there. Also move some targets from
the old build-system-* jobs to these new targets, to distribute the
load and reduce the runtime of the CI.

Signed-off-by: Thomas Huth 
---
 .gitlab-ci.yml | 88 +-
 1 file changed, 73 insertions(+), 15 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 362e5ee755..e96bcd50f8 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -53,68 +53,126 @@ include:
 - python3 -c 'import json; r = 
json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for 
t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
 - du -chs $HOME/avocado/data/cache
 
-build-system-ubuntu-main:
+build-system-ubuntu:
   <<: *native_build_job_definition
   variables:
 IMAGE: ubuntu2004
-TARGETS: aarch64-softmmu alpha-softmmu cris-softmmu hppa-softmmu 
lm32-softmmu
-  moxie-softmmu microblazeel-softmmu mips64el-softmmu m68k-softmmu 
ppc-softmmu
-  riscv64-softmmu sparc-softmmu
+TARGETS: aarch64-softmmu alpha-softmmu cris-softmmu hppa-softmmu
+  moxie-softmmu microblazeel-softmmu mips64el-softmmu
 MAKE_CHECK_ARGS: check-build
   artifacts:
 paths:
   - build
 
-check-system-ubuntu-main:
+check-system-ubuntu:
   <<: *native_test_job_definition
   needs:
-- job: build-system-ubuntu-main
+- job: build-system-ubuntu
   artifacts: true
   variables:
 IMAGE: ubuntu2004
 MAKE_CHECK_ARGS: check
 
-acceptance-system-ubuntu-main:
+acceptance-system-ubuntu:
   <<: *native_test_job_definition
   needs:
-- job: build-system-ubuntu-main
+- job: build-system-ubuntu
   artifacts: true
   variables:
 IMAGE: ubuntu2004
 MAKE_CHECK_ARGS: check-acceptance
   <<: *post_acceptance
 
-build-system-fedora-alt:
+build-system-debian:
+  <<: *native_build_job_definition
+  variables:
+IMAGE: debian-amd64
+TARGETS: arm-softmmu avr-softmmu i386-softmmu mipsel-softmmu
+  riscv64-softmmu sh4eb-softmmu sparc-softmmu xtensaeb-softmmu
+MAKE_CHECK_ARGS: check-build
+  artifacts:
+paths:
+  - build
+
+check-system-debian:
+  <<: *native_test_job_definition
+  needs:
+- job: build-system-debian
+  artifacts: true
+  variables:
+IMAGE: debian-amd64
+MAKE_CHECK_ARGS: check
+
+acceptance-system-debian:
+  <<: *native_test_job_definition
+  needs:
+- job: build-system-debian
+  artifacts: true
+  variables:
+IMAGE: debian-amd64
+MAKE_CHECK_ARGS: check-acceptance
+  <<: *post_acceptance
+
+build-system-fedora:
   <<: *native_build_job_definition
   variables:
 IMAGE: fedora
 TARGETS: tricore-softmmu unicore32-softmmu microblaze-softmmu mips-softmmu
-  riscv32-softmmu s390x-softmmu sh4-softmmu sparc64-softmmu x86_64-softmmu
-  xtensa-softmmu nios2-softmmu or1k-softmmu
+  xtensa-softmmu m68k-softmmu riscv32-softmmu ppc-softmmu sparc64-softmmu
 MAKE_CHECK_ARGS: check-build
   artifacts:
 paths:
   - build
 
-check-system-fedora-alt:
+check-system-fedora:
   <<: *native_test_job_definition
   needs:
-- job: build-system-fedora-alt
+- job: build-system-fedora
   artifacts: true
   variables:
 IMAGE: fedora
 MAKE_CHECK_ARGS: check
 
-acceptance-system-fedora-alt:
+acceptance-system-fedora:
   <<: *native_test_job_definition
   needs:
-- job: build-system-fedora-alt
+- job: build-system-fedora
   artifacts: true
   variables:
 IMAGE: fedora
 MAKE_CHECK_ARGS: check-acceptance
   <<: *post_acceptance
 
+build-system-centos:
+  <<: *native_build_job_definition
+  variables:
+IMAGE: centos8
+TARGETS: ppc64-softmmu lm32-softmmu or1k-softmmu s390x-softmmu
+  x86_64-softmmu rx-softmmu sh4-softmmu nios2-softmmu
+MAKE_CHECK_ARGS: check-build
+  artifacts:
+paths:
+  - build
+
+check-system-centos:
+  <<: *native_test_job_definition
+  needs:
+- job: build-system-centos
+  artifacts: true
+  variables:
+IMAGE: centos8
+MAKE_CHECK_ARGS: check
+
+acceptance-system-centos:
+  <<: *native_test_job_definition
+  needs:
+- job: build-system-centos
+  artifacts: true
+  variables:
+IMAGE: centos8
+MAKE_CHECK_ARGS: check-acceptance
+  <<: *post_acceptance
+
 build-disabled:
   <<: *native_build_job_definition
   variables:
-- 
2.18.1

[PATCH 3/4] tests/acceptance: Disable the rx sash and arm cubieboard replay test on Gitlab

[Thomas Huth]

These tests always time out on Gitlab, not sure what's happening here.
Let's disable them until somebody has enough spare time to debug the
issues.

Signed-off-by: Thomas Huth 
---
 tests/acceptance/machine_rx_gdbsim.py | 4 
 tests/acceptance/replay_kernel.py | 1 +
 2 files changed, 5 insertions(+)

diff --git a/tests/acceptance/machine_rx_gdbsim.py 
b/tests/acceptance/machine_rx_gdbsim.py
index bff63e421d..0c72506028 100644
--- a/tests/acceptance/machine_rx_gdbsim.py
+++ b/tests/acceptance/machine_rx_gdbsim.py
@@ -8,6 +8,9 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later.  See the COPYING file in the top-level directory.
 
+import os
+
+from avocado import skipIf
 from avocado_qemu import Test
 from avocado_qemu import exec_command_and_wait_for_pattern
 from avocado_qemu import wait_for_console_pattern
@@ -42,6 +45,7 @@ class RxGdbSimMachine(Test):
 # FIXME limit baudrate on chardev, else we type too fast
 #exec_command_and_wait_for_pattern(self, 'version', gcc_version)
 
+@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
 def test_linux_sash(self):
 """
 Boots a Linux kernel and checks that the console is operational.
diff --git a/tests/acceptance/replay_kernel.py 
b/tests/acceptance/replay_kernel.py
index 62d2db8c64..b79fc8daf8 100644
--- a/tests/acceptance/replay_kernel.py
+++ b/tests/acceptance/replay_kernel.py
@@ -126,6 +126,7 @@ class ReplayKernel(LinuxKernelTest):
 
 self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=1)
 
+@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
 def test_arm_cubieboard_initrd(self):
 """
 :avocado: tags=arch:arm
-- 
2.18.1

Re: [Virtio-fs] [PATCH for-5.1 2/3] virtiofsd: add container-friendly -o chroot sandboxing option

[Stefan Hajnoczi]

On Wed, Jul 22, 2020 at 02:17:10PM -0400, Vivek Goyal wrote:
> On Wed, Jul 22, 2020 at 02:02:05PM +0100, Stefan Hajnoczi wrote:
> > virtiofsd cannot run in an unprivileged container because CAP_SYS_ADMIN
> > is required to create namespaces.
> > 
> > Introduce a weaker sandbox that is sufficient in container environments
> > because the container runtime already sets up namespaces. Use chroot to
> > restrict path traversal to the shared directory.
> > 
> > virtiofsd loses the following:
> > 
> > 1. Mount namespace. The process chroots to the shared directory but
> >leaves the mounts in place. Seccomp rejects mount(2)/umount(2)
> >syscalls.
> > 
> > 2. Pid namespace. This should be fine because virtiofsd is the only
> >process running in the container.
> > 
> > 3. Network namespace. This should be fine because seccomp already
> >rejects the connect(2) syscall, but an additional layer of security
> >is lost. Container runtime-specific network security policies can be
> >used drop network traffic (except for the vhost-user UNIX domain
> >socket).
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  tools/virtiofsd/helper.c |  3 +++
> >  tools/virtiofsd/passthrough_ll.c | 44 ++--
> >  2 files changed, 45 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> > index 3105b6c23a..7421c9ca1a 100644
> > --- a/tools/virtiofsd/helper.c
> > +++ b/tools/virtiofsd/helper.c
> > @@ -151,6 +151,9 @@ void fuse_cmdline_help(void)
> > "-o cache=cache mode. could be one of 
> > \"auto, "
> > "always, none\"\n"
> > "   default: auto\n"
> > +   "-o chroot|no_chrootuse container-friendly chroot 
> > instead\n"
> > +   "   of stronger mount namespace 
> > sandbox\n"
> > +   "   default: false\n"
> 
> This option name disabling namespace setup is little confusing to me.
> 
> Will it make sense to provide another option to disable/enable
> namespaces. "-o no-namespaces" and that disables setting up
> namespaces.

Thanks, I'll propose a new syntax.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH] acpi: Fix access to PM1 control and status registers

[Michael S. Tsirkin]

On Thu, Jul 16, 2020 at 11:05:06AM +0200, Cédric Le Goater wrote:
> On 7/2/20 1:12 PM, Michael S. Tsirkin wrote:
> > On Wed, Jul 01, 2020 at 01:48:36PM +0100, Anthony PERARD wrote:
> >> On Wed, Jul 01, 2020 at 08:01:55AM -0400, Michael S. Tsirkin wrote:
> >>> On Wed, Jul 01, 2020 at 12:05:49PM +0100, Anthony PERARD wrote:
>  The ACPI spec state that "Accesses to PM1 control registers are
>  accessed through byte and word accesses." (In section 4.7.3.2.1 PM1
>  Control Registers of my old spec copy rev 4.0a).
> 
>  With commit 5d971f9e6725 ("memory: Revert "memory: accept mismatching
>  sizes in memory_region_access_valid""), it wasn't possible anymore to
>  access the pm1_cnt register by reading a single byte, and that is use
>  by at least a Xen firmware called "hvmloader".
> 
>  Also, take care of the PM1 Status Registers which also have "Accesses
>  to the PM1 status registers are done through byte or word accesses"
>  (In section 4.7.3.1.1 PM1 Status Registers).
> 
>  Signed-off-by: Anthony PERARD 
> >>>
> >>>
> >>> Can't we set impl.min_access_size to convert byte accesses
> >>> to word accesses?
> >>
> >> I actually tried, but when reading `addr` or `addr+1` I had the same
> >> value. So I guess `addr` wasn't taken into account.
> >>
> >> I've checked again, with `.impl.min_access_size = 2`, the width that the
> >> function acpi_pm_cnt_read() get is 2, but addr isn't changed so the
> >> function is still supposed to shift the result (or the value to write)
> >> based on addr, I guess.
> > 
> > True address is misaligned.  I think memory core should just align it -
> > this is what devices seem to expect.
> > However result is shifted properly so just align addr and be done with
> > it.
> > 
> > 
> > In fact I have a couple more questions. Paolo - maybe you can answer some 
> > of these?
> > 
> > 
> > 
> > if (!access_size_min) {
> > access_size_min = 1;
> > }
> > if (!access_size_max) {
> > access_size_max = 4;
> > }
> > 
> >
> > 
> > So 8 byte accesses are split up unless one requests 8 bytes.
> > Undocumented right?  Why are we doing this?
> > 
> >
> > 
> > 
> > /* FIXME: support unaligned access? */
> > 
> >
> > 
> > Shouldn't we document impl.unaligned is ignored right now?
> > Shouldn't we do something to make sure callbacks do not get
> > unaligned accesses they don't expect?
> > 
> > 
> > In fact, there are just 2 devices which set valid.unaligned but
> > not impl.unaligned:
> > aspeed_smc_ops
> > raven_io_ops
> > 
> > 
> > Is this intentional? 
> 
> I think it is a leftover from the initial implementation. The model works 
> fine 
> without valid.unaligned being set and with your patch.
> 
> C. 

Oh good, we can drop this. What about raven? Hervé could you comment pls?


> 
> > Do these in fact expect memory core to
> > provide aligned addresses to the callbacks?
> > Given impl.unaligned is not implemented, can we drop it completely?
> > Cc a bunch of people who might know.
> > 
> > Can relevant maintainers please comment? Thanks a lot!
> > 
> >
> > 
> > 
> > access_size = MAX(MIN(size, access_size_max), access_size_min);
> > access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> > 
> >
> > 
> > 
> > So with a 1 byte access at address 1, with impl.min_access_size = 2, we get:
> > access_size = 2
> > access_mask = 0x
> > addr = 1
> > 
> > 
> > 
> > 
> > 
> > 
> > if (memory_region_big_endian(mr)) {
> > for (i = 0; i < size; i += access_size) {
> > r |= access_fn(mr, addr + i, value, access_size,
> > (size - access_size - i) * 8, access_mask, attrs);
> > 
> 
> > 
> > now shift is -8.
> > 
> > 
> > 
> > 
> > }
> > } else {
> > for (i = 0; i < size; i += access_size) {
> > r |= access_fn(mr, addr + i, value, access_size, i * 8,
> > access_mask, attrs);
> > }
> > }
> > 
> > 
> > 
> > 
> > callback is invoked with addr 1 and size 2:
> > 
> >
> > 
> > 
> > uint64_t tmp;
> > 
> > tmp = mr->ops->read(mr->opaque, addr, size);
> > if (mr->subpage) {
> > trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, 
> > size);
> > } else if 
> > (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) {
> > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> > trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, 
> > size);
> > }
> > memory_region_shift_read_access(value, shift, mask, tmp);
> > return MEMTX_OK;
> > 
> > 
> > 
> > let's assume callback returned 0xabcd
> > 
> > this is where we are shifting the return value:
> > 
> >
> > 
> > 
> > static inline void memory_region_shift_read_access(uint64_t *value,
> >signed shift,
> >

Re: [PATCH v2] hw/pci-host: save/restore pci host config register

[Michael S. Tsirkin]

On Thu, Jul 23, 2020 at 08:23:01PM +0800, Wang King wrote:
> From: Hogan Wang 
> 
> The pci host config register is used to save PCI address for
> read/write config data. If guest write a value to config register,
> and then pause the vcpu to migrate, After the migration, the guest
> continue to write pci config data, and the write data will be ignored
> because of new qemu process lost the config register state.
> 
> Reproduction steps are:
> 1. guest booting in seabios.
> 2. guest enable the SMRAM in seabios:piix4_apmc_smm_setup, and then
>expect to disable the SMRAM by pci_config_writeb.
> 3. after guest write the pci host config register, and then pasued vcpu
>to finish migration.
> 4. guest write config data(0x0A) fail to disable the SMRAM becasue of
>config register state lost.
> 5. guest continue to boot and crash in ipxe option ROM due to SMRAM in
>enabled state.


Could you pls add: changes from v1?
Also my comments on v1 still apply ...

> ---
>  hw/pci-host/i440fx.c   | 11 +++
>  hw/pci-host/q35.c  | 11 +++
>  hw/pci/pci_host.c  | 11 +++
>  hw/pci/pcie_host.c | 11 +++
>  include/hw/pci/pci_host.h  | 10 ++
>  include/hw/pci/pcie_host.h | 10 ++
>  6 files changed, 64 insertions(+)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 8ed2417f0c..17705bb025 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
>  }
>  };
>  
> +static const VMStateDescription vmstate_i440fx_pcihost = {
> +.name = "I440FX_PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_PCI_HOST(parent_obj, I440FXState),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
>const char *name, void *opaque,
>Error **errp)
> @@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
> void *data)
>  hc->root_bus_path = i440fx_pcihost_root_bus_path;
>  dc->realize = i440fx_pcihost_realize;
>  dc->fw_name = "pci";
> +dc->vmsd = _i440fx_pcihost;
>  device_class_set_props(dc, i440fx_props);
>  /* Reason: needs to be wired up by pc_init1 */
>  dc->user_creatable = false;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b67cb9c29f..5e323be2e3 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
> Visitor *v,
>  visit_type_uint64(v, name, , errp);
>  }
>  
> +static const VMStateDescription vmstate_q35_pcihost = {
> +.name = "Q35_PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  /*
>   * NOTE: setting defaults for the mch.* fields in this table
>   * doesn't work, because mch is a separate QOM object that is
> @@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, void 
> *data)
>  
>  hc->root_bus_path = q35_host_root_bus_path;
>  dc->realize = q35_host_realize;
> +dc->vmsd = _q35_pcihost;
>  device_class_set_props(dc, q35_host_props);
>  /* Reason: needs to be wired up by pc_q35_init */
>  dc->user_creatable = false;
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index ce7bcdb1d5..7cdd5a3ea3 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -24,6 +24,7 @@
>  #include "hw/pci/pci_host.h"
>  #include "qemu/module.h"
>  #include "hw/pci/pci_bus.h"
> +#include "migration/vmstate.h"
>  #include "trace.h"
>  
>  /* debug PCI */
> @@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
>  .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> +const VMStateDescription vmstate_pcihost = {
> +.name = "PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(config_reg, PCIHostState),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  static const TypeInfo pci_host_type_info = {
>  .name = TYPE_PCI_HOST_BRIDGE,
>  .parent = TYPE_SYS_BUS_DEVICE,
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index 3534006f99..a653c39bb7 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -24,6 +24,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "qemu/module.h"
>  #include "exec/address-spaces.h"
> +#include "migration/vmstate.h"
>  
>  /* a helper function to get a PCIDevice for a given mmconfig address */
>  static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
> @@ -121,6 +122,16 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>  memory_region_transaction_commit();
>  }
>  
> +const VMStateDescription vmstate_pciehost = {
> +

Re: [PATCH for-5.1] nbd: Fix large trim/zero requests

[Vladimir Sementsov-Ogievskiy]

23.07.2020 14:47, Eric Blake wrote:

On 7/23/20 2:23 AM, Vladimir Sementsov-Ogievskiy wrote:

23.07.2020 00:22, Eric Blake wrote:

Although qemu as NBD client limits requests to <2G, the NBD protocol
allows clients to send requests almost all the way up to 4G.  But
because our block layer is not yet 64-bit clean, we accidentally wrap
such requests into a negative size, and fail with EIO instead of
performing the intended operation.




@@ -2378,8 +2378,17 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
  if (request->flags & NBD_CMD_FLAG_FAST_ZERO) {
  flags |= BDRV_REQ_NO_FALLBACK;
  }
-    ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
-    request->len, flags);
+    ret = 0;
+    /* FIXME simplify this when blk_pwrite_zeroes switches to 64-bit */
+    while (ret == 0 && request->len) {
+    int align = client->check_align ?: 1;
+    int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
+    align));
+    ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
+    len, flags);
+    request->len -= len;
+    request->from += len;
+    }
  return nbd_send_generic_reply(client, request->handle, ret,
    "writing to file failed", errp);


It's easy enough to audit that blk_pwrite_zeroes returns 0 (and not arbitrary 
positive) on success.


Yes, that's why I've posted my commend to blk_co_pdiscard :)





@@ -2393,8 +2402,17 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
    "flush failed", errp);

  case NBD_CMD_TRIM:
-    ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
-  request->len);
+    ret = 0;
+    /* FIXME simplify this when blk_co_pdiscard switches to 64-bit */
+    while (ret == 0 && request->len) {


Did you check all the paths not to return positive ret on success? I'd prefer to 
compare ret >= 0 for this temporary code to not care of it


And for blk_co_pdiscard, the audit is already right here in the patch: 
pre-patch, ret = blk_co_pdiscard, followed by...




+    int align = client->check_align ?: 1;
+    int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
+    align));
+    ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
+  len);
+    request->len -= len;
+    request->from += len;


Hmm.. Modifying the function parameter. Safe now, as nbd_handle_request() call 
is the last usage of request in nbd_trip().


+    }
  if (ret == 0 && request->flags & NBD_CMD_FLAG_FUA) {


...a check for ret == 0.


Hmm. Is it a bug or not? Anyway, bdrv_co_pdiscard() does "if (ret && .." as 
well, so if some driver return positive ret,
it may fail earlier..




  ret = blk_co_flush(exp->blk);
  }






Yes, I can respin the patch to use >= 0 as the check for success in both 
functions, but it doesn't change the behavior.



OK. Anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: 5.1.0-rc1 regression: reset fails with kvm and -cpu host

[Paolo Bonzini]

Yes, that seems correct.

Paolo


Il gio 23 lug 2020, 15:26 Vitaly Kuznetsov  ha scritto:

> This depends on whether the guest has performed VMXON or not I believe.
>
> Anyways, I *think* the fix will be:
>
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 2b6b744..75c2e68 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -3883,7 +3883,7 @@ static int kvm_put_nested_state(X86CPU *cpu)
>  } else {
>  env->nested_state->flags &= ~KVM_STATE_NESTED_GUEST_MODE;
>  }
> -if (env->hflags2 & HF2_GIF_MASK) {
> +if (cpu_has_svm(env) && (env->hflags2 & HF2_GIF_MASK)) {
>  env->nested_state->flags |= KVM_STATE_NESTED_GIF_SET;
>  } else {
>  env->nested_state->flags &= ~KVM_STATE_NESTED_GIF_SET;
>
> As "KVM_STATE_NESTED_GIF_SET" is not relevant to nVMX, this works for me
> but let me explore kernel side of this a bit more.
>
> --
> Vitaly
>
>

Re: [PATCH 3/4] error: Remove NULL checks on error_propagate() calls (again)

[Eric Blake]

On 7/23/20 8:38 AM, Markus Armbruster wrote:


+++ b/migration/colo.c
@@ -798,9 +798,7 @@ static void 
colo_incoming_process_checkpoint(MigrationIncomingState *mis,
 colo_send_message(mis->to_src_file,
COLO_MESSAGE_VMSTATE_LOADED,
_err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
   }


As this is mechanical, it is fine. But there is now a further cleanup
possible of passing errp directly to colo_send_message, and possibly
dropping local_err altogether.


True.

The patch is small and simple enough for squashing in further manual
cleanups.  I'd like to first check whether a followup patch created with
the machinery I used for eliminating error_propagate() comes out better.


Vladimir's scripts/coccinelle/errp-guard.cocci will take care of it.


Good to know.  Then I'm fine deferring those cleanups to the mechanical 
patches down the road, rather than a manual effort now.




Eliminating error propagation altogether would be even better, but it's
also more work: several void functions need to return bool instead.


Correct, but also doesn't change the fact that this patch is ready to go 
regardless of how much further cleanup we plan on doing.





Reviewed-by: Eric Blake 


Thanks!


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

[PATCH 2/2] ppc: Enable 2nd DAWR support on p10

[Ravi Bangoria]

As per the PAPR, bit 0 of byte 64 in pa-features property indicates
availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
find whether kvm supports 2nd DAWR or nor. If it's supported, set
the pa-feature bit in guest DT so the guest kernel can support 2nd
DAWR.

Signed-off-by: Ravi Bangoria 
---
 hw/ppc/spapr.c  | 33 +
 include/hw/ppc/spapr.h  |  1 +
 linux-headers/asm-powerpc/kvm.h |  4 
 linux-headers/linux/kvm.h   |  1 +
 target/ppc/cpu.h|  2 ++
 target/ppc/kvm.c|  7 +++
 target/ppc/kvm_ppc.h|  6 ++
 target/ppc/translate_init.inc.c | 17 -
 8 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0ae293ec94..4416319363 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -252,6 +252,31 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
 /* 60: NM atomic, 62: RNG */
 0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
 };
+uint8_t pa_features_310[] = { 66, 0,
+/* 0: MMU|FPU|SLB|RUN|DABR|NX, 1: fri[nzpm]|DABRX|SPRG3|SLB0|PP110 */
+/* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, SSO, 5: LE|CFAR|EB|LSQ */
+0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0, /* 0 - 5 */
+/* 6: DS207 */
+0x80, 0x00, 0x00, 0x00, 0x00, 0x00, /* 6 - 11 */
+/* 16: Vector */
+0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 12 - 17 */
+/* 18: Vec. Scalar, 20: Vec. XOR, 22: HTM */
+0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 18 - 23 */
+/* 24: Ext. Dec, 26: 64 bit ftrs, 28: PM ftrs */
+0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 24 - 29 */
+/* 30: MMR, 32: LE atomic, 34: EBB + ext EBB */
+0x80, 0x00, 0x80, 0x00, 0xC0, 0x00, /* 30 - 35 */
+/* 36: SPR SO, 38: Copy/Paste, 40: Radix MMU */
+0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 36 - 41 */
+/* 42: PM, 44: PC RA, 46: SC vec'd */
+0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 42 - 47 */
+/* 48: SIMD, 50: QP BFP, 52: String */
+0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
+/* 54: DecFP, 56: DecI, 58: SHA */
+0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
+/* 60: NM atomic, 62: RNG, 64: DAWR1 */
+0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
+};
 uint8_t *pa_features = NULL;
 size_t pa_size;
 
@@ -267,6 +292,10 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
 pa_features = pa_features_300;
 pa_size = sizeof(pa_features_300);
 }
+if (ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, cpu->compat_pvr)) {
+pa_features = pa_features_310;
+pa_size = sizeof(pa_features_310);
+}
 if (!pa_features) {
 return;
 }
@@ -291,6 +320,10 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
 pa_features[40 + 2] &= ~0x80; /* Radix MMU */
 }
 
+if (kvm_enabled() && kvmppc_has_cap_dawr1()) {
+pa_features[66] |= 0x80;
+}
+
 _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 6ba43bc9b8..2f2beb4571 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -352,6 +352,7 @@ struct SpaprMachineState {
 #define H_SET_MODE_RESOURCE_SET_DAWR0   2
 #define H_SET_MODE_RESOURCE_ADDR_TRANS_MODE 3
 #define H_SET_MODE_RESOURCE_LE  4
+#define H_SET_MODE_RESOURCE_SET_DAWR1   5
 
 /* Flags for H_SET_MODE_RESOURCE_LE */
 #define H_SET_MODE_ENDIAN_BIG0
diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
index 38d61b73f5..c5c0f128b4 100644
--- a/linux-headers/asm-powerpc/kvm.h
+++ b/linux-headers/asm-powerpc/kvm.h
@@ -640,6 +640,10 @@ struct kvm_ppc_cpu_char {
 #define KVM_REG_PPC_ONLINE (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xbf)
 #define KVM_REG_PPC_PTCR   (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc0)
 
+/* POWER10 registers. */
+#define KVM_REG_PPC_DAWR1  (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc1)
+#define KVM_REG_PPC_DAWRX1 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc2)
+
 /* Transactional Memory checkpointed state:
  * This is all GPRs, all VSX regs and a subset of SPRs
  */
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index a28c366737..015fa4b44b 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_PPC_DAWR1 184
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 0f641becf7..52e17ef013 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1465,9 +1465,11 @@ typedef PowerPCCPU ArchCPU;
 #define SPR_PSPB  (0x09F)
 #define SPR_DPDES (0x0B0)
 

[Bug 1888601] Re: QEMU v5.1.0-rc0/rc1 hang with nested virtualization

[Simon Kaegi]

** Description changed:

  We're running Kata Containers using QEMU and with v5.1.0rc0 and rc1 have
  noticed a problem at startup where QEMu appears to hang. We are not
  seeing this problem on our bare metal nodes and only on a VSI that
  supports nested virtualization.
  
  We unfortunately see nothing at all in the QEMU logs to help understand
  the problem and a hung process is just a guess at this point.
  
  Using git bisect we first see the problem with...
  
  ---
  
  f19bcdfedd53ee93412d535a842a89fa27cae7f2 is the first bad commit
  commit f19bcdfedd53ee93412d535a842a89fa27cae7f2
  Author: Jason Wang 
  Date:   Wed Jul 1 22:55:28 2020 +0800
  
- virtio-pci: implement queue_enabled method
- 
- With version 1, we can detect whether a queue is enabled via
- queue_enabled.
- 
- Signed-off-by: Jason Wang 
- Signed-off-by: Cindy Lu 
- Message-Id: <20200701145538.22333-5-l...@redhat.com>
- Reviewed-by: Michael S. Tsirkin 
- Signed-off-by: Michael S. Tsirkin 
- Acked-by: Jason Wang 
+ virtio-pci: implement queue_enabled method
  
-  hw/virtio/virtio-pci.c | 13 +
-  1 file changed, 13 insertions(+)
+ With version 1, we can detect whether a queue is enabled via
+ queue_enabled.
+ 
+ Signed-off-by: Jason Wang 
+ Signed-off-by: Cindy Lu 
+ Message-Id: <20200701145538.22333-5-l...@redhat.com>
+ Reviewed-by: Michael S. Tsirkin 
+ Signed-off-by: Michael S. Tsirkin 
+ Acked-by: Jason Wang 
+ 
+  hw/virtio/virtio-pci.c | 13 +
+  1 file changed, 13 insertions(+)
  
  ---
  
- Reverting this commit seems to work and prevent the hanging.
+ Reverting this commit (on top of 5.1.0-rc1) seems to work and prevent
+ the hanging.
  
  ---
  
- Here's how kata ends up launching qemu in our environment -- 
+ Here's how kata ends up launching qemu in our environment --
  /opt/kata/bin/qemu-system-x86_64 -name 
sandbox-849df14c6065931adedb9d18bc9260a6d896f1814a8c5cfa239865772f1b7a5f -uuid 
6bec458e-1da7-4847-a5d7-5ab31d4d2465 -machine pc,accel=kvm,kernel_irqchip -cpu 
host,pmu=off -qmp 
unix:/run/vc/vm/849df14c6065931adedb9d18bc9260a6d896f1814a8c5cfa239865772f1b7a5f/qmp.sock,server,nowait
 -m 4096M,slots=10,maxmem=30978M -device 
pci-bridge,bus=pci.0,id=pci-bridge-0,chassis_nr=1,shpc=on,addr=2,romfile= 
-device virtio-serial-pci,disable-modern=true,id=serial0,romfile= -device 
virtconsole,chardev=charconsole0,id=console0 -chardev 
socket,id=charconsole0,path=/run/vc/vm/849df14c6065931adedb9d18bc9260a6d896f1814a8c5cfa239865772f1b7a5f/console.sock,server,nowait
 -device virtio-scsi-pci,id=scsi0,disable-modern=true,romfile= -object 
rng-random,id=rng0,filename=/dev/urandom -device 
virtio-rng-pci,rng=rng0,romfile= -device 
virtserialport,chardev=charch0,id=channel0,name=agent.channel.0 -chardev 
socket,id=charch0,path=/run/vc/vm/849df14c6065931adedb9d18bc9260a6d896f1814a8c5cfa239865772f1b7a5f/kata.sock,server,nowait
 -chardev 
socket,id=char-396c5c3e19e29353,path=/run/vc/vm/849df14c6065931adedb9d18bc9260a6d896f1814a8c5cfa239865772f1b7a5f/vhost-fs.sock
 -device 
vhost-user-fs-pci,chardev=char-396c5c3e19e29353,tag=kataShared,romfile= -netdev 
tap,id=network-0,vhost=on,vhostfds=3:4,fds=5:6 -device 
driver=virtio-net-pci,netdev=network-0,mac=52:ac:2d:02:1f:6f,disable-modern=true,mq=on,vectors=6,romfile=
 -global kvm-pit.lost_tick_policy=discard -vga none -no-user-config -nodefaults 
-nographic -daemonize -object 
memory-backend-file,id=dimm1,size=4096M,mem-path=/dev/shm,share=on -numa 
node,memdev=dimm1 -kernel /opt/kata/share/kata-containers/vmlinuz-5.7.9-74 
-initrd 
/opt/kata/share/kata-containers/kata-containers-initrd_alpine_1.11.2-6_agent.initrd
 -append tsc=reliable no_timer_check rcupdate.rcu_expedited=1 i8042.direct=1 
i8042.dumbkbd=1 i8042.nopnp=1 i8042.noaux=1 noreplace-smp reboot=k console=hvc0 
console=hvc1 iommu=off cryptomgr.notests net.ifnames=0 pci=lastbus=0 debug 
panic=1 nr_cpus=4 agent.use_vsock=false scsi_mod.scan=none 
init=/usr/bin/kata-agent -pidfile 
/run/vc/vm/849df14c6065931adedb9d18bc9260a6d896f1814a8c5cfa239865772f1b7a5f/pid 
-D 
/run/vc/vm/849df14c6065931adedb9d18bc9260a6d896f1814a8c5cfa239865772f1b7a5f/qemu.log
 -smp 2,cores=1,threads=1,sockets=4,maxcpus=4
  
  ---

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1888601

Title:
  QEMU v5.1.0-rc0/rc1 hang with nested virtualization

Status in QEMU:
  New

Bug description:
  We're running Kata Containers using QEMU and with v5.1.0rc0 and rc1
  have noticed a problem at startup where QEMu appears to hang. We are
  not seeing this problem on our bare metal nodes and only on a VSI that
  supports nested virtualization.

  We unfortunately see nothing at all in the QEMU logs to help
  understand the problem and a hung process is just a guess at this
  point.

  Using git bisect we first see the problem with...

  ---

  f19bcdfedd53ee93412d535a842a89fa27cae7f2 

Re: please try to avoid sending pullreqs late on release-candidate day

[Markus Armbruster]

Alex Bennée  writes:

> Kevin Wolf  writes:
>
>> Am 21.07.2020 um 17:56 hat Peter Maydell geschrieben:
>>> It is not helpful if everybody sends their pullrequests late
>>> on the Tuesday afternoon, as there just isn't enough time in the
>>> day to merge test and apply them all before I have to cut the tag.
>>> Please, if you can, try to send pullrequests earlier, eg Monday.
>>
> 
>>
>> So given that we _will_ have some late patches, what can we do to
>> improve the situation?
>>
>> Maybe I could send the pull request before testing it to save some time.
>> Your tests will take a while anyway, so if my own testing fails (e.g.
>> for the parts of iotests that you don't test), I would still have time
>> to NACK my own pull request. This wouldn't buy us more than an hour at
>> most and could lead to wasted testing effort on your side (which is
>> exactly the resource we want to save).
>>
>> Can you test multiple pull requests at once? The Tuesday ones tend to be
>> small (between 1 and 3 patches was what I saw yesterday), so they should
>> be much less likely to fail than large pull requests. If you test two
>> pull requests together and it fails so you have to retest one of them in
>> isolation, you still haven't really lost time compared to testing both
>> individually. And if it succeeds, you cut the testing time in half.
>
> I've taken to just stacking up patches from my multiple trees to avoid
> sending more than one PR a week. Of course sometimes the stack grows a
> bit too tall and becomes unwieldy :-/

You're right, stacking unrelated smaller pull requests makes sense when
pulling all the pull requests in flight races with a deadline.

Re: [PATCH] Fix vhost-user buffer over-read on ram hot-unplug

[Marc-André Lureau]

On Fri, Jul 17, 2020 at 8:21 AM Raphael Norwitz
 wrote:
>
> The VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS vhost-user protocol
> feature introduced a shadow-table, used by the backend to dynamically
> determine how a vdev's memory regions have changed since the last
> vhost_user_set_mem_table() call. On hot-remove, a memmove() operation
> is used to overwrite the removed shadow region descriptor(s). The size
> parameter of this memmove was off by 1 such that if a VM with a backend
> supporting the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS filled it's
> shadow-table (by performing the maximum number of supported hot-add
> operatons) and attempted to remove the last region, Qemu would read an
> out of bounds value and potentially crash.
>
> This change fixes the memmove() bounds such that this erroneous read can
> never happen.
>
> Signed-off-by: Peter Turschmid 
> Signed-off-by: Raphael Norwitz 

Fixes: f1aeb14b0809 ("Transmit vhost-user memory regions individually")
Reviewed-by: Marc-André Lureau 

> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 3123121..d7e2423 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -672,7 +672,7 @@ static int send_remove_regions(struct vhost_dev *dev,
>  memmove(>shadow_regions[shadow_reg_idx],
>  >shadow_regions[shadow_reg_idx + 1],
>  sizeof(struct vhost_memory_region) *
> -(u->num_shadow_regions - shadow_reg_idx));
> +(u->num_shadow_regions - shadow_reg_idx - 1));
>  u->num_shadow_regions--;
>  }
>
> --
> 1.8.3.1
>

[PATCH] trace/simple: Allow enabling simple traces from command line

[Josh DuBois]

The simple trace backend is enabled / disabled with a call
to st_set_trace_file_enabled().  When initializing tracing
from the command-line, this must be enabled on startup.
(Prior to db25d56c014aa1a9, command-line initialization of
simple trace worked because every call to st_set_trace_file
enabled tracing.)

Fixes: db25d56c014aa1a96319c663e0a60346a223b31e
Signed-off-by: Josh DuBois 
---
 trace/control.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/trace/control.c b/trace/control.c
index 2ffe000818..6558b5c906 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -225,6 +225,7 @@ void trace_init_file(const char *file)
 {
 #ifdef CONFIG_TRACE_SIMPLE
 st_set_trace_file(file);
+st_set_trace_file_enabled(true);
 #elif defined CONFIG_TRACE_LOG
 /*
  * If both the simple and the log backends are enabled, "--trace file"
-- 
2.25.1

Re: [PATCH v2 17/20] backup: move to block-copy

[Max Reitz]

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> This brings async request handling and block-status driven chunk sizes
> to backup out of the box, which improves backup performance.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block-copy.h |   9 +--
>  block/backup.c | 145 +++--
>  block/block-copy.c |  21 ++
>  3 files changed, 83 insertions(+), 92 deletions(-)

This patch feels like it should be multiple ones.  I don’t see why a
patch that lets backup use block-copy (more) should have to modify the
block-copy code.

More specifically: I think that block_copy_set_progress_callback()
should be removed in a separate patch.  Also, moving
@cb_opaque/@progress_opaque from BlockCopyState into BlockCopyCallState
seems like a separate patch to me, too.

(I presume if the cb had had its own opaque object from patch 5 on,
there wouldn’t be this problem now.  We’d just stop using the progress
callback in backup, and remove it in one separate patch.)

[...]

> diff --git a/block/backup.c b/block/backup.c
> index ec2676abc2..59c00f5293 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -44,42 +44,19 @@ typedef struct BackupBlockJob {
>  BlockdevOnError on_source_error;
>  BlockdevOnError on_target_error;
>  uint64_t len;
> -uint64_t bytes_read;
>  int64_t cluster_size;
>  int max_workers;
>  int64_t max_chunk;
>  
>  BlockCopyState *bcs;
> +
> +BlockCopyCallState *bcs_call;

Can this be more descriptive?  E.g. background_bcs?  bg_bcs_call?  bg_bcsc?

> +int ret;
> +bool error_is_read;
>  } BackupBlockJob;
>  
>  static const BlockJobDriver backup_job_driver;
>  

[...]

>  static int coroutine_fn backup_loop(BackupBlockJob *job)
>  {
> -bool error_is_read;
> -int64_t offset;
> -BdrvDirtyBitmapIter *bdbi;
> -int ret = 0;
> +while (true) { /* retry loop */
> +assert(!job->bcs_call);
> +job->bcs_call = block_copy_async(job->bcs, 0,
> + QEMU_ALIGN_UP(job->len,
> +   job->cluster_size),
> + true, job->max_workers, 
> job->max_chunk,
> + backup_block_copy_callback, job);
>  
> -bdbi = bdrv_dirty_iter_new(block_copy_dirty_bitmap(job->bcs));
> -while ((offset = bdrv_dirty_iter_next(bdbi)) != -1) {
> -do {
> -if (yield_and_check(job)) {
> -goto out;
> +while (job->bcs_call && !job->common.job.cancelled) {
> +/* wait and handle pauses */

Doesn’t someone need to reset BlockCopyCallState.cancelled when the job
is unpaused?  I can’t see anyone doing that.

Well, or even just reentering the block-copy operation after
backup_pause() has cancelled it.  Is there some magic I’m missing?

Does pausing (which leads to cancelling the block-copy operation) just
yield to the CB being invoked, so the copy operation is considered done,
and then we just enter the next iteration of the loop and try again?
But cancelling the block-copy operation leads to it returning 0, AFAIR,
so...

> +
> +job_pause_point(>common.job);
> +
> +if (job->bcs_call && !job->common.job.cancelled) {
> +job_yield(>common.job);
>  }
> -ret = backup_do_cow(job, offset, job->cluster_size, 
> _is_read);
> -if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
> -   BLOCK_ERROR_ACTION_REPORT)
> -{
> -goto out;
> +}
> +
> +if (!job->bcs_call && job->ret == 0) {
> +/* Success */
> +return 0;

...I would assume we return here when the job is paused.

> +}
> +
> +if (job->common.job.cancelled) {
> +if (job->bcs_call) {
> +block_copy_cancel(job->bcs_call);
>  }
> -} while (ret < 0);
> +return 0;
> +}
> +
> +if (!job->bcs_call && job->ret < 0 &&

Is it even possible for bcs_call to be non-NULL here?

> +(backup_error_action(job, job->error_is_read, -job->ret) ==
> + BLOCK_ERROR_ACTION_REPORT))
> +{
> +return job->ret;
> +}

So if we get an error, and the error action isn’t REPORT, we just try
the whole operation again?  That doesn’t sound very IGNORE-y to me.

>  }
>  
> - out:
> -bdrv_dirty_iter_free(bdbi);
> -return ret;
> +g_assert_not_reached();
>  }
>  
>  static void backup_init_bcs_bitmap(BackupBlockJob *job)
> @@ -246,9 +227,14 @@ static int coroutine_fn backup_run(Job *job, Error 
> **errp)
>  int64_t count;
>  
>  for (offset = 0; offset < s->len; ) {
> -if (yield_and_check(s)) {
> -ret = -ECANCELED;
> -goto out;
> +if (job_is_cancelled(job)) {
> +   

Re: [PATCH-for-5.1?] qapi/error: Check format string argument in error_propagate_prepend()

[Philippe Mathieu-Daudé]

On 7/23/20 11:44 AM, Stefan Weil wrote:
> Am 23.07.20 um 11:13 schrieb Philippe Mathieu-Daudé:
> 
>> error_propagate_prepend() "behaves like error_prepend()", and
>> error_prepend() uses "formatting @fmt, ... like printf()".
>> error_prepend() checks its format string argument, but
>> error_propagate_prepend() does not. Fix that.
>>
>> This would have catched the invalid format introduced in commit
>> b98e8d1230f:
>>
>> CC  hw/sd/milkymist-memcard.o
>>   hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’:
>>   hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching 
>> ‘char *’ argument [-Werror=format=]
>> 284 | error_propagate_prepend(errp, err, "failed to init SD 
>> card: %s");
>> |
>>  ~^
>> |
>>   |
>> |
>>   char *
>>
>> Fixes: 4b5766488f ("Fix use of error_prepend() with _fatal, 
>> _abort")
>> Inspired-by: Stefan Weil 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  include/qapi/error.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 7932594dce..f1a34d 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err);
>>   * error_propagate(dst_errp, local_err);
>>   * Please use ERRP_GUARD() and error_prepend() instead when possible.
>>   */
>> +GCC_FMT_ATTR(3, 4)
>>  void error_propagate_prepend(Error **dst_errp, Error *local_err,
>>   const char *fmt, ...);
>>  
> 
> 
> Reviewed-by: Stefan Weil 
> 
> error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add
> that, too.

This one is different as it uses a va_list. Now I realize it is
only called in util/error.c, and all its callers are guarded with
GCC_FMT_ATTR. Maybe we can make it static to simplify... Markus?

> 
> Regards,
> 
> Stefan
> 
> 

[PATCH-for-5.1 v2 0/2] tpm: Improve error reporting

[Philippe Mathieu-Daudé]

Improve error reporting by listing TPM backends.

Philippe Mathieu-Daudé (2):
  tpm: Display when no backend is available
  tpm: List the available TPM backends

 tpm.c | 40 ++--
 1 file changed, 30 insertions(+), 10 deletions(-)

-- 
2.21.3

[PATCH-for-5.1 v2 1/2] tpm: Display when no backend is available

[Philippe Mathieu-Daudé]

Display "No TPM backend available in this binary." error when
no backend is available.

Reviewed-by: Stefan Berger 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tpm.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/tpm.c b/tpm.c
index fe03b24858..e36803a64d 100644
--- a/tpm.c
+++ b/tpm.c
@@ -41,6 +41,22 @@ tpm_be_find_by_type(enum TpmType type)
 return TPM_BACKEND_CLASS(oc);
 }
 
+/*
+ * Walk the list of available TPM backend drivers and count them.
+ */
+static int tpm_backend_drivers_count(void)
+{
+int count = 0, i;
+
+for (i = 0; i < TPM_TYPE__MAX; i++) {
+const TPMBackendClass *bc = tpm_be_find_by_type(i);
+if (bc) {
+count++;
+}
+}
+return count;
+}
+
 /*
  * Walk the list of available TPM backend drivers and display them on the
  * screen.
@@ -87,6 +103,11 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
Error **errp)
 TPMBackend *drv;
 int i;
 
+if (!tpm_backend_drivers_count()) {
+error_setg(errp, "No TPM backend available in this binary.");
+return 1;
+}
+
 if (!QLIST_EMPTY(_backends)) {
 error_setg(errp, "Only one TPM is allowed.");
 return 1;
-- 
2.21.3

[PATCH-for-5.1 v2 2/2] tpm: List the available TPM backends

[Philippe Mathieu-Daudé]

When an incorrect backend is selected, tpm_display_backend_drivers()
is supposed to list the available backends. However the error is
directly propagated, and we never display the list. The user only
gets "Parameter 'type' expects a TPM backend type" error.

Convert the fprintf(stderr,) calls to error hints propagated with
the error.

Signed-off-by: Philippe Mathieu-Daudé 
---
Since v1:
- Use g_assert_not_reached after processing 'help' in tpm_config_parse
---
 tpm.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tpm.c b/tpm.c
index e36803a64d..f883340d1a 100644
--- a/tpm.c
+++ b/tpm.c
@@ -58,23 +58,21 @@ static int tpm_backend_drivers_count(void)
 }
 
 /*
- * Walk the list of available TPM backend drivers and display them on the
- * screen.
+ * Walk the list of available TPM backend drivers and list them as Error hint.
  */
-static void tpm_display_backend_drivers(void)
+static void tpm_list_backend_drivers_hint(Error **errp)
 {
 int i;
 
-fprintf(stderr, "Supported TPM types (choose only one):\n");
+error_append_hint(errp, "Supported TPM types (choose only one):\n");
 
 for (i = 0; i < TPM_TYPE__MAX; i++) {
 const TPMBackendClass *bc = tpm_be_find_by_type(i);
 if (!bc) {
 continue;
 }
-fprintf(stderr, "%12s   %s\n", TpmType_str(i), bc->desc);
+error_append_hint(errp, "%12s   %s\n", TpmType_str(i), bc->desc);
 }
-fprintf(stderr, "\n");
 }
 
 /*
@@ -97,6 +95,7 @@ TPMBackend *qemu_find_tpm_be(const char *id)
 
 static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
 {
+ERRP_GUARD();
 const char *value;
 const char *id;
 const TPMBackendClass *be;
@@ -122,7 +121,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
Error **errp)
 value = qemu_opt_get(opts, "type");
 if (!value) {
 error_setg(errp, QERR_MISSING_PARAMETER, "type");
-tpm_display_backend_drivers();
+tpm_list_backend_drivers_hint(errp);
 return 1;
 }
 
@@ -131,7 +130,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
Error **errp)
 if (be == NULL) {
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a TPM backend type");
-tpm_display_backend_drivers();
+tpm_list_backend_drivers_hint(errp);
 return 1;
 }
 
@@ -184,8 +183,8 @@ int tpm_config_parse(QemuOptsList *opts_list, const char 
*optarg)
 QemuOpts *opts;
 
 if (!strcmp(optarg, "help")) {
-tpm_display_backend_drivers();
-return -1;
+tpm_list_backend_drivers_hint(_fatal);
+g_assert_not_reached(); /* Using _fatal triggers exit(1). */
 }
 opts = qemu_opts_parse_noisily(opts_list, optarg, true);
 if (!opts) {
-- 
2.21.3

Re: [RFC PATCH-for-5.1 2/2] tpm: List the available TPM backends

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> When an incorrect backend is selected, tpm_display_backend_drivers()
> is supposed to list the available backends. However the error is
> directly propagated, and we never display the list. The user only
> gets "Parameter 'type' expects a TPM backend type" error.
>
> Convert the fprintf(stderr,) calls to error hints propagated with
> the error.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> RFC because this is now odd in tpm_config_parse():
>
>   tpm_list_backend_drivers_hint(_fatal);
>   return -1;
> ---
>  tpm.c | 17 -
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/tpm.c b/tpm.c
> index e36803a64d..358566cb10 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -58,23 +58,21 @@ static int tpm_backend_drivers_count(void)
>  }
>  
>  /*
> - * Walk the list of available TPM backend drivers and display them on the
> - * screen.
> + * Walk the list of available TPM backend drivers and list them as Error 
> hint.
>   */
> -static void tpm_display_backend_drivers(void)
> +static void tpm_list_backend_drivers_hint(Error **errp)
>  {
>  int i;
>  
> -fprintf(stderr, "Supported TPM types (choose only one):\n");
> +error_append_hint(errp, "Supported TPM types (choose only one):\n");
>  
>  for (i = 0; i < TPM_TYPE__MAX; i++) {
>  const TPMBackendClass *bc = tpm_be_find_by_type(i);
>  if (!bc) {
>  continue;
>  }
> -fprintf(stderr, "%12s   %s\n", TpmType_str(i), bc->desc);
> +error_append_hint(errp, "%12s   %s\n", TpmType_str(i), bc->desc);
>  }
> -fprintf(stderr, "\n");
>  }
>  
>  /*
> @@ -97,6 +95,7 @@ TPMBackend *qemu_find_tpm_be(const char *id)
>  
>  static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>  {
> +ERRP_GUARD();
>  const char *value;
>  const char *id;
>  const TPMBackendClass *be;
> @@ -122,7 +121,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>  value = qemu_opt_get(opts, "type");
>  if (!value) {
>  error_setg(errp, QERR_MISSING_PARAMETER, "type");
> -tpm_display_backend_drivers();
> +tpm_list_backend_drivers_hint(errp);
>  return 1;
>  }
>  

Yes, this is how we should list available backends together with
error_setg().  Simply printing them to stderr is wrong then.

> @@ -131,7 +130,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>  if (be == NULL) {
>  error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
> "a TPM backend type");
> -tpm_display_backend_drivers();
> +tpm_list_backend_drivers_hint(errp);
>  return 1;
>  }
>  
> @@ -184,7 +183,7 @@ int tpm_config_parse(QemuOptsList *opts_list, const char 
> *optarg)
>  QemuOpts *opts;
>  
>  if (!strcmp(optarg, "help")) {
> -tpm_display_backend_drivers();
> +tpm_list_backend_drivers_hint(_fatal);
>  return -1;
>  }
>  opts = qemu_opts_parse_noisily(opts_list, optarg, true);

A bit worse than weird:

$ qemu-system-x86_64 -tpmdev help
upstream-qemu: /work/armbru/qemu/util/error.c:158: error_append_hint: 
Assertion `err && errp != _abort && errp != _fatal' failed.
Aborted (core dumped)

If we choose to use Error here, then I'd recommend two functions:

1. One to append a *short* hint.  Something like this:

qemu-system-x86_64: -tpmdev xxx,id=tpm0: Parameter 'type' expects a TPM 
backend type
Supported TPM types are passthrough, emulator.

   Actually, I wouldn't even make it a function, but simply do it inline
   for the "invalid value" case.  The missing value case can do without.
   Matter of taste.

2. Another one to print help.

Let's first decide whether to revert commit d10e05f15d5 instead.

Re: [PATCH 2/2] ppc: Enable 2nd DAWR support on p10

[Cornelia Huck]

On Thu, 23 Jul 2020 16:12:20 +0530
Ravi Bangoria  wrote:

> As per the PAPR, bit 0 of byte 64 in pa-features property indicates
> availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
> find whether kvm supports 2nd DAWR or nor. If it's supported, set
> the pa-feature bit in guest DT so the guest kernel can support 2nd
> DAWR.
> 
> Signed-off-by: Ravi Bangoria 
> ---
>  hw/ppc/spapr.c  | 33 +
>  include/hw/ppc/spapr.h  |  1 +
>  linux-headers/asm-powerpc/kvm.h |  4 
>  linux-headers/linux/kvm.h   |  1 +
>  target/ppc/cpu.h|  2 ++
>  target/ppc/kvm.c|  7 +++
>  target/ppc/kvm_ppc.h|  6 ++
>  target/ppc/translate_init.inc.c | 17 -
>  8 files changed, 70 insertions(+), 1 deletion(-)
> 

(...)

> diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
> index 38d61b73f5..c5c0f128b4 100644
> --- a/linux-headers/asm-powerpc/kvm.h
> +++ b/linux-headers/asm-powerpc/kvm.h
> @@ -640,6 +640,10 @@ struct kvm_ppc_cpu_char {
>  #define KVM_REG_PPC_ONLINE   (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xbf)
>  #define KVM_REG_PPC_PTCR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc0)
>  
> +/* POWER10 registers. */
> +#define KVM_REG_PPC_DAWR1(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc1)
> +#define KVM_REG_PPC_DAWRX1   (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc2)
> +
>  /* Transactional Memory checkpointed state:
>   * This is all GPRs, all VSX regs and a subset of SPRs
>   */
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index a28c366737..015fa4b44b 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_PPC_SECURE_GUEST 181
>  #define KVM_CAP_HALT_POLL 182
>  #define KVM_CAP_ASYNC_PF_INT 183
> +#define KVM_CAP_PPC_DAWR1 184
>  
>  #ifdef KVM_CAP_IRQ_ROUTING

Same here, this should go together with the headers changes from the
first patch.

Re: [PATCH] hw/pci-host: save/restore pci host config register

[Laszlo Ersek]

On 07/23/20 12:49, Wang King wrote:
> From: Hogan Wang 
> 
> The pci host config register is used to save PCI address for
> read/write config data. If guest write a value to config register,
> and then pause the vcpu to migrate, After the migration, the guest
> continue to write pci config data, and the write data will be ignored
> because of new qemu process lost the config register state.
> 
> Example:
> 1. guest booting in seabios.
> 2. guest enabled the SMM memory window in piix4_apmc_smm_setup, and
> then try to close the SMM memory window.
> 3. pasued vcpu to finish migration.
> 4. guest close the SMM memory window fail becasue of config register
> state lost.
> 5. guest continue to boot and crash in ipxe option ROM (SMM memory
> window is enabled).
> 
> Due to the complex guest, the negative effect is unpredictable.
> ---
>  hw/pci-host/i440fx.c   | 11 +++
>  hw/pci-host/q35.c  | 11 +++
>  hw/pci/pci_host.c  | 11 +++
>  hw/pci/pcie_host.c | 11 +++
>  include/hw/pci/pci_host.h  | 10 ++
>  include/hw/pci/pcie_host.h | 10 ++
>  6 files changed, 64 insertions(+)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 8ed2417f0c..17705bb025 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
>  }
>  };
>  
> +static const VMStateDescription vmstate_i440fx_pcihost = {
> +.name = "I440FX_PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_PCI_HOST(parent_obj, I440FXState),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
>const char *name, void *opaque,
>Error **errp)
> @@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
> void *data)
>  hc->root_bus_path = i440fx_pcihost_root_bus_path;
>  dc->realize = i440fx_pcihost_realize;
>  dc->fw_name = "pci";
> +dc->vmsd = _i440fx_pcihost;
>  device_class_set_props(dc, i440fx_props);
>  /* Reason: needs to be wired up by pc_init1 */
>  dc->user_creatable = false;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b67cb9c29f..5e323be2e3 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
> Visitor *v,
>  visit_type_uint64(v, name, , errp);
>  }
>  
> +static const VMStateDescription vmstate_q35_pcihost = {
> +.name = "Q35_PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  /*
>   * NOTE: setting defaults for the mch.* fields in this table
>   * doesn't work, because mch is a separate QOM object that is
> @@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, void 
> *data)
>  
>  hc->root_bus_path = q35_host_root_bus_path;
>  dc->realize = q35_host_realize;
> +dc->vmsd = _q35_pcihost;
>  device_class_set_props(dc, q35_host_props);
>  /* Reason: needs to be wired up by pc_q35_init */
>  dc->user_creatable = false;
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index ce7bcdb1d5..7cdd5a3ea3 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -24,6 +24,7 @@
>  #include "hw/pci/pci_host.h"
>  #include "qemu/module.h"
>  #include "hw/pci/pci_bus.h"
> +#include "migration/vmstate.h"
>  #include "trace.h"
>  
>  /* debug PCI */
> @@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
>  .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> +const VMStateDescription vmstate_pcihost = {
> +.name = "PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(config_reg, PCIHostState),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  static const TypeInfo pci_host_type_info = {
>  .name = TYPE_PCI_HOST_BRIDGE,
>  .parent = TYPE_SYS_BUS_DEVICE,
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index 3534006f99..a653c39bb7 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -24,6 +24,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "qemu/module.h"
>  #include "exec/address-spaces.h"
> +#include "migration/vmstate.h"
>  
>  /* a helper function to get a PCIDevice for a given mmconfig address */
>  static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
> @@ -121,6 +122,16 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>  memory_region_transaction_commit();
>  }
>  
> +const VMStateDescription vmstate_pciehost = {
> +.name = "PCIEHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_PCI_HOST(pci, 

Re: [PATCH 2/2] e1000e: make TX reentrant

[Stefan Hajnoczi]

On Thu, Jul 23, 2020 at 10:25:35AM +0800, Jason Wang wrote:
> 
> On 2020/7/22 下午8:53, Michael Tokarev wrote:
> > FWIW, this is not "making TX reentrant", it is about forbidding
> > reentrancy instead :)
> > 
> > /mjt
> 
> 
> Indeed, I will rename the title.

Please also include a comment explaining the purpose of the early return
in the code.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH for-5.1 2/3] virtiofsd: add container-friendly -o chroot sandboxing option

[Stefan Hajnoczi]

On Wed, Jul 22, 2020 at 05:58:11PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 22, 2020 at 02:02:05PM +0100, Stefan Hajnoczi wrote:
> > virtiofsd cannot run in an unprivileged container because CAP_SYS_ADMIN
> > is required to create namespaces.
> > 
> > Introduce a weaker sandbox that is sufficient in container environments
> > because the container runtime already sets up namespaces. Use chroot to
> > restrict path traversal to the shared directory.
> > 
> > virtiofsd loses the following:
> > 
> > 1. Mount namespace. The process chroots to the shared directory but
> >leaves the mounts in place. Seccomp rejects mount(2)/umount(2)
> >syscalls.
> > 
> > 2. Pid namespace. This should be fine because virtiofsd is the only
> >process running in the container.
> > 
> > 3. Network namespace. This should be fine because seccomp already
> >rejects the connect(2) syscall, but an additional layer of security
> >is lost. Container runtime-specific network security policies can be
> >used drop network traffic (except for the vhost-user UNIX domain
> >socket).
> 
> IIUC this relies on the fact that the container will still have 
> CAP_SYS_CHROOT IOW, we still don't have a solution for running
> virtiofsd as an unprivileged user.

Yes, this still requires root in the container.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH for-5.1 2/3] virtiofsd: add container-friendly -o chroot sandboxing option

[Stefan Hajnoczi]

On Wed, Jul 22, 2020 at 08:03:18PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > +/*
> > + * Make the shared directory the file system root so that FUSE_OPEN
> > + * (lo_open()) cannot escape the shared directory by opening a symlink.
> > + *
> > + * It's still possible to escape the chroot via lo->proc_self_fd but 
> > that
> > + * requires gaining control of the process first.
> > + */
> > +if (chroot(lo->source) != 0) {
> > +fuse_log(FUSE_LOG_ERR, "chroot(\"%s\"): %m\n", lo->source);
> > +exit(1);
> > +}
> 
> I'm seeing suggestions that you should drop CAP_SYS_CHROOT after
> chroot'ing to stop an old escape (where you create another jail inside
> the jail and the kernel then lets you walk outside of the old one).

That's already the case:

1. setup_seccomp() blocks further chroot(2) calls.
2. setup_capabilities() drops CAP_SYS_CHROOT.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH for-5.1 2/3] virtiofsd: add container-friendly -o chroot sandboxing option

[Stefan Hajnoczi]

On Wed, Jul 22, 2020 at 06:58:20PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > virtiofsd cannot run in an unprivileged container because CAP_SYS_ADMIN
> > is required to create namespaces.
> > 
> > Introduce a weaker sandbox that is sufficient in container environments
> > because the container runtime already sets up namespaces. Use chroot to
> > restrict path traversal to the shared directory.
> > 
> > virtiofsd loses the following:
> > 
> > 1. Mount namespace. The process chroots to the shared directory but
> >leaves the mounts in place. Seccomp rejects mount(2)/umount(2)
> >syscalls.
> 
> OK, I'm guessing the behaviour of what happens if the host adds another
> mount afterwards might be different?

Running inside a container with -o chroot:
 - The container has its own mount namespace and it is therefore not
   affected, modulo shared subtrees (see mount(8)).

Running outside a container with -o chroot:
 - Path traversal can only reach mounts that are made within the shared
   directory tree. Technically other mounts are still there but it is
   not possible to reach them via file system paths.

> > 2. Pid namespace. This should be fine because virtiofsd is the only
> >process running in the container.
> 
> Is it ? Isn't the qemu and any other vhost-user processes also in the
> same container?

No. QEMU, virtiofsd, and other vhost-user processes should run in their
own containers. Application container images are typically designed to
run a single program per container. It's technically possible to launch
multiple programs but that is considered bad practice for application
containers.

Kubernetes:
Containers in a pod do not share a single pid namespace by default.
Pods do share a single network namespace so they can communicate via
UNIX domain sockets.

> > 3. Network namespace. This should be fine because seccomp already
> >rejects the connect(2) syscall, but an additional layer of security
> >is lost. Container runtime-specific network security policies can be
> >used drop network traffic (except for the vhost-user UNIX domain
> >socket).
> 
> Should this be tied to the same flag - this feels different from the
> chroot specific problem.

Good point. Daniel Berrange has suggested another command-line syntax
that makes sandbox configuration more modular. I'll try to implement
something like that.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH] acpi: Fix access to PM1 control and status registers

[Michael S. Tsirkin]

On Fri, Jul 10, 2020 at 10:42:58AM +0100, Anthony PERARD wrote:
> On Thu, Jul 02, 2020 at 07:12:08AM -0400, Michael S. Tsirkin wrote:
> > memory: align to min access size
> > 
> > If impl.min_access_size > valid.min_access_size access callbacks
> > can get a misaligned access as size is increased.
> > They don't expect that, let's fix it in the memory core.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > 
> > ---
> > 
> > 
> > diff --git a/memory.c b/memory.c
> > index 9200b20130..ea489ce405 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -532,6 +532,7 @@ static MemTxResult access_with_adjusted_size(hwaddr 
> > addr,
> >  }
> >  
> >  /* FIXME: support unaligned access? */
> > +addr &= ~(access_size_min - 1);
> >  access_size = MAX(MIN(size, access_size_max), access_size_min);
> >  access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> >  if (memory_region_big_endian(mr)) {
> 
> I've tried this (and .impl.min_access_size=2) but that wasn't enough.
> 
> In the guest, I did `inb(base_addr + 1)`, but I've got back the value as
> if `inb(base_addr)` was run.
> 
> The device emulation read callbacks did get addr=0 width=2, so that's
> fine, but the result returned to the guest wasn't shifted. Same thing
> for write access, the write value isn't shifted, so a write to the
> second byte would be written to the first.
> 
> Thanks,

So is there still an issue with my latest pull req?
Or is everything fixed?


> -- 
> Anthony PERARD

[PATCH v1] hw/pci-host: save/restore pci host config register

[Wang King]

From: Hogan Wang 

The pci host config register is used to save PCI address for
read/write config data. If guest write a value to config register,
and then pause the vcpu to migrate, After the migration, the guest
continue to write pci config data, and the write data will be ignored
because of new qemu process lost the config register state.

Reproduction steps are:
1. guest booting in seabios.
2. guest enable the SMRAM in seabios:piix4_apmc_smm_setup, and then
   expect to disable the SMRAM by pci_config_writeb.
3. after guest write the pci host config register, and then pasued vcpu
   to finish migration.
4. guest write config data(0x0A) fail to disable the SMRAM becasue of
   config register state lost.
5. guest continue to boot and crash in ipxe option ROM due to SMRAM in
   enabled state.

Signed-off-by: Hogan Wang 

---
 hw/pci-host/i440fx.c   | 11 +++
 hw/pci-host/q35.c  | 11 +++
 hw/pci/pci_host.c  | 11 +++
 hw/pci/pcie_host.c | 11 +++
 include/hw/pci/pci_host.h  | 10 ++
 include/hw/pci/pcie_host.h | 10 ++
 6 files changed, 64 insertions(+)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 8ed2417f0c..17705bb025 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
 }
 };
 
+static const VMStateDescription vmstate_i440fx_pcihost = {
+.name = "I440FX_PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_HOST(parent_obj, I440FXState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
   const char *name, void *opaque,
   Error **errp)
@@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
void *data)
 hc->root_bus_path = i440fx_pcihost_root_bus_path;
 dc->realize = i440fx_pcihost_realize;
 dc->fw_name = "pci";
+dc->vmsd = _i440fx_pcihost;
 device_class_set_props(dc, i440fx_props);
 /* Reason: needs to be wired up by pc_init1 */
 dc->user_creatable = false;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b67cb9c29f..5e323be2e3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
Visitor *v,
 visit_type_uint64(v, name, , errp);
 }
 
+static const VMStateDescription vmstate_q35_pcihost = {
+.name = "Q35_PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
+VMSTATE_END_OF_LIST()
+}
+};
+
 /*
  * NOTE: setting defaults for the mch.* fields in this table
  * doesn't work, because mch is a separate QOM object that is
@@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, void 
*data)
 
 hc->root_bus_path = q35_host_root_bus_path;
 dc->realize = q35_host_realize;
+dc->vmsd = _q35_pcihost;
 device_class_set_props(dc, q35_host_props);
 /* Reason: needs to be wired up by pc_q35_init */
 dc->user_creatable = false;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index ce7bcdb1d5..7cdd5a3ea3 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pci_host.h"
 #include "qemu/module.h"
 #include "hw/pci/pci_bus.h"
+#include "migration/vmstate.h"
 #include "trace.h"
 
 /* debug PCI */
@@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
 .endianness = DEVICE_BIG_ENDIAN,
 };
 
+const VMStateDescription vmstate_pcihost = {
+.name = "PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(config_reg, PCIHostState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static const TypeInfo pci_host_type_info = {
 .name = TYPE_PCI_HOST_BRIDGE,
 .parent = TYPE_SYS_BUS_DEVICE,
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index 3534006f99..a653c39bb7 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pcie_host.h"
 #include "qemu/module.h"
 #include "exec/address-spaces.h"
+#include "migration/vmstate.h"
 
 /* a helper function to get a PCIDevice for a given mmconfig address */
 static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
@@ -121,6 +122,16 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
 memory_region_transaction_commit();
 }
 
+const VMStateDescription vmstate_pciehost = {
+.name = "PCIEHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_HOST(pci, PCIExpressHost),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static const TypeInfo pcie_host_type_info = {
 .name = TYPE_PCIE_HOST_BRIDGE,
 .parent = TYPE_PCI_HOST_BRIDGE,
diff --git a/include/hw/pci/pci_host.h 

Re: [PATCH 3/4] tests/acceptance: Disable the rx sash and arm cubieboard replay test on Gitlab

[Philippe Mathieu-Daudé]

On 7/23/20 2:27 PM, Thomas Huth wrote:
> These tests always time out on Gitlab, not sure what's happening here.
> Let's disable them until somebody has enough spare time to debug the
> issues.

Is the Avocado cache working? Is it failing with an empty cache?
If so, maybe we need to run avocado cache fetching in a previous
step, before the testing job.

Can you share a url of failed job so I can have a look?

> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/acceptance/machine_rx_gdbsim.py | 4 
>  tests/acceptance/replay_kernel.py | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/tests/acceptance/machine_rx_gdbsim.py 
> b/tests/acceptance/machine_rx_gdbsim.py
> index bff63e421d..0c72506028 100644
> --- a/tests/acceptance/machine_rx_gdbsim.py
> +++ b/tests/acceptance/machine_rx_gdbsim.py
> @@ -8,6 +8,9 @@
>  # This work is licensed under the terms of the GNU GPL, version 2 or
>  # later.  See the COPYING file in the top-level directory.
>  
> +import os
> +
> +from avocado import skipIf
>  from avocado_qemu import Test
>  from avocado_qemu import exec_command_and_wait_for_pattern
>  from avocado_qemu import wait_for_console_pattern
> @@ -42,6 +45,7 @@ class RxGdbSimMachine(Test):
>  # FIXME limit baudrate on chardev, else we type too fast
>  #exec_command_and_wait_for_pattern(self, 'version', gcc_version)
>  
> +@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>  def test_linux_sash(self):
>  """
>  Boots a Linux kernel and checks that the console is operational.
> diff --git a/tests/acceptance/replay_kernel.py 
> b/tests/acceptance/replay_kernel.py
> index 62d2db8c64..b79fc8daf8 100644
> --- a/tests/acceptance/replay_kernel.py
> +++ b/tests/acceptance/replay_kernel.py
> @@ -126,6 +126,7 @@ class ReplayKernel(LinuxKernelTest):
>  
>  self.run_rr(kernel_path, kernel_command_line, console_pattern, 
> shift=1)
>  
> +@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>  def test_arm_cubieboard_initrd(self):
>  """
>  :avocado: tags=arch:arm
> 

Re: [PATCH] hw/pci-host: save/restore pci host config register

[Michael S. Tsirkin]

On Thu, Jul 23, 2020 at 01:48:47PM +0200, Laszlo Ersek wrote:
> On 07/23/20 12:49, Wang King wrote:
> > From: Hogan Wang 
> > 
> > The pci host config register is used to save PCI address for
> > read/write config data. If guest write a value to config register,
> > and then pause the vcpu to migrate, After the migration, the guest
> > continue to write pci config data, and the write data will be ignored
> > because of new qemu process lost the config register state.
> > 
> > Example:
> > 1. guest booting in seabios.
> > 2. guest enabled the SMM memory window in piix4_apmc_smm_setup, and
> > then try to close the SMM memory window.
> > 3. pasued vcpu to finish migration.
> > 4. guest close the SMM memory window fail becasue of config register
> > state lost.
> > 5. guest continue to boot and crash in ipxe option ROM (SMM memory
> > window is enabled).
> > 
> > Due to the complex guest, the negative effect is unpredictable.

Is there a way to build a unit test for this btw?
That would be great ...


> > ---
> >  hw/pci-host/i440fx.c   | 11 +++
> >  hw/pci-host/q35.c  | 11 +++
> >  hw/pci/pci_host.c  | 11 +++
> >  hw/pci/pcie_host.c | 11 +++
> >  include/hw/pci/pci_host.h  | 10 ++
> >  include/hw/pci/pcie_host.h | 10 ++
> >  6 files changed, 64 insertions(+)
> > 
> > diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> > index 8ed2417f0c..17705bb025 100644
> > --- a/hw/pci-host/i440fx.c
> > +++ b/hw/pci-host/i440fx.c
> > @@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
> >  }
> >  };
> >  
> > +static const VMStateDescription vmstate_i440fx_pcihost = {
> > +.name = "I440FX_PCIHost",
> > +.version_id = 1,
> > +.minimum_version_id = 1,
> > +.fields = (VMStateField[]) {
> > +VMSTATE_PCI_HOST(parent_obj, I440FXState),
> > +VMSTATE_END_OF_LIST()
> > +}
> > +};
> > +
> >  static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
> >const char *name, void 
> > *opaque,
> >Error **errp)
> > @@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass 
> > *klass, void *data)
> >  hc->root_bus_path = i440fx_pcihost_root_bus_path;
> >  dc->realize = i440fx_pcihost_realize;
> >  dc->fw_name = "pci";
> > +dc->vmsd = _i440fx_pcihost;
> >  device_class_set_props(dc, i440fx_props);
> >  /* Reason: needs to be wired up by pc_init1 */
> >  dc->user_creatable = false;
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index b67cb9c29f..5e323be2e3 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
> > Visitor *v,
> >  visit_type_uint64(v, name, , errp);
> >  }
> >  
> > +static const VMStateDescription vmstate_q35_pcihost = {
> > +.name = "Q35_PCIHost",
> > +.version_id = 1,
> > +.minimum_version_id = 1,
> > +.fields = (VMStateField[]) {
> > +VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
> > +VMSTATE_END_OF_LIST()
> > +}
> > +};
> > +
> >  /*
> >   * NOTE: setting defaults for the mch.* fields in this table
> >   * doesn't work, because mch is a separate QOM object that is
> > @@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, 
> > void *data)
> >  
> >  hc->root_bus_path = q35_host_root_bus_path;
> >  dc->realize = q35_host_realize;
> > +dc->vmsd = _q35_pcihost;
> >  device_class_set_props(dc, q35_host_props);
> >  /* Reason: needs to be wired up by pc_q35_init */
> >  dc->user_creatable = false;
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index ce7bcdb1d5..7cdd5a3ea3 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/pci/pci_host.h"
> >  #include "qemu/module.h"
> >  #include "hw/pci/pci_bus.h"
> > +#include "migration/vmstate.h"
> >  #include "trace.h"
> >  
> >  /* debug PCI */
> > @@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
> >  .endianness = DEVICE_BIG_ENDIAN,
> >  };
> >  
> > +const VMStateDescription vmstate_pcihost = {
> > +.name = "PCIHost",
> > +.version_id = 1,
> > +.minimum_version_id = 1,
> > +.fields = (VMStateField[]) {
> > +VMSTATE_UINT32(config_reg, PCIHostState),
> > +VMSTATE_END_OF_LIST()
> > +}
> > +};
> > +
> >  static const TypeInfo pci_host_type_info = {
> >  .name = TYPE_PCI_HOST_BRIDGE,
> >  .parent = TYPE_SYS_BUS_DEVICE,
> > diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> > index 3534006f99..a653c39bb7 100644
> > --- a/hw/pci/pcie_host.c
> > +++ b/hw/pci/pcie_host.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/pci/pcie_host.h"
> >  #include "qemu/module.h"
> >  #include "exec/address-spaces.h"
> > +#include "migration/vmstate.h"
> >  
> >  /* a helper function to get a PCIDevice for a given 

Re: [PATCH] qapi: enable use of g_autoptr with QAPI types

[Daniel P . Berrangé]

On Thu, Jul 23, 2020 at 02:50:51PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Thu, Jul 23, 2020 at 06:49:44AM -0500, Eric Blake wrote:
> >> On 7/23/20 6:12 AM, Daniel P. Berrangé wrote:
> >> > Currently QAPI generates a type and function for free'ing it:
> >> > 
> >> >typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
> >> >void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions 
> >> > *obj);
> >> > 
> >> 
> >> > The above code example now becomes
> >> > 
> >> >g_autoptr(QCryptoBlockCreateOptions) opts = NULL;
> >> > 
> >> >opts = g_new0(QCryptoBlockCreateOptions, 1);
> >> > 
> >> >do stuff with opts...
> >> > 
> >> > Note, if the local pointer needs to live beyond the scope holding the
> >> > variable, then g_steal_pointer can be used. This is useful to return the
> >> > pointer to the caller in the success codepath, while letting it be freed
> >> > in all error codepaths.
> >> > 
> >> >return g_steal_pointer();
> >> > 
> >> 
> >> Yep, the idea makes sense!
> 
> Agree.
> 
> >> > Signed-off-by: Daniel P. Berrangé 
> >> > ---
> >> >   include/crypto/block.h | 2 --
> >> >   scripts/qapi/types.py  | 1 +
> >> >   2 files changed, 1 insertion(+), 2 deletions(-)
> >> 
> >> Missing a counterpart change to docs/devel/qapi-code-gen.txt.
> 
> Yes.  Can do that in my tree.
> 
> >>And it might
> >> be nice to make this a series with at least one followup patch using the 
> >> new
> >> capability, or at least so 'make check' coverage.  But otherwise on the
> >> right track.
> >
> > The crypto/block.c already makes use of this capability, which is why
> > I had to remove the line from block.h to avoid declaring the same thing
> > twice !
> 
> Could be mentioned in the commit message.
> 
> Still, using it somewhere in tests would be nice.
> test-qobject-input-visitor.c's test_visitor_in_struct_nested() looks
> trivial to convert.  Feel free to pick something else.

Ok, I'll convert some.

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 3/4] tests/acceptance: Disable the rx sash and arm cubieboard replay test on Gitlab

[Thomas Huth]

On 23/07/2020 14.53, Philippe Mathieu-Daudé wrote:
> On 7/23/20 2:27 PM, Thomas Huth wrote:
>> These tests always time out on Gitlab, not sure what's happening here.
>> Let's disable them until somebody has enough spare time to debug the
>> issues.
> 
> Is the Avocado cache working? Is it failing with an empty cache?
> If so, maybe we need to run avocado cache fetching in a previous
> step, before the testing job.
> 
> Can you share a url of failed job so I can have a look?

CentOS:
 https://gitlab.com/huth/qemu/-/jobs/651179303

Debian:
 https://gitlab.com/huth/qemu/-/jobs/651179296

... might be a cache issue, indeed.

 Thomas

Re: [PATCH v2 0/2] linux-user: fix clock_nanosleep()

[Alex Bennée]

Laurent Vivier  writes:

> Update the "remain" time only if errno is EINTR and flags is TIMER_ABSTIME.
>
> The v2 restores the get_errno() as our safe_clock_nanosleep() uses
> errno to return the error value (and not ret).
>
> As we use errno, we don't need the special case for ppc here, the CRF
> bit is correctly managed in cpu_loop.c if ret is -errno.
>
> Laurent Vivier (2):
>   linux-user: fix clock_nanosleep()
>   linux-user,ppc: fix clock_nanosleep() for linux-user-ppc
>
>  linux-user/syscall.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)

Queued to for-5.1/fixes-for-rc1-v3, thanks.

-- 
Alex Bennée

[PATCH] target/unicore32: Remove CURSES stuff from the Makefile.objs

[Thomas Huth]

The dependency on curses has been removed in commit c7a856b42e403e2b
("target/unicore32: Prefer qemu_semihosting_log_out() over curses").
So we can remove the related lines in the Makefile now, too.

Signed-off-by: Thomas Huth 
---
 target/unicore32/Makefile.objs | 4 
 1 file changed, 4 deletions(-)

diff --git a/target/unicore32/Makefile.objs b/target/unicore32/Makefile.objs
index 35d8bf530d..6b41b1e9ef 100644
--- a/target/unicore32/Makefile.objs
+++ b/target/unicore32/Makefile.objs
@@ -2,7 +2,3 @@ obj-y += translate.o op_helper.o helper.o cpu.o
 obj-y += ucf64_helper.o
 
 obj-$(CONFIG_SOFTMMU) += softmmu.o
-
-# Huh? Uses curses directly instead of using ui/console.h interfaces ...
-helper.o-cflags := $(CURSES_CFLAGS)
-helper.o-libs := $(CURSES_LIBS)
-- 
2.18.1

Re: [Virtio-fs] [PATCH for-5.1 3/3] virtiofsd: probe unshare(CLONE_FS) and print an error

[Vivek Goyal]

On Thu, Jul 23, 2020 at 01:50:35PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 23, 2020 at 01:46:11PM +0100, Stefan Hajnoczi wrote:
> > On Wed, Jul 22, 2020 at 06:03:28PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Jul 22, 2020 at 02:02:06PM +0100, Stefan Hajnoczi wrote:
> > > > An assertion failure is raised during request processing if
> > > > unshare(CLONE_FS) fails. Implement a probe at startup so the problem can
> > > > be detected right away.
> > > > 
> > > > Unfortunately Docker/Moby does not include unshare in the seccomp.json
> > > > list unless CAP_SYS_ADMIN is given. Other seccomp.json lists always
> > > > include unshare (e.g. podman is unaffected):
> > > > https://raw.githubusercontent.com/seccomp/containers-golang/master/seccomp.json
> > > > 
> > > > Use "docker run --security-opt seccomp=path/to/seccomp.json ..." if the
> > > > default seccomp.json is missing unshare.
> > > > 
> > > > Cc: Misono Tomohiro 
> > > > Signed-off-by: Stefan Hajnoczi 
> > > > ---
> > > >  tools/virtiofsd/fuse_virtio.c | 13 +
> > > >  1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/tools/virtiofsd/fuse_virtio.c 
> > > > b/tools/virtiofsd/fuse_virtio.c
> > > > index 3b6d16a041..ebeb352514 100644
> > > > --- a/tools/virtiofsd/fuse_virtio.c
> > > > +++ b/tools/virtiofsd/fuse_virtio.c
> > > > @@ -949,6 +949,19 @@ int virtio_session_mount(struct fuse_session *se)
> > > >  {
> > > >  int ret;
> > > >  
> > > > +/*
> > > > + * Test that unshare(CLONE_FS) works. fv_queue_worker() will need 
> > > > it. It's
> > > > + * an unprivileged system call but some Docker/Moby versions are 
> > > > known to
> > > > + * reject it via seccomp when CAP_SYS_ADMIN is not given.
> > > > + */
> > > > +ret = unshare(CLONE_FS);
> > > > +if (ret == -1 && errno == EPERM) {
> > > > +fuse_log(FUSE_LOG_ERR, "unshare(CLONE_FS) failed with EPERM. 
> > > > If "
> > > > +"running in a container please check that the 
> > > > container "
> > > > +"runtime seccomp policy allows unshare.\n");
> > > > +return -1;
> > > > +}
> > > > +
> > > 
> > > This describes the unshare() call as a "probe" and a "test", but that's
> > > misleading IMHO. A "probe" / "test" implies that after it has completed,
> > > there's no lingering side-effect, which isn't the case here.
> > > 
> > > This is actively changing the process' namespace environment in the
> > > success case, and not putting it back how it was originally.
> > > 
> > > May be this is in fact OK, but if so I think the commit message and
> > > comment should explain/justify what its fine to have this lingering
> > > side-effect.
> > > 
> > > If we want to avoid the side-effect then we need to fork() and run
> > > unshare() in the child, and use a check of exit status of the child
> > > to determine the result.
> > 
> > Thanks for pointing this out. I'll add a comment explaining that
> > virtiofsd is single-threaded at this point. No other threads share the
> > file system attributes so the call has no observable side-effects.
> 
> Also, if we do an unshare() here, do we still need the unshare() later
> on in the code ?

I think so. That unshare() later is to isolate one thread from other.

Thanks
Vivek

[PATCH] configure: Allow to build tools without pixman

[Thomas Huth]

If pixman is not installed, it is currently not possible to run:

 .../configure  --disable-system --enable-tools

Seems like there was a dependency from one of the required source
files to pixman in the past, but since commit 1ac0206b2ae1ffaeec56
("qemu-timer.c: Trim list of included headers"), this dependency
should be gone. Thus allow to compile the tools without pixman now.

Signed-off-by: Thomas Huth 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 4bd80ed507..2acc4d1465 100755
--- a/configure
+++ b/configure
@@ -4065,7 +4065,7 @@ fi
 ##
 # pixman support probe
 
-if test "$want_tools" = "no" && test "$softmmu" = "no"; then
+if test "$softmmu" = "no"; then
   pixman_cflags=
   pixman_libs=
 elif $pkg_config --atleast-version=0.21.8 pixman-1 > /dev/null 2>&1; then
-- 
2.18.1

[PATCH for-5.1 0/2] tpm: Fix error reporting, improve help

[Markus Armbruster]

The alternative to PATCH 1 is a proper conversion to Error, as
discussed in

Subject: Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device)
Message-ID: <87tuxyoauy@dusky.pond.sub.org>

Such a conversion would be much too invasive for 5.1.  Going the other
way, like PATCH 1 does, is simple enough to be considered for 5.1.

To make this series a complete alternative to Philippe's
"[PATCH-for-5.1 v2 0/2] tpm: Improve error reporting", PATCH 2
improves help.

Markus Armbruster (2):
  Revert "tpm: Clean up error reporting in tpm_init_tpmdev()"
  tpm: Improve help on TPM types when none are available

 include/sysemu/tpm.h |  2 +-
 softmmu/vl.c |  4 +++-
 stubs/tpm.c  |  3 ++-
 tpm.c| 43 ++-
 4 files changed, 36 insertions(+), 16 deletions(-)

-- 
2.26.2

[PATCH for-5.1 2/2] tpm: Improve help on TPM types when none are available

[Markus Armbruster]

Help is a bit awkward when no TPM types are built into QEMU:

$ upstream-qemu -tpmdev nonexistent,id=tpm0
upstream-qemu: -tpmdev nonexistent,id=tpm0: Parameter 'type' expects a TPM 
backend type
Supported TPM types (choose only one):

Improve to

upstream-qemu: -tpmdev nonexistent,id=tpm0: Parameter 'type' expects a TPM 
backend type
No TPM backend types are available

Signed-off-by: Markus Armbruster 
---
 tpm.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tpm.c b/tpm.c
index f6045bb6da..cab206355a 100644
--- a/tpm.c
+++ b/tpm.c
@@ -47,18 +47,23 @@ tpm_be_find_by_type(enum TpmType type)
  */
 static void tpm_display_backend_drivers(void)
 {
+bool got_one = false;
 int i;
 
-fprintf(stderr, "Supported TPM types (choose only one):\n");
-
 for (i = 0; i < TPM_TYPE__MAX; i++) {
 const TPMBackendClass *bc = tpm_be_find_by_type(i);
 if (!bc) {
 continue;
 }
-fprintf(stderr, "%12s   %s\n", TpmType_str(i), bc->desc);
+if (!got_one) {
+error_printf("Supported TPM types (choose only one):\n");
+got_one = true;
+}
+error_printf("%12s   %s\n", TpmType_str(i), bc->desc);
+}
+if (!got_one) {
+error_printf("No TPM backend types are available\n");
 }
-fprintf(stderr, "\n");
 }
 
 /*
-- 
2.26.2

[PATCH v2] hw/pci-host: save/restore pci host config register

[Wang King]

From: Hogan Wang 

The pci host config register is used to save PCI address for
read/write config data. If guest write a value to config register,
and then pause the vcpu to migrate, After the migration, the guest
continue to write pci config data, and the write data will be ignored
because of new qemu process lost the config register state.

Reproduction steps are:
1. guest booting in seabios.
2. guest enable the SMRAM in seabios:piix4_apmc_smm_setup, and then
   expect to disable the SMRAM by pci_config_writeb.
3. after guest write the pci host config register, and then pasued vcpu
   to finish migration.
4. guest write config data(0x0A) fail to disable the SMRAM becasue of
   config register state lost.
5. guest continue to boot and crash in ipxe option ROM due to SMRAM in
   enabled state.

---
 hw/pci-host/i440fx.c   | 11 +++
 hw/pci-host/q35.c  | 11 +++
 hw/pci/pci_host.c  | 11 +++
 hw/pci/pcie_host.c | 11 +++
 include/hw/pci/pci_host.h  | 10 ++
 include/hw/pci/pcie_host.h | 10 ++
 6 files changed, 64 insertions(+)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 8ed2417f0c..17705bb025 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
 }
 };
 
+static const VMStateDescription vmstate_i440fx_pcihost = {
+.name = "I440FX_PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_HOST(parent_obj, I440FXState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
   const char *name, void *opaque,
   Error **errp)
@@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
void *data)
 hc->root_bus_path = i440fx_pcihost_root_bus_path;
 dc->realize = i440fx_pcihost_realize;
 dc->fw_name = "pci";
+dc->vmsd = _i440fx_pcihost;
 device_class_set_props(dc, i440fx_props);
 /* Reason: needs to be wired up by pc_init1 */
 dc->user_creatable = false;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b67cb9c29f..5e323be2e3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
Visitor *v,
 visit_type_uint64(v, name, , errp);
 }
 
+static const VMStateDescription vmstate_q35_pcihost = {
+.name = "Q35_PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
+VMSTATE_END_OF_LIST()
+}
+};
+
 /*
  * NOTE: setting defaults for the mch.* fields in this table
  * doesn't work, because mch is a separate QOM object that is
@@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, void 
*data)
 
 hc->root_bus_path = q35_host_root_bus_path;
 dc->realize = q35_host_realize;
+dc->vmsd = _q35_pcihost;
 device_class_set_props(dc, q35_host_props);
 /* Reason: needs to be wired up by pc_q35_init */
 dc->user_creatable = false;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index ce7bcdb1d5..7cdd5a3ea3 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pci_host.h"
 #include "qemu/module.h"
 #include "hw/pci/pci_bus.h"
+#include "migration/vmstate.h"
 #include "trace.h"
 
 /* debug PCI */
@@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
 .endianness = DEVICE_BIG_ENDIAN,
 };
 
+const VMStateDescription vmstate_pcihost = {
+.name = "PCIHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(config_reg, PCIHostState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static const TypeInfo pci_host_type_info = {
 .name = TYPE_PCI_HOST_BRIDGE,
 .parent = TYPE_SYS_BUS_DEVICE,
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index 3534006f99..a653c39bb7 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pcie_host.h"
 #include "qemu/module.h"
 #include "exec/address-spaces.h"
+#include "migration/vmstate.h"
 
 /* a helper function to get a PCIDevice for a given mmconfig address */
 static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
@@ -121,6 +122,16 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
 memory_region_transaction_commit();
 }
 
+const VMStateDescription vmstate_pciehost = {
+.name = "PCIEHost",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_HOST(pci, PCIExpressHost),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static const TypeInfo pcie_host_type_info = {
 .name = TYPE_PCIE_HOST_BRIDGE,
 .parent = TYPE_PCI_HOST_BRIDGE,
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 

[PATCH 2/4] iotests: Select a default machine for the rx and avr targets

[Thomas Huth]

If you are building only with either the new rx-softmmu or avr-softmmu
target, "make check-block" fails a couple of tests since there is no
default machine defined in these new targets. We have to select a machine
in the "check" script for these, just like we already do for the arm- and
tricore-softmmu targets.

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/check | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index e0d8049012..0657f7286c 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -595,15 +595,19 @@ then
 fi
 export QEMU_PROG="$(type -p "$QEMU_PROG")"
 
+export QEMU_OPTIONS="-nodefaults -display none -accel qtest"
 case "$QEMU_PROG" in
 *qemu-system-arm|*qemu-system-aarch64)
-export QEMU_OPTIONS="-nodefaults -display none -machine virt -accel 
qtest"
+export QEMU_OPTIONS="$QEMU_OPTIONS -machine virt"
 ;;
-*qemu-system-tricore)
-export QEMU_OPTIONS="-nodefaults -display none -machine 
tricore_testboard -accel qtest"
+*qemu-system-avr)
+export QEMU_OPTIONS="$QEMU_OPTIONS -machine mega2560"
+;;
+*qemu-system-rx)
+export QEMU_OPTIONS="$QEMU_OPTIONS -machine gdbsim-r5f562n8"
 ;;
-*)
-export QEMU_OPTIONS="-nodefaults -display none -accel qtest"
+*qemu-system-tricore)
+export QEMU_OPTIONS="-$QEMU_OPTIONS -machine tricore_testboard"
 ;;
 esac
 
-- 
2.18.1

[PATCH 0/4] Test more in less time in the Gitlab-CI

[Thomas Huth]

This patch series adds two new "build-system" pipelines to the Gitlab-CI,
one based on Debian and one on CentOS. We then use these build pipelines
to test the targets that were missing so far (e.g. the two new targets
rx-softmmu and avr-softmmu), and move some of the targets from the other
build-system pipelines here, too, so that the total testing time gets
shorter (at least 5 minutes from what I've seen so far).

Thomas Huth (4):
  tests/docker: Add python3-venv and netcat to the debian-amd64
container
  iotests: Select a default machine for the rx and avr targets
  tests/acceptance: Disable the rx sash and arm cubieboard replay test
on Gitlab
  gitlab-ci.yml: Add build-system-debian and build-system-centos jobs

 .gitlab-ci.yml   | 88 
 tests/acceptance/machine_rx_gdbsim.py|  4 +
 tests/acceptance/replay_kernel.py|  1 +
 tests/docker/dockerfiles/debian-amd64.docker |  4 +-
 tests/qemu-iotests/check | 14 ++--
 5 files changed, 90 insertions(+), 21 deletions(-)

-- 
2.18.1

[PATCH 1/4] tests/docker: Add python3-venv and netcat to the debian-amd64 container

[Thomas Huth]

Without python3-venv, I get the following message when trying to
run the acceptance tests within the debian container:

 The virtual environment was not created successfully because ensurepip is not
 available.  On Debian/Ubuntu systems, you need to install the python3-venv
 package using the following command.
apt-get install python3-venv
 You may need to use sudo with that command.  After installing the python3-venv
 package, recreate your virtual environment.

Let's do it as the message suggests.

And while we're at it, also add netcat here since it is required for
some of the acceptance tests.

Signed-off-by: Thomas Huth 
---
 tests/docker/dockerfiles/debian-amd64.docker | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/debian-amd64.docker 
b/tests/docker/dockerfiles/debian-amd64.docker
index 8fdfd6a6b0..d2500dcff1 100644
--- a/tests/docker/dockerfiles/debian-amd64.docker
+++ b/tests/docker/dockerfiles/debian-amd64.docker
@@ -20,7 +20,9 @@ RUN apt update && \
 librdmacm-dev \
 libsasl2-dev \
 libsnappy-dev \
-libvte-dev
+libvte-dev \
+netcat-openbsd \
+python3-venv
 
 # virgl
 RUN apt update && \
-- 
2.18.1

Re: [PATCH 3/4] tests/acceptance: Disable the rx sash and arm cubieboard replay test on Gitlab

[Wainer dos Santos Moschetta]

On 7/23/20 9:27 AM, Thomas Huth wrote:

These tests always time out on Gitlab, not sure what's happening here.
Let's disable them until somebody has enough spare time to debug the
issues.


It's fair to me.



Signed-off-by: Thomas Huth 
---
  tests/acceptance/machine_rx_gdbsim.py | 4 
  tests/acceptance/replay_kernel.py | 1 +
  2 files changed, 5 insertions(+)



Reviewed-by: Wainer dos Santos Moschetta 




diff --git a/tests/acceptance/machine_rx_gdbsim.py 
b/tests/acceptance/machine_rx_gdbsim.py
index bff63e421d..0c72506028 100644
--- a/tests/acceptance/machine_rx_gdbsim.py
+++ b/tests/acceptance/machine_rx_gdbsim.py
@@ -8,6 +8,9 @@
  # This work is licensed under the terms of the GNU GPL, version 2 or
  # later.  See the COPYING file in the top-level directory.
  
+import os

+
+from avocado import skipIf
  from avocado_qemu import Test
  from avocado_qemu import exec_command_and_wait_for_pattern
  from avocado_qemu import wait_for_console_pattern
@@ -42,6 +45,7 @@ class RxGdbSimMachine(Test):
  # FIXME limit baudrate on chardev, else we type too fast
  #exec_command_and_wait_for_pattern(self, 'version', gcc_version)
  
+@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')

  def test_linux_sash(self):
  """
  Boots a Linux kernel and checks that the console is operational.
diff --git a/tests/acceptance/replay_kernel.py 
b/tests/acceptance/replay_kernel.py
index 62d2db8c64..b79fc8daf8 100644
--- a/tests/acceptance/replay_kernel.py
+++ b/tests/acceptance/replay_kernel.py
@@ -126,6 +126,7 @@ class ReplayKernel(LinuxKernelTest):
  
  self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=1)
  
+@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')

  def test_arm_cubieboard_initrd(self):
  """
  :avocado: tags=arch:arm

Re: [PATCH] hw/pci-host: save/restore pci host config register

[Michael S. Tsirkin]

On Thu, Jul 23, 2020 at 06:49:35PM +0800, Wang King wrote:
> From: Hogan Wang 
> 
> The pci host config register is used to save PCI address for
> read/write config data. If guest write a value to config register,
> and then pause the vcpu to migrate, After the migration, the guest
> continue to write pci config data, and the write data will be ignored
> because of new qemu process lost the config register state.

Wow I can't believe we have such a bug after so many years.
Question is, this will break cross-version migration if we just add it.
Could we use some trick so people can upgrade in the field
without breaking migration?

I regret we still don't have an extensible format where we could
add fields without breaking everything ...
CC Julua, Dgilbert to take a look.

> Example:
> 1. guest booting in seabios.
> 2. guest enabled the SMM memory window in piix4_apmc_smm_setup, and
> then try to close the SMM memory window.
> 3. pasued vcpu to finish migration.
> 4. guest close the SMM memory window fail becasue of config register
> state lost.
> 5. guest continue to boot and crash in ipxe option ROM (SMM memory
> window is enabled).
> 
> Due to the complex guest, the negative effect is unpredictable.

Could we get a sign-off please?

The sign-off is a simple line at the end of the explanation for the
patch, which certifies that you wrote it or otherwise have the right to
pass it on as an open-source patch.  The rules are pretty simple: if you
can certify the below:

Developer's Certificate of Origin 1.1
^

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

then you just add a line saying::

Signed-off-by: Random J Developer 

using your real name (sorry, no pseudonyms or anonymous contributions.)
   

> ---
>  hw/pci-host/i440fx.c   | 11 +++
>  hw/pci-host/q35.c  | 11 +++
>  hw/pci/pci_host.c  | 11 +++
>  hw/pci/pcie_host.c | 11 +++
>  include/hw/pci/pci_host.h  | 10 ++
>  include/hw/pci/pcie_host.h | 10 ++
>  6 files changed, 64 insertions(+)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 8ed2417f0c..17705bb025 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
>  }
>  };
>  
> +static const VMStateDescription vmstate_i440fx_pcihost = {
> +.name = "I440FX_PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_PCI_HOST(parent_obj, I440FXState),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
>const char *name, void *opaque,
>Error **errp)
> @@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
> void *data)
>  hc->root_bus_path = i440fx_pcihost_root_bus_path;
>  dc->realize = i440fx_pcihost_realize;
>  dc->fw_name = "pci";
> +dc->vmsd = _i440fx_pcihost;
>  device_class_set_props(dc, i440fx_props);
>  /* Reason: needs to be wired up by pc_init1 */
>  dc->user_creatable = false;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b67cb9c29f..5e323be2e3 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
> Visitor *v,
>  visit_type_uint64(v, name, , errp);
>  }
>  
> +static const VMStateDescription vmstate_q35_pcihost = {
> +.name = "Q35_PCIHost",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
> +

Re: [PATCH for-5.1 3/3] virtiofsd: probe unshare(CLONE_FS) and print an error

[Daniel P . Berrangé]

On Thu, Jul 23, 2020 at 01:46:11PM +0100, Stefan Hajnoczi wrote:
> On Wed, Jul 22, 2020 at 06:03:28PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 22, 2020 at 02:02:06PM +0100, Stefan Hajnoczi wrote:
> > > An assertion failure is raised during request processing if
> > > unshare(CLONE_FS) fails. Implement a probe at startup so the problem can
> > > be detected right away.
> > > 
> > > Unfortunately Docker/Moby does not include unshare in the seccomp.json
> > > list unless CAP_SYS_ADMIN is given. Other seccomp.json lists always
> > > include unshare (e.g. podman is unaffected):
> > > https://raw.githubusercontent.com/seccomp/containers-golang/master/seccomp.json
> > > 
> > > Use "docker run --security-opt seccomp=path/to/seccomp.json ..." if the
> > > default seccomp.json is missing unshare.
> > > 
> > > Cc: Misono Tomohiro 
> > > Signed-off-by: Stefan Hajnoczi 
> > > ---
> > >  tools/virtiofsd/fuse_virtio.c | 13 +
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> > > index 3b6d16a041..ebeb352514 100644
> > > --- a/tools/virtiofsd/fuse_virtio.c
> > > +++ b/tools/virtiofsd/fuse_virtio.c
> > > @@ -949,6 +949,19 @@ int virtio_session_mount(struct fuse_session *se)
> > >  {
> > >  int ret;
> > >  
> > > +/*
> > > + * Test that unshare(CLONE_FS) works. fv_queue_worker() will need 
> > > it. It's
> > > + * an unprivileged system call but some Docker/Moby versions are 
> > > known to
> > > + * reject it via seccomp when CAP_SYS_ADMIN is not given.
> > > + */
> > > +ret = unshare(CLONE_FS);
> > > +if (ret == -1 && errno == EPERM) {
> > > +fuse_log(FUSE_LOG_ERR, "unshare(CLONE_FS) failed with EPERM. If "
> > > +"running in a container please check that the container "
> > > +"runtime seccomp policy allows unshare.\n");
> > > +return -1;
> > > +}
> > > +
> > 
> > This describes the unshare() call as a "probe" and a "test", but that's
> > misleading IMHO. A "probe" / "test" implies that after it has completed,
> > there's no lingering side-effect, which isn't the case here.
> > 
> > This is actively changing the process' namespace environment in the
> > success case, and not putting it back how it was originally.
> > 
> > May be this is in fact OK, but if so I think the commit message and
> > comment should explain/justify what its fine to have this lingering
> > side-effect.
> > 
> > If we want to avoid the side-effect then we need to fork() and run
> > unshare() in the child, and use a check of exit status of the child
> > to determine the result.
> 
> Thanks for pointing this out. I'll add a comment explaining that
> virtiofsd is single-threaded at this point. No other threads share the
> file system attributes so the call has no observable side-effects.

Also, if we do an unshare() here, do we still need the unshare() later
on in the code ?


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: 5.1.0-rc1 regression: reset fails with kvm and -cpu host

[Dr. David Alan Gilbert]

* Vitaly Kuznetsov (vkuzn...@redhat.com) wrote:
> Philippe Mathieu-Daudé  writes:
> 
> > +Vitaly
> >
> > On 7/23/20 10:40 AM, Dr. David Alan Gilbert wrote:
> >> * Eduardo Habkost (ehabk...@redhat.com) wrote:
> >>> On Wed, Jul 22, 2020 at 04:47:32PM -0400, Eduardo Habkost wrote:
>  On Wed, Jul 22, 2020 at 08:05:01PM +0200, Jan Kiszka wrote:
> > On 22.07.20 19:35, Eduardo Habkost wrote:
> >> Hi Jan,
> >>
> >> What was the last version where it worked for you?  Does using
> >> "-cpu host,-vmx" help?
> >
> > Yeah, -vmx does indeed help.
> >
> > I didn't have the time to bisect yet. Just check my reflog, picked
> > eb6490f544, and that works.
> 
>  Thanks!
> 
>  I could reproduce it locally[1], I will bisect it.
> 
>  The good news is that "-cpu host,+vmx" still works, on commit
>  eb6490f544.
> 
>  [1] Linux 5.6.19-300.fc32.x86_64, Intel Core i7-8665U CPU.
> >>>
> >>> Bisected to:
> >>>
> >>> commit b16c0e20c74218f2d69710cedad11da7dd4d2190
> >>> Author: Paolo Bonzini 
> >>> Date:   Wed May 20 10:49:22 2020 -0400
> >>>
> >>> KVM: add support for AMD nested live migration
> >>>
> >>> Support for nested guest live migration is part of Linux 5.8, add the
> >>> corresponding code to QEMU.  The migration format consists of a few
> >>> flags, is an opaque 4k blob.
> >>>
> >>> The blob is in VMCB format (the control area represents the L1 VMCB
> >>> control fields, the save area represents the pre-vmentry state; KVM 
> >>> does
> >>> not use the host save area since the AMD manual allows that) but QEMU
> >>> does not really care about that.  However, the flags need to be
> >>> copied to hflags/hflags2 and back.
> >>>
> >>> In addition, support for retrieving and setting the AMD nested 
> >>> virtualization
> >>> states allows the L1 guest to be reset while running a nested guest, 
> >>> but
> >>> a small bug in CPU reset needs to be fixed for that to work.
> >>>
> >>> Signed-off-by: Paolo Bonzini 
> >> 
> >> Guesswork led me to try reverting the chunk in kvm_put_nested_state;
> >> without it the reset seems to work; I can't explain that code though.
> >> 
> 
> (sorry, missed the beginning of the discussion)
> 
> So one does:
> 
> (qemu) system_reset 
> 
> on Intel wiht '-cpu host' and the result is:
> 
> (qemu) KVM: entry failed, hardware error 0x8021

Interesting; I hadn't seen that error - I just see a hard hung guest
rather than a reset one.

(i7-8650U laptop 5.7.9 fedora 32)

Dave

> If you're running a guest on an Intel machine without unrestricted mode
> support, the failure can be most likely due to the guest entering an invalid
> state for Intel VT. For example, the guest maybe running in big real mode
> which is not supported on less recent Intel processors.
> 
> EAX=0064 EBX=91df5efe ECX= EDX=03f8
> ESI= EDI=91ee32c0 EBP=90643260 ESP=00013c68
> EIP=906428e6 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =   9300
> CS =f000   9b00
> SS =   9300
> DS =   9300
> FS =   9300
> GS =   9300
> LDT=   8200
> TR =   8b00
> GDT=  
> IDT=  
> CR0=6010 CR2= CR3= CR4=
> DR0= DR1= DR2= 
> DR3= 
> DR6=0ff0 DR7=0400
> EFER=
> Code=?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??  ?? ?? 
> ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? 
> ??
> 
> I can take a look (if no one beats me to it).
> 
> -- 
> Vitaly
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH] qapi: enable use of g_autoptr with QAPI types

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Thu, Jul 23, 2020 at 06:49:44AM -0500, Eric Blake wrote:
>> On 7/23/20 6:12 AM, Daniel P. Berrangé wrote:
>> > Currently QAPI generates a type and function for free'ing it:
>> > 
>> >typedef struct QCryptoBlockCreateOptions QCryptoBlockCreateOptions;
>> >void qapi_free_QCryptoBlockCreateOptions(QCryptoBlockCreateOptions 
>> > *obj);
>> > 
>> 
>> > The above code example now becomes
>> > 
>> >g_autoptr(QCryptoBlockCreateOptions) opts = NULL;
>> > 
>> >opts = g_new0(QCryptoBlockCreateOptions, 1);
>> > 
>> >do stuff with opts...
>> > 
>> > Note, if the local pointer needs to live beyond the scope holding the
>> > variable, then g_steal_pointer can be used. This is useful to return the
>> > pointer to the caller in the success codepath, while letting it be freed
>> > in all error codepaths.
>> > 
>> >return g_steal_pointer();
>> > 
>> 
>> Yep, the idea makes sense!

Agree.

>> > Signed-off-by: Daniel P. Berrangé 
>> > ---
>> >   include/crypto/block.h | 2 --
>> >   scripts/qapi/types.py  | 1 +
>> >   2 files changed, 1 insertion(+), 2 deletions(-)
>> 
>> Missing a counterpart change to docs/devel/qapi-code-gen.txt.

Yes.  Can do that in my tree.

>>And it might
>> be nice to make this a series with at least one followup patch using the new
>> capability, or at least so 'make check' coverage.  But otherwise on the
>> right track.
>
> The crypto/block.c already makes use of this capability, which is why
> I had to remove the line from block.h to avoid declaring the same thing
> twice !

Could be mentioned in the commit message.

Still, using it somewhere in tests would be nice.
test-qobject-input-visitor.c's test_visitor_in_struct_nested() looks
trivial to convert.  Feel free to pick something else.

In case you prefer not to, with a qapi-code-gen.txt update:
Reviewed-by: Markus Armbruster