Re: [Qemu-devel] [PULL 03/13] target-ppc: Use 32-bit rotate instead of deposit + 64-bit rotate

[David Gibson]

On Wed, Jun 15, 2016 at 10:17:19PM +1000, Anton Blanchard wrote:
> Hi,
> 
> > From: Richard Henderson 
> > 
> > A 32-bit rotate insn is more common on hosts than a deposit insn,
> > and if the host has neither the result is truely horrific.
> > 
> > At the same time, tidy up the temporaries within these functions,
> > drop the over-use of "likely", drop some checks for identity that
> > will also be checked by tcg-op.c functions, and special case mask
> > without rotate within rlwinm.
> 
> This breaks masks that wrap:
> 
> li  r3,-1
> li  r4,-1
> rlwnm   r3,r3,r4,22,8
> 
> We expect:
> 
> ff8003ff
> 
> But get:
> 
> ff8003ff
> 
> Anton

Bother.  I've tentatively put a revert into ppc-for-2.7.  Richard, do
you have a better idea how to fix it?

> 
> > Signed-off-by: Richard Henderson 
> > Signed-off-by: David Gibson 
> > ---
> >  target-ppc/translate.c | 172
> > - 1 file changed, 70
> > insertions(+), 102 deletions(-)
> > 
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index 3ea6625..b392ecc 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -1610,141 +1610,109 @@ static void gen_cntlzd(DisasContext *ctx)
> >  /* rlwimi & rlwimi. */
> >  static void gen_rlwimi(DisasContext *ctx)
> >  {
> > -uint32_t mb, me, sh;
> > -
> > -mb = MB(ctx->opcode);
> > -me = ME(ctx->opcode);
> > -sh = SH(ctx->opcode);
> > -if (likely(sh == (31-me) && mb <= me)) {
> > -tcg_gen_deposit_tl(cpu_gpr[rA(ctx->opcode)],
> > cpu_gpr[rA(ctx->opcode)],
> > -   cpu_gpr[rS(ctx->opcode)], sh, me - mb +
> > 1);
> > +TCGv t_ra = cpu_gpr[rA(ctx->opcode)];
> > +TCGv t_rs = cpu_gpr[rS(ctx->opcode)];
> > +uint32_t sh = SH(ctx->opcode);
> > +uint32_t mb = MB(ctx->opcode);
> > +uint32_t me = ME(ctx->opcode);
> > +
> > +if (sh == (31-me) && mb <= me) {
> > +tcg_gen_deposit_tl(t_ra, t_ra, t_rs, sh, me - mb + 1);
> >  } else {
> >  target_ulong mask;
> > +TCGv_i32 t0;
> >  TCGv t1;
> > -TCGv t0 = tcg_temp_new();
> > -#if defined(TARGET_PPC64)
> > -tcg_gen_deposit_i64(t0, cpu_gpr[rS(ctx->opcode)],
> > -cpu_gpr[rS(ctx->opcode)], 32, 32);
> > -tcg_gen_rotli_i64(t0, t0, sh);
> > -#else
> > -tcg_gen_rotli_i32(t0, cpu_gpr[rS(ctx->opcode)], sh);
> > -#endif
> > +
> >  #if defined(TARGET_PPC64)
> >  mb += 32;
> >  me += 32;
> >  #endif
> >  mask = MASK(mb, me);
> > +
> > +t0 = tcg_temp_new_i32();
> >  t1 = tcg_temp_new();
> > -tcg_gen_andi_tl(t0, t0, mask);
> > -tcg_gen_andi_tl(t1, cpu_gpr[rA(ctx->opcode)], ~mask);
> > -tcg_gen_or_tl(cpu_gpr[rA(ctx->opcode)], t0, t1);
> > -tcg_temp_free(t0);
> > +tcg_gen_trunc_tl_i32(t0, t_rs);
> > +tcg_gen_rotli_i32(t0, t0, sh);
> > +tcg_gen_extu_i32_tl(t1, t0);
> > +tcg_temp_free_i32(t0);
> > +
> > +tcg_gen_andi_tl(t1, t1, mask);
> > +tcg_gen_andi_tl(t_ra, t_ra, ~mask);
> > +tcg_gen_or_tl(t_ra, t_ra, t1);
> >  tcg_temp_free(t1);
> >  }
> > -if (unlikely(Rc(ctx->opcode) != 0))
> > -gen_set_Rc0(ctx, cpu_gpr[rA(ctx->opcode)]);
> > +if (unlikely(Rc(ctx->opcode) != 0)) {
> > +gen_set_Rc0(ctx, t_ra);
> > +}
> >  }
> >  
> >  /* rlwinm & rlwinm. */
> >  static void gen_rlwinm(DisasContext *ctx)
> >  {
> > -uint32_t mb, me, sh;
> > -
> > -sh = SH(ctx->opcode);
> > -mb = MB(ctx->opcode);
> > -me = ME(ctx->opcode);
> > +TCGv t_ra = cpu_gpr[rA(ctx->opcode)];
> > +TCGv t_rs = cpu_gpr[rS(ctx->opcode)];
> > +uint32_t sh = SH(ctx->opcode);
> > +uint32_t mb = MB(ctx->opcode);
> > +uint32_t me = ME(ctx->opcode);
> >  
> > -if (likely(mb == 0 && me == (31 - sh))) {
> > -if (likely(sh == 0)) {
> > -tcg_gen_ext32u_tl(cpu_gpr[rA(ctx->opcode)],
> > cpu_gpr[rS(ctx->opcode)]);
> > -} else {
> > -TCGv t0 = tcg_temp_new();
> > -tcg_gen_ext32u_tl(t0, cpu_gpr[rS(ctx->opcode)]);
> > -tcg_gen_shli_tl(t0, t0, sh);
> > -tcg_gen_ext32u_tl(cpu_gpr[rA(ctx->opcode)], t0);
> > -tcg_temp_free(t0);
> > -}
> > -} else if (likely(sh != 0 && me == 31 && sh == (32 - mb))) {
> > -TCGv t0 = tcg_temp_new();
> > -tcg_gen_ext32u_tl(t0, cpu_gpr[rS(ctx->opcode)]);
> > -tcg_gen_shri_tl(t0, t0, mb);
> > -tcg_gen_ext32u_tl(cpu_gpr[rA(ctx->opcode)], t0);
> > -tcg_temp_free(t0);
> > -} else if (likely(mb == 0 && me == 31)) {
> > -TCGv_i32 t0 = tcg_temp_new_i32();
> > -tcg_gen_trunc_tl_i32(t0, cpu_gpr[rS(ctx->opcode)]);
> > -tcg_gen_rotli_i32(t0, t0, sh);
> > -tcg_gen_extu_i32_tl(cpu_gpr[rA(ctx->opcode)], t0);
> > -tcg_temp_free_i32(t0);
> > 

Re: [Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into BlockLimit

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> It makes more sense to have ALL block size limit constraints
> in the same struct.  Improve the documentation while at it.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 16/17] block: Split bdrv_merge_limits() from bdrv_refresh_limits()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> The raw block driver was blindly copying all limits from bs->file,
> even though: 1. the main bdrv_refresh_limits() already does this
> for many of gthe limits, and 2. blindly copying from the children

s/gthe/the ?

> can weaken any stricter limits that were already inherited from
> the backing dhain during the main bdrv_refresh_limits().  Also,
> the next patch is about to move .request_alignment into
> BlockLimits, and that is a limit that should NOT be copied from
> other layers in the BDS chain.
> 
> Solve the issue by factoring out a new bdrv_merge_limits(),
> and using that function to properly merge limits when comparing
> two BlockDriverState objects.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 15/17] block: Switch discard length bounds to byte-based

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> Sector-based limits are awkward to think about; in our on-going
> quest to move to byte-based interfaces, convert max_discard and
> discard_alignment.  Rename them, using 'pdiscard' as an aid to
> track which remaining discard interfaces need conversion, and so
> that the compiler will help us catch the change in semantics
> across any rebased code.  In iscsi.c, sector_limits_lun2qemu()
> is no longer needed; and the BlockLimits type is now completely
> byte-based.
> 
> Signed-off-by: Eric Blake 
> 
> ---
> v2: rebase nbd and iscsi limits across earlier improvements
> ---
>  include/block/block_int.h | 21 +++--
>  block/io.c| 16 +---
>  block/iscsi.c | 19 ++-
>  block/nbd.c   |  2 +-
>  qemu-img.c|  3 ++-
>  5 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 2b45d57..0169019 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -324,18 +324,27 @@ struct BlockDriver {
>  };
> 
>  typedef struct BlockLimits {
> -/* maximum number of sectors that can be discarded at once */
> -int max_discard;
> +/* maximum number of bytes that can be discarded at once (since it
> + * is signed, it must be < 2G, if set), should be multiple of
> + * pdiscard_alignment, but need not be power of 2. May be 0 if no
> + * inherent 32-bit limit */
> +int32_t max_pdiscard;

Why not use uint32_t?

> 
> -/* optimal alignment for discard requests in sectors */
> -int64_t discard_alignment;
> +/* optimal alignment for discard requests in bytes, must be power
> + * of 2, less than max_discard if that is set, and multiple of
> + * bs->request_alignment. May be 0 if bs->request_alignment is
> + * good enough */
> +uint32_t pdiscard_alignment;
> 
>  /* maximum number of bytes that can zeroized at once (since it is
> - * signed, it must be < 2G, if set) */
> + * signed, it must be < 2G, if set), should be multiple of
> + * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
>  int32_t max_pwrite_zeroes;
> 
>  /* optimal alignment for write zeroes requests in bytes, must be
> - * power of 2, and less than max_pwrite_zeroes if that is set */
> + * power of 2, less than max_pwrite_zeroes if that is set, and
> + * multiple of bs->request_alignment. May be 0 if
> + * bs->request_alignment is good enough */
>  uint32_t pwrite_zeroes_alignment;
> 
>  /* optimal transfer length in bytes (must be power of 2, and
> diff --git a/block/io.c b/block/io.c
> index 5e38ab4..0b5c40d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2352,19 +2352,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState 
> *bs, int64_t sector_num,
>BDRV_TRACKED_DISCARD);
>  bdrv_set_dirty(bs, sector_num, nb_sectors);
> 
> -max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
> +max_discard = MIN_NON_ZERO(bs->bl.max_pdiscard >> BDRV_SECTOR_BITS,
> +   BDRV_REQUEST_MAX_SECTORS);
>  while (nb_sectors > 0) {
>  int ret;
>  int num = nb_sectors;
> +int discard_alignment = bs->bl.pdiscard_alignment >> 
> BDRV_SECTOR_BITS;
> 
>  /* align request */
> -if (bs->bl.discard_alignment &&
> -num >= bs->bl.discard_alignment &&
> -sector_num % bs->bl.discard_alignment) {
> -if (num > bs->bl.discard_alignment) {
> -num = bs->bl.discard_alignment;
> +if (discard_alignment &&
> +num >= discard_alignment &&
> +sector_num % discard_alignment) {
> +if (num > discard_alignment) {
> +num = discard_alignment;
>  }
> -num -= sector_num % bs->bl.discard_alignment;
> +num -= sector_num % discard_alignment;
>  }
> 
>  /* limit request size */
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 739d5ca..af9babf 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1696,13 +1696,6 @@ static void iscsi_close(BlockDriverState *bs)
>  memset(iscsilun, 0, sizeof(IscsiLun));
>  }
> 
> -static int sector_limits_lun2qemu(int64_t sector, IscsiLun *iscsilun)
> -{
> -int limit = MIN(sector_lun2qemu(sector, iscsilun), INT_MAX / 2 + 1);
> -
> -return limit < BDRV_REQUEST_MAX_SECTORS ? limit : 0;
> -}
> -
>  static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>  /* We don't actually refresh here, but just return data queried in
> @@ -1722,14 +1715,14 @@ static void iscsi_refresh_limits(BlockDriverState 
> *bs, Error **errp)
>  }
> 
>  if (iscsilun->lbp.lbpu) {
> -if (iscsilun->bl.max_unmap < 0x) {
> -bs->bl.max_discard =
> -

Re: [Qemu-devel] [PATCH v2 14/17] block: Switch transfer length bounds to byte-based

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> Sector-based limits are awkward to think about; in our on-going
> quest to move to byte-based interfaces, convert max_transfer_length
> and opt_transfer_length.  Rename them (dropping the _length suffix)
> so that the compiler will help us catch the change in semantics
> across any rebased code, and improve the documentation.  Use unsigned
> values, so that we don't have to worry about negative values and
> so that bit-twiddling is easier; however, we are still constrained
> by 2^31 of signed int in most APIs.
> 
> Signed-off-by: Eric Blake 

Looks good apart from the scsi-generic blocksize calculation, which is not an
issue of this patch.

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 13/17] block: Set default request_alignment during bdrv_refresh_limits()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We want to eventually stick request_alignment alongside other
> BlockLimits, but first, we must ensure it is populated at the
> same time as all other limits, rather than being a special case
> that is set only when a block is first opened.
> 
> Now that all drivers have been updated to supply an override
> of request_alignment during their .bdrv_refresh_limits(), as
> needed, the block layer itself can defer setting the default
> alignment until part of the overall bdrv_refresh_limits().
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 12/17] block: Set request_alignment during .bdrv_refresh_limits()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We want to eventually stick request_alignment alongside other
> BlockLimits, but first, we must ensure it is populated at the
> same time as all other limits, rather than being a special case
> that is set only when a block is first opened.
> 
> Add a .bdrv_refresh_limits() to all four of our legacy devices
> that will always be sector-only (bochs, cloop, dmg, vvfat), in
> spite of their recent conversion to expose a byte interface.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 11/17] raw-win32: Set request_alignment during .bdrv_refresh_limits()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We want to eventually stick request_alignment alongside other
> BlockLimits, but first, we must ensure it is populated at the
> same time as all other limits, rather than being a special case
> that is set only when a block is first opened.
> 
> In this case, raw_probe_alignment() already did what we needed,
> so just fix its signature and wire it in correctly.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 10/17] qcow2: Set request_alignment during .bdrv_refresh_limits()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We want to eventually stick request_alignment alongside other
> BlockLimits, but first, we must ensure it is populated at the
> same time as all other limits, rather than being a special case
> that is set only when a block is first opened.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 09/17] iscsi: Set request_alignment during .bdrv_refresh_limits()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We want to eventually stick request_alignment alongside other
> BlockLimits, but first, we must ensure it is populated at the
> same time as all other limits, rather than being a special case
> that is set only when a block is first opened.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 08/17] blkdebug: Set request_alignment during .bdrv_refresh_limits()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We want to eventually stick request_alignment alongside other
> BlockLimits, but first, we must ensure it is populated at the
> same time as all other limits, rather than being a special case
> that is set only when a block is first opened.
> 
> qemu-iotests 77 is particularly sensitive to the fact that we
> can specify an artificial alignment override in blkdebug, and
> that override must continue to work even when limits are
> refreshed on an already open device.
> 
> A later patch will be altering when bs->request_alignment
> defaults are set, so fall back to sector alignment if we have
> not inherited anything yet.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 07/17] block: Give nonzero result to blk_get_max_transfer_length()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> Making all callers special-case 0 as unlimited is awkward,
> and we DO have a hard maximum of BDRV_REQUEST_MAX_SECTORS given
> our current block layer API limits.
> 
> In the case of scsi, this means that we now always advertise a
> limit to the guest, even in cases where the underlying layers
> previously use 0 for no inherent limit beyond the block layer.
> 
> Signed-off-by: Eric Blake 
> 
> ---
> v2: new patch
> ---
>  block/block-backend.c  |  7 ---
>  hw/block/virtio-blk.c  |  3 +--
>  hw/scsi/scsi-generic.c | 12 ++--
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 34500e6..1fb070b 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1303,15 +1303,16 @@ int blk_get_flags(BlockBackend *blk)
>  }
>  }
> 
> +/* Returns the maximum transfer length, in sectors; guaranteed nonzero */
>  int blk_get_max_transfer_length(BlockBackend *blk)
>  {
>  BlockDriverState *bs = blk_bs(blk);
> +int max = 0;
> 
>  if (bs) {
> -return bs->bl.max_transfer_length;
> -} else {
> -return 0;
> +max = bs->bl.max_transfer_length;
>  }
> +return MIN_NON_ZERO(max, BDRV_REQUEST_MAX_SECTORS);
>  }
> 
>  int blk_get_max_iov(BlockBackend *blk)
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 284e646..1d2792e 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -382,7 +382,7 @@ static int multireq_compare(const void *a, const void *b)
>  void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
>  {
>  int i = 0, start = 0, num_reqs = 0, niov = 0, nb_sectors = 0;
> -int max_xfer_len = 0;
> +int max_xfer_len;
>  int64_t sector_num = 0;
> 
>  if (mrb->num_reqs == 1) {
> @@ -392,7 +392,6 @@ void virtio_blk_submit_multireq(BlockBackend *blk, 
> MultiReqBuffer *mrb)
>  }
> 
>  max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk);
> -max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS);
> 
>  qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs),
>_compare);
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 71372a8..db04a76 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -226,12 +226,12 @@ static void scsi_read_complete(void * opaque, int ret)
>  r->req.cmd.buf[0] == INQUIRY &&
>  r->req.cmd.buf[2] == 0xb0) {
>  uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk);
> -if (max_xfer_len) {
> -stl_be_p(>buf[8], max_xfer_len);
> -/* Also take care of the opt xfer len. */
> -if (ldl_be_p(>buf[12]) > max_xfer_len) {
> -stl_be_p(>buf[12], max_xfer_len);
> -}
> +
> +assert(max_xfer_len);
> +stl_be_p(>buf[8], max_xfer_len);
> +/* Also take care of the opt xfer len. */
> +if (ldl_be_p(>buf[12]) > max_xfer_len) {
> +stl_be_p(>buf[12], max_xfer_len);

Paolo: should we take s->blocksize into account? I missed it when I wrote this
piece of code.

Fam

>  }
>  }
>  scsi_req_data(>req, len);
> -- 
> 2.5.5
> 
> 

Re: [Qemu-devel] [PATCH v2 06/17] iscsi: Advertise realistic limits to block layer

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> The function sector_limits_lun2qemu() returns a value in units of
> the block layer's 512-byte sector, and can be as large as
> 0x4000, which is much larger than the block layer's inherent
> limit of BDRV_REQUEST_MAX_SECTORS.  The block layer already
> handles '0' as a synonym to the inherent limit, and it is nicer
> to return this value than it is to calculate an arbitrary
> maximum, for two reasons: we want to ensure that the block layer
> continues to special-case '0' as 'no limit beyond the inherent
> limits'; and we want to be able to someday expand the block
> layer to allow 64-bit limits, where auditing for uses of
> BDRV_REQUEST_MAX_SECTORS will help us make sure we aren't
> artificially constraining iscsi to old block layer limits.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 05/17] nbd: Advertise realistic limits to block layer

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We were basing the advertisement of maximum discard and transfer
> length off of UINT32_MAX, but since the rest of the block layer
> has signed int limits on a transaction, nothing could ever reach
> that maximum, and we risk overflowing an int once things are
> converted to byte-based rather than sector-based limits.  What's
> more, we DO have a much smaller limit: both the current kernel
> and qemu-nbd have a hard limit of 32M on a read or write
> transaction, and while they may also permit up to a full 32 bits
> on a discard transaction, the upstream NBD protocol is proposing
> wording that without any explicit advertisement otherwise,
> clients should limit ALL requests to the same limits as read and
> write, even though the other requests do not actually require as
> many bytes across the wire.  So the better limit to tell the
> block layer is 32M for both values.
> 
> Signed-off-by: Eric Blake 
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 04/17] nbd: Allow larger requests

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> The NBD layer was breaking up request at a limit of 2040 sectors
> (just under 1M) to cater to old qemu-nbd. But the server limit
> was raised to 32M in commit 2d8214885 to match the kernel, more
> than three years ago; and the upstream NBD Protocol is proposing
> documentation that without any explicit communication to state
> otherwise, a client should be able to safely assume that a 32M
> transaction will work.  It is time to rely on the larger sizing,
> and any downstream distro that cares about maximum
> interoperability to older qemu-nbd servers can just tweak the
> value of #define NBD_MAX_SECTORS.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 03/17] block: Fix harmless off-by-one in bdrv_aligned_preadv()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> If the amount of data to read ends exactly on the total size
> of the bs, then we were wasting time creating a local qiov
> to read the data in preparation for what would normally be
> appending zeroes beyond the end, even though this corner case
> has nothing further to do.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 02/17] block: Document supported flags during bdrv_aligned_preadv()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> We don't pass any flags on to drivers to handle.  Tighten an
> assert to explain why we pass 0 to bdrv_driver_preadv(), and add
> some comments on things to be aware of if we want to turn on
> per-BDS BDRV_REQ_FUA support during reads in the future.  Also,
> document that we may want to consider using unmap during
> copy-on-read operations where the read is all zeroes.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 01/17] block: Tighter assertions on bdrv_aligned_pwritev()

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> For symmetry with bdrv_aligned_preadv(), assert that the caller
> really has aligned things properly. This requires adding an align
> parameter, which is used now only in the new asserts, but will
> come in handy in a later patch that adds auto-fragmentation to the
> max transfer size, since that value need not always be a multiple
> of the alignment, and therefore must be rounded down.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH 1/2] blockdev: Add dynamic generation of module_block.h

[Fam Zheng]

On Wed, 06/15 14:40, Colin Lord wrote:
> From: Marc Mari 
> 
> To simplify the addition of new block modules, add a script that generates
> include/qemu/module_block.h automatically from the modules' source code.
> 
> This script assumes that the QEMU coding style rules are followed.
> 
> Signed-off-by: Marc Marí 
> Signed-off-by: Colin Lord 
> ---
>  .gitignore  |   1 +
>  Makefile|   8 +++
>  scripts/modules/module_block.py | 134 
> 
>  3 files changed, 143 insertions(+)
>  create mode 100644 scripts/modules/module_block.py
> 
> diff --git a/.gitignore b/.gitignore
> index 38ee1c5..06aa064 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -110,3 +110,4 @@ tags
>  TAGS
>  docker-src.*
>  *~
> +/include/qemu/module_block.h
> diff --git a/Makefile b/Makefile
> index ed4032a..8f8b6a2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -76,6 +76,8 @@ GENERATED_HEADERS += trace/generated-ust-provider.h
>  GENERATED_SOURCES += trace/generated-ust.c
>  endif
>  
> +GENERATED_HEADERS += include/qemu/module_block.h
> +
>  # Don't try to regenerate Makefile or configure
>  # We don't generate any of them
>  Makefile: ;
> @@ -352,6 +354,12 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) 
> libqemuutil.a libqemustub.a
>  ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
>   $(call LINK, $^)
>  
> +include/qemu/module_block.h: $(SRC_PATH)/scripts/modules/module_block.py 
> config-host.mak
> + $(call quiet-command,$(PYTHON) \
> +$(SRC_PATH)/scripts/modules/module_block.py \
> + $(SRC_PATH)/"./include/qemu/" $(addprefix $(SRC_PATH)/,$(patsubst 
> %.mo,%.c,$(block-obj-m))), \
> + "  GEN   $@")
> +
>  clean:
>  # avoid old build problems by removing potentially incorrect old files
>   rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h 
> gen-op-arm.h
> diff --git a/scripts/modules/module_block.py b/scripts/modules/module_block.py
> new file mode 100644
> index 000..005bc49
> --- /dev/null
> +++ b/scripts/modules/module_block.py
> @@ -0,0 +1,134 @@
> +#!/usr/bin/python
> +#
> +# Module information generator
> +#
> +# Copyright Red Hat, Inc. 2015
> +#
> +# Authors:
> +#  Marc Mari 

Address hidden seems like a mistake during copy from web. :)

One more below..

> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.
> +# See the COPYING file in the top-level directory.
> +
> +from __future__ import print_function
> +import sys
> +import os
> +
> +def get_string_struct(line):
> +data = line.split()
> +
> +# data[0] -> struct element name
> +# data[1] -> =
> +# data[2] -> value
> +
> +return data[2].replace('"', '')[:-1]
> +
> +def add_module(fhader, library, format_name, protocol_name,
> +probe, probe_device):
> +lines = []
> +lines.append('.library_name = "' + library + '",')
> +if format_name != "":
> +lines.append('.format_name = "' + format_name + '",')
> +if protocol_name != "":
> +lines.append('.protocol_name = "' + protocol_name + '",')
> +if probe:
> +lines.append('.has_probe = true,')
> +if probe_device:
> +lines.append('.has_probe_device = true,')
> +
> +text = '\n\t'.join(lines)
> +fheader.write('\n\t{\n\t' + text + '\n\t},')
> +
> +def process_file(fheader, filename):
> +# This parser assumes the coding style rules are being followed
> +with open(filename, "r") as cfile:
> +found_something = False
> +found_start = False
> +library, _ = os.path.splitext(os.path.basename(filename))
> +for line in cfile:
> +if found_start:
> +line = line.replace('\n', '')
> +if line.find(".format_name") != -1:
> +format_name = get_string_struct(line)
> +elif line.find(".protocol_name") != -1:
> +protocol_name = get_string_struct(line)
> +elif line.find(".bdrv_probe") != -1:
> +probe = True
> +elif line.find(".bdrv_probe_device") != -1:
> +probe_device = True
> +elif line == "};":
> +add_module(fheader, library, format_name, protocol_name,
> +probe, probe_device)
> +found_start = False
> +elif line.find("static BlockDriver") != -1:
> +found_something = True
> +found_start = True
> +format_name = ""
> +protocol_name = ""
> +probe = False
> +probe_device = False
> +
> +if not found_something:
> +print("No BlockDriver struct found in " + filename + ". \
> +Is this really a module?", file=sys.stderr)
> +sys.exit(1)
> +
> +def print_top(fheader):
> +

[Qemu-devel] [PULL 7/8] migration: fix typos in qapi-schema from latest migration additions

[Amit Shah]

From: "Daniel P. Berrange" 

Recent migration QAPI enhancements had a few spelling mistakes
and also incorrect version number in a few places.

Signed-off-by: Daniel P. Berrange 
Reviewed-by: Eric Blake 
Message-id: 1464776234-9910-2-git-send-email-berra...@redhat.com
Message-Id: <1464776234-9910-2-git-send-email-berra...@redhat.com>
Signed-off-by: Amit Shah 
---
 qapi-schema.json | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index af0129a..40b1db4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -490,7 +490,7 @@
 #
 # @error-desc: #optional the human readable error description string, when
 #  @status is 'failed'. Clients should not attempt to parse the
-#  error strings. (Since 2.6)
+#  error strings. (Since 2.7)
 #
 # Since: 0.14.0
 ##
@@ -635,7 +635,7 @@
 #migration URI does not already include a hostname. For
 #example if using fd: or exec: based migration, the
 #hostname must be provided so that the server's x509
-#certificate identity canbe validated. (Since 2.7)
+#certificate identity can be validated. (Since 2.7)
 #
 # Since: 2.4
 ##
@@ -676,7 +676,7 @@
 #migration URI does not already include a hostname. For
 #example if using fd: or exec: based migration, the
 #hostname must be provided so that the server's x509
-#certificate identity canbe validated. (Since 2.7)
+#certificate identity can be validated. (Since 2.7)
 #
 # Since: 2.4
 ##
@@ -712,14 +712,14 @@
 # be for a 'client' endpoint, while for the incoming side the
 # credentials must be for a 'server' endpoint. Setting this
 # will enable TLS for all migrations. The default is unset,
-# resulting in unsecured migration at the QEMU level. (Since 2.6)
+# resulting in unsecured migration at the QEMU level. (Since 2.7)
 #
 # @tls-hostname: hostname of the target host for the migration. This is
 #required when using x509 based TLS credentials and the
 #migration URI does not already include a hostname. For
 #example if using fd: or exec: based migration, the
 #hostname must be provided so that the server's x509
-#certificate identity canbe validated. (Since 2.6)
+#certificate identity can be validated. (Since 2.7)
 #
 # Since: 2.4
 ##
-- 
2.7.4

[Qemu-devel] [PULL 8/8] migration: rename functions to starting migrations

[Amit Shah]

From: "Daniel P. Berrange" 

Apply the following renames for starting incoming migration:

 process_incoming_migration -> migration_fd_process_incoming
 migration_set_incoming_channel -> migration_channel_process_incoming
 migration_tls_set_incoming_channel -> migration_tls_channel_process_incoming

and for starting outgoing migration:

 migration_set_outgoing_channel -> migration_channel_connect
 migration_tls_set_outgoing_channel -> migration_tls_channel_connect

Signed-off-by: Daniel P. Berrange 
Reviewed-by: Eric Blake 
Message-id: 1464776234-9910-3-git-send-email-berra...@redhat.com
Message-Id: <1464776234-9910-3-git-send-email-berra...@redhat.com>
Signed-off-by: Amit Shah 
---
 include/migration/migration.h | 26 +-
 migration/exec.c  |  4 ++--
 migration/fd.c|  4 ++--
 migration/migration.c | 18 +-
 migration/rdma.c  |  4 ++--
 migration/socket.c|  6 +++---
 migration/tls.c   | 18 +-
 7 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 836c4e3..3c96623 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -183,25 +183,25 @@ struct MigrationState
 
 void migrate_set_state(int *state, int old_state, int new_state);
 
-void process_incoming_migration(QEMUFile *f);
+void migration_fd_process_incoming(QEMUFile *f);
 
 void qemu_start_incoming_migration(const char *uri, Error **errp);
 
-void migration_set_incoming_channel(MigrationState *s,
-QIOChannel *ioc);
+void migration_channel_process_incoming(MigrationState *s,
+QIOChannel *ioc);
 
-void migration_tls_set_incoming_channel(MigrationState *s,
-QIOChannel *ioc,
-Error **errp);
+void migration_tls_channel_process_incoming(MigrationState *s,
+QIOChannel *ioc,
+Error **errp);
 
-void migration_set_outgoing_channel(MigrationState *s,
-QIOChannel *ioc,
-const char *hostname);
+void migration_channel_connect(MigrationState *s,
+   QIOChannel *ioc,
+   const char *hostname);
 
-void migration_tls_set_outgoing_channel(MigrationState *s,
-QIOChannel *ioc,
-const char *hostname,
-Error **errp);
+void migration_tls_channel_connect(MigrationState *s,
+   QIOChannel *ioc,
+   const char *hostname,
+   Error **errp);
 
 uint64_t migrate_max_downtime(void);
 
diff --git a/migration/exec.c b/migration/exec.c
index 1515cc3..2af63cc 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -38,7 +38,7 @@ void exec_start_outgoing_migration(MigrationState *s, const 
char *command, Error
 return;
 }
 
-migration_set_outgoing_channel(s, ioc, NULL);
+migration_channel_connect(s, ioc, NULL);
 object_unref(OBJECT(ioc));
 }
 
@@ -46,7 +46,7 @@ static gboolean exec_accept_incoming_migration(QIOChannel 
*ioc,
GIOCondition condition,
gpointer opaque)
 {
-migration_set_incoming_channel(migrate_get_current(), ioc);
+migration_channel_process_incoming(migrate_get_current(), ioc);
 object_unref(OBJECT(ioc));
 return FALSE; /* unregister */
 }
diff --git a/migration/fd.c b/migration/fd.c
index fc5c9ee..84a10fd 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -38,7 +38,7 @@ void fd_start_outgoing_migration(MigrationState *s, const 
char *fdname, Error **
 return;
 }
 
-migration_set_outgoing_channel(s, ioc, NULL);
+migration_channel_connect(s, ioc, NULL);
 object_unref(OBJECT(ioc));
 }
 
@@ -46,7 +46,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
  GIOCondition condition,
  gpointer opaque)
 {
-migration_set_incoming_channel(migrate_get_current(), ioc);
+migration_channel_process_incoming(migrate_get_current(), ioc);
 object_unref(OBJECT(ioc));
 return FALSE; /* unregister */
 }
diff --git a/migration/migration.c b/migration/migration.c
index 253395c..20f8875 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -416,7 +416,7 @@ static void process_incoming_migration_co(void *opaque)
 qemu_bh_schedule(mis->bh);
 }
 
-void process_incoming_migration(QEMUFile *f)
+void 

[Qemu-devel] [PULL 4/8] test: Postcopy

[Amit Shah]

From: "Dr. David Alan Gilbert" 

This is a postcopy test (x86 only) that actually runs the guest
and checks the memory contents.

The test runs from an x86 boot block with the hex embedded in the test;
the source for this is:

...

.code16
.org 0x7c00
.file   "fill.s"
.text
.globl  start
.type   start, @function
start: # at 0x7c00 ?
cli
lgdt gdtdesc
mov $1,%eax
mov %eax,%cr0  # Protected mode enable
data32 ljmp $8,$0x7c20

.org 0x7c20
.code32
# A20 enable - not sure I actually need this
inb $0x92,%al
or  $2,%al
outb %al, $0x92

# set up DS for the whole of RAM (needed on KVM)
mov $16,%eax
mov %eax,%ds

mov $65,%ax
mov $0x3f8,%dx
outb %al,%dx

# bl keeps a counter so we limit the output speed
mov $0, %bl
mainloop:
# Start from 1MB
mov $(1024*1024),%eax
innerloop:
incb (%eax)
add $4096,%eax
cmp $(100*1024*1024),%eax
jl innerloop

inc %bl
jnz mainloop

mov $66,%ax
mov $0x3f8,%dx
outb %al,%dx

jmp mainloop

# GDT magic from old (GPLv2)  Grub startup.S
.p2align2   /* force 4-byte alignment */
gdt:
.word   0, 0
.byte   0, 0, 0, 0

/* -- code segment --
 * base = 0x, limit = 0xF (4 KiB Granularity), present
 * type = 32bit code execute/read, DPL = 0
 */
.word   0x, 0
.byte   0, 0x9A, 0xCF, 0

/* -- data segment --
 * base = 0x, limit 0xF (4 KiB Granularity), present
 * type = 32 bit data read/write, DPL = 0
 */
.word   0x, 0
.byte   0, 0x92, 0xCF, 0

gdtdesc:
.word   0x27/* limit */
.long   gdt /* addr */

/* I'm a bootable disk */
.org 0x7dfe
.byte 0x55
.byte 0xAA

...

and that can be assembled by the following magic:
as --32 -march=i486 fill.s -o fill.o
objcopy -O binary fill.o fill.boot
dd if=fill.boot of=bootsect bs=256 count=2 skip=124
xxd -i bootsect

Signed-off-by: Dr. David Alan Gilbert 
Reviewed-by: Marcel Apfelbaum 
Message-id: 1465816605-29488-5-git-send-email-dgilb...@redhat.com
Message-Id: <1465816605-29488-5-git-send-email-dgilb...@redhat.com>
Signed-off-by: Amit Shah 
---
 tests/Makefile.include |   2 +
 tests/postcopy-test.c  | 455 +
 2 files changed, 457 insertions(+)
 create mode 100644 tests/postcopy-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7d63d16..6135875 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -234,6 +234,7 @@ endif
 check-qtest-i386-y += tests/test-netfilter$(EXESUF)
 check-qtest-i386-y += tests/test-filter-mirror$(EXESUF)
 check-qtest-i386-y += tests/test-filter-redirector$(EXESUF)
+check-qtest-i386-y += tests/postcopy-test$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst 
i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -599,6 +600,7 @@ tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o 
$(libqos-usb-obj-y)
 tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
 tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
+tests/postcopy-test$(EXESUF): tests/postcopy-test.o
 tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o 
qemu-timer.o $(qtest-obj-y) $(test-io-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): 
tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
new file mode 100644
index 000..9ff88ee
--- /dev/null
+++ b/tests/postcopy-test.c
@@ -0,0 +1,455 @@
+/*
+ * QTest testcase for postcopy
+ *
+ * Copyright (c) 2016 Red Hat, Inc. and/or its affiliates
+ *   based on the vhost-user-test.c that is:
+ *  Copyright (c) 2014 Virtual Open Systems Sarl.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include 
+
+#include "libqtest.h"
+#include "qemu/option.h"
+#include "qemu/range.h"
+#include "sysemu/char.h"
+#include "sysemu/sysemu.h"
+
+#include 
+
+const unsigned start_address = 1024 * 1024;
+const unsigned end_address = 100 * 1024 * 1024;
+bool got_stop;
+
+#if defined(__linux__)
+#include 
+#include 
+#include 
+#endif
+
+#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
+#include 
+#include 
+#include 
+
+static bool ufd_version_check(void)
+{
+

[Qemu-devel] [PULL 6/8] Postcopy: Check for support when setting the capability

[Amit Shah]

From: "Dr. David Alan Gilbert" 

Knowing whether the destination host supports migration with
postcopy can be tricky.
The destination doesn't need the capability set, however
if we set it then use the opportunity to do the test and
tell the user/management layer early.

Signed-off-by: Dr. David Alan Gilbert 
Message-id: 1465816605-29488-7-git-send-email-dgilb...@redhat.com
Message-Id: <1465816605-29488-7-git-send-email-dgilb...@redhat.com>
Signed-off-by: Amit Shah 
---
 migration/migration.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 7bc406a..253395c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -720,6 +720,7 @@ void 
qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
 {
 MigrationState *s = migrate_get_current();
 MigrationCapabilityStatusList *cap;
+bool old_postcopy_cap = migrate_postcopy_ram();
 
 if (migration_is_setup_or_active(s->state)) {
 error_setg(errp, QERR_MIGRATION_ACTIVE);
@@ -742,6 +743,19 @@ void 
qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
 s->enabled_capabilities[MIGRATION_CAPABILITY_POSTCOPY_RAM] =
 false;
 }
+/* This check is reasonably expensive, so only when it's being
+ * set the first time, also it's only the destination that needs
+ * special support.
+ */
+if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) &&
+!postcopy_ram_supported_by_host()) {
+/* postcopy_ram_supported_by_host will have emitted a more
+ * detailed message
+ */
+error_report("Postcopy is not supported");
+s->enabled_capabilities[MIGRATION_CAPABILITY_POSTCOPY_RAM] =
+false;
+}
 }
 }
 
-- 
2.7.4

[Qemu-devel] [PULL 5/8] tests: fix libqtest socket timeouts

[Amit Shah]

From: Andrea Arcangeli 

I kept getting timeouts and unix socket accept failures under high
load, the patch fixes it.

Signed-off-by: Andrea Arcangeli 
Reviewed-by: Marcel Apfelbaum 
Message-id: 1465816605-29488-6-git-send-email-dgilb...@redhat.com
Message-Id: <1465816605-29488-6-git-send-email-dgilb...@redhat.com>
Signed-off-by: Amit Shah 
---
 tests/libqtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 5c82348..eb00f13 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -26,7 +26,7 @@
 #include "qapi/qmp/qjson.h"
 
 #define MAX_IRQ 256
-#define SOCKET_TIMEOUT 5
+#define SOCKET_TIMEOUT 50
 
 QTestState *global_qtest;
 
-- 
2.7.4

[Qemu-devel] [PULL 0/8] migration: fixes

[Amit Shah]

The following changes since commit 49237b856ae58ee7955be0b959c504c51b014f20:

  Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20160614-tag' into 
staging (2016-06-14 16:32:32 +0100)

are available in the git repository at:

  https://git.kernel.org/pub/scm/virt/qemu/amit/migration.git 
tags/migration-for-2.7-4

for you to fetch changes up to 22724f4921e17919dc1b7b796c2c48eadfbcf2da:

  migration: rename functions to starting migrations (2016-06-16 09:51:37 +0530)


Migration:

- Fixes for TLS series
- Postcopy: Add stats, fix, test case



Andrea Arcangeli (1):
  tests: fix libqtest socket timeouts

Daniel P. Berrange (2):
  migration: fix typos in qapi-schema from latest migration additions
  migration: rename functions to starting migrations

Dr. David Alan Gilbert (5):
  Postcopy: Avoid 0 length discards
  Migration: Split out ram part of qmp_query_migrate
  Postcopy: Add stats on page requests
  test: Postcopy
  Postcopy: Check for support when setting the capability

 hmp.c |   4 +
 include/migration/migration.h |  28 +--
 migration/exec.c  |   4 +-
 migration/fd.c|   4 +-
 migration/migration.c |  91 +
 migration/ram.c   |   5 +-
 migration/rdma.c  |   4 +-
 migration/socket.c|   6 +-
 migration/tls.c   |  18 +-
 qapi-schema.json  |  16 +-
 tests/Makefile.include|   2 +
 tests/libqtest.c  |   2 +-
 tests/postcopy-test.c | 455 ++
 13 files changed, 556 insertions(+), 83 deletions(-)
 create mode 100644 tests/postcopy-test.c

-- 
2.7.4

[Qemu-devel] [PULL 3/8] Postcopy: Add stats on page requests

[Amit Shah]

From: "Dr. David Alan Gilbert" 

On the source, add a count of page requests received from the
destination.

Signed-off-by: Dr. David Alan Gilbert 
Reviewed-by: Eric Blake 
Reviewed-by: Denis V. Lunev 
Message-id: 1465816605-29488-4-git-send-email-dgilb...@redhat.com
Message-Id: <1465816605-29488-4-git-send-email-dgilb...@redhat.com>
Signed-off-by: Amit Shah 
---
 hmp.c | 4 
 include/migration/migration.h | 2 ++
 migration/migration.c | 2 ++
 migration/ram.c   | 1 +
 qapi-schema.json  | 6 +-
 5 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hmp.c b/hmp.c
index a4b1d3d..4b8e987 100644
--- a/hmp.c
+++ b/hmp.c
@@ -217,6 +217,10 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
 monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
info->ram->dirty_pages_rate);
 }
+if (info->ram->postcopy_requests) {
+monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
+   info->ram->postcopy_requests);
+}
 }
 
 if (info->has_disk) {
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 13b12b7..836c4e3 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -160,6 +160,8 @@ struct MigrationState
 int64_t xbzrle_cache_size;
 int64_t setup_time;
 int64_t dirty_sync_count;
+/* Count of requests incoming from destination */
+int64_t postcopy_requests;
 
 /* Flag set once the migration has been asked to enter postcopy */
 bool start_postcopy;
diff --git a/migration/migration.c b/migration/migration.c
index 1954987..7bc406a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -614,6 +614,7 @@ static void populate_ram_info(MigrationInfo *info, 
MigrationState *s)
 info->ram->normal_bytes = norm_mig_bytes_transferred();
 info->ram->mbps = s->mbps;
 info->ram->dirty_sync_count = s->dirty_sync_count;
+info->ram->postcopy_requests = s->postcopy_requests;
 
 if (s->state != MIGRATION_STATUS_COMPLETED) {
 info->ram->remaining = ram_bytes_remaining();
@@ -991,6 +992,7 @@ MigrationState *migrate_init(const MigrationParams *params)
 s->dirty_sync_count = 0;
 s->start_postcopy = false;
 s->postcopy_after_devices = false;
+s->postcopy_requests = 0;
 s->migration_thread_running = false;
 s->last_req_rb = NULL;
 error_free(s->error);
diff --git a/migration/ram.c b/migration/ram.c
index 5f929d6..42fb8ac 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1169,6 +1169,7 @@ int ram_save_queue_pages(MigrationState *ms, const char 
*rbname,
 {
 RAMBlock *ramblock;
 
+ms->postcopy_requests++;
 rcu_read_lock();
 if (!rbname) {
 /* Reuse last RAMBlock */
diff --git a/qapi-schema.json b/qapi-schema.json
index 48c3a6f..af0129a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -382,13 +382,17 @@
 #
 # @dirty-sync-count: number of times that dirty ram was synchronized (since 
2.1)
 #
+# @postcopy-requests: The number of page requests received from the destination
+#(since 2.7)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
-   'mbps' : 'number', 'dirty-sync-count' : 'int' } }
+   'mbps' : 'number', 'dirty-sync-count' : 'int',
+   'postcopy-requests' : 'int' } }
 
 ##
 # @XBZRLECacheStats
-- 
2.7.4

[Qemu-devel] [PULL 1/8] Postcopy: Avoid 0 length discards

[Amit Shah]

From: "Dr. David Alan Gilbert" 

The discard code in migration/ram.c would send request for
zero length discards in the case where no discards were needed.
It doesn't appear to have had any bad effect.

Signed-off-by: Dr. David Alan Gilbert 
Reviewed-by: Denis V. Lunev 
Message-id: 1465816605-29488-2-git-send-email-dgilb...@redhat.com
Message-Id: <1465816605-29488-2-git-send-email-dgilb...@redhat.com>
Signed-off-by: Amit Shah 
---
 migration/ram.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 844ea46..5f929d6 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1557,7 +1557,9 @@ static int postcopy_send_discard_bm_ram(MigrationState 
*ms,
 } else {
 discard_length = zero - one;
 }
-postcopy_discard_send_range(ms, pds, one, discard_length);
+if (discard_length) {
+postcopy_discard_send_range(ms, pds, one, discard_length);
+}
 current = one + discard_length;
 } else {
 current = one;
-- 
2.7.4

[Qemu-devel] [PULL 2/8] Migration: Split out ram part of qmp_query_migrate

[Amit Shah]

From: "Dr. David Alan Gilbert" 

The RAM section of qmp_query_migrate is reasonably complex
and repeated 3 times.  Split it out into a helper.

Signed-off-by: Dr. David Alan Gilbert 
Reviewed-by: Eric Blake 
Message-id: 1465816605-29488-3-git-send-email-dgilb...@redhat.com
Reviwed-by: Denis V. Lunev 
Message-Id: <1465816605-29488-3-git-send-email-dgilb...@redhat.com>
Signed-off-by: Amit Shah 
---
 migration/migration.c | 57 ---
 1 file changed, 22 insertions(+), 35 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 7ecbade..1954987 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -602,6 +602,25 @@ static void get_xbzrle_cache_stats(MigrationInfo *info)
 }
 }
 
+static void populate_ram_info(MigrationInfo *info, MigrationState *s)
+{
+info->has_ram = true;
+info->ram = g_malloc0(sizeof(*info->ram));
+info->ram->transferred = ram_bytes_transferred();
+info->ram->total = ram_bytes_total();
+info->ram->duplicate = dup_mig_pages_transferred();
+info->ram->skipped = skipped_mig_pages_transferred();
+info->ram->normal = norm_mig_pages_transferred();
+info->ram->normal_bytes = norm_mig_bytes_transferred();
+info->ram->mbps = s->mbps;
+info->ram->dirty_sync_count = s->dirty_sync_count;
+
+if (s->state != MIGRATION_STATUS_COMPLETED) {
+info->ram->remaining = ram_bytes_remaining();
+info->ram->dirty_pages_rate = s->dirty_pages_rate;
+}
+}
+
 MigrationInfo *qmp_query_migrate(Error **errp)
 {
 MigrationInfo *info = g_malloc0(sizeof(*info));
@@ -626,18 +645,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
 info->has_setup_time = true;
 info->setup_time = s->setup_time;
 
-info->has_ram = true;
-info->ram = g_malloc0(sizeof(*info->ram));
-info->ram->transferred = ram_bytes_transferred();
-info->ram->remaining = ram_bytes_remaining();
-info->ram->total = ram_bytes_total();
-info->ram->duplicate = dup_mig_pages_transferred();
-info->ram->skipped = skipped_mig_pages_transferred();
-info->ram->normal = norm_mig_pages_transferred();
-info->ram->normal_bytes = norm_mig_bytes_transferred();
-info->ram->dirty_pages_rate = s->dirty_pages_rate;
-info->ram->mbps = s->mbps;
-info->ram->dirty_sync_count = s->dirty_sync_count;
+populate_ram_info(info, s);
 
 if (blk_mig_active()) {
 info->has_disk = true;
@@ -665,18 +673,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
 info->has_setup_time = true;
 info->setup_time = s->setup_time;
 
-info->has_ram = true;
-info->ram = g_malloc0(sizeof(*info->ram));
-info->ram->transferred = ram_bytes_transferred();
-info->ram->remaining = ram_bytes_remaining();
-info->ram->total = ram_bytes_total();
-info->ram->duplicate = dup_mig_pages_transferred();
-info->ram->skipped = skipped_mig_pages_transferred();
-info->ram->normal = norm_mig_pages_transferred();
-info->ram->normal_bytes = norm_mig_bytes_transferred();
-info->ram->dirty_pages_rate = s->dirty_pages_rate;
-info->ram->mbps = s->mbps;
-info->ram->dirty_sync_count = s->dirty_sync_count;
+populate_ram_info(info, s);
 
 if (blk_mig_active()) {
 info->has_disk = true;
@@ -699,17 +696,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
 info->has_setup_time = true;
 info->setup_time = s->setup_time;
 
-info->has_ram = true;
-info->ram = g_malloc0(sizeof(*info->ram));
-info->ram->transferred = ram_bytes_transferred();
-info->ram->remaining = 0;
-info->ram->total = ram_bytes_total();
-info->ram->duplicate = dup_mig_pages_transferred();
-info->ram->skipped = skipped_mig_pages_transferred();
-info->ram->normal = norm_mig_pages_transferred();
-info->ram->normal_bytes = norm_mig_bytes_transferred();
-info->ram->mbps = s->mbps;
-info->ram->dirty_sync_count = s->dirty_sync_count;
+populate_ram_info(info, s);
 break;
 case MIGRATION_STATUS_FAILED:
 info->has_status = true;
-- 
2.7.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 0/9] Core based CPU hotplug for PowerPC sPAPR

[Bharata B Rao]

On Wed, Jun 15, 2016 at 04:04:14PM +1000, David Gibson wrote:
> On Fri, Jun 10, 2016 at 03:14:14PM +1000, David Gibson wrote:
> > On Fri, Jun 10, 2016 at 06:28:59AM +0530, Bharata B Rao wrote:
> > > Hi,
> > > 
> > > This is the next version of the CPU hotplug patchset for PowerPC
> > > sPAPR target. The hotplug semantics looks like this:
> > > 
> > > (qemu) device_add POWER8E-spapr-cpu-core,id=core2,core-id=16[,threads=4]
> > > (qemu) device_add host-spapr-cpu-core,id=core2,core-id=16[,threads=4]
> > > (qemu) device_add
> > > POWER8E_v2.1-spapr-cpu-core,id=core2,core-id=16[,threads=4]
> > 
> > I've merged these to my ppc-cpu-hotplug branch, and I'm doing some
> > testing.  It it checks out, I hope to send a pull request.
> 
> I've now merged this into my main ppc-for-2.7 branch.  I'll be doing
> some testing, obviously, but I'd certain appreciate any other tests of
> this branch with the hotplug code.

David, I don't see the unplug patch in your tree, intentional ?

Also I am working on supporting the CPU compat settings with the following
semantics:

-cpu host,power7 -device host-spapr-cpu-core,compat-cpu=power7

Whatever CPU compat level that is specifed with -cpu host is considered
as base and the compat-cpu property specified with -device will be
validated against the base compat level. And this property 'compat-cpu'
will be supported only for host-spapr-cpu-core type.

Is this semantics fine ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Peter Xu]

On Wed, Jun 15, 2016 at 09:56:03AM -0600, Alex Williamson wrote:
> VT-d emulation is currently incompatible with device assignment due
> to intel_iommu's lack of support for memory_region_notify_iommu().
> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
> structure that adds callbacks when the first iommu notifier is
> registered and the last is removed.  For POWER this will allow them
> to switch the view of the iommu depending on whether anyone in
> userspace is watching.  For VT-d I expect that eventually we'll use
> these callbacks to enable and disable code paths so that we avoid
> notifier overhead when there are no registered notifiy-ees.  For now,
> we don't support calling memory_region_notify_iommu(), so this
> signals an incompatible hardware configuration.  If we choose to make
> CM=0 a user selectable option, something like this might continue to
> be useful if we only support notifies via invalidations rather than
> full VT-d data structure shadowing.
> 
> Even though we're currently working on enabling users like vfio-pci
> with VT-d, I believe this is correct for the current state of things.
> We might even want to consider this stable for v2.6.x so that
> downstreams pick it up to avoid incompatible configurations.

Reviewed-by: Peter Xu 
Tested-by: Peter Xu 

(This is much more friendly than a dead loop.)

> 
> Alexey, I hope I'm not stepping on your toes by extracting this
> from your latest patch series.  Please let us know whether you
> approve.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (1):
>   intel_iommu: Throw hw_error on notify_started
> 
> Alexey Kardashevskiy (1):
>   memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
> 
> 
>  hw/i386/intel_iommu.c |   12 
>  hw/vfio/common.c  |5 +++--
>  include/exec/memory.h |8 +++-
>  memory.c  |   10 +-
>  4 files changed, 31 insertions(+), 4 deletions(-)

Re: [Qemu-devel] [PATCH 2/9] m25p80: Make a table for JEDEC ID.

[Cédric Le Goater]

On 06/15/2016 03:41 PM, marcin.krzemin...@nokia.com wrote:
> From: Marcin Krzeminski 
> 
> Since it is now longer than 4. This work based on Pawel Lenkow
> changes and the kernel SPI framework.
>
> Signed-off-by: Marcin Krzeminski 

Reviewed-by: Cédric Le Goater 

> ---
>  hw/block/m25p80.c | 61 
> ++-
>  1 file changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 15765f5..342f7c9 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -53,12 +53,17 @@
>  /* 16 MiB max in 3 byte address mode */
>  #define MAX_3BYTES_SIZE 0x100
> 
> +#define SPI_NOR_MAX_ID_LEN 6
> +
>  typedef struct FlashPartInfo {
>  const char *part_name;
> -/* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
> -uint32_t jedec;
> -/* extended jedec code */
> -uint16_t ext_jedec;
> +/*
> + * This array stores the ID bytes.
> + * The first three bytes are the JEDIC ID.
> + * JEDEC ID zero means "no ID" (mostly older chips).
> + */
> +uint8_t id[SPI_NOR_MAX_ID_LEN];
> +uint8_t id_len;
>  /* there is confusion between manufacturers as to what a sector is. In 
> this
>   * device model, a "sector" is the size that is erased by the 
> ERASE_SECTOR
>   * command (opcode 0xd8).
> @@ -70,11 +75,33 @@ typedef struct FlashPartInfo {
>  } FlashPartInfo;
> 
>  /* adapted from linux */
> -
> -#define INFO(_part_name, _jedec, _ext_jedec, _sector_size, _n_sectors, 
> _flags)\
> -.part_name = (_part_name),\
> -.jedec = (_jedec),\
> -.ext_jedec = (_ext_jedec),\
> +/* Used when the "_ext_id" is two bytes at most */
> +#define INFO(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, 
> _flags)\
> +.part_name = _part_name,\
> +.id = {\
> +((_jedec_id) >> 16) & 0xff,\
> +((_jedec_id) >> 8) & 0xff,\
> +(_jedec_id) & 0xff,\
> +((_ext_id) >> 8) & 0xff,\
> +(_ext_id) & 0xff,\
> +  },\
> +.id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),\
> +.sector_size = (_sector_size),\
> +.n_sectors = (_n_sectors),\
> +.page_size = 256,\
> +.flags = (_flags),
> +
> +#define INFO6(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, 
> _flags)\
> +.part_name = _part_name,\
> +.id = {\
> +((_jedec_id) >> 16) & 0xff,\
> +((_jedec_id) >> 8) & 0xff,\
> +(_jedec_id) & 0xff,\
> +((_ext_id) >> 16) & 0xff,\
> +((_ext_id) >> 8) & 0xff,\
> +(_ext_id) & 0xff,\
> +  },\
> +.id_len = 6,\
>  .sector_size = (_sector_size),\
>  .n_sectors = (_n_sectors),\
>  .page_size = 256,\
> @@ -360,7 +387,7 @@ typedef struct M25P80Class {
> 
>  static inline Manufacturer get_man(Flash *s)
>  {
> -switch (((s->pi->jedec >> 16) & 0xFF)) {
> +switch (s->pi->id[0]) {
>  case 0x20:
>  return MAN_NUMONYX;
>  case 0xEF:
> @@ -630,6 +657,7 @@ static void reset_memory(Flash *s)
>  static void decode_new_cmd(Flash *s, uint32_t value)
>  {
>  s->cmd_in_progress = value;
> +int i;
>  DB_PRINT_L(0, "decoded new command:%x\n", value);
> 
>  if (value != RESET_MEMORY) {
> @@ -743,16 +771,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> 
>  case JEDEC_READ:
>  DB_PRINT_L(0, "populated jedec code\n");
> -s->data[0] = (s->pi->jedec >> 16) & 0xff;
> -s->data[1] = (s->pi->jedec >> 8) & 0xff;
> -s->data[2] = s->pi->jedec & 0xff;
> -if (s->pi->ext_jedec) {
> -s->data[3] = (s->pi->ext_jedec >> 8) & 0xff;
> -s->data[4] = s->pi->ext_jedec & 0xff;
> -s->len = 5;
> -} else {
> -s->len = 3;
> +for (i = 0; i < s->pi->id_len; i++) {
> +s->data[i] = s->pi->id[i];
>  }
> +
> +s->len = s->pi->id_len;
>  s->pos = 0;
>  s->state = STATE_READING_DATA;
>  break;
> 

Re: [Qemu-devel] [PATCH v3 00/20] GICv3 emulation

[Shannon Zhao]

On 2016/6/15 18:10, Peter Maydell wrote:
> On 15 June 2016 at 11:06, Peter Maydell  wrote:
>> > On 15 June 2016 at 10:20, Andrew Jones  wrote:
>>> >> There may be a bug in the freebsd kernel. Maybe they need the equivalent
>>> >> of Linux's 7c9b973061 "irqchip/gic-v3: Configure all interrupts as
>>> >> non-secure Group-1". You could add the hack back that was in the initial
>>> >> posting of this series to see if that "fixes" things.
>> >
>> > I agree it's possible this is a freebsd bug, but the hack patch
>> > won't help here because we're booting via EFI.
> A quick scan through http://fxr.watson.org/fxr/source/arm64/arm64/gic_v3.c
> doesn't seem to show it setting the IGROUPR registers anywhere,
> so it probably is a guest bug. (You can use "-d 'trace:gicv3*'" to
> enable the tracepoints for the GIC which would let you check whether
> the guest ever tries to write to the group config registers.)

I use "-d 'trace:gicv3*'" as you said. Attachment is the trace log. Look
like it doesn't write to GICD_IGROUPR registers.

Thanks,
-- 
Shannon
3145@1466069678.305134:gicv3_dist_read GICv3 distributor read: offset 0x4 data 
0x3780007 size 4 secure 0
3145@1466069678.305423:gicv3_dist_read GICv3 distributor read: offset 0x0 data 
0x50 size 4 secure 0
3145@1466069678.305498:gicv3_cpuif_update GICv3 CPU i/f 0 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305506:gicv3_cpuif_set_irqs GICv3 CPU i/f 0 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.305511:gicv3_cpuif_update GICv3 CPU i/f 1 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305516:gicv3_cpuif_set_irqs GICv3 CPU i/f 1 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.305521:gicv3_cpuif_update GICv3 CPU i/f 2 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305526:gicv3_cpuif_set_irqs GICv3 CPU i/f 2 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.305530:gicv3_cpuif_update GICv3 CPU i/f 3 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305534:gicv3_cpuif_set_irqs GICv3 CPU i/f 3 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.305538:gicv3_cpuif_update GICv3 CPU i/f 4 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305542:gicv3_cpuif_set_irqs GICv3 CPU i/f 4 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.305546:gicv3_cpuif_update GICv3 CPU i/f 5 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305550:gicv3_cpuif_set_irqs GICv3 CPU i/f 5 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.30:gicv3_cpuif_update GICv3 CPU i/f 6 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305558:gicv3_cpuif_set_irqs GICv3 CPU i/f 6 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.305562:gicv3_cpuif_update GICv3 CPU i/f 7 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.305566:gicv3_cpuif_set_irqs GICv3 CPU i/f 7 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.305570:gicv3_dist_write GICv3 distributor write: offset 0x0 
data 0x50 size 4 secure 0
3145@1466069678.307067:gicv3_redist_read GICv3 redistributor 0 read: offset 0x8 
data 0x100 size 4 secure 0
3145@1466069678.307078:gicv3_redist_read GICv3 redistributor 0 read: offset 0xc 
data 0x0 size 4 secure 0
3145@1466069678.307456:gicv3_cpuif_update GICv3 CPU i/f 0 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307467:gicv3_cpuif_set_irqs GICv3 CPU i/f 0 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307472:gicv3_redist_write GICv3 redistributor 0 write: offset 
0x10180 data 0x1 size 4 secure 0
3145@1466069678.307792:gicv3_dist_read GICv3 distributor read: offset 0x400 
data 0x0 size 4 secure 0
3145@1466069678.307868:gicv3_cpuif_update GICv3 CPU i/f 0 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307874:gicv3_cpuif_set_irqs GICv3 CPU i/f 0 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307878:gicv3_cpuif_update GICv3 CPU i/f 1 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307882:gicv3_cpuif_set_irqs GICv3 CPU i/f 1 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307886:gicv3_cpuif_update GICv3 CPU i/f 2 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307890:gicv3_cpuif_set_irqs GICv3 CPU i/f 2 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307894:gicv3_cpuif_update GICv3 CPU i/f 3 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307897:gicv3_cpuif_set_irqs GICv3 CPU i/f 3 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307901:gicv3_cpuif_update GICv3 CPU i/f 4 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307905:gicv3_cpuif_set_irqs GICv3 CPU i/f 4 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307908:gicv3_cpuif_update GICv3 CPU i/f 5 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307912:gicv3_cpuif_set_irqs GICv3 CPU i/f 5 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307916:gicv3_cpuif_update GICv3 CPU i/f 6 HPPI update: irq 0 
group 0 prio 255
3145@1466069678.307920:gicv3_cpuif_set_irqs GICv3 CPU i/f 6 HPPI update: 
setting FIQ 0 IRQ 0
3145@1466069678.307923:gicv3_cpuif_update GICv3 CPU i/f 7 HPPI update: irq 0 
group 0 prio 255

Re: [Qemu-devel] [PATCH v3 08/20] hw/intc/arm_gicv3: Add vmstate descriptors

[Shannon Zhao]

On 2016/6/14 22:38, Peter Maydell wrote:
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index acc1730..d08808d 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c

miss adding #include "migration/migration.h"
otherwise there is a compiling error:
 error: implicit declaration of function 'migrate_add_blocker'
[-Werror=implicit-function-declaration]

> @@ -119,6 +119,13 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, 
> Error **errp)
>  KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
>  kvm_arm_register_device(>iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
> +
> +/* Block migration of a KVM GICv3 device: the API for saving and 
> restoring
> + * the state in the kernel is not yet finalised in the kernel or
> + * implemented in QEMU.
> + */
> +error_setg(>migration_blocker, "vGICv3 migration is not implemented");
> +migrate_add_blocker(s->migration_blocker);
>  }
>  
>  static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
> 

-- 
Shannon

[Qemu-devel] [PATCH] vnc: wrap vnc initialization code with CONFIG_VNC

[Chao Peng]

commit f8c75b2486 (vnc: Initialization stubs) removed CONFIG_VNC in vl.c
code. However qemu_find_opts("vnc") is NULL when vnc is configured out.
Crash will happen in qemu_opts_foreach() before stub vnc_init_func() is
called. This patch add it back.

Cc: Eduardo Habkost 
Signed-off-by: Chao Peng 
---
Note: Of course there is other ways to fix it (e.g. check against NULL).
  I feel comfortable in any way.
---
 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index 45eff56..2088491 100644
--- a/vl.c
+++ b/vl.c
@@ -4557,8 +4557,10 @@ int main(int argc, char **argv, char **envp)
 os_setup_signal_handling();
 
 /* init remote displays */
+#ifdef CONFIG_VNC
 qemu_opts_foreach(qemu_find_opts("vnc"),
   vnc_init_func, NULL, NULL);
+#endif
 if (show_vnc_port) {
 char *ret = vnc_display_local_addr("default");
 printf("VNC server running on '%s'\n", ret);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Alexey Kardashevskiy]

On 16/06/16 01:56, Alex Williamson wrote:
> VT-d emulation is currently incompatible with device assignment due
> to intel_iommu's lack of support for memory_region_notify_iommu().
> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
> structure that adds callbacks when the first iommu notifier is
> registered and the last is removed.  For POWER this will allow them
> to switch the view of the iommu depending on whether anyone in
> userspace is watching.  For VT-d I expect that eventually we'll use
> these callbacks to enable and disable code paths so that we avoid
> notifier overhead when there are no registered notifiy-ees.  For now,
> we don't support calling memory_region_notify_iommu(), so this
> signals an incompatible hardware configuration.  If we choose to make
> CM=0 a user selectable option, something like this might continue to
> be useful if we only support notifies via invalidations rather than
> full VT-d data structure shadowing.
> 
> Even though we're currently working on enabling users like vfio-pci
> with VT-d, I believe this is correct for the current state of things.
> We might even want to consider this stable for v2.6.x so that
> downstreams pick it up to avoid incompatible configurations.
> 
> Alexey, I hope I'm not stepping on your toes by extracting this
> from your latest patch series.  Please let us know whether you
> approve.  Thanks,

I am totally fine with either way of accepting my patches, after all the
idea was yours, I just put it in the code :)


> 
> Alex
> 
> ---
> 
> Alex Williamson (1):
>   intel_iommu: Throw hw_error on notify_started
> 
> Alexey Kardashevskiy (1):
>   memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
> 
> 
>  hw/i386/intel_iommu.c |   12 
>  hw/vfio/common.c  |5 +++--
>  include/exec/memory.h |8 +++-
>  memory.c  |   10 +-
>  4 files changed, 31 insertions(+), 4 deletions(-)
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Throw hw_error on notify_started

[David Gibson]

On Wed, Jun 15, 2016 at 09:56:16AM -0600, Alex Williamson wrote:
> We don't currently support the MemoryRegionIOMMUOps notifier, so throw
> an error should a device require it.
> 
> Signed-off-by: Alex Williamson 

Reviewed-by: David Gibson 

> ---
>  hw/i386/intel_iommu.c |   12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 347718f..5eba704 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -24,6 +24,7 @@
>  #include "exec/address-spaces.h"
>  #include "intel_iommu_internal.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -1871,6 +1872,16 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion 
> *iommu, hwaddr addr,
>  return ret;
>  }
>  
> +static void vtd_iommu_notify_started(MemoryRegion *iommu)
> +{
> +VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> +
> +hw_error("Device at bus %s addr %02x.%d requires iommu notifier which "
> + "is currently not supported by intel-iommu emulation",
> + vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> + PCI_FUNC(vtd_as->devfn));
> +}
> +
>  static const VMStateDescription vtd_vmstate = {
>  .name = "iommu-intel",
>  .unmigratable = 1,
> @@ -1938,6 +1949,7 @@ static void vtd_init(IntelIOMMUState *s)
>  memset(s->womask, 0, DMAR_REG_SIZE);
>  
>  s->iommu_ops.translate = vtd_iommu_translate;
> +s->iommu_ops.notify_started = vtd_iommu_notify_started;
>  s->root = 0;
>  s->root_extended = false;
>  s->dmar_enabled = false;
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 01/10] ppc: Fix rfi/rfid/hrfi/... emulation

[David Gibson]

On Mon, Jun 13, 2016 at 07:24:47AM +0200, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt 
> 
> This reworks emulation of the various "rfi" variants. I removed
> some masking bits that I couldn't make sense of, the only bit that
> I am aware we should mask here is POW, the CPU's MSR mask should
> take care of the rest.
> 
> This also fixes some problems when running 32-bit userspace under
> a 64-bit kernel.
> 
> Signed-off-by: Benjamin Herrenschmidt 
> Reviewed-by: David Gibson 

I've merged this patch to ppc-for-2.7.

The reset of the series I'd like to see a respin for, even if it's
just to clean up the commit messages.

> ---
>  target-ppc/excp_helper.c | 51 
> +++-
>  target-ppc/translate.c   |  7 +++
>  2 files changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 30e960e30b63..aa0b63f4b0de 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -922,25 +922,20 @@ void helper_store_msr(CPUPPCState *env, target_ulong 
> val)
>  }
>  }
>  
> -static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong 
> msr,
> -  target_ulong msrm, int keep_msrh)
> +static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong 
> msr)
>  {
>  CPUState *cs = CPU(ppc_env_get_cpu(env));
>  
> +/* MSR:POW cannot be set by any form of rfi */
> +msr &= ~(1ULL << MSR_POW);
> +
>  #if defined(TARGET_PPC64)
> -if (msr_is_64bit(env, msr)) {
> -nip = (uint64_t)nip;
> -msr &= (uint64_t)msrm;
> -} else {
> +/* Switching to 32-bit ? Crop the nip */
> +if (!msr_is_64bit(env, msr)) {
>  nip = (uint32_t)nip;
> -msr = (uint32_t)(msr & msrm);
> -if (keep_msrh) {
> -msr |= env->msr & ~((uint64_t)0x);
> -}
>  }
>  #else
>  nip = (uint32_t)nip;
> -msr &= (uint32_t)msrm;
>  #endif
>  /* XXX: beware: this is false if VLE is supported */
>  env->nip = nip & ~((target_ulong)0x0003);
> @@ -959,26 +954,24 @@ static inline void do_rfi(CPUPPCState *env, 
> target_ulong nip, target_ulong msr,
>  
>  void helper_rfi(CPUPPCState *env)
>  {
> -if (env->excp_model == POWERPC_EXCP_BOOKE) {
> -do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> -   ~((target_ulong)0), 0);
> -} else {
> -do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> -   ~((target_ulong)0x783F), 1);
> -}
> +do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xul);
>  }
>  
> +#define MSR_BOOK3S_MASK
>  #if defined(TARGET_PPC64)
>  void helper_rfid(CPUPPCState *env)
>  {
> -do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> -   ~((target_ulong)0x783F), 0);
> +/* The architeture defines a number of rules for which bits
> + * can change but in practice, we handle this in hreg_store_msr()
> + * which will be called by do_rfi(), so there is no need to filter
> + * here
> + */
> +do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1]);
>  }
>  
>  void helper_hrfid(CPUPPCState *env)
>  {
> -do_rfi(env, env->spr[SPR_HSRR0], env->spr[SPR_HSRR1],
> -   ~((target_ulong)0x783F), 0);
> +do_rfi(env, env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
>  }
>  #endif
>  
> @@ -986,28 +979,24 @@ void helper_hrfid(CPUPPCState *env)
>  /* Embedded PowerPC specific helpers */
>  void helper_40x_rfci(CPUPPCState *env)
>  {
> -do_rfi(env, env->spr[SPR_40x_SRR2], env->spr[SPR_40x_SRR3],
> -   ~((target_ulong)0x), 0);
> +do_rfi(env, env->spr[SPR_40x_SRR2], env->spr[SPR_40x_SRR3]);
>  }
>  
>  void helper_rfci(CPUPPCState *env)
>  {
> -do_rfi(env, env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1],
> -   ~((target_ulong)0), 0);
> +do_rfi(env, env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1]);
>  }
>  
>  void helper_rfdi(CPUPPCState *env)
>  {
>  /* FIXME: choose CSRR1 or DSRR1 based on cpu type */
> -do_rfi(env, env->spr[SPR_BOOKE_DSRR0], env->spr[SPR_BOOKE_DSRR1],
> -   ~((target_ulong)0), 0);
> +do_rfi(env, env->spr[SPR_BOOKE_DSRR0], env->spr[SPR_BOOKE_DSRR1]);
>  }
>  
>  void helper_rfmci(CPUPPCState *env)
>  {
>  /* FIXME: choose CSRR1 or MCSRR1 based on cpu type */
> -do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1],
> -   ~((target_ulong)0), 0);
> +do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
>  }
>  #endif
>  
> @@ -1045,7 +1034,7 @@ void helper_td(CPUPPCState *env, target_ulong arg1, 
> target_ulong arg2,
>  
>  void helper_rfsvc(CPUPPCState *env)
>  {
> -do_rfi(env, env->lr, env->ctr, 0x, 0);
> +do_rfi(env, env->lr, env->ctr & 0x);
>  }
>  
>  /* Embedded.Processor Control */
> diff --git a/target-ppc/translate.c 

Re: [Qemu-devel] [PATCH v2] Fix confusing argument names in some common functions

[David Gibson]

On Wed, Jun 15, 2016 at 01:45:58PM +0300, Sergey Sorokin wrote:
> 15.06.2016, 06:03, "David Gibson" :
> > On Tue, Jun 14, 2016 at 03:26:17PM +0300, Sergey Sorokin wrote:
> >>  There are functions tlb_fill(), cpu_unaligned_access() and
> >>  do_unaligned_access() that are called with access type and mmu index
> >>  arguments. But these arguments are named 'is_write' and 'is_user' in their
> >>  declarations. The patches fix the arguments to avoid a confusion.
> >>
> >>  Signed-off-by: Sergey Sorokin 
> >>  ---
> >>  In the second version of the patch a type of access_type argument
> >>  was changed from int to MMUAccessType. To allow it the declaration of
> >>  MMUAccessType was moved from exec/cpu-common.h into qom/cpu.h
> >>  The series of two patches was merged into one.
> >>
> >>   include/exec/cpu-common.h | 6 --
> >>   include/exec/exec-all.h | 4 ++--
> >>   include/qom/cpu.h | 15 +++
> >>   target-alpha/cpu.h | 3 ++-
> >>   target-alpha/mem_helper.c | 7 ---
> >>   target-arm/internals.h | 5 +++--
> >>   target-arm/op_helper.c | 30 +-
> >>   target-cris/op_helper.c | 6 +++---
> >>   target-i386/mem_helper.c | 6 +++---
> >>   target-lm32/op_helper.c | 6 +++---
> >>   target-m68k/op_helper.c | 6 +++---
> >>   target-microblaze/op_helper.c | 6 +++---
> >>   target-mips/cpu.h | 3 ++-
> >>   target-mips/op_helper.c | 10 +-
> >>   target-moxie/helper.c | 6 +++---
> >>   target-openrisc/mmu_helper.c | 4 ++--
> >>   target-ppc/mmu_helper.c | 8 
> >>   target-s390x/mem_helper.c | 6 +++---
> >>   target-sh4/op_helper.c | 6 +++---
> >>   target-sparc/cpu.h | 7 ---
> >>   target-sparc/ldst_helper.c | 13 +++--
> >>   target-tricore/op_helper.c | 6 +++---
> >>   target-unicore32/op_helper.c | 4 ++--
> >>   target-xtensa/cpu.h | 3 ++-
> >>   target-xtensa/op_helper.c | 11 ++-
> >>   25 files changed, 100 insertions(+), 87 deletions(-)
> >>
> >>  diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> >>  index aaee995..9ac1eaf 100644
> >>  --- a/include/exec/cpu-common.h
> >>  +++ b/include/exec/cpu-common.h
> >>  @@ -23,12 +23,6 @@ typedef struct CPUListState {
> >>   FILE *file;
> >>   } CPUListState;
> >>
> >>  -typedef enum MMUAccessType {
> >>  - MMU_DATA_LOAD = 0,
> >>  - MMU_DATA_STORE = 1,
> >>  - MMU_INST_FETCH = 2
> >>  -} MMUAccessType;
> >>  -
> >>   #if !defined(CONFIG_USER_ONLY)
> >>
> >>   enum device_endian {
> >>  diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> >>  index c1f59fa..db79ab6 100644
> >>  --- a/include/exec/exec-all.h
> >>  +++ b/include/exec/exec-all.h
> >>  @@ -361,8 +361,8 @@ extern uintptr_t tci_tb_ptr;
> >>   struct MemoryRegion *iotlb_to_region(CPUState *cpu,
> >>    hwaddr index, MemTxAttrs attrs);
> >>
> >>  -void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int 
> >> mmu_idx,
> >>  - uintptr_t retaddr);
> >>  +void tlb_fill(CPUState *cpu, target_ulong addr, MMUAccessType 
> >> access_type,
> >>  + int mmu_idx, uintptr_t retaddr);
> >>
> >>   #endif
> >>
> >>  diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> >>  index 32f3af3..422ac41 100644
> >>  --- a/include/qom/cpu.h
> >>  +++ b/include/qom/cpu.h
> >>  @@ -60,6 +60,12 @@ typedef uint64_t vaddr;
> >>   #define CPU_CLASS(class) OBJECT_CLASS_CHECK(CPUClass, (class), TYPE_CPU)
> >>   #define CPU_GET_CLASS(obj) OBJECT_GET_CLASS(CPUClass, (obj), TYPE_CPU)
> >>
> >>  +typedef enum MMUAccessType {
> >>  + MMU_DATA_LOAD = 0,
> >>  + MMU_DATA_STORE = 1,
> >>  + MMU_INST_FETCH = 2
> >>  +} MMUAccessType;
> >>  +
> >>   typedef struct CPUWatchpoint CPUWatchpoint;
> >>
> >>   typedef void (*CPUUnassignedAccess)(CPUState *cpu, hwaddr addr,
> >>  @@ -142,7 +148,8 @@ typedef struct CPUClass {
> >>   void (*do_interrupt)(CPUState *cpu);
> >>   CPUUnassignedAccess do_unassigned_access;
> >>   void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
> >>  - int is_write, int is_user, uintptr_t retaddr);
> >>  + MMUAccessType access_type,
> >>  + int mmu_idx, uintptr_t retaddr);
> >>   bool (*virtio_is_big_endian)(CPUState *cpu);
> >>   int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
> >>  uint8_t *buf, int len, bool is_write);
> >>  @@ -716,12 +723,12 @@ static inline void cpu_unassigned_access(CPUState 
> >> *cpu, hwaddr addr,
> >>   }
> >>
> >>   static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
> >>  - int is_write, int is_user,
> >>  - uintptr_t retaddr)
> >>  + MMUAccessType access_type,
> >>  + int mmu_idx, uintptr_t retaddr)
> >>   {
> >>   CPUClass *cc = CPU_GET_CLASS(cpu);
> >>
> >>  - cc->do_unaligned_access(cpu, addr, is_write, is_user, retaddr);
> >>  + cc->do_unaligned_access(cpu, addr, access_type, mmu_idx, retaddr);
> >>   }
> >>   #endif
> >>
> >>  diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
> >>  index e71ea70..cfbb615 100644
> >>  

Re: [Qemu-devel] [PATCH] vfio: Fix broken EEH

[David Gibson]

On Wed, Jun 15, 2016 at 10:52:55AM -0600, Alex Williamson wrote:
> On Wed, 15 Jun 2016 15:03:15 +1000
> David Gibson  wrote:
> 
> > On Wed, Jun 15, 2016 at 02:46:23PM +1000, David Gibson wrote:
> > > On Wed, Jun 15, 2016 at 02:28:27PM +1000, Gavin Shan wrote:  
> > > > vfio_eeh_container_op() is the backend that communicates with
> > > > host kernel to support EEH functionality in QEMU. However, the
> > > > functon should return the value from host kernel instead of 0
> > > > unconditionally.
> > > > 
> > > > Signed-off-by: Gavin Shan   
> > > 
> > > Applied to ppc-for-2.7, thanks.  
> > 
> > Hang on, wait.. forgot I should get an ack for this from you, Alex.
> > I'll keep it in my tree for now, unless you tell me you'd prefer to
> > take it through yours.
> 
> Hmm, clearly this patch should have cc'd qemu-devel from the start.

Yeah. Gavin's not usually a qemu developer and I forgot a bunch of
things in my 15s version of how-to-submit-qemu-patches.

> Go
> ahead and take it through your tree David.

Will do, thanks.

> 
> Acked-by: Alex Williamson 
> 
> > 
> > > 
> > > We should probably look at applying this to the 2.6 stable branch as well.
> > >   
> > > > ---
> > > >  hw/vfio/common.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > index e51ed3a..e51db46 100644
> > > > --- a/hw/vfio/common.c
> > > > +++ b/hw/vfio/common.c
> > > > @@ -1258,7 +1258,7 @@ static int vfio_eeh_container_op(VFIOContainer 
> > > > *container, uint32_t op)
> > > >  return -errno;
> > > >  }
> > > >  
> > > > -return 0;
> > > > +return ret;
> > > >  }
> > > >  
> > > >  static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)  
> > >   
> > 
> > 
> > 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH RFC 4/4] range: Replace internal representation of Range

[Eric Blake]

On 06/15/2016 02:41 PM, Markus Armbruster wrote:
> Range represents a range as follows.  Member @start is the inclusive
> lower bound, member @end is the exclusive upper bound.  Zero @end is
> special: if @start is also zero, the range is empty, else @end is to
> be interpreted as 2^64.  No other empty ranges may occur.
> 
> The range [0,2^64-1] cannot be represented.  If you try to create it
> with range_set_bounds1(), you get the empty range instead.  If you try
> to create it with range_set_bounds() or range_extend(), assertions
> fail.  Before range_set_bounds() existed, the open-coded creation
> usually got you the empty range instead.  Open deathtrap.
> 
> Moreover, the code dealing with the janus-faced @end is too clever by
> half.
> 
> Dumb this down to a more pedestrian representation: members @lob and
> @upb are inclusive lower and upper bounds.  The empty range is encoded
> as @lob = 1, @upb = 0.

And since all users now go through accessors, we've freed ourselves to
change the underlying representation.

> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qemu/range.h | 55 
> +---
>  util/range.c | 13 +++--
>  2 files changed, 29 insertions(+), 39 deletions(-)

Not only does it have more power, it takes fewer lines of code!

>  
>  /* Compound literal encoding the empty range */
> -#define range_empty ((Range){ .begin = 0, .end = 0 })
> +#define range_empty ((Range){ .lob = 1, .upb = 0 })

well, one particular representation of the empty range, but the comment
is fine as-is.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH RFC 3/4] range: Drop the previous commit's trickery

[Eric Blake]

On 06/15/2016 02:41 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  include/qemu/range.h | 21 -
>  util/range.c |  1 -
>  2 files changed, 22 deletions(-)

As mentioned on 2/4, you may want to squash this in.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH RFC 2/4] range: Eliminate direct Range member access

[Eric Blake]

On 06/15/2016 02:41 PM, Markus Armbruster wrote:
> Users of struct Range mess liberally with its members, which makes
> refactoring hard.  Create a set of methods, and convert all users to
> call them instead of accessing members.  The methods have carefully
> worded contracts, and use assertions to check them.
> 
> To help with tracking down the places that access members of struct
> Range directly, hide the implementation of struct Range outside of
> range.c by trickery.  The trickery will be dropped in the next commit.

Can't we just make Range an opaque type, and move its definition into
range.c?  Oh, except we have inline functions that would no longer be
inline, unless we also had a range-impl.h private header.

> 
> Signed-off-by: Markus Armbruster 
> ---


> @@ -2015,7 +2017,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>   0, 0x000A, 0x000B, 0, 0x0002));
>  
>  crs_replace_with_free_ranges(mem_ranges,
> - pci_hole->begin, pci_hole->end - 1);
> + range_lob(pci_hole),
> + range_upb(pci_hole));

Changes like this are nice, isolating us from what the actual struct stores.


> +++ b/include/qemu/range.h
> @@ -30,18 +30,110 @@
>   *   - this can not represent a full 0 to ~0x0LL range.
>   */
>  
> +bool range_is_empty(Range *range);
> +bool range_contains(Range *range, uint64_t val);
> +void range_make_empty(Range *range);
> +void range_set_bounds(Range *range, uint64_t lob, uint64_t upb);
> +void range_set_bounds1(Range *range, uint64_t lob, uint64_t upb_plus1);
> +uint64_t range_lob(Range *range);
> +uint64_t range_upb(Range *range);
> +void range_extend(Range *range, Range *extend_by);
> +#ifdef RANGE_IMPL
> +
>  /* A structure representing a range of addresses. */
>  struct Range {
>  uint64_t begin; /* First byte of the range, or 0 if empty. */
>  uint64_t end;   /* 1 + the last byte. 0 if range empty or ends at 
> ~0x0LL. */
>  };
>  
> +static inline void range_invariant(Range *range)
> +{
> +assert((!range->begin && !range->end) /* empty */
> +   || range->begin <= range->end - 1); /* non-empty */
> +}
> +
> +#define static
> +#define inline

Yeah, that's a bit of a hack.  I can live with it though.

The other alternative is to squash 2 and 3 into a single patch on
commit; but having them separate was a nice review aid.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH RFC 1/4] log: Clean up misuse of Range for -dfilter

[Eric Blake]

On 06/15/2016 02:41 PM, Markus Armbruster wrote:
> Range encodes an integer interval [a,b] as { begin = a, end = b + 1 },
> where a \in [0,2^64-1] and b \in [1,2^64].  Thus, zero end is to be
> interpreted as 2^64.
> 
> The implementation of -dfilter (commit 3514552) uses Range
> differently: it encodes [a,b] as { begin = a, end = b }.  The code
> works, but it contradicts the specification of Range in range.h.
> 
> Switch to the specified representation.  Since it can't represent
> [0,UINT64_MAX], we have to reject that now.  Add a test for it.
> 
> While we're rejecting anyway: observe that we reject -dfilter LOB..UPB
> where LOB > UPB when UPB is zero, but happily create an empty Range
> when it isn't.  Reject it then, too, and add a test for it.
> 
> While there, add a positive test for the problematic upper bound
> UINT64_MAX.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  tests/test-logging.c | 10 ++
>  util/log.c   | 28 +++-
>  2 files changed, 25 insertions(+), 13 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/2] blockdev: Add dynamic module loading for block drivers

[Paolo Bonzini]

On 15/06/2016 20:40, Colin Lord wrote:
> 
> The only block drivers that can be converted into modules are the drivers
> that don't perform any init operation except for registering themselves. This
> is why libiscsi has been disabled as a module.

I don't think it has in this patch :) but you can also move the
iscsi_opts registration to vl.c.

Paolo

Re: [Qemu-devel] [PATCH 1/2] blockdev: Add dynamic generation of module_block.h

[Paolo Bonzini]

On 15/06/2016 20:40, Colin Lord wrote:
> +def add_module(fhader, library, format_name, protocol_name,

fhader looks like a typo.

Paolo

> +probe, probe_device):
> +lines = []
> +lines.append('.library_name = "' + library + '",')
> +if format_name != "":
> +lines.append('.format_name = "' + format_name + '",')
> +if protocol_name != "":
> +lines.append('.protocol_name = "' + protocol_name + '",')
> +if probe:
> +lines.append('.has_probe = true,')
> +if probe_device:
> +lines.append('.has_probe_device = true,')
> +
> +text = '\n\t'.join(lines)
> +fheader.write('\n\t{\n\t' + text + '\n\t},')

[Qemu-devel] [Bug 1591628] Re: 2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

[Peter Maloney]

It's an AMD FX(tm)-8150 with a GA-990FXA-UD5 board bios version F11. I
also tested without the usb controllers, such as with your suggested
commands. And again below.

root@peter:~ # uname -a
Linux peter 4.6.2-1-MANJARO #1 SMP PREEMPT Wed Jun 8 11:00:08 UTC 2016 x86_64 
GNU/Linux

root@peter:~ # cat /proc/cmdline 
BOOT_IMAGE=/boot/vmlinuz-4.6-x86_64 
root=UUID=dc395127-6336-448f-a950-137c100420c9 rw pcie_acs_override=downstream 
apparmor=1 security=apparmor 
vfio-pci.pci-ids=00:13.0,00:13.2,00:14.2,00:16.0,00:16.2,01:00.1,04:00.0,04:00.1,05:00.0,05:00.1

the vfio-pci.pci-ids=... is for a mkinitcpio hook I wrote that binds
vfio-pci early so X has no chance to touch the GPUs and risk hanging the
system; it runs before the radeon driver is loaded; it does 3 steps:
unbind on each listed, then vfio-pci bind which annoyingly takes non-
unique device:vendor rather than pci address, then unbind anything not
listed to solve the non-unique problem (relevant since the host also has
the same GPU device:vendor id, and usb controllers)

and you can ignore the apparmor stuff since this stock kernel has no
apparmor support

Testing with my full command or your command, with minimal changes (pci id, 
path to romfile, my disk is lvm rather than file)
...
instead of a black screen/manjaro logo, I get a screen more like the first 
photo with colored pixel mess.
And a new error (that plus the non-black screen are possibly because I 
waited longer rather than changing the test):

root@peter:~/kvm # qemu-system-x86_64 -enable-kvm -M q35 -m 4G -cpu host 
-smp 8 \
> -vga none -device 
ioh3420,bus=pcie.0,addr=1c.0,port=1,chassis=1,id=root.1 \
> -device 
vfio-pci,host=05:00.0,bus=root.1,x-vga=on,addr=0.0,romfile=/mnt/archive/software/vgarom/Sapphire.HD6770.1024/Sapphire.HD6770.1024.120105.rom
 \
> -device ahci,bus=pcie.0,id=ahci \
> -drive 
file=/dev/data/qemutest2,id=iso,index=0,media=disk,format=raw \
> -net none -nographic -monitor stdio -serial none -parallel 
none
QEMU 2.6.0 monitor - type 'help' for more information
(qemu) KVM internal error. Suberror: 1
emulation failure
EAX=b000 EBX= ECX=000f80f2 EDX=7f729950
ESI=005a3c00 EDI=7feb82e0 EBP=0007fe1c ESP=0007fe14
EIP=cd12 EFL=0017 [APC] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0010   00c09300 DPL=0 DS   [-WA]
CS =0008   00c09b00 DPL=0 CS32 [-RA]
SS =0010   00c09300 DPL=0 DS   [-WA]
DS =0010   00c09300 DPL=0 DS   [-WA]
FS =0010   00c09300 DPL=0 DS   [-WA]
GS =0010   00c09300 DPL=0 DS   [-WA]
LDT=   8200 DPL=0 LDT
TR =   8b00 DPL=0 TSS32-busy
GDT= 8280 0027
IDT=  
CR0=0011 CR2= CR3= CR4=
DR0= DR1= DR2= 
DR3= 
DR6=0ff0 DR7=0400
EFER=
Code=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 <00> 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

and here's without the acs thing
vfio: No available IOMMU models
vfio: failed to setup container for group 20

ummm I though this worked in the past and this acs thing was only needed 
for my onboard sound to pass through correctly. Not sure what to do.

If you want me to try another setting for
pcie_acs_override=downstream, feel free to suggest.

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

Title:
  2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

Status in QEMU:
  New

Bug description:
  Not a duplicate of my old bug 1488363

  qemu version 2.5.1 works fine
  qemu version 2.6.0 fails

  seabios 1.9.2-1

  using kernel 4.5.5 with grsecurity

  I built using the arch packaging tools, but commented out all the
  patch code, so it should be vanilla.

  The problem is just that I start a Linux vm using either my radeon R7
  260x or radeon HD 6770, and with qemu 2.6.0, it looks normal until
  after the grub menu, and then the screen looks broken (with mostly
  black, and some pixely junk spread horizontally in a few places on the
  screen... first we thought maybe the monitor died). I'm not sure if
  it's before or only at the moment where the screen resolution changes
  (I could check that or record it on request). Also, the VM is not
  pingable and does not respond to "system_powerdown" on qemu monitor.

  However, the same setup works fine with windows 8. And it works fine
  without graphics cards passed through. A usb controller passed through
  works fine too.

  
  And then I ran a bisect...

  2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62 is the 

Re: [Qemu-devel] [PATCH v2] ssi: change ssi_slave_init to be a realize ops

[Paolo Bonzini]

On 15/06/2016 23:16, Cédric Le Goater wrote:
> This enables qemu to handle late inits and report errors. All the SSI
> slave routine names were changed accordingly. Code was modified to
> handle errors when possible (m25p80 and ssi-sd)
> 
> Tested with the m25p80 slave object.
> 
> Suggested-by: Paolo Bonzini 
> Signed-off-by: Cédric Le Goater 
> ---
> 
>  Should apply on top of :
>  
> m25p80: fix test on blk_pread() return value
> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg05574.html
> 
>  Changes since v1 :
> 
>  - added a error_setg() report in ssi_sd_realize()
> 
>  hw/arm/spitz.c   |   12 
>  hw/arm/tosa.c|5 ++---
>  hw/arm/z2.c  |6 ++
>  hw/block/m25p80.c|   12 +---
>  hw/display/ads7846.c |5 ++---
>  hw/display/ssd0323.c |5 ++---
>  hw/misc/max111x.c|   12 ++--
>  hw/sd/ssi-sd.c   |9 +
>  hw/ssi/ssi.c |6 +++---
>  include/hw/ssi/ssi.h |2 +-
>  10 files changed, 32 insertions(+), 42 deletions(-)
> 
> Index: qemu-ast2400-mainline.git/hw/arm/spitz.c
> ===
> --- qemu-ast2400-mainline.git.orig/hw/arm/spitz.c
> +++ qemu-ast2400-mainline.git/hw/arm/spitz.c
> @@ -598,15 +598,13 @@ static uint32_t spitz_lcdtg_transfer(SSI
>  return 0;
>  }
>  
> -static int spitz_lcdtg_init(SSISlave *dev)
> +static void spitz_lcdtg_realize(SSISlave *dev, Error **errp)
>  {
>  SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, dev);
>  
>  spitz_lcdtg = s;
>  s->bl_power = 0;
>  s->bl_intensity = 0x20;
> -
> -return 0;
>  }
>  
>  /* SSP devices */
> @@ -666,7 +664,7 @@ static void spitz_adc_temp_on(void *opaq
>  max111x_set_input(max, MAX_BATT_TEMP, 0);
>  }
>  
> -static int corgi_ssp_init(SSISlave *d)
> +static void corgi_ssp_realize(SSISlave *d, Error **errp)
>  {
>  DeviceState *dev = DEVICE(d);
>  CorgiSSPState *s = FROM_SSI_SLAVE(CorgiSSPState, d);
> @@ -675,8 +673,6 @@ static int corgi_ssp_init(SSISlave *d)
>  s->bus[0] = ssi_create_bus(dev, "ssi0");
>  s->bus[1] = ssi_create_bus(dev, "ssi1");
>  s->bus[2] = ssi_create_bus(dev, "ssi2");
> -
> -return 0;
>  }
>  
>  static void spitz_ssp_attach(PXA2xxState *cpu)
> @@ -1121,7 +1117,7 @@ static void corgi_ssp_class_init(ObjectC
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
>  
> -k->init = corgi_ssp_init;
> +k->realize = corgi_ssp_realize;
>  k->transfer = corgi_ssp_transfer;
>  dc->vmsd = _corgi_ssp_regs;
>  }
> @@ -1150,7 +1146,7 @@ static void spitz_lcdtg_class_init(Objec
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
>  
> -k->init = spitz_lcdtg_init;
> +k->realize = spitz_lcdtg_realize;
>  k->transfer = spitz_lcdtg_transfer;
>  dc->vmsd = _spitz_lcdtg_regs;
>  }
> Index: qemu-ast2400-mainline.git/hw/arm/tosa.c
> ===
> --- qemu-ast2400-mainline.git.orig/hw/arm/tosa.c
> +++ qemu-ast2400-mainline.git/hw/arm/tosa.c
> @@ -127,10 +127,9 @@ static uint32_t tosa_ssp_tansfer(SSISlav
>  return 0;
>  }
>  
> -static int tosa_ssp_init(SSISlave *dev)
> +static void tosa_ssp_realize(SSISlave *dev, Error **errp)
>  {
>  /* Nothing to do.  */
> -return 0;
>  }
>  
>  #define TYPE_TOSA_DAC "tosa_dac"
> @@ -283,7 +282,7 @@ static void tosa_ssp_class_init(ObjectCl
>  {
>  SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
>  
> -k->init = tosa_ssp_init;
> +k->realize = tosa_ssp_realize;
>  k->transfer = tosa_ssp_tansfer;
>  }
>  
> Index: qemu-ast2400-mainline.git/hw/arm/z2.c
> ===
> --- qemu-ast2400-mainline.git.orig/hw/arm/z2.c
> +++ qemu-ast2400-mainline.git/hw/arm/z2.c
> @@ -151,14 +151,12 @@ static void z2_lcd_cs(void *opaque, int
>  z2_lcd->selected = !level;
>  }
>  
> -static int zipit_lcd_init(SSISlave *dev)
> +static void zipit_lcd_realize(SSISlave *dev, Error **errp)
>  {
>  ZipitLCD *z = FROM_SSI_SLAVE(ZipitLCD, dev);
>  z->selected = 0;
>  z->enabled = 0;
>  z->pos = 0;
> -
> -return 0;
>  }
>  
>  static VMStateDescription vmstate_zipit_lcd_state = {
> @@ -181,7 +179,7 @@ static void zipit_lcd_class_init(ObjectC
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
>  
> -k->init = zipit_lcd_init;
> +k->realize = zipit_lcd_realize;
>  k->transfer = zipit_lcd_transfer;
>  dc->vmsd = _zipit_lcd_state;
>  }
> Index: qemu-ast2400-mainline.git/hw/block/m25p80.c
> ===
> --- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c
> +++ qemu-ast2400-mainline.git/hw/block/m25p80.c
> @@ -28,6 +28,7 @@
>  #include "hw/ssi/ssi.h"
>  #include "qemu/bitops.h"
>  #include "qemu/log.h"
> 

[Qemu-devel] [PATCH v2] ssi: change ssi_slave_init to be a realize ops

[Cédric Le Goater]

This enables qemu to handle late inits and report errors. All the SSI
slave routine names were changed accordingly. Code was modified to
handle errors when possible (m25p80 and ssi-sd)

Tested with the m25p80 slave object.

Suggested-by: Paolo Bonzini 
Signed-off-by: Cédric Le Goater 
---

 Should apply on top of :
 
m25p80: fix test on blk_pread() return value
https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg05574.html

 Changes since v1 :

 - added a error_setg() report in ssi_sd_realize()

 hw/arm/spitz.c   |   12 
 hw/arm/tosa.c|5 ++---
 hw/arm/z2.c  |6 ++
 hw/block/m25p80.c|   12 +---
 hw/display/ads7846.c |5 ++---
 hw/display/ssd0323.c |5 ++---
 hw/misc/max111x.c|   12 ++--
 hw/sd/ssi-sd.c   |9 +
 hw/ssi/ssi.c |6 +++---
 include/hw/ssi/ssi.h |2 +-
 10 files changed, 32 insertions(+), 42 deletions(-)

Index: qemu-ast2400-mainline.git/hw/arm/spitz.c
===
--- qemu-ast2400-mainline.git.orig/hw/arm/spitz.c
+++ qemu-ast2400-mainline.git/hw/arm/spitz.c
@@ -598,15 +598,13 @@ static uint32_t spitz_lcdtg_transfer(SSI
 return 0;
 }
 
-static int spitz_lcdtg_init(SSISlave *dev)
+static void spitz_lcdtg_realize(SSISlave *dev, Error **errp)
 {
 SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, dev);
 
 spitz_lcdtg = s;
 s->bl_power = 0;
 s->bl_intensity = 0x20;
-
-return 0;
 }
 
 /* SSP devices */
@@ -666,7 +664,7 @@ static void spitz_adc_temp_on(void *opaq
 max111x_set_input(max, MAX_BATT_TEMP, 0);
 }
 
-static int corgi_ssp_init(SSISlave *d)
+static void corgi_ssp_realize(SSISlave *d, Error **errp)
 {
 DeviceState *dev = DEVICE(d);
 CorgiSSPState *s = FROM_SSI_SLAVE(CorgiSSPState, d);
@@ -675,8 +673,6 @@ static int corgi_ssp_init(SSISlave *d)
 s->bus[0] = ssi_create_bus(dev, "ssi0");
 s->bus[1] = ssi_create_bus(dev, "ssi1");
 s->bus[2] = ssi_create_bus(dev, "ssi2");
-
-return 0;
 }
 
 static void spitz_ssp_attach(PXA2xxState *cpu)
@@ -1121,7 +1117,7 @@ static void corgi_ssp_class_init(ObjectC
 DeviceClass *dc = DEVICE_CLASS(klass);
 SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
 
-k->init = corgi_ssp_init;
+k->realize = corgi_ssp_realize;
 k->transfer = corgi_ssp_transfer;
 dc->vmsd = _corgi_ssp_regs;
 }
@@ -1150,7 +1146,7 @@ static void spitz_lcdtg_class_init(Objec
 DeviceClass *dc = DEVICE_CLASS(klass);
 SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
 
-k->init = spitz_lcdtg_init;
+k->realize = spitz_lcdtg_realize;
 k->transfer = spitz_lcdtg_transfer;
 dc->vmsd = _spitz_lcdtg_regs;
 }
Index: qemu-ast2400-mainline.git/hw/arm/tosa.c
===
--- qemu-ast2400-mainline.git.orig/hw/arm/tosa.c
+++ qemu-ast2400-mainline.git/hw/arm/tosa.c
@@ -127,10 +127,9 @@ static uint32_t tosa_ssp_tansfer(SSISlav
 return 0;
 }
 
-static int tosa_ssp_init(SSISlave *dev)
+static void tosa_ssp_realize(SSISlave *dev, Error **errp)
 {
 /* Nothing to do.  */
-return 0;
 }
 
 #define TYPE_TOSA_DAC "tosa_dac"
@@ -283,7 +282,7 @@ static void tosa_ssp_class_init(ObjectCl
 {
 SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
 
-k->init = tosa_ssp_init;
+k->realize = tosa_ssp_realize;
 k->transfer = tosa_ssp_tansfer;
 }
 
Index: qemu-ast2400-mainline.git/hw/arm/z2.c
===
--- qemu-ast2400-mainline.git.orig/hw/arm/z2.c
+++ qemu-ast2400-mainline.git/hw/arm/z2.c
@@ -151,14 +151,12 @@ static void z2_lcd_cs(void *opaque, int
 z2_lcd->selected = !level;
 }
 
-static int zipit_lcd_init(SSISlave *dev)
+static void zipit_lcd_realize(SSISlave *dev, Error **errp)
 {
 ZipitLCD *z = FROM_SSI_SLAVE(ZipitLCD, dev);
 z->selected = 0;
 z->enabled = 0;
 z->pos = 0;
-
-return 0;
 }
 
 static VMStateDescription vmstate_zipit_lcd_state = {
@@ -181,7 +179,7 @@ static void zipit_lcd_class_init(ObjectC
 DeviceClass *dc = DEVICE_CLASS(klass);
 SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
 
-k->init = zipit_lcd_init;
+k->realize = zipit_lcd_realize;
 k->transfer = zipit_lcd_transfer;
 dc->vmsd = _zipit_lcd_state;
 }
Index: qemu-ast2400-mainline.git/hw/block/m25p80.c
===
--- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c
+++ qemu-ast2400-mainline.git/hw/block/m25p80.c
@@ -28,6 +28,7 @@
 #include "hw/ssi/ssi.h"
 #include "qemu/bitops.h"
 #include "qemu/log.h"
+#include "qapi/error.h"
 
 #ifndef M25P80_ERR_DEBUG
 #define M25P80_ERR_DEBUG 0
@@ -878,7 +879,7 @@ static uint32_t m25p80_transfer8(SSISlav
 return r;
 }
 
-static int m25p80_init(SSISlave *ss)
+static void m25p80_realize(SSISlave *ss, Error **errp)
 {
 DriveInfo *dinfo;
 Flash *s = M25P80(ss);
@@ -899,18 +900,15 @@ static int 

[Qemu-devel] [PATCH RFC 2/4] range: Eliminate direct Range member access

[Markus Armbruster]

Users of struct Range mess liberally with its members, which makes
refactoring hard.  Create a set of methods, and convert all users to
call them instead of accessing members.  The methods have carefully
worded contracts, and use assertions to check them.

To help with tracking down the places that access members of struct
Range directly, hide the implementation of struct Range outside of
range.c by trickery.  The trickery will be dropped in the next commit.

Signed-off-by: Markus Armbruster 
---
 hw/i386/acpi-build.c |  35 +++---
 hw/pci-host/piix.c   |  26 +++
 hw/pci-host/q35.c|  41 +++--
 hw/pci/pci.c |  17 +++
 include/qemu/range.h | 106 ++-
 qapi/string-input-visitor.c  |  20 
 qapi/string-output-visitor.c |  18 
 util/log.c   |   5 +-
 util/range.c |   4 +-
 9 files changed, 198 insertions(+), 74 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 02fc534..6c36c24 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -232,18 +232,20 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
 pci_host = acpi_get_i386_pci_host();
 g_assert(pci_host);
 
-hole->begin = object_property_get_int(pci_host,
-  PCI_HOST_PROP_PCI_HOLE_START,
-  NULL);
-hole->end = object_property_get_int(pci_host,
-PCI_HOST_PROP_PCI_HOLE_END,
-NULL);
-hole64->begin = object_property_get_int(pci_host,
-PCI_HOST_PROP_PCI_HOLE64_START,
-NULL);
-hole64->end = object_property_get_int(pci_host,
-  PCI_HOST_PROP_PCI_HOLE64_END,
-  NULL);
+range_set_bounds1(hole,
+  object_property_get_int(pci_host,
+  PCI_HOST_PROP_PCI_HOLE_START,
+  NULL),
+  object_property_get_int(pci_host,
+  PCI_HOST_PROP_PCI_HOLE_END,
+  NULL));
+range_set_bounds1(hole64,
+  object_property_get_int(pci_host,
+  PCI_HOST_PROP_PCI_HOLE64_START,
+  NULL),
+  object_property_get_int(pci_host,
+  PCI_HOST_PROP_PCI_HOLE64_END,
+  NULL));
 }
 
 #define ACPI_PORT_SMI_CMD   0x00b2 /* TODO: this is APM_CNT_IOPORT */
@@ -2015,7 +2017,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
  0, 0x000A, 0x000B, 0, 0x0002));
 
 crs_replace_with_free_ranges(mem_ranges,
- pci_hole->begin, pci_hole->end - 1);
+ range_lob(pci_hole),
+ range_upb(pci_hole));
 for (i = 0; i < mem_ranges->len; i++) {
 entry = g_ptr_array_index(mem_ranges, i);
 aml_append(crs,
@@ -2025,12 +2028,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
  0, entry->limit - entry->base + 1));
 }
 
-if (pci_hole64->begin) {
+if (!range_is_empty(pci_hole64)) {
 aml_append(crs,
 aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
  AML_CACHEABLE, AML_READ_WRITE,
- 0, pci_hole64->begin, pci_hole64->end - 1, 0,
- pci_hole64->end - pci_hole64->begin));
+ 0, range_lob(pci_hole64), range_upb(pci_hole64), 
0,
+ range_upb(pci_hole64) + 1 - 
range_lob(pci_hole64)));
 }
 
 if (misc->tpm_version != TPM_VERSION_UNSPEC) {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 8db0f09..1df327f 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -221,8 +221,12 @@ static void i440fx_pcihost_get_pci_hole_start(Object *obj, 
Visitor *v,
   Error **errp)
 {
 I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
-uint32_t value = s->pci_hole.begin;
+uint64_t val64;
+uint32_t value;
 
+val64 = range_is_empty(>pci_hole) ? 0 : range_lob(>pci_hole);
+value = val64;
+assert(value == val64);
 visit_type_uint32(v, name, , errp);
 }
 
@@ -231,8 +235,12 @@ static void i440fx_pcihost_get_pci_hole_end(Object *obj, 
Visitor *v,
 Error **errp)
 {
 I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
-uint32_t value = s->pci_hole.end;
+

Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM

[Stefan Berger]

On 06/15/2016 03:30 PM, Dr. David Alan Gilbert wrote:

* Stefan Berger (stef...@linux.vnet.ibm.com) wrote:

On 05/31/2016 09:58 PM, Xu, Quan wrote:

On Wednesday, June 01, 2016 2:59 AM, BICKFORD, JEFFREY E  wrote:

* Daniel P. Berrange (berra...@redhat.com) wrote:

On Wed, Jan 20, 2016 at 10:54:47AM -0500, Stefan Berger wrote:

On 01/20/2016 10:46 AM, Daniel P. Berrange wrote:

On Wed, Jan 20, 2016 at 10:31:56AM -0500, Stefan Berger wrote:

"Daniel P. Berrange"  wrote on 01/20/2016
10:00:41
AM:



process at all - it would make sense if there was a single
swtpm_cuse shared across all QEMU's, but if there's one per
QEMU device, it feels like it'd be much simpler to just have
the functionality linked in QEMU.  That avoids the problem

I tried having it linked in QEMU before. It was basically rejected.

I remember an impl you did many years(?) ago now, but don't
recall the results of the discussion. Can you elaborate on why it
was rejected as an approach ? It just doesn't make much sense to
me to have to create an external daemon, a CUSE device and comms
protocol, simply to be able to read/write a plain file containing
the TPM state. Its massive over engineering IMHO and adding way
more complexity and thus scope for failure

The TPM 1.2 implementation adds 10s of thousands of lines of code.
The TPM 2 implementation is in the same range. The concern was
having this code right in the QEMU address space. It's big, it can
have bugs, so we don't want it to harm QEMU. So we now put this
into an external process implemented by the swtpm project that
builds on libtpms which provides TPM 1.2 functionality (to be
extended with TPM 2). We cannot call APIs of libtpms directly
anymore, so we need a control channel, which is implemented through

ioctls on the CUSE device.

Ok, the security separation concern does make some sense. The use of
CUSE still seems fairly questionable to me. CUSE makes sense if you
want to provide a drop-in replacement for the kernel TPM device
driver, which would avoid ned for a new QEMU backend. If you're not
emulating an existing kernel driver ABI though, CUSE + ioctl is
feels like a really awful RPC transport between 2 userspace processes.

While I don't really like CUSE; I can see some of the reasoning here.
By providing the existing TPM ioctl interface I think it means you can
use existing host-side TPM tools to initialise/query the soft-tpm, and
those should be independent of the soft-tpm implementation.
As for the extra interfaces you need because it's a soft-tpm to set it
up, once you've already got that ioctl interface as above, then it
seems to make sense to extend that to add the extra interfaces needed.
The only thing you have to watch for there are that the extra
interfaces don't clash with any future kernel ioctl extensions, and
that the interface defined is generic enough for different soft-tpm

implementations.


Dave
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Over the past several months, AT Security Research has been testing the
Virtual TPM software from IBM on the Power (ppc64) platform.

What about x86 platform?


Based on our
testing results, the vTPM software works well and as expected. Support for
libvirt and the CUSE TPM allows us to create VMs with the vTPM functionality
and was tested in a full-fledged OpenStack environment.


Cool..


We believe the vTPM functionality will improve various aspects of VM security
in our enterprise-grade cloud environment. AT would like to see these
patches accepted into the QEMU community as the default-standard build so
this technology can be easily adopted in various open source cloud
deployments.

Stefan: could you update status about this patch set? I'd really appreciate 
your patch..

What do you mean by 'update status'. It's pretty much still the same as
before.

https://github.com/stefanberger/qemu-tpm/tree/v2.6.0+tpm


The implementation of the swtpm that I use for connecting QEMU to now has
more interface choices. There's the existing CUSE + ioctl for data and
control channel or any combination of TCP and Unix sockets for data and
control channel. The libvirt based management stack I built on top of QEMU
with vTPM assumes QEMU using the CUSE interface.

So what was the multi-instance vTPM proxy driver patch set about?


That's for containers.

Stefan

[Qemu-devel] [PATCH RFC 0/4] range: Make it simpler & safer

[Markus Armbruster]

This is an RFC because PATCH 2 adds hackery we might not want to
commit, and PATCH 3 takes it out again.

Prerequisites:
* [PATCH 0/3] log: Fix error handling and a memory leak
* [PATCH 0/2] Clean up around the PCI holes
* [PATCH v2 0/3] Fix leak in handling of integer lists as strings

Markus Armbruster (4):
  log: Clean up misuse of Range for -dfilter
  range: Eliminate direct Range member access
  range: Drop the previous commit's trickery
  range: Replace internal representation of Range

 hw/i386/acpi-build.c |  35 ---
 hw/pci-host/piix.c   |  26 +++
 hw/pci-host/q35.c|  41 +++--
 hw/pci/pci.c |  17 
 include/qemu/range.h | 102 ++-
 qapi/string-input-visitor.c  |  20 -
 qapi/string-output-visitor.c |  18 
 tests/test-logging.c |  10 +
 util/log.c   |  27 ++--
 util/range.c |  16 ++-
 10 files changed, 208 insertions(+), 104 deletions(-)

-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 4/4] linux-user: pass strace argument in execve

[Laurent Vivier]

This is not needed: if you use QEMU_STRACE environment variable, it is
propagated to the child processes (this is also true for "-L" and
QEMU_LD_PREFIX).

In fact, your patch 2 breaks this...

Did you try to use a statically linked qemu?

IMHO, the best way to avoid environment problem is to have a qemu
ignoring it, not modifying it.

Thanks,
Laurent

Le 14/06/2016 à 21:26, Joel Holdsworth a écrit :
> ---
>  linux-user/syscall.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1513f0f..00ee7a6 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6778,6 +6778,8 @@ static abi_long qemu_execve(char *filename, char 
> *argv[],
>  qemu_argc += undef_envc * 2;
>  
>  /* allocate the argument list */
> +if (do_strace)
> +qemu_argc++;
>  argp = qemu_argp = alloca((qemu_argc + 1) * sizeof(void *));
>  
>  /* set up the qemu arguments */
> @@ -6785,6 +6787,9 @@ static abi_long qemu_execve(char *filename, char 
> *argv[],
>  *argp++ = strdup("-L");
>  *argp++ = strdup(path("/"));
>  
> +if (do_strace)
> +*argp++ = strdup("-strace");
> +
>  /* add arguments for the enironment variables */
>  for (i = 0; i < def_envc; i++) {
>  *argp++ = strdup("-E");
> 

[Qemu-devel] [PATCH RFC 1/4] log: Clean up misuse of Range for -dfilter

[Markus Armbruster]

Range encodes an integer interval [a,b] as { begin = a, end = b + 1 },
where a \in [0,2^64-1] and b \in [1,2^64].  Thus, zero end is to be
interpreted as 2^64.

The implementation of -dfilter (commit 3514552) uses Range
differently: it encodes [a,b] as { begin = a, end = b }.  The code
works, but it contradicts the specification of Range in range.h.

Switch to the specified representation.  Since it can't represent
[0,UINT64_MAX], we have to reject that now.  Add a test for it.

While we're rejecting anyway: observe that we reject -dfilter LOB..UPB
where LOB > UPB when UPB is zero, but happily create an empty Range
when it isn't.  Reject it then, too, and add a test for it.

While there, add a positive test for the problematic upper bound
UINT64_MAX.

Signed-off-by: Markus Armbruster 
---
 tests/test-logging.c | 10 ++
 util/log.c   | 28 +++-
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index 440e75f..b6fa94e 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -68,6 +68,16 @@ static void test_parse_range(void)
 g_assert(qemu_log_in_addr_range(0x2050));
 g_assert(qemu_log_in_addr_range(0x3050));
 
+qemu_set_dfilter_ranges("0x-1", _abort);
+g_assert(qemu_log_in_addr_range(UINT64_MAX));
+g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
+
+qemu_set_dfilter_ranges("0..0x", );
+error_free_or_abort();
+
+qemu_set_dfilter_ranges("2..1", );
+error_free_or_abort();
+
 qemu_set_dfilter_ranges("0x1000+onehundred", );
 error_free_or_abort();
 
diff --git a/util/log.c b/util/log.c
index 32e4160..f811d61 100644
--- a/util/log.c
+++ b/util/log.c
@@ -131,8 +131,8 @@ bool qemu_log_in_addr_range(uint64_t addr)
 if (debug_regions) {
 int i = 0;
 for (i = 0; i < debug_regions->len; i++) {
-struct Range *range = _array_index(debug_regions, Range, i);
-if (addr >= range->begin && addr <= range->end) {
+Range *range = _array_index(debug_regions, Range, i);
+if (addr >= range->begin && addr <= range->end - 1) {
 return true;
 }
 }
@@ -158,7 +158,7 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error 
**errp)
 for (i = 0; ranges[i]; i++) {
 const char *r = ranges[i];
 const char *range_op, *r2, *e;
-uint64_t r1val, r2val;
+uint64_t r1val, r2val, lob, upb;
 struct Range range;
 
 range_op = strstr(r, "-");
@@ -187,27 +187,29 @@ void qemu_set_dfilter_ranges(const char *filter_spec, 
Error **errp)
(int)(r2 - range_op), range_op);
 goto out;
 }
-if (r2val == 0) {
-error_setg(errp, "Invalid range");
-goto out;
-}
 
 switch (*range_op) {
 case '+':
-range.begin = r1val;
-range.end = r1val + (r2val - 1);
+lob = r1val;
+upb = r1val + r2val - 1;
 break;
 case '-':
-range.end = r1val;
-range.begin = r1val - (r2val - 1);
+upb = r1val;
+lob = r1val - (r2val - 1);
 break;
 case '.':
-range.begin = r1val;
-range.end = r2val;
+lob = r1val;
+upb = r2val;
 break;
 default:
 g_assert_not_reached();
 }
+if (lob > upb || (lob == 0 && upb == UINT64_MAX)) {
+error_setg(errp, "Invalid range");
+goto out;
+}
+range.begin = lob;
+range.end = upb + 1;
 g_array_append_val(debug_regions, range);
 }
 out:
-- 
2.5.5

[Qemu-devel] [PATCH 06/10] machine: Add machine_register_compat_props() function

[Eduardo Habkost]

Move the compat_props handling to core machine code.

Signed-off-by: Eduardo Habkost 
---
 hw/core/machine.c   | 16 
 include/hw/boards.h |  1 +
 vl.c|  9 ++---
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ccdd5fa..754a054 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -580,6 +580,22 @@ static void machine_class_finalize(ObjectClass *klass, 
void *data)
 }
 }
 
+void machine_register_compat_props(MachineState *machine)
+{
+MachineClass *mc = MACHINE_GET_CLASS(machine);
+int i;
+GlobalProperty *p;
+
+if (!mc->compat_props) {
+return;
+}
+
+for (i = 0; i < mc->compat_props->len; i++) {
+p = g_array_index(mc->compat_props, GlobalProperty *, i);
+qdev_prop_register_global(p);
+}
+}
+
 static const TypeInfo machine_info = {
 .name = TYPE_MACHINE,
 .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index d268bd0..ee71dc0 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -40,6 +40,7 @@ int machine_kvm_shadow_mem(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
 bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
+void machine_register_compat_props(MachineState *machine);
 
 /**
  * CPUArchId:
diff --git a/vl.c b/vl.c
index d88ddba..86d53ca 100644
--- a/vl.c
+++ b/vl.c
@@ -4483,13 +4483,8 @@ int main(int argc, char **argv, char **envp)
 exit (i == 1 ? 1 : 0);
 }
 
-if (machine_class->compat_props) {
-GlobalProperty *p;
-for (i = 0; i < machine_class->compat_props->len; i++) {
-p = g_array_index(machine_class->compat_props, GlobalProperty *, 
i);
-qdev_prop_register_global(p);
-}
-}
+machine_register_compat_props(current_machine);
+
 qemu_opts_foreach(qemu_find_opts("global"),
   global_init_func, NULL, );
 if (err) {
-- 
2.5.5

[Qemu-devel] [PATCH 09/10] qdev: Eliminate GlobalProperty 'used' and 'user_provided' fields

[Eduardo Habkost]

Those fields are not used for anyting and not needed anymore.

Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties.c | 2 --
 include/hw/qdev-core.h| 5 -
 vl.c  | 1 -
 3 files changed, 8 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index c14791d..733cc45 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1048,7 +1048,6 @@ static void qdev_prop_set_globals_for_type(DeviceState 
*dev,
 if (strcmp(typename, prop->driver) != 0) {
 continue;
 }
-prop->used = true;
 object_property_parse(OBJECT(dev), prop->value, prop->property, );
 if (err != NULL) {
 error_prepend(, "can't apply global %s.%s=%s: ",
@@ -1056,7 +1055,6 @@ static void qdev_prop_set_globals_for_type(DeviceState 
*dev,
 if (prop->errp) {
 error_propagate(prop->errp, err);
 } else {
-assert(prop->user_provided);
 error_reportf_err(err, "Warning: ");
 }
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 16e7cc0..5bbd4b0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -259,17 +259,12 @@ struct PropertyInfo {
  * @errp: If set, error_propagate() will be called on errors when applying
  *the property. _abort or _fatal may be used to make
  *errors automatically abort or exit QEMU.
- * @user_provided: Set to true if property comes from user-provided config
- * (command-line or config file).
- * @used: Set to true if property was used when initializing a device.
  */
 typedef struct GlobalProperty {
 const char *driver;
 const char *property;
 const char *value;
 Error **errp;
-bool user_provided;
-bool used;
 } GlobalProperty;
 
 /*** Board API.  This should go away once we have a machine config file.  ***/
diff --git a/vl.c b/vl.c
index bf3bfa3..ee3e3c8 100644
--- a/vl.c
+++ b/vl.c
@@ -2951,7 +2951,6 @@ static int global_init_func(void *opaque, QemuOpts *opts, 
Error **errp)
 g->driver   = driver;
 g->property = prop;
 g->value= qemu_opt_get(opts, "value");
-g->user_provided = true;
 qdev_prop_register_global(g);
 return 0;
 }
-- 
2.5.5

[Qemu-devel] [PATCH RFC 3/4] range: Drop the previous commit's trickery

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 include/qemu/range.h | 21 -
 util/range.c |  1 -
 2 files changed, 22 deletions(-)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 9296ba0..c8c46a9 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -30,16 +30,6 @@
  *   - this can not represent a full 0 to ~0x0LL range.
  */
 
-bool range_is_empty(Range *range);
-bool range_contains(Range *range, uint64_t val);
-void range_make_empty(Range *range);
-void range_set_bounds(Range *range, uint64_t lob, uint64_t upb);
-void range_set_bounds1(Range *range, uint64_t lob, uint64_t upb_plus1);
-uint64_t range_lob(Range *range);
-uint64_t range_upb(Range *range);
-void range_extend(Range *range, Range *extend_by);
-#ifdef RANGE_IMPL
-
 /* A structure representing a range of addresses. */
 struct Range {
 uint64_t begin; /* First byte of the range, or 0 if empty. */
@@ -52,9 +42,6 @@ static inline void range_invariant(Range *range)
|| range->begin <= range->end - 1); /* non-empty */
 }
 
-#define static
-#define inline
-
 /* Compound literal encoding the empty range */
 #define range_empty ((Range){ .begin = 0, .end = 0 })
 
@@ -148,14 +135,6 @@ static inline void range_extend(Range *range, Range 
*extend_by)
 assert(!range_is_empty(range));
 }
 
-#undef static
-#undef inline
-#else
-struct Range {
-uint64_t begin_, end_;
-};
-#endif
-
 /* Get last byte of a range from offset + length.
  * Undefined for ranges that wrap around 0. */
 static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
diff --git a/util/range.c b/util/range.c
index ab5102a..ca149a0 100644
--- a/util/range.c
+++ b/util/range.c
@@ -19,7 +19,6 @@
  */
 
 #include "qemu/osdep.h"
-#define RANGE_IMPL
 #include "qemu/range.h"
 
 /*
-- 
2.5.5

[Qemu-devel] [PATCH 07/10] vl: Set errp to _abort on machine compat_props

[Eduardo Habkost]

Use the new GlobalProperty.errp field to handle compat_props
errors.

Example output before this change:
(with an intentionally broken entry added to PC_COMPAT_1_3 just
for testing)

  $ qemu-system-x86_64 -machine pc-1.3
  qemu-system-x86_64: hw/core/qdev-properties.c:1091: 
qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed.
  Aborted (core dumped)

After:

  $ qemu-system-x86_64 -machine pc-1.3
  Unexpected error in x86_cpuid_set_vendor() at 
/home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:1688:
  qemu-system-x86_64: can't apply global cpu.vendor=x: Property '.vendor' 
doesn't take value 'x'
  Aborted (core dumped)

Suggested-by: Paolo Bonzini 
Signed-off-by: Eduardo Habkost 
---
 hw/core/machine.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 754a054..d6f6be4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -592,6 +592,8 @@ void machine_register_compat_props(MachineState *machine)
 
 for (i = 0; i < mc->compat_props->len; i++) {
 p = g_array_index(mc->compat_props, GlobalProperty *, i);
+/* Machine compat_props must never cause errors: */
+p->errp = _abort;
 qdev_prop_register_global(p);
 }
 }
-- 
2.5.5

[Qemu-devel] [PATCH 10/10] machine: Skip global registration for non-existing classes

[Eduardo Habkost]

MachineClass::compat_props may point to class names that are not
compiled into the QEMU binary. Skip registering those as global
properties. This will allow the qdev global property code to
implement stricter checks on the global property values in the
future.

Signed-off-by: Eduardo Habkost 
---
 hw/core/machine.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index d6f6be4..51697cb 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -583,6 +583,7 @@ static void machine_class_finalize(ObjectClass *klass, void 
*data)
 void machine_register_compat_props(MachineState *machine)
 {
 MachineClass *mc = MACHINE_GET_CLASS(machine);
+ObjectClass *oc;
 int i;
 GlobalProperty *p;
 
@@ -592,6 +593,14 @@ void machine_register_compat_props(MachineState *machine)
 
 for (i = 0; i < mc->compat_props->len; i++) {
 p = g_array_index(mc->compat_props, GlobalProperty *, i);
+
+/* Skip registering globals for non-existing device classes */
+oc = object_class_by_name(p->driver);
+oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
+if (!oc) {
+continue;
+}
+
 /* Machine compat_props must never cause errors: */
 p->errp = _abort;
 qdev_prop_register_global(p);
-- 
2.5.5

[Qemu-devel] [PATCH RFC 4/4] range: Replace internal representation of Range

[Markus Armbruster]

Range represents a range as follows.  Member @start is the inclusive
lower bound, member @end is the exclusive upper bound.  Zero @end is
special: if @start is also zero, the range is empty, else @end is to
be interpreted as 2^64.  No other empty ranges may occur.

The range [0,2^64-1] cannot be represented.  If you try to create it
with range_set_bounds1(), you get the empty range instead.  If you try
to create it with range_set_bounds() or range_extend(), assertions
fail.  Before range_set_bounds() existed, the open-coded creation
usually got you the empty range instead.  Open deathtrap.

Moreover, the code dealing with the janus-faced @end is too clever by
half.

Dumb this down to a more pedestrian representation: members @lob and
@upb are inclusive lower and upper bounds.  The empty range is encoded
as @lob = 1, @upb = 0.

Signed-off-by: Markus Armbruster 
---
 include/qemu/range.h | 55 +---
 util/range.c | 13 +++--
 2 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index c8c46a9..06ff361 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -26,37 +26,37 @@
 /*
  * Operations on 64 bit address ranges.
  * Notes:
- *   - ranges must not wrap around 0, but can include the last byte ~0x0LL.
- *   - this can not represent a full 0 to ~0x0LL range.
+ * - Ranges must not wrap around 0, but can include UINT64_MAX.
  */
 
-/* A structure representing a range of addresses. */
 struct Range {
-uint64_t begin; /* First byte of the range, or 0 if empty. */
-uint64_t end;   /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. 
*/
+/*
+ * A non-empty range has @lob <= @upb.
+ * An empty range has @lob == @upb + 1.
+ */
+uint64_t lob;/* inclusive lower bound */
+uint64_t upb;/* inclusive upper bound */
 };
 
 static inline void range_invariant(Range *range)
 {
-assert((!range->begin && !range->end) /* empty */
-   || range->begin <= range->end - 1); /* non-empty */
+assert(range->lob <= range->upb || range->lob == range->upb + 1);
 }
 
 /* Compound literal encoding the empty range */
-#define range_empty ((Range){ .begin = 0, .end = 0 })
+#define range_empty ((Range){ .lob = 1, .upb = 0 })
 
 /* Is @range empty? */
 static inline bool range_is_empty(Range *range)
 {
 range_invariant(range);
-return !range->begin && !range->end;
+return range->lob > range->upb;
 }
 
 /* Does @range contain @val? */
 static inline bool range_contains(Range *range, uint64_t val)
 {
-return !range_is_empty(range)
-&& val >= range->begin && val <= range->end - 1;
+return val >= range->lob && val <= range->upb;
 }
 
 /* Initialize @range to the empty range */
@@ -71,14 +71,11 @@ static inline void range_make_empty(Range *range)
  * Both bounds are inclusive.
  * The interval must not be empty, i.e. @lob must be less than or
  * equal @upb.
- * The interval must not be [0,UINT64_MAX], because Range can't
- * represent that.
  */
 static inline void range_set_bounds(Range *range, uint64_t lob, uint64_t upb)
 {
-assert(lob <= upb);
-range->begin = lob;
-range->end = upb + 1;   /* may wrap to zero, that's okay */
+range->lob = lob;
+range->upb = upb;
 assert(!range_is_empty(range));
 }
 
@@ -91,8 +88,12 @@ static inline void range_set_bounds(Range *range, uint64_t 
lob, uint64_t upb)
 static inline void range_set_bounds1(Range *range,
  uint64_t lob, uint64_t upb_plus1)
 {
-range->begin = lob;
-range->end = upb_plus1;
+if (!lob && !upb_plus1) {
+*range = range_empty;
+} else {
+range->lob = lob;
+range->upb = upb_plus1 - 1;
+}
 range_invariant(range);
 }
 
@@ -100,20 +101,18 @@ static inline void range_set_bounds1(Range *range,
 static inline uint64_t range_lob(Range *range)
 {
 assert(!range_is_empty(range));
-return range->begin;
+return range->lob;
 }
 
 /* Return @range's upper bound.  @range must not be empty. */
 static inline uint64_t range_upb(Range *range)
 {
 assert(!range_is_empty(range));
-return range->end - 1;
+return range->upb;
 }
 
 /*
  * Extend @range to the smallest interval that includes @extend_by, too.
- * This must not extend @range to cover the interval [0,UINT64_MAX],
- * because Range can't represent that.
  */
 static inline void range_extend(Range *range, Range *extend_by)
 {
@@ -124,15 +123,13 @@ static inline void range_extend(Range *range, Range 
*extend_by)
 *range = *extend_by;
 return;
 }
-if (range->begin > extend_by->begin) {
-range->begin = extend_by->begin;
+if (range->lob > extend_by->lob) {
+range->lob = extend_by->lob;
 }
-/* Compare last byte in case region ends at ~0x0LL */
-if (range->end - 1 < extend_by->end - 1) {
-range->end = extend_by->end;
+if 

[Qemu-devel] [PATCH 05/10] qdev: GlobalProperty.errp field

[Eduardo Habkost]

The new field will allow error handling to be configured by
qdev_prop_register_global() callers: _fatal and
_abort can be used to make QEMU exit or abort if any errors
are reported when applying the properties.

Suggested-by: Paolo Bonzini 
Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties.c | 8 ++--
 include/hw/qdev-core.h| 4 
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index cd19603..0fe7214 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1078,10 +1078,14 @@ static void qdev_prop_set_globals_for_type(DeviceState 
*dev,
 prop->used = true;
 object_property_parse(OBJECT(dev), prop->value, prop->property, );
 if (err != NULL) {
-assert(prop->user_provided);
 error_prepend(, "can't apply global %s.%s=%s: ",
   prop->driver, prop->property, prop->value);
-error_reportf_err(err, "Warning: ");
+if (prop->errp) {
+error_propagate(prop->errp, err);
+} else {
+assert(prop->user_provided);
+error_reportf_err(err, "Warning: ");
+}
 }
 }
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 24aa0a7..16e7cc0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -256,6 +256,9 @@ struct PropertyInfo {
 
 /**
  * GlobalProperty:
+ * @errp: If set, error_propagate() will be called on errors when applying
+ *the property. _abort or _fatal may be used to make
+ *errors automatically abort or exit QEMU.
  * @user_provided: Set to true if property comes from user-provided config
  * (command-line or config file).
  * @used: Set to true if property was used when initializing a device.
@@ -264,6 +267,7 @@ typedef struct GlobalProperty {
 const char *driver;
 const char *property;
 const char *value;
+Error **errp;
 bool user_provided;
 bool used;
 } GlobalProperty;
-- 
2.5.5

[Qemu-devel] [PATCH 04/10] qdev: Use error_prepend() for errors applying globals

[Eduardo Habkost]

The same Error* will be used in an error_propagate() call in the
future, so prepend a "can't apply global" prefix to it.

Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 64e17aa..cd19603 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1079,8 +1079,9 @@ static void qdev_prop_set_globals_for_type(DeviceState 
*dev,
 object_property_parse(OBJECT(dev), prop->value, prop->property, );
 if (err != NULL) {
 assert(prop->user_provided);
-error_reportf_err(err, "Warning: global %s.%s=%s ignored: ",
-  prop->driver, prop->property, prop->value);
+error_prepend(, "can't apply global %s.%s=%s: ",
+  prop->driver, prop->property, prop->value);
+error_reportf_err(err, "Warning: ");
 }
 }
 }
-- 
2.5.5

[Qemu-devel] [PATCH 00/10] globals: Clean up validation and error checking

[Eduardo Habkost]

This series includes multiple changes to the way errors are
handled by the global property system.

The series is based on my machine-next branch, available at:
  https://github.com/ehabkost/qemu.git machine-next

The series itself can be found at:
  https://github.com/ehabkost/qemu-hacks.git work/global-error-handling

Eduardo Habkost (10):
  qdev: Don't stop applying globals on first error
  qdev: Eliminate qemu_add_globals() function
  vl: Reject invalid class names on -global
  qdev: Use error_prepend() for errors applying globals
  qdev: GlobalProperty.errp field
  machine: Add machine_register_compat_props() function
  vl: Set errp to _abort on machine compat_props
  qdev: Eliminate "global not used" warning
  qdev: Eliminate GlobalProperty 'used' and 'user_provided' fields
  machine: Skip global registration for non-existing classes

 hw/core/machine.c| 27 +++
 hw/core/qdev-properties-system.c | 21 +-
 hw/core/qdev-properties.c| 46 ++--
 include/hw/boards.h  |  1 +
 include/hw/qdev-core.h   |  9 
 include/hw/qdev-properties.h |  1 -
 include/qemu/config-file.h   |  1 -
 vl.c | 38 ++---
 8 files changed, 70 insertions(+), 74 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 08/10] qdev: Eliminate "global not used" warning

[Eduardo Habkost]

qdev_prop_check_globals() tries to warn the user if a given
-global option was not used. But it does that only if the device
is not hotpluggable.

The warning also makes it harder for management code or people
that write their own scripts or config files: there's no way to
know if a given -global option will trigger a warning or not,
because we don't have any introspection mechanism to check if a
machine-type instantiates a given device or not.

The warning is also the only reason we have the 'user_provided'
and 'used' fields in struct GlobalProperty. Removing the check
will let us simplify the code.

In other words, the warning is nearly useless and gets in our way
and our users' way. Remove it.

Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties.c| 27 ---
 include/hw/qdev-properties.h |  1 -
 vl.c |  1 -
 3 files changed, 29 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 0fe7214..c14791d 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1036,33 +1036,6 @@ void qdev_prop_register_global_list(GlobalProperty 
*props)
 }
 }
 
-int qdev_prop_check_globals(void)
-{
-GList *l;
-int ret = 0;
-
-for (l = global_props; l; l = l->next) {
-GlobalProperty *prop = l->data;
-ObjectClass *oc;
-DeviceClass *dc;
-if (prop->used) {
-continue;
-}
-if (!prop->user_provided) {
-continue;
-}
-oc = object_class_by_name(prop->driver);
-dc = DEVICE_CLASS(oc);
-if (!dc->hotpluggable && !prop->used) {
-error_report("Warning: global %s.%s=%s not used",
-   prop->driver, prop->property, prop->value);
-ret = 1;
-continue;
-}
-}
-return ret;
-}
-
 static void qdev_prop_set_globals_for_type(DeviceState *dev,
 const char *typename)
 {
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 034b75a..3bd5ea9 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -191,7 +191,6 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, 
void *value);
 
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
-int qdev_prop_check_globals(void);
 void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
 Property *prop, const char *value);
diff --git a/vl.c b/vl.c
index 86d53ca..bf3bfa3 100644
--- a/vl.c
+++ b/vl.c
@@ -4623,7 +4623,6 @@ int main(int argc, char **argv, char **envp)
 }
 }
 
-qdev_prop_check_globals();
 if (vmstate_dump_file) {
 /* dump and exit */
 dump_vmstate_json_to_file(vmstate_dump_file);
-- 
2.5.5

[Qemu-devel] [PATCH 01/10] qdev: Don't stop applying globals on first error

[Eduardo Habkost]

qdev_prop_set_globals_for_type() stops applying global properties
on the first error. It is a leftover from when QEMU exited on any
error when applying global property. Now we print a warning about
the first error, bug ignore all other global properties after it.

For example, the following command-line will not set CPUID level
to 3, but will warn only about "x86_64-cpu.vendor" being ignored.

  $ ./x86_64-softmmu/qemu-system-x86_64 \
  -global x86_64-cpu.vendor=x \
  -global x86_64-cpu.level=3
  qemu-system-x86_64: Warning: global x86_64-cpu.vendor=x ignored: Property 
'.vendor' doesn't take value 'x'

Fix this by not returning from qdev_prop_set_globals_for_type()
on the first error.

Cc: Markus Armbruster 
Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index e3b2184..c10edee 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1088,7 +1088,6 @@ static void qdev_prop_set_globals_for_type(DeviceState 
*dev,
 assert(prop->user_provided);
 error_reportf_err(err, "Warning: global %s.%s=%s ignored: ",
   prop->driver, prop->property, prop->value);
-return;
 }
 }
 }
-- 
2.5.5

[Qemu-devel] [PATCH 02/10] qdev: Eliminate qemu_add_globals() function

[Eduardo Habkost]

The function is just a helper to handle the -global options, it
can stay in vl.c like most qemu_opts_foreach() calls.

Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties-system.c | 21 +
 include/qemu/config-file.h   |  1 -
 vl.c | 16 +++-
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 891219a..cf7139d 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -1,5 +1,5 @@
 /*
- * qdev property parsing and global properties
+ * qdev property parsing
  * (parts specific for qemu-system-*)
  *
  * This file is based on code from hw/qdev-properties.c from
@@ -394,22 +394,3 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
 }
 nd->instantiated = 1;
 }
-
-static int qdev_add_one_global(void *opaque, QemuOpts *opts, Error **errp)
-{
-GlobalProperty *g;
-
-g = g_malloc0(sizeof(*g));
-g->driver   = qemu_opt_get(opts, "driver");
-g->property = qemu_opt_get(opts, "property");
-g->value= qemu_opt_get(opts, "value");
-g->user_provided = true;
-qdev_prop_register_global(g);
-return 0;
-}
-
-void qemu_add_globals(void)
-{
-qemu_opts_foreach(qemu_find_opts("global"),
-  qdev_add_one_global, NULL, NULL);
-}
diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 3b8ecb0..8603e86 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -12,7 +12,6 @@ void qemu_add_opts(QemuOptsList *list);
 void qemu_add_drive_opts(QemuOptsList *list);
 int qemu_set_option(const char *str);
 int qemu_global_option(const char *str);
-void qemu_add_globals(void);
 
 void qemu_config_write(FILE *fp);
 int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
diff --git a/vl.c b/vl.c
index 45eff56..d2d756a 100644
--- a/vl.c
+++ b/vl.c
@@ -2932,6 +2932,19 @@ static void set_memory_options(uint64_t *ram_slots, 
ram_addr_t *maxram_size,
 loc_pop();
 }
 
+static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
+{
+GlobalProperty *g;
+
+g = g_malloc0(sizeof(*g));
+g->driver   = qemu_opt_get(opts, "driver");
+g->property = qemu_opt_get(opts, "property");
+g->value= qemu_opt_get(opts, "value");
+g->user_provided = true;
+qdev_prop_register_global(g);
+return 0;
+}
+
 int main(int argc, char **argv, char **envp)
 {
 int i;
@@ -4466,7 +4479,8 @@ int main(int argc, char **argv, char **envp)
 qdev_prop_register_global(p);
 }
 }
-qemu_add_globals();
+qemu_opts_foreach(qemu_find_opts("global"),
+  global_init_func, NULL, NULL);
 
 /* This checkpoint is required by replay to separate prior clock
reading from the other reads, because timer polling functions query
-- 
2.5.5

[Qemu-devel] [PATCH 03/10] vl: Reject invalid class names on -global

[Eduardo Habkost]

Instead of just printing a warning very late, reject obviously
invalid -global arguments by validating the class name.

Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties.c |  7 ---
 vl.c  | 21 ++---
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index c10edee..64e17aa 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1052,13 +1052,6 @@ int qdev_prop_check_globals(void)
 continue;
 }
 oc = object_class_by_name(prop->driver);
-oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
-if (!oc) {
-error_report("Warning: global %s.%s has invalid class name",
-   prop->driver, prop->property);
-ret = 1;
-continue;
-}
 dc = DEVICE_CLASS(oc);
 if (!dc->hotpluggable && !prop->used) {
 error_report("Warning: global %s.%s=%s not used",
diff --git a/vl.c b/vl.c
index d2d756a..d88ddba 100644
--- a/vl.c
+++ b/vl.c
@@ -2935,10 +2935,21 @@ static void set_memory_options(uint64_t *ram_slots, 
ram_addr_t *maxram_size,
 static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
 GlobalProperty *g;
+ObjectClass *oc;
+const char *driver = qemu_opt_get(opts, "driver");
+const char *prop = qemu_opt_get(opts, "property");
+
+oc = object_class_by_name(driver);
+oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
+if (!oc) {
+error_setg(errp, "global %s.%s has invalid class name",
+   driver, prop);
+return -1;
+}
 
 g = g_malloc0(sizeof(*g));
-g->driver   = qemu_opt_get(opts, "driver");
-g->property = qemu_opt_get(opts, "property");
+g->driver   = driver;
+g->property = prop;
 g->value= qemu_opt_get(opts, "value");
 g->user_provided = true;
 qdev_prop_register_global(g);
@@ -4480,7 +4491,11 @@ int main(int argc, char **argv, char **envp)
 }
 }
 qemu_opts_foreach(qemu_find_opts("global"),
-  global_init_func, NULL, NULL);
+  global_init_func, NULL, );
+if (err) {
+error_report_err(err);
+exit(1);
+}
 
 /* This checkpoint is required by replay to separate prior clock
reading from the other reads, because timer polling functions query
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/1] hmp: acquire aio_context in hmp_qemu_io

[Denis V. Lunev]

On 06/14/2016 11:44 AM, Kevin Wolf wrote:

Am 14.06.2016 um 10:34 hat Denis V. Lunev geschrieben:

On 06/08/2016 02:23 PM, Kevin Wolf wrote:

Am 08.06.2016 um 11:39 hat Denis V. Lunev geschrieben:

From: Vladimir Sementsov-Ogievskiy 

Acquire aio context before run command, this is mandatory for unit tests.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Kevin Wolf 
CC: Paolo Bonzini 

Looks right to me, but why "mandatory for unit tests"? Does this fix an
observable bug in unit tests? If so, we should be more specific in the
commit message.

But in fact, I would only expect it to make a difference for dataplane
and I don't think we have test cases that use both the 'qemu-io' HMP
command and dataplane.

Kevin

the problem is that it is usually difficult to understand that
the problem is in locking and find where the locking is missed.
Here we do have a place, which is used by unit testing, with
a locking missed.

May be now tests are not failing, but IMHO these tests MUST
be fixed to accommodate future changes and thus this patch
is mandatory for them.

I was never objecting to the patch, but just curious about the details
of a possible failure you were seeing because I didn't see how to hit
it in practice without dataplane.

Sorry for forgetting about the patch, I've applied it now.

Kevin



FYI: I have faced today the problematic case in current QEMU.
Test number 132 could suffer from this issue. It performs
'discard' through qemu-io HMP command and have drive-mirror
running, which uses dirty-bitmap.

Den

Re: [Qemu-devel] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[alarson]

David Gibson  wrote on 06/14/2016 11:17:57 
PM:
Aaron Larson 

AL> 1. There is a defect in ppce500_spin.c:spin_kick() that creates an
AL>incorrectly sized TLB entry.  This was reported as bug
AL>https://bugs.launchpad.net/qemu/+bug/1587535  I can provide a
AL>patch if desired.

DG> Absolutely.

OK, I'll start with this one.   Let me know if there is anything you'd 
like me to do
with that bug report.

When e500 PPC is booted multi-core, the non-boot cores are started via
the spin table.  ppce500_spin.c:spin_kick() calls
mmubooke_create_initial_mapping() to allocate a 64MB TLB entry, but
the created TLB entry is only 256KB.

The root cause is that the function computing the size of the TLB
entry, namely booke206_page_size_to_tlb assumes MAS1.TSIZE as defined
by latter PPC cores, specifically n to the power of FOUR * 1KB. The
result is then used by mmubooke_create_initial_mapping using
MAS1_TSIZE_SHIFT, but MAS1_TSIZE_SHIFT is defined assuming TLB entries
are n to the power of TWO * 1KB. I.e., a difference of shift=7 or
shift=8.

Simply changing MAS1_TSIZE_SHIFT from 7 to 8 is not appropriate since
the macro is used elsewhere.

The following patch has a fix for that, and also raises a separate
issue that I'd be happy to resolve after getting some guidance.




ppce500_spin-tlb.patch
Description: Binary data

[Qemu-devel] [Bug 1591628] Re: 2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

[Alex Williamson]

Ran as:

# /usr/local/bin/qemu-system-x86_64 -enable-kvm -M q35 -m 4G -cpu host -smp 8 \
  -vga none -device ioh3420,bus=pcie.0,addr=1c.0,port=1,chassis=1,id=root.1 \
  -device 
vfio-pci,host=02:00.0,bus=root.1,x-vga=on,addr=0.0,romfile=/root/HD8570.rom \
  -device ahci,bus=pcie.0,id=ahci \
  -drive file=/root/qemutest2.img,id=iso,index=0,media=disk,format=raw \
  -net none -nographic -monitor stdio -serial none -parallel none

Works fine, memtest86+ works fine too.  Are you on an AMD or Intel host?
Can you try with a recent, stock (non-grsecurity) kernel?  Can you
reproduce without the assigned USB devices?

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

Title:
  2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

Status in QEMU:
  New

Bug description:
  Not a duplicate of my old bug 1488363

  qemu version 2.5.1 works fine
  qemu version 2.6.0 fails

  seabios 1.9.2-1

  using kernel 4.5.5 with grsecurity

  I built using the arch packaging tools, but commented out all the
  patch code, so it should be vanilla.

  The problem is just that I start a Linux vm using either my radeon R7
  260x or radeon HD 6770, and with qemu 2.6.0, it looks normal until
  after the grub menu, and then the screen looks broken (with mostly
  black, and some pixely junk spread horizontally in a few places on the
  screen... first we thought maybe the monitor died). I'm not sure if
  it's before or only at the moment where the screen resolution changes
  (I could check that or record it on request). Also, the VM is not
  pingable and does not respond to "system_powerdown" on qemu monitor.

  However, the same setup works fine with windows 8. And it works fine
  without graphics cards passed through. A usb controller passed through
  works fine too.

  
  And then I ran a bisect...

  2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62 is the first bad commit
  commit 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62
  Author: Alex Williamson 
  Date:   Thu Mar 10 09:39:08 2016 -0700

  vfio/pci: Convert all MemoryRegion to dynamic alloc and 
consistent functions
  
  Match common vfio code with setup, exit, and finalize functions 
for
  BAR, quirk, and VGA management.  VGA is also changed to dynamic
  allocation to match the other MemoryRegions.
  
  Signed-off-by: Alex Williamson 

  :04 04 0acfd49b6ecae780b6f52a34080ecec6b3ec3672 
e0cfdadede08f553463c0b23931eda81107f41b8 M  hw
  
  then confirm it by reverting that commit
  git checkout v2.6.0
  git revert 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62
  git mergetool -t kdiff3
  "select all from C", save
  not sure if this is the right way to do this...but it compiles 
and works (bug fixed)
  git commit -m "revert 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62 
resolve conflicts"

  And that 2.6.0 build with that one patch reverted works fine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1591628/+subscriptions

Re: [Qemu-devel] [PATCH v2 3/4] linux-user: pass elf interpreter prefix in execve

[Laurent Vivier]

More details: why do we need this?

Add your Signed-off-by.

Le 14/06/2016 à 21:26, Joel Holdsworth a écrit :
> ---
>  linux-user/syscall.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 440986e..1513f0f 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6667,7 +6667,7 @@ static abi_long qemu_execve(char *filename, char 
> *argv[],
>  char *i_arg = NULL, *i_name = NULL;
>  char **qemu_argp, **argp;
>  int i, j;
> -size_t qemu_argc = 3, argc, host_envc, envpc;
> +size_t qemu_argc = 5, argc, host_envc, envpc;
>  int fd, ret;
>  char *cp;
>  size_t def_envc = 0, undef_envc = 0;
> @@ -6782,6 +6782,8 @@ static abi_long qemu_execve(char *filename, char 
> *argv[],
>  
>  /* set up the qemu arguments */
>  *argp++ = strdup(qemu_execve_path);
> +*argp++ = strdup("-L");
> +*argp++ = strdup(path("/"));

why "/"?

You should propagate the one from the parent (if != NULL).

>  
>  /* add arguments for the enironment variables */
>  for (i = 0; i < def_envc; i++) {
> 

Laurent

Re: [Qemu-devel] exec: Safe work in quiescent state

[Sergey Fedorov]

On 15/06/16 18:25, alvise rigo wrote:
> On Wed, Jun 15, 2016 at 4:51 PM, Alex Bennée  wrote:
>> alvise rigo  writes:
>>> On Wed, Jun 15, 2016 at 2:59 PM, Sergey Fedorov  
>>> wrote:
 On 10/06/16 00:51, Sergey Fedorov wrote:
> For certain kinds of tasks we might need a quiescent state to perform an
> operation safely. Quiescent state means no CPU thread executing, and
> probably BQL held as well. The tasks could include:
>> 
 Alvise's async_wait_run_on_cpu() [3]:
 - uses the same queue as async_run_on_cpu();
 - the CPU that requested the job is recorded in qemu_work_item;
 - each CPU has a counter of such jobs it has requested;
 - the counter is decremented upon job completion;
 - only the target CPU is forced to exit the execution loop, i.e. the job
 is not run in quiescent state;
>>> async_wait_run_on_cpu() kicks the target VCPU before calling
>>> cpu_exit() on the current VCPU, so all the VCPUs are forced to exit.
>>> Moreover, the current VCPU waits for all the tasks to be completed.
>> The effect of qemu_cpu_kick() for TCG is effectively just doing a
>> cpu_exit() anyway. Once done any TCG code will exit on it's next
>> intra-block transition.

I was just meaning that async_wait_run_on_cpu() does not stop all the
CPUs: it only affects the current CPU and the target CPU. So this
mechanism cannot be used for tb_flush().

>> 
 Distilling the requirements, safe work mechanism should:
 - support both system and user-mode emulation;
 - allow to schedule an asynchronous operation to be performed out of CPU
 execution loop;
 - guarantee that all CPUs are out of execution loop before the operation
 can begin;
>>> This requirement is probably not necessary if we need to query TLB
>>> flushes to other VCPUs, since every VCPU will flush its own TLB.
>>> For this reason we probably need to mechanisms:
>>> - The first allows a VCPU to query a job to all the others and wait
>>> for all of them to be done (like for global TLB flush)
>> Do we need to wait?
> Yes, otherwise the instruction (like MCR which allows to do TLB
> invalidation) is not completely emulated before executing the
> following one.

I think I need to specify this in the requirements: the CPU which
requested an asynchronous safe operation must exit its execution loop at
the end of the current TB and wait for operation completion. Then guest
cross-CPU TLB invalidation instruction can force end of the TB to ensure
no further instructions get executed until the flush is complete.

> During the LL emulation is also required since it avoids possible race
> conditions.

As it was pointed in [1], LL can be implemented using such "safe work in
quiescent state" mechanism.

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/413978/focus=418664


>>> - The second allows a VCPU to perform a task in quiescent state i.e.
>>> the task starts and finishes when all VCPUs are out of the execution
>>> loop (translation buffer flush)
>> If you really want to ensure everything is done then you can exit the
>> block early. To get the sort of dsb() flush semantics mentioned you
>> simply:
>>
>>   - queue your async safe work
>>   - exit block on dsb()
>>
>>   This ensures by the time the TCG thread restarts for the next
>>   instruction all pending work has been flushed.

Indeed, if we kick the CPU which requested the job and just end the TB
at DSB instruction then the CPU will see the exit request and go out of
its execution loop to wait for operation completion.

>>> Does this make sense?
>> I think we want one way of doing things for anything that is Cross CPU
>> and requires a degree of synchronisation. If it ends up being too
>> expensive then we can look at more efficient special casing solutions.
> OK, I agree that we should start with an approach that fits the two use cases.

So refined the requirements, safe work mechanism should:
- support both system and user-mode emulation;
- allow to schedule an asynchronous operation to be performed out of CPU
execution loop;
- force all CPUs to exit execution loop at the end of the currently
executed TB once an operation is scheduled;
- guarantee that all CPUs are out of execution loop before the operation
can begin;
- guarantee that no CPU enters execution loop until all the scheduled
operations are complete.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH v2 2/4] linux-user: pass environment arguments in execve

[Laurent Vivier]

Le 14/06/2016 à 21:26, Joel Holdsworth a écrit :
> Previously, when emulating execve(2), qemu would execute a child
> instance of the emulator with the environment variables provided by
> the parent process. This caused problems with qemu if any of the
> variables affected the child emulator's behaviour e.g.
> LD_LIBRARY_PATH.

The best way to avoid that is to use a statically linked qemu.

> This patch solves this issue by passing the environment variables
> with '-E' arguments to the child qemu instance. The call to
> execve(2) is replaced by a call to execv(2) so that the parent
> emulator's environment variable state is propagated into the child.
> 
> Any variables from the host environment that are not in the in the
> execve() call are removed with a '-U' argument.

Run ./scripts/checkpatch.pl on your patch...

and add your Signed-off-by here.

The environment is already managed in linux-user/main.c:main(), I don't
understand why the qemu_execve() special case should differ from the
general case.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2 1/4] linux-user: add option to intercept execve() syscalls

[Laurent Vivier]

Le 14/06/2016 à 21:26, Joel Holdsworth a écrit :
> From: Petros Angelatos 
> 
> In order for one to use QEMU user mode emulation under a chroot, it is
> required to use binfmt_misc. This can be avoided by QEMU never doing a
> raw execve() to the host system.
> 
> Introduce a new option, -execve, that uses the current QEMU interpreter
> to intercept execve().
> 
> qemu_execve() will prepend the interpreter path , similar to what
> binfmt_misc would do, and then pass the modified execve() to the host.
> 
> It is necessary to parse hashbang scripts in that function otherwise
> the kernel will try to run the interpreter of a script without QEMU and
> get an invalid exec format error.
> 
> Signed-off-by: Petros Angelatos 
> Tested-by: Laurent Vivier 
> Reviewed-by: Laurent Vivier 
> ---
>  linux-user/main.c|  37 ++
>  linux-user/qemu.h|   1 +
>  linux-user/syscall.c | 137 
> ++-
>  3 files changed, 164 insertions(+), 11 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index f8a8764..429dc06 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -18,6 +18,8 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qemu-version.h"
> +
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -79,6 +81,7 @@ static void usage(int exitcode);
>  
>  static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
>  const char *qemu_uname_release;
> +const char *qemu_execve_path;
>  
>  /* XXX: on x86 MAP_GROWSDOWN only works if ESP <= address + 32, so
> we allocate a bigger stack. Need a better solution, for example
> @@ -3947,6 +3950,38 @@ static void handle_arg_guest_base(const char *arg)
>  have_guest_base = 1;
>  }
>  
> +static void handle_arg_execve(const char *arg)
> +{
> +const char *execfn;
> +char buf[PATH_MAX];
> +char *ret;
> +int len;
> +
> +/* try getauxval() */
> +execfn = (const char *) getauxval(AT_EXECFN);
> +
> +if (execfn != 0) {
> +ret = realpath(execfn, buf);
> +
> +if (ret != NULL) {
> +qemu_execve_path = strdup(buf);
> +return;
> +}
> +}
> +
> +/* try /proc/self/exe */
> +len = readlink("/proc/self/exe", buf, sizeof(buf) - 1);
> +
> +if (len != -1) {
> +buf[len] = '\0';
> +qemu_execve_path = strdup(buf);
> +return;
> +}
> +
> +fprintf(stderr, "qemu_execve: unable to determine intepreter's path\n");
> +exit(EXIT_FAILURE);
> +}
> +
>  static void handle_arg_reserved_va(const char *arg)
>  {
>  char *p;
> @@ -4032,6 +4067,8 @@ static const struct qemu_argument arg_table[] = {
>   "uname",  "set qemu uname release string to 'uname'"},
>  {"B",  "QEMU_GUEST_BASE",  true,  handle_arg_guest_base,
>   "address","set guest_base address to 'address'"},
> +{"execve", "QEMU_EXECVE",  false, handle_arg_execve,
> + "",   "use this interpreter when a process calls execve()"},
>  {"R",  "QEMU_RESERVED_VA", true,  handle_arg_reserved_va,
>   "size",   "reserve 'size' bytes for guest virtual address space"},
>  {"d",  "QEMU_LOG", true,  handle_arg_log,
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 56f29c3..567bcc1 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -149,6 +149,7 @@ void init_task_state(TaskState *ts);
>  void task_settid(TaskState *);
>  void stop_all_tasks(void);
>  extern const char *qemu_uname_release;
> +extern const char *qemu_execve_path;
>  extern unsigned long mmap_min_addr;
>  
>  /* ??? See if we can avoid exposing so much of the loader internals.  */
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 71ccbd9..a478f56 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -106,6 +106,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
>  #include 
>  #endif
>  #include 
> +#include 
>  #include "linux_loop.h"
>  #include "uname.h"
>  
> @@ -6659,6 +6660,128 @@ static target_timer_t get_timer_id(abi_long arg)
>  return timerid;
>  }
>  
> +/* qemu_execve() Must return target values and target errnos. */
> +static abi_long qemu_execve(char *filename, char *argv[],
> +  char *envp[])
> +{
> +char *i_arg = NULL, *i_name = NULL;
> +char **new_argp;
> +int argc, fd, ret, i, offset = 3;
> +char *cp;
> +char buf[BINPRM_BUF_SIZE];
> +
> +/* normal execve case */
> +if (qemu_execve_path == NULL || *qemu_execve_path == 0) {
> +return get_errno(execve(filename, argv, envp));

You need a safe_execve() here, too.

Laurent

Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM

[Dr. David Alan Gilbert]

* Stefan Berger (stef...@linux.vnet.ibm.com) wrote:
> On 05/31/2016 09:58 PM, Xu, Quan wrote:
> > On Wednesday, June 01, 2016 2:59 AM, BICKFORD, JEFFREY E  
> > wrote:
> > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > On Wed, Jan 20, 2016 at 10:54:47AM -0500, Stefan Berger wrote:
> > > > > > On 01/20/2016 10:46 AM, Daniel P. Berrange wrote:
> > > > > > > On Wed, Jan 20, 2016 at 10:31:56AM -0500, Stefan Berger wrote:
> > > > > > > > "Daniel P. Berrange"  wrote on 01/20/2016
> > > > > > > > 10:00:41
> > > > > > > > AM:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > process at all - it would make sense if there was a single
> > > > > > > > > swtpm_cuse shared across all QEMU's, but if there's one per
> > > > > > > > > QEMU device, it feels like it'd be much simpler to just have
> > > > > > > > > the functionality linked in QEMU.  That avoids the problem
> > > > > > > > I tried having it linked in QEMU before. It was basically 
> > > > > > > > rejected.
> > > > > > > I remember an impl you did many years(?) ago now, but don't
> > > > > > > recall the results of the discussion. Can you elaborate on why it
> > > > > > > was rejected as an approach ? It just doesn't make much sense to
> > > > > > > me to have to create an external daemon, a CUSE device and comms
> > > > > > > protocol, simply to be able to read/write a plain file containing
> > > > > > > the TPM state. Its massive over engineering IMHO and adding way
> > > > > > > more complexity and thus scope for failure
> > > > > > The TPM 1.2 implementation adds 10s of thousands of lines of code.
> > > > > > The TPM 2 implementation is in the same range. The concern was
> > > > > > having this code right in the QEMU address space. It's big, it can
> > > > > > have bugs, so we don't want it to harm QEMU. So we now put this
> > > > > > into an external process implemented by the swtpm project that
> > > > > > builds on libtpms which provides TPM 1.2 functionality (to be
> > > > > > extended with TPM 2). We cannot call APIs of libtpms directly
> > > > > > anymore, so we need a control channel, which is implemented through
> > > ioctls on the CUSE device.
> > > > > Ok, the security separation concern does make some sense. The use of
> > > > > CUSE still seems fairly questionable to me. CUSE makes sense if you
> > > > > want to provide a drop-in replacement for the kernel TPM device
> > > > > driver, which would avoid ned for a new QEMU backend. If you're not
> > > > > emulating an existing kernel driver ABI though, CUSE + ioctl is
> > > > > feels like a really awful RPC transport between 2 userspace processes.
> > > > While I don't really like CUSE; I can see some of the reasoning here.
> > > > By providing the existing TPM ioctl interface I think it means you can
> > > > use existing host-side TPM tools to initialise/query the soft-tpm, and
> > > > those should be independent of the soft-tpm implementation.
> > > > As for the extra interfaces you need because it's a soft-tpm to set it
> > > > up, once you've already got that ioctl interface as above, then it
> > > > seems to make sense to extend that to add the extra interfaces needed.
> > > > The only thing you have to watch for there are that the extra
> > > > interfaces don't clash with any future kernel ioctl extensions, and
> > > > that the interface defined is generic enough for different soft-tpm
> > > implementations.
> > > 
> > > > Dave
> > > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> > > 
> > > Over the past several months, AT Security Research has been testing the
> > > Virtual TPM software from IBM on the Power (ppc64) platform.
> > What about x86 platform?
> > 
> > > Based on our
> > > testing results, the vTPM software works well and as expected. Support for
> > > libvirt and the CUSE TPM allows us to create VMs with the vTPM 
> > > functionality
> > > and was tested in a full-fledged OpenStack environment.
> > > 
> > Cool..
> > 
> > > We believe the vTPM functionality will improve various aspects of VM 
> > > security
> > > in our enterprise-grade cloud environment. AT would like to see these
> > > patches accepted into the QEMU community as the default-standard build so
> > > this technology can be easily adopted in various open source cloud
> > > deployments.
> > Stefan: could you update status about this patch set? I'd really appreciate 
> > your patch..
> 
> What do you mean by 'update status'. It's pretty much still the same as
> before.
> 
> https://github.com/stefanberger/qemu-tpm/tree/v2.6.0+tpm
> 
> 
> The implementation of the swtpm that I use for connecting QEMU to now has
> more interface choices. There's the existing CUSE + ioctl for data and
> control channel or any combination of TCP and Unix sockets for data and
> control channel. The libvirt based management stack I built on top of QEMU
> with vTPM assumes QEMU using the CUSE interface.

So what was the multi-instance vTPM proxy driver patch set 

Re: [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo

[Eric Blake]

On 06/15/2016 11:56 AM, Markus Armbruster wrote:
> PcPciInfo has two (ill-named) members: Range w32 is the PCI hole, and
> w64 is the PCI64 hole.
> 
> Three users:
> 
> * I440FXState and MCHPCIState have a member PcPciInfo pci_info, but
>   only pci_info.w32 is actually used.  This is confusing.  Replace by
>   Range pci_hole.
> 
> * acpi_build() uses auto PcPciInfo pci_info to forward both PCI holes
>   from acpi_get_pci_info() to build_dsdt().  Replace by two variables
>   Range pci_hole, pci_hole64.  Rename acpi_get_pci_info() to
>   acpi_get_pci_holes().
> 
> PcPciInfo is now unused; drop it.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/i386/acpi-build.c  | 43 ++-
>  hw/pci-host/piix.c| 10 +-
>  hw/pci-host/q35.c | 12 ++--
>  include/hw/i386/pc.h  |  5 -
>  include/hw/pci-host/q35.h |  2 +-
>  5 files changed, 34 insertions(+), 38 deletions(-)
> 

> +++ b/include/hw/i386/pc.h
> @@ -148,11 +148,6 @@ struct PCMachineClass {
>  
>  /* PC-style peripherals (also used by other machines).  */
>  
> -typedef struct PcPciInfo {
> -Range w32;
> -Range w64;
> -} PcPciInfo;

Confusing indeed.  Good riddance.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] exec: Safe work in quiescent state

[Sergey Fedorov]

On 15/06/16 17:56, Alex Bennée wrote:
> Sergey Fedorov  writes:
(snip)
> Just some quick comments for context:
>
>> Alex's reiteration of Fred's approach [2]:
>> - maintains a single global safe work queue;
> Having separate queues can lead to problems with draining queues as only
> queue gets drained at a time and some threads exit more frequently than
> others.

I think it can't happen if we drain all the queues from all the CPUs, as
we should. The requirement is: stop all the CPUs and process all the
pending work. If we follow this requirement, I think it's not important
whether we have separate queues for each CPU or just a single global queue.

>
>> - uses GArray rather than linked list to implement the work queue;
> This was to minimise g_malloc on job creation and working through the
> list. An awful lot of jobs just need the CPU id and a single parameter.
> This is why I made it the simple case.

I think it would be nice to avoid g_malloc but don't use an array at the
same time. I have some thoughts how to do this easily, let's see the
code ;-)

>> - introduces a global counter of CPUs which have entered their execution
>> loop;
>> - makes use of the last CPU exited its execution loop to drain the safe
>> work queue;
> I suspect you can still race with other deferred work as those tasks are
> being done outside the exec loop. This should be fixable though.

Will keep an eye on this, thanks.

>
>> - still does not support user-mode emulation.
> There is not particular reason it couldn't. However it would mean
> updating the linux-user cpu_exec loop which most likely needs a good
> clean-up and re-factoring to avoid making the change to $ARCH loops.

Yes, you are right, I just fixed the facts here :)

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place

[Eric Blake]

On 06/15/2016 11:56 AM, Markus Armbruster wrote:
> Range pci_info.w32 records the location of the PCI hole.
> 
> It's initialized to empty when QOM zeroes I440FXState.  That's a fine
> value for a still unknown PCI hole.
> 
> i440fx_init() sets pci_info.w32.begin = below_4g_mem_size.  Changes
> the PCI hole from empty to [below_4g_mem_size, UINT64_MAX].  That's a
> bogus value.
> 
> i440fx_pcihost_initfn() sets pci_info.end = IO_APIC_DEFAULT_ADDRESS.
> Since i440fx_init() ran already, this changes the PCI hole to
> [below_4g_mem_size, IO_APIC_DEFAULT_ADDRESS-1].  That's the correct
> value.
> 
> Setting the bounds of the PCI hole in two separate places is
> confusing, and begs the question whether the bogus intermediate value
> could be used by something, or what would happen if we somehow managed
> to realize an i440FX device without having run the board init function
> i440fx_init() first.
> 
> Avoid the confusion by setting the (constant) upper bound along with
> the lower bound in i440fx_init().
> 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/pci-host/piix.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/3] log: Fix qemu_set_log_filename() error handling

[Eric Blake]

On 06/15/2016 11:27 AM, Markus Armbruster wrote:
> When qemu_set_log_filename() detects an invalid file name, it reports
> an error, closes the log file (if any), and starts logging to stderr
> (unless daemonized or nothing is being logged).
> 
> This is wrong.  Asking for an invalid log file on the command line
> should be fatal.  Asking for one in the monitor should fail without
> messing up an existing logfile.
> 
> Fix by converting qemu_set_log_filename() to Error.  Pass it
> _fatal, except for hmp_logfile report errors.
> 
> This also permits testing without a subprocess, so do that.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  bsd-user/main.c  |  3 ++-
>  include/qemu/log.h   |  2 +-
>  linux-user/main.c|  3 ++-
>  monitor.c|  7 ++-
>  tests/test-logging.c | 41 -
>  trace/control.c  |  3 ++-
>  util/log.c   |  6 +++---
>  vl.c |  2 +-
>  8 files changed, 25 insertions(+), 42 deletions(-)
> 
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 9f592be..3d6a4f4 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  
> +#include "qapi/error.h"
>  #include "qemu.h"
>  #include "qemu/path.h"
>  #include "qemu/help_option.h"
> @@ -848,7 +849,7 @@ int main(int argc, char **argv)
>  
>  /* init debug */
>  qemu_log_needs_buffers();
> -qemu_set_log_filename(log_file);
> +qemu_set_log_filename(log_file, _fatal);

Commit race between this patch and Denis' series:

https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg00515.html

Whoever goes in second will have a build failure until all the new uses
are also converted.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/3] log: Fix qemu_set_dfilter_ranges() error reporting

[Eric Blake]

On 06/15/2016 11:27 AM, Markus Armbruster wrote:
> g_error() is not an acceptable way to report errors to the user:
> 
> $ qemu-system-x86_64 -dfilter 1000+0
> 
> ** (process:17187): ERROR **: Failed to parse range in: 1000+0
> Trace/breakpoint trap (core dumped)
> 
> g_assert() isn't, either:
> 
> $ qemu-system-x86_64 -dfilter 1000x+64
> **
> ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges: assertion 
> failed: (e == range_op)
> Aborted (core dumped)

I see you're trying to improve my range.h patches, and got dragged into
this stuff.

> 
> Convert qemu_set_dfilter_ranges() to Error.  Rework its deeply nested
> control flow.  Touch up the error messages.  Call it with
> _fatal.
> 
> This also permits testing without a subprocess, so do that.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qemu/log.h   |   2 +-
>  tests/test-logging.c |  49 --
>  util/log.c   | 113 
> ++-
>  vl.c |   2 +-
>  4 files changed, 75 insertions(+), 91 deletions(-)
> 

> +qemu_set_dfilter_ranges("0x1000+onehundred", );
> +error_free_or_abort();
> +
> +qemu_set_dfilter_ranges("0x1000+0", );
> +error_free_or_abort();
>  }
>  

Maybe also worth testing "0x" and "0x1000+0x" for being invalid?

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [Bug 1591628] Re: 2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

[Peter Maloney]

** Attachment added: "disk image that reproduces problem"
   
https://bugs.launchpad.net/qemu/+bug/1591628/+attachment/4684474/+files/qemutest2.img.xz

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

Title:
  2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

Status in QEMU:
  New

Bug description:
  Not a duplicate of my old bug 1488363

  qemu version 2.5.1 works fine
  qemu version 2.6.0 fails

  seabios 1.9.2-1

  using kernel 4.5.5 with grsecurity

  I built using the arch packaging tools, but commented out all the
  patch code, so it should be vanilla.

  The problem is just that I start a Linux vm using either my radeon R7
  260x or radeon HD 6770, and with qemu 2.6.0, it looks normal until
  after the grub menu, and then the screen looks broken (with mostly
  black, and some pixely junk spread horizontally in a few places on the
  screen... first we thought maybe the monitor died). I'm not sure if
  it's before or only at the moment where the screen resolution changes
  (I could check that or record it on request). Also, the VM is not
  pingable and does not respond to "system_powerdown" on qemu monitor.

  However, the same setup works fine with windows 8. And it works fine
  without graphics cards passed through. A usb controller passed through
  works fine too.

  
  And then I ran a bisect...

  2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62 is the first bad commit
  commit 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62
  Author: Alex Williamson 
  Date:   Thu Mar 10 09:39:08 2016 -0700

  vfio/pci: Convert all MemoryRegion to dynamic alloc and 
consistent functions
  
  Match common vfio code with setup, exit, and finalize functions 
for
  BAR, quirk, and VGA management.  VGA is also changed to dynamic
  allocation to match the other MemoryRegions.
  
  Signed-off-by: Alex Williamson 

  :04 04 0acfd49b6ecae780b6f52a34080ecec6b3ec3672 
e0cfdadede08f553463c0b23931eda81107f41b8 M  hw
  
  then confirm it by reverting that commit
  git checkout v2.6.0
  git revert 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62
  git mergetool -t kdiff3
  "select all from C", save
  not sure if this is the right way to do this...but it compiles 
and works (bug fixed)
  git commit -m "revert 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62 
resolve conflicts"

  And that 2.6.0 build with that one patch reverted works fine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1591628/+subscriptions

[Qemu-devel] [Bug 1591628] Re: 2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

[Peter Maloney]

Attached is a 4.7 MB xz image of a 20 MB disk image that triggers the
problem. It contains a grub2 (2.02~beta2) bootloader, /boot with
memtest86+, and no rootfs.

If I load that as is, it hangs.

If I comment out the "load_video" line, it works. (memtest doesn't run
properly, but auto-reboots, but that's not the issue)

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

Title:
  2.6.0 hangs linux vm using vfio for pci passthrough of graphics card

Status in QEMU:
  New

Bug description:
  Not a duplicate of my old bug 1488363

  qemu version 2.5.1 works fine
  qemu version 2.6.0 fails

  seabios 1.9.2-1

  using kernel 4.5.5 with grsecurity

  I built using the arch packaging tools, but commented out all the
  patch code, so it should be vanilla.

  The problem is just that I start a Linux vm using either my radeon R7
  260x or radeon HD 6770, and with qemu 2.6.0, it looks normal until
  after the grub menu, and then the screen looks broken (with mostly
  black, and some pixely junk spread horizontally in a few places on the
  screen... first we thought maybe the monitor died). I'm not sure if
  it's before or only at the moment where the screen resolution changes
  (I could check that or record it on request). Also, the VM is not
  pingable and does not respond to "system_powerdown" on qemu monitor.

  However, the same setup works fine with windows 8. And it works fine
  without graphics cards passed through. A usb controller passed through
  works fine too.

  
  And then I ran a bisect...

  2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62 is the first bad commit
  commit 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62
  Author: Alex Williamson 
  Date:   Thu Mar 10 09:39:08 2016 -0700

  vfio/pci: Convert all MemoryRegion to dynamic alloc and 
consistent functions
  
  Match common vfio code with setup, exit, and finalize functions 
for
  BAR, quirk, and VGA management.  VGA is also changed to dynamic
  allocation to match the other MemoryRegions.
  
  Signed-off-by: Alex Williamson 

  :04 04 0acfd49b6ecae780b6f52a34080ecec6b3ec3672 
e0cfdadede08f553463c0b23931eda81107f41b8 M  hw
  
  then confirm it by reverting that commit
  git checkout v2.6.0
  git revert 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62
  git mergetool -t kdiff3
  "select all from C", save
  not sure if this is the right way to do this...but it compiles 
and works (bug fixed)
  git commit -m "revert 2d82f8a3cdb276bc3cb92d6f01bf8f66bf328d62 
resolve conflicts"

  And that 2.6.0 build with that one patch reverted works fine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1591628/+subscriptions

[Qemu-devel] [PATCH 2/2] blockdev: Add dynamic module loading for block drivers

[Colin Lord]

From: Marc Mari 

Extend the current module interface to allow for block drivers to be loaded
dynamically on request.

The only block drivers that can be converted into modules are the drivers
that don't perform any init operation except for registering themselves. This
is why libiscsi has been disabled as a module.

All the necessary module information is located in a new structure found in
include/qemu/module_block.h

Signed-off-by: Marc Marí 
Signed-off-by: Colin Lord 
---
 Makefile  |  3 --
 block.c   | 86 +--
 include/qemu/module.h |  3 ++
 util/module.c | 37 ++
 4 files changed, 90 insertions(+), 39 deletions(-)

diff --git a/Makefile b/Makefile
index 8f8b6a2..461187c 100644
--- a/Makefile
+++ b/Makefile
@@ -247,9 +247,6 @@ Makefile: $(version-obj-y) $(version-lobj-y)
 libqemustub.a: $(stub-obj-y)
 libqemuutil.a: $(util-obj-y)
 
-block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst /,-,$o))",) NULL
-util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
-
 ##
 
 qemu-img.o: qemu-img-cmds.h
diff --git a/block.c b/block.c
index f54bc25..7a91434 100644
--- a/block.c
+++ b/block.c
@@ -26,6 +26,7 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "qemu/error-report.h"
+#include "qemu/module_block.h"
 #include "qemu/module.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
@@ -242,11 +243,29 @@ BlockDriverState *bdrv_new(void)
 BlockDriver *bdrv_find_format(const char *format_name)
 {
 BlockDriver *drv1;
+size_t i;
+
 QLIST_FOREACH(drv1, _drivers, list) {
 if (!strcmp(drv1->format_name, format_name)) {
 return drv1;
 }
 }
+
+for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
+if (!strcmp(block_driver_modules[i].format_name, format_name)) {
+block_module_load_one(block_driver_modules[i].library_name);
+/* Copying code is not nice, but this way the current discovery is
+ * not modified. Calling recursively could fail if the library
+ * has been deleted.
+ */
+QLIST_FOREACH(drv1, _drivers, list) {
+if (!strcmp(drv1->format_name, format_name)) {
+return drv1;
+}
+}
+}
+}
+
 return NULL;
 }
 
@@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size)
 static BlockDriver *find_hdev_driver(const char *filename)
 {
 int score_max = 0, score;
+size_t i;
 BlockDriver *drv = NULL, *d;
 
+for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
+if (block_driver_modules[i].has_probe_device) {
+block_module_load_one(block_driver_modules[i].library_name);
+}
+}
+
 QLIST_FOREACH(d, _drivers, list) {
 if (d->bdrv_probe_device) {
 score = d->bdrv_probe_device(filename);
@@ -470,6 +496,7 @@ BlockDriver *bdrv_find_protocol(const char *filename,
 char protocol[128];
 int len;
 const char *p;
+size_t i;
 
 /* TODO Drivers without bdrv_file_open must be specified explicitly */
 
@@ -496,6 +523,7 @@ BlockDriver *bdrv_find_protocol(const char *filename,
 len = sizeof(protocol) - 1;
 memcpy(protocol, filename, len);
 protocol[len] = '\0';
+
 QLIST_FOREACH(drv1, _drivers, list) {
 if (drv1->protocol_name &&
 !strcmp(drv1->protocol_name, protocol)) {
@@ -503,6 +531,23 @@ BlockDriver *bdrv_find_protocol(const char *filename,
 }
 }
 
+for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
+if (block_driver_modules[i].protocol_name &&
+!strcmp(block_driver_modules[i].protocol_name, protocol)) {
+block_module_load_one(block_driver_modules[i].library_name);
+/* Copying code is not nice, but this way the current discovery is
+ * not modified. Calling recursively could fail if the library
+ * has been deleted.
+ */
+QLIST_FOREACH(drv1, _drivers, list) {
+if (drv1->protocol_name &&
+!strcmp(drv1->protocol_name, protocol)) {
+return drv1;
+}
+}
+}
+}
+
 error_setg(errp, "Unknown protocol '%s'", protocol);
 return NULL;
 }
@@ -525,8 +570,15 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int 
buf_size,
 const char *filename)
 {
 int score_max = 0, score;
+size_t i;
 BlockDriver *drv = NULL, *d;
 
+for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
+if (block_driver_modules[i].has_probe) {
+block_module_load_one(block_driver_modules[i].library_name);
+}
+}
+
 QLIST_FOREACH(d, _drivers, list) {
 if (d->bdrv_probe) {
  

Re: [Qemu-devel] [PATCH] ssi: change ssi_slave_init to be a realize ops

[Paolo Bonzini]

On 15/06/2016 17:44, Cédric Le Goater wrote:
>  s->sd = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, true);
>  if (s->sd == NULL) {
> -return -1;

This needs an error_setg (see device_realize in hw/core/qdev.c for an
example) until sd_init is changed to take Error *.

Otherwise looks okay.

Paolo

> +return;
>  }
>  register_savevm(dev, "ssi_sd", -1, 1, ssi_sd_save, ssi_sd_load, s);
> -return 0;
>  }

[Qemu-devel] [PATCH 1/2] blockdev: Add dynamic generation of module_block.h

[Colin Lord]

From: Marc Mari 

To simplify the addition of new block modules, add a script that generates
include/qemu/module_block.h automatically from the modules' source code.

This script assumes that the QEMU coding style rules are followed.

Signed-off-by: Marc Marí 
Signed-off-by: Colin Lord 
---
 .gitignore  |   1 +
 Makefile|   8 +++
 scripts/modules/module_block.py | 134 
 3 files changed, 143 insertions(+)
 create mode 100644 scripts/modules/module_block.py

diff --git a/.gitignore b/.gitignore
index 38ee1c5..06aa064 100644
--- a/.gitignore
+++ b/.gitignore
@@ -110,3 +110,4 @@ tags
 TAGS
 docker-src.*
 *~
+/include/qemu/module_block.h
diff --git a/Makefile b/Makefile
index ed4032a..8f8b6a2 100644
--- a/Makefile
+++ b/Makefile
@@ -76,6 +76,8 @@ GENERATED_HEADERS += trace/generated-ust-provider.h
 GENERATED_SOURCES += trace/generated-ust.c
 endif
 
+GENERATED_HEADERS += include/qemu/module_block.h
+
 # Don't try to regenerate Makefile or configure
 # We don't generate any of them
 Makefile: ;
@@ -352,6 +354,12 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) 
libqemuutil.a libqemustub.a
 ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
$(call LINK, $^)
 
+include/qemu/module_block.h: $(SRC_PATH)/scripts/modules/module_block.py 
config-host.mak
+   $(call quiet-command,$(PYTHON) \
+$(SRC_PATH)/scripts/modules/module_block.py \
+   $(SRC_PATH)/"./include/qemu/" $(addprefix $(SRC_PATH)/,$(patsubst 
%.mo,%.c,$(block-obj-m))), \
+   "  GEN   $@")
+
 clean:
 # avoid old build problems by removing potentially incorrect old files
rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h 
gen-op-arm.h
diff --git a/scripts/modules/module_block.py b/scripts/modules/module_block.py
new file mode 100644
index 000..005bc49
--- /dev/null
+++ b/scripts/modules/module_block.py
@@ -0,0 +1,134 @@
+#!/usr/bin/python
+#
+# Module information generator
+#
+# Copyright Red Hat, Inc. 2015
+#
+# Authors:
+#  Marc Mari 
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+from __future__ import print_function
+import sys
+import os
+
+def get_string_struct(line):
+data = line.split()
+
+# data[0] -> struct element name
+# data[1] -> =
+# data[2] -> value
+
+return data[2].replace('"', '')[:-1]
+
+def add_module(fhader, library, format_name, protocol_name,
+probe, probe_device):
+lines = []
+lines.append('.library_name = "' + library + '",')
+if format_name != "":
+lines.append('.format_name = "' + format_name + '",')
+if protocol_name != "":
+lines.append('.protocol_name = "' + protocol_name + '",')
+if probe:
+lines.append('.has_probe = true,')
+if probe_device:
+lines.append('.has_probe_device = true,')
+
+text = '\n\t'.join(lines)
+fheader.write('\n\t{\n\t' + text + '\n\t},')
+
+def process_file(fheader, filename):
+# This parser assumes the coding style rules are being followed
+with open(filename, "r") as cfile:
+found_something = False
+found_start = False
+library, _ = os.path.splitext(os.path.basename(filename))
+for line in cfile:
+if found_start:
+line = line.replace('\n', '')
+if line.find(".format_name") != -1:
+format_name = get_string_struct(line)
+elif line.find(".protocol_name") != -1:
+protocol_name = get_string_struct(line)
+elif line.find(".bdrv_probe") != -1:
+probe = True
+elif line.find(".bdrv_probe_device") != -1:
+probe_device = True
+elif line == "};":
+add_module(fheader, library, format_name, protocol_name,
+probe, probe_device)
+found_start = False
+elif line.find("static BlockDriver") != -1:
+found_something = True
+found_start = True
+format_name = ""
+protocol_name = ""
+probe = False
+probe_device = False
+
+if not found_something:
+print("No BlockDriver struct found in " + filename + ". \
+Is this really a module?", file=sys.stderr)
+sys.exit(1)
+
+def print_top(fheader):
+fheader.write('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+/*
+ * QEMU Block Module Infrastructure
+ *
+ * Copyright Red Hat, Inc. 2015
+ *
+ * Authors:
+ *  Marc Mari   
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+''')
+
+fheader.write('''#ifndef QEMU_MODULE_BLOCK_H

Re: [Qemu-devel] [PATCH 1/3] log: Plug memory leak on multiple -dfilter

[Eric Blake]

On 06/15/2016 11:27 AM, Markus Armbruster wrote:
> -dfilter overwrites any previous filter.  The overwritten filter is
> leaked.  Leaks since the beginning (commit 3514552, v2.6.0).  Free it
> properly.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  util/log.c | 7 +++
>  1 file changed, 7 insertions(+)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 0/2] Dynamic module loading for block drivers

[Colin Lord]

This is a repost of some previous patches written by Marc Marí which
were also reposted by Richard Jones a few months ago. The original
series and reposted series are here:

https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01995.html
https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg01994.html

I've tried to take some of the previous feedback and integrate it into
this series. There are also a few things I haven't touched that were
previously mentioned though:

1) Denis Lunev suggested having block_module_load_one return the
loaded driver to reduce duplicated for loops in many of the functions
in block.c. I'd be happy to do this but wasn't completely sure how
error handling would happen in that case since currently the return
value is an integer error code. Would I switch to using the
error handling mechanisms provided in util/error.c?

2) In the past some debate was had about the design of the modules.
I haven't made any changes in this regard because it didn't seem a
conclusion had been reached. Some links for background:

Fam Zheng suggested a simpler parser:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02201.html

Denis Lunev suggested a design more similar to Linux kernel modules:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05331.html

Richard Jones suggested reasons to keep things as is:
https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg01994.html

Marc Mari (2):
  blockdev: Add dynamic generation of module_block.h
  blockdev: Add dynamic module loading for block drivers

 .gitignore  |   1 +
 Makefile|  11 +++-
 block.c |  86 +++---
 include/qemu/module.h   |   3 +
 scripts/modules/module_block.py | 134 
 util/module.c   |  37 +++
 6 files changed, 233 insertions(+), 39 deletions(-)
 create mode 100644 scripts/modules/module_block.py

-- 
2.5.5

Re: [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read

[Paolo Bonzini]

On 15/06/2016 19:18, P J P wrote:
>   Hello Paolo,
> 
> +-- On Wed, 15 Jun 2016, Paolo Bonzini wrote --+
> | Actually, the commit message is wrong.  The length parameter cannot
> | exceed the buffer size anymore.
> 
>   It wouldn't exceed after this patch, right? Is it possible 'esp_do_dma' is 
> called via 'esp_transfer_data' with 's->do_cmd' set? 'len' isn't checked 
> there.

No, it's not possible; see the discussion in reply to v1.

Paolo

[Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo

[Markus Armbruster]

PcPciInfo has two (ill-named) members: Range w32 is the PCI hole, and
w64 is the PCI64 hole.

Three users:

* I440FXState and MCHPCIState have a member PcPciInfo pci_info, but
  only pci_info.w32 is actually used.  This is confusing.  Replace by
  Range pci_hole.

* acpi_build() uses auto PcPciInfo pci_info to forward both PCI holes
  from acpi_get_pci_info() to build_dsdt().  Replace by two variables
  Range pci_hole, pci_hole64.  Rename acpi_get_pci_info() to
  acpi_get_pci_holes().

PcPciInfo is now unused; drop it.

Signed-off-by: Markus Armbruster 
---
 hw/i386/acpi-build.c  | 43 ++-
 hw/pci-host/piix.c| 10 +-
 hw/pci-host/q35.c | 12 ++--
 include/hw/i386/pc.h  |  5 -
 include/hw/pci-host/q35.h |  2 +-
 5 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 8ca2032..02fc534 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -225,26 +225,25 @@ static Object *acpi_get_i386_pci_host(void)
 return OBJECT(host);
 }
 
-static void acpi_get_pci_info(PcPciInfo *info)
+static void acpi_get_pci_holes(Range *hole, Range *hole64)
 {
 Object *pci_host;
 
-
 pci_host = acpi_get_i386_pci_host();
 g_assert(pci_host);
 
-info->w32.begin = object_property_get_int(pci_host,
-  PCI_HOST_PROP_PCI_HOLE_START,
-  NULL);
-info->w32.end = object_property_get_int(pci_host,
-PCI_HOST_PROP_PCI_HOLE_END,
-NULL);
-info->w64.begin = object_property_get_int(pci_host,
-  PCI_HOST_PROP_PCI_HOLE64_START,
-  NULL);
-info->w64.end = object_property_get_int(pci_host,
-PCI_HOST_PROP_PCI_HOLE64_END,
+hole->begin = object_property_get_int(pci_host,
+  PCI_HOST_PROP_PCI_HOLE_START,
+  NULL);
+hole->end = object_property_get_int(pci_host,
+PCI_HOST_PROP_PCI_HOLE_END,
+NULL);
+hole64->begin = object_property_get_int(pci_host,
+PCI_HOST_PROP_PCI_HOLE64_START,
 NULL);
+hole64->end = object_property_get_int(pci_host,
+  PCI_HOST_PROP_PCI_HOLE64_END,
+  NULL);
 }
 
 #define ACPI_PORT_SMI_CMD   0x00b2 /* TODO: this is APM_CNT_IOPORT */
@@ -1867,7 +1866,7 @@ static Aml *build_q35_osc_method(void)
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker,
AcpiPmInfo *pm, AcpiMiscInfo *misc,
-   PcPciInfo *pci, MachineState *machine)
+   Range *pci_hole, Range *pci_hole64, MachineState *machine)
 {
 CrsRangeEntry *entry;
 Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
@@ -2015,7 +2014,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
  AML_CACHEABLE, AML_READ_WRITE,
  0, 0x000A, 0x000B, 0, 0x0002));
 
-crs_replace_with_free_ranges(mem_ranges, pci->w32.begin, pci->w32.end - 1);
+crs_replace_with_free_ranges(mem_ranges,
+ pci_hole->begin, pci_hole->end - 1);
 for (i = 0; i < mem_ranges->len; i++) {
 entry = g_ptr_array_index(mem_ranges, i);
 aml_append(crs,
@@ -2025,12 +2025,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
  0, entry->limit - entry->base + 1));
 }
 
-if (pci->w64.begin) {
+if (pci_hole64->begin) {
 aml_append(crs,
 aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
  AML_CACHEABLE, AML_READ_WRITE,
- 0, pci->w64.begin, pci->w64.end - 1, 0,
- pci->w64.end - pci->w64.begin));
+ 0, pci_hole64->begin, pci_hole64->end - 1, 0,
+ pci_hole64->end - pci_hole64->begin));
 }
 
 if (misc->tpm_version != TPM_VERSION_UNSPEC) {
@@ -2518,7 +2518,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
*machine)
 AcpiPmInfo pm;
 AcpiMiscInfo misc;
 AcpiMcfgInfo mcfg;
-PcPciInfo pci;
+Range pci_hole, pci_hole64;
 uint8_t *u;
 size_t aml_len = 0;
 GArray *tables_blob = tables->table_data;
@@ -2526,7 +2526,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
*machine)
 
 acpi_get_pm_info();
 acpi_get_misc_info();
-acpi_get_pci_info();
+acpi_get_pci_holes(_hole, _hole64);
 acpi_get_slic_oem(_oem);
 
 table_offsets = g_array_new(false, 

[Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place

[Markus Armbruster]

Range pci_info.w32 records the location of the PCI hole.

It's initialized to empty when QOM zeroes I440FXState.  That's a fine
value for a still unknown PCI hole.

i440fx_init() sets pci_info.w32.begin = below_4g_mem_size.  Changes
the PCI hole from empty to [below_4g_mem_size, UINT64_MAX].  That's a
bogus value.

i440fx_pcihost_initfn() sets pci_info.end = IO_APIC_DEFAULT_ADDRESS.
Since i440fx_init() ran already, this changes the PCI hole to
[below_4g_mem_size, IO_APIC_DEFAULT_ADDRESS-1].  That's the correct
value.

Setting the bounds of the PCI hole in two separate places is
confusing, and begs the question whether the bogus intermediate value
could be used by something, or what would happen if we somehow managed
to realize an i440FX device without having run the board init function
i440fx_init() first.

Avoid the confusion by setting the (constant) upper bound along with
the lower bound in i440fx_init().

Signed-off-by: Markus Armbruster 
---
 hw/pci-host/piix.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index df2b0e2..c63c424 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -263,7 +263,6 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, 
Visitor *v,
 static void i440fx_pcihost_initfn(Object *obj)
 {
 PCIHostState *s = PCI_HOST_BRIDGE(obj);
-I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
 
 memory_region_init_io(>conf_mem, obj, _host_conf_le_ops, s,
   "pci-conf-idx", 4);
@@ -285,8 +284,6 @@ static void i440fx_pcihost_initfn(Object *obj)
 object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
 i440fx_pcihost_get_pci_hole64_end,
 NULL, NULL, NULL, NULL);
-
-d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
 static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
@@ -348,6 +345,7 @@ PCIBus *i440fx_init(const char *host_type, const char 
*pci_type,
 
 i440fx = I440FX_PCI_HOST_BRIDGE(dev);
 i440fx->pci_info.w32.begin = below_4g_mem_size;
+i440fx->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 
 /* setup pci memory mapping */
 pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
-- 
2.5.5

[Qemu-devel] [PATCH 0/2] Clean up around the PCI holes

[Markus Armbruster]

Markus Armbruster (2):
  piix: Set I440FXState member pci_info.w32 in one place
  pc: Eliminate PcPciInfo

 hw/i386/acpi-build.c  | 43 ++-
 hw/pci-host/piix.c| 12 +---
 hw/pci-host/q35.c | 12 ++--
 include/hw/i386/pc.h  |  5 -
 include/hw/pci-host/q35.h |  2 +-
 5 files changed, 34 insertions(+), 40 deletions(-)

-- 
2.5.5

[Qemu-devel] Odp.: [PATCH 6/9] m25p80: Introduce quad and equad modes.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 15.06.2016 o 16:25, Cédric Le Goater pisze:
> On 06/15/2016 03:41 PM, marcin.krzemin...@nokia.com wrote:
>> From: Marcin Krzeminski 
>>
>> Quad and Equad modes for Spansion and Macronix flash devices.
>> This commit also includes modification and new command to manipulate
>> quad mode (status registers and dedicated commands).
>> This work is based on Pawel Lenkow work.
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 70 
>> +++
>>  1 file changed, 65 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 55b4377..d1c4d46 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -281,6 +281,7 @@ typedef enum {
>>  JEDEC_READ = 0x9f,
>>  BULK_ERASE = 0xc7,
>>  READ_FSR = 0x70,
>> +RDCR = 0x15,
>>
>>  READ = 0x03,
>>  READ4 = 0x13,
>> @@ -317,6 +318,13 @@ typedef enum {
>>  RESET_ENABLE = 0x66,
>>  RESET_MEMORY = 0x99,
>>
>> +/*
>> + * Micron: 0x35 - enable QPI
>> + * Spansion: 0x35 - read control register
>> + */
>> +RDCR_EQIO = 0x35,
>> +RSTQIO = 0xf5,
>> +
>>  RNVCR = 0xB5,
>>  WNVCR = 0xB1,
>>
>> @@ -366,6 +374,7 @@ typedef struct Flash {
>>  bool write_enable;
>>  bool four_bytes_address_mode;
>>  bool reset_enable;
>> +bool quad_enable;
>>  uint8_t ear;
>>
>>  int64_t dirty_page;
>> @@ -586,6 +595,16 @@ static void complete_collecting_data(Flash *s)
>>  flash_erase(s, s->cur_addr, s->cmd_in_progress);
>>  break;
>>  case WRSR:
>> +switch (get_man(s)) {
>> +case MAN_SPANSION:
>> +s->quad_enable = !!(s->data[1] & 0x02);
>> +break;
>> +case MAN_MACRONIX:
>> +s->quad_enable = extract32(s->data[0], 6, 1);
>> +break;
>> +default:
>> +break;
>> +}
>>  if (s->write_enable) {
>>  s->write_enable = false;
>>  }
>> @@ -619,6 +638,7 @@ static void reset_memory(Flash *s)
>>  s->state = STATE_IDLE;
>>  s->write_enable = false;
>>  s->reset_enable = false;
>> +s->quad_enable = false;
>>
>>  switch (get_man(s)) {
>>  case MAN_NUMONYX:
>> @@ -747,10 +767,20 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>
>>  case WRSR:
>>  if (s->write_enable) {
>> -s->needed_bytes = 1;
>> +switch (get_man(s)) {
>> +case MAN_SPANSION:
>> +s->needed_bytes = 2;
>> +s->state = STATE_COLLECTING_DATA;
>> +break;
>> +case MAN_MACRONIX:
>> +s->needed_bytes = 2;
>> +s->state = STATE_COLLECTING_VAR_LEN_DATA;
>> +break;
>
> ah. I am glad you address this topic because I am having a little issue
> with the mx25l25635e and the mx25l25635f.
>
> mx25l25635e is a macronix which supports the WRSR command with one byte 
> and the mx25l25635f needs two. The fun part is that they have the same
> JEDEC : 0xc22019.
>
> So not all macronix need 2 bytes. Should we add a flag for that ? 
>
I think this case should be partially handled with path 4 and this one.
The new state (patch 4) was introduced to allow use variable write cmds.
As in my case mx66u51235f allow write up to 2 bytes, but linux writes only one,
and that is allowed and works on real HW. From the datasheet mx25l25635f
should behave exactly same way.
In case you mention, the problem might pop up, when guest will try
to write 2 bytes to mx25l25635e. Real HW will treat second byte as new command,
but this model accept this situation.
Generally my approach and I think original author was to not emulate exactly
real hw behaviour but allow to model it in such way the quest can boot up and 
use it.
If we go witch very restricted command behaviour we will end up in unreadable 
code
(eg. newest Spansions/Cypress family does not support extended address register,
but such commands will work in this model). The question is if you really need 
to flag
and support this. If yes additional if is not a problem.
>
>> +default:
>> +s->needed_bytes = 1;
>> +s->state = STATE_COLLECTING_DATA;
>> +}
>>  s->pos = 0;
>> -s->len = 0;
>> -s->state = STATE_COLLECTING_DATA;
>>  }
>>  break;
>>
>> @@ -763,6 +793,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>
>>  case RDSR:
>>  s->data[0] = (!!s->write_enable) << 1;
>> +if (get_man(s) == MAN_MACRONIX) {
>> +s->data[0] |= (!!s->quad_enable) << 6;
>> +}
>>  s->pos = 0;
>>  s->len = 1;
>>  s->state = STATE_READING_DATA;
>> @@ -789,6 +822,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  s->state = STATE_READING_DATA;
>>  break;
>>
>> +case RDCR:
>> +   

[Qemu-devel] [Bug 1569988] Re: [2.6] user network broken reaching foreign servers on Win64

[Stefan Weil]

Fixed since v2.6.0-rc3 release.

** Changed in: qemu
   Status: In Progress => Fix Released

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

Title:
  [2.6] user network broken reaching foreign servers on Win64

Status in QEMU:
  Fix Released

Bug description:
  Both on master and, starting with 2016-03-22 builds from
  http://qemu.weilnetz.de/w64/, user mode network can't reach foreign
  servers. For example, wget http://mifritscher resolves the DNS, but
  then the message "Network is unreachable" occures. 2016-03-03 works
  fine. I suspect the IPv6 changes. My connection is IPv4 only.

  I tested via knoppix 7.6. The command line is

  qemu-system-x86_64.exe -m 512 -k de --cdrom
  c:\Users\michaelfritscher\Downloads\linux\KNOPPIX_V7.4.2DVD-2014-09-28-DE.iso
  -netdev user,id=mynet0,restrict=n -device e1000,netdev=mynet0

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1569988/+subscriptions

[Qemu-devel] [PATCH] qapi: Fix crash on missing alternate member of QAPI struct

[Eric Blake]

If a QAPI struct has a mandatory alternate member which is not
present on input, the input visitor reports an error for the
missing alternate without setting the discriminator, but the
cleanup code for the struct still tries to use the dealloc
visitor to clean up the alternate.

Commit dbf11922 changed visit_start_alternate to set *obj to NULL
when an error occurs, where it was previously left untouched.
Thus, before the patch, the dealloc visitor is blindly trying to
cleanup whatever branch corresponds to (*obj)->type == 0 (that is,
QTYPE_NONE, because *obj still pointed to zeroed memory), which
selects the default branch of the switch and sets an error, but
this second error is ignored by the way the dealloc visitor is
used; but after the patch, the attempt to switch dereferences NULL.

When cleaning up after a partial object parse, we specifically
check for !*obj after visit_start_struct() (see gen_visit_object());
doing the same for alternates fixes the crash. Enhance the testsuite
to give coverage for both missing struct and missing alternate
members. Also add an abort - we expect visit_start_alternate() to
either set an error or to set (*obj)->type to a valid QType that
corresponds to actual user input, and QTYPE_NONE should never
be reachable from valid input.  Had the abort() been in place
earlier, we might have noticed the dealloc visitor dereferencing
bogus zeroed memory prior to when commit dbf11922 forced our hand
by setting *obj to NULL and causing a fault.

Test case:

{'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}}

The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat
struct, which has a mandatory 'file':'BlockdevRef' in QAPI.  Since
'file' is missing as a sibling of 'driver', this should report a
graceful error rather than fault.  After this patch, we are back to:

{"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}}

Generated code in qapi-visit.c changes as:

|@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v,
| if (err) {
| goto out;
| }
|+if (!*obj) {
|+goto out_obj;
|+}
| switch ((*obj)->type) {
| case QTYPE_QDICT:
| visit_start_struct(v, name, NULL, 0, );
|@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v,
| case QTYPE_QSTRING:
| visit_type_str(v, name, &(*obj)->u.reference, );
| break;
|+case QTYPE_NONE:
|+abort();
| default:
| error_setg(, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
|"BlockdevRef");
| }
|+out_obj:
| visit_end_alternate(v);

Reported by Kashyap Chamarthy 
CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
---
 scripts/qapi-visit.py  |  6 ++
 tests/test-qmp-input-visitor.c | 12 
 2 files changed, 18 insertions(+)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 70ea8ca..ffb635c 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,6 +172,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 if (err) {
 goto out;
 }
+if (!*obj) {
+goto out_obj;
+}
 switch ((*obj)->type) {
 ''',
  c_name=c_name(name), promote_int=promote_int)
@@ -206,10 +209,13 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 ''')

 ret += mcgen('''
+case QTYPE_NONE:
+abort();
 default:
 error_setg(, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"%(name)s");
 }
+out_obj:
 visit_end_alternate(v);
 if (err && visit_is_input(v)) {
 qapi_free_%(c_name)s(*obj);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 3b6b39e..1a4585c 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -766,6 +766,8 @@ static void test_visitor_in_errors(TestInputVisitorData 
*data,
 Error *err = NULL;
 Visitor *v;
 strList *q = NULL;
+UserDefTwo *r = NULL;
+WrapAlternate *s = NULL;

 v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', "
 "'string': -42 }");
@@ -778,6 +780,16 @@ static void test_visitor_in_errors(TestInputVisitorData 
*data,
 visit_type_strList(v, NULL, , );
 error_free_or_abort();
 assert(!q);
+
+v = visitor_input_test_init(data, "{ 'str':'hi' }");
+visit_type_UserDefTwo(v, NULL, , );
+error_free_or_abort();
+assert(!r);
+
+v = visitor_input_test_init(data, "{ }");
+visit_type_WrapAlternate(v, NULL, , );
+error_free_or_abort();
+assert(!s);
 }

 static void test_visitor_in_wrong_type(TestInputVisitorData *data,
-- 
2.5.5

[Qemu-devel] [PATCH v4] scsi: esp: check length before dma read

[P J P]

From: Prasad J Pandit 

While doing DMA read into ESP command buffer 's->cmdbuf', it could
write past the 's->cmdbuf' area, if it was partially filled;
ie. 's->cmdlen' wasn't set at the start of the buffer.
Check 'len' to avoid OOB access. Also increase the command buffer
size to 32, which is maximum when 's->do_cmd' is set.

Reported-by: Li Qiang 
Signed-off-by: Prasad J Pandit 
---
 hw/scsi/esp.c | 6 --
 include/hw/scsi/esp.h | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

Update as per
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg04269.html

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4b94bbc..cc74ba8 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -249,6 +249,8 @@ static void esp_do_dma(ESPState *s)
 len = s->dma_left;
 if (s->do_cmd) {
 trace_esp_do_dma(s->cmdlen, len);
+assert (s->cmdlen <= sizeof(s->cmdbuf) &&
+len <= sizeof(s->cmdbuf) - s->cmdlen);
 s->dma_memory_read(s->dma_opaque, >cmdbuf[s->cmdlen], len);
 s->ti_size = 0;
 s->cmdlen = 0;
@@ -348,7 +350,7 @@ static void handle_ti(ESPState *s)
 s->dma_counter = dmalen;
 
 if (s->do_cmd)
-minlen = (dmalen < 32) ? dmalen : 32;
+minlen = (dmalen < ESP_CMDBUF_SZ) ? dmalen : ESP_CMDBUF_SZ;
 else if (s->ti_size < 0)
 minlen = (dmalen < -s->ti_size) ? dmalen : -s->ti_size;
 else
@@ -452,7 +454,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t 
val)
 break;
 case ESP_FIFO:
 if (s->do_cmd) {
-if (s->cmdlen < TI_BUFSZ) {
+if (s->cmdlen < ESP_CMDBUF_SZ) {
 s->cmdbuf[s->cmdlen++] = val & 0xff;
 } else {
 trace_esp_error_fifo_overrun();
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 6c79527..d2c4886 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -14,6 +14,7 @@ void esp_init(hwaddr espaddr, int it_shift,
 
 #define ESP_REGS 16
 #define TI_BUFSZ 16
+#define ESP_CMDBUF_SZ 32
 
 typedef struct ESPState ESPState;
 
@@ -31,7 +32,7 @@ struct ESPState {
 SCSIBus bus;
 SCSIDevice *current_dev;
 SCSIRequest *current_req;
-uint8_t cmdbuf[TI_BUFSZ];
+uint8_t cmdbuf[ESP_CMDBUF_SZ];
 uint32_t cmdlen;
 uint32_t do_cmd;
 
-- 
2.5.5

[Qemu-devel] [PATCH 1/3] log: Plug memory leak on multiple -dfilter

[Markus Armbruster]

-dfilter overwrites any previous filter.  The overwritten filter is
leaked.  Leaks since the beginning (commit 3514552, v2.6.0).  Free it
properly.

Signed-off-by: Markus Armbruster 
---
 util/log.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/util/log.c b/util/log.c
index 5ad72c1..6f45e0a 100644
--- a/util/log.c
+++ b/util/log.c
@@ -145,9 +145,16 @@ bool qemu_log_in_addr_range(uint64_t addr)
 void qemu_set_dfilter_ranges(const char *filter_spec)
 {
 gchar **ranges = g_strsplit(filter_spec, ",", 0);
+
+if (debug_regions) {
+g_array_unref(debug_regions);
+debug_regions = NULL;
+}
+
 if (ranges) {
 gchar **next = ranges;
 gchar *r = *next++;
+
 debug_regions = g_array_sized_new(FALSE, FALSE,
   sizeof(Range), 
g_strv_length(ranges));
 while (r) {
-- 
2.5.5

[Qemu-devel] [PATCH 0/3] log: Fix error handling and a memory leak

[Markus Armbruster]

log.c has no maintainer.  I can take this through my error-next
branch.

Markus Armbruster (3):
  log: Plug memory leak on multiple -dfilter
  log: Fix qemu_set_dfilter_ranges() error reporting
  log: Fix qemu_set_log_filename() error handling

 bsd-user/main.c  |   3 +-
 include/qemu/log.h   |   4 +-
 linux-user/main.c|   3 +-
 monitor.c|   7 ++-
 tests/test-logging.c |  88 ++---
 trace/control.c  |   3 +-
 util/log.c   | 122 +++
 vl.c |   4 +-
 8 files changed, 104 insertions(+), 130 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 3/3] log: Fix qemu_set_log_filename() error handling

[Markus Armbruster]

When qemu_set_log_filename() detects an invalid file name, it reports
an error, closes the log file (if any), and starts logging to stderr
(unless daemonized or nothing is being logged).

This is wrong.  Asking for an invalid log file on the command line
should be fatal.  Asking for one in the monitor should fail without
messing up an existing logfile.

Fix by converting qemu_set_log_filename() to Error.  Pass it
_fatal, except for hmp_logfile report errors.

This also permits testing without a subprocess, so do that.

Signed-off-by: Markus Armbruster 
---
 bsd-user/main.c  |  3 ++-
 include/qemu/log.h   |  2 +-
 linux-user/main.c|  3 ++-
 monitor.c|  7 ++-
 tests/test-logging.c | 41 -
 trace/control.c  |  3 ++-
 util/log.c   |  6 +++---
 vl.c |  2 +-
 8 files changed, 25 insertions(+), 42 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 9f592be..3d6a4f4 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 
+#include "qapi/error.h"
 #include "qemu.h"
 #include "qemu/path.h"
 #include "qemu/help_option.h"
@@ -848,7 +849,7 @@ int main(int argc, char **argv)
 
 /* init debug */
 qemu_log_needs_buffers();
-qemu_set_log_filename(log_file);
+qemu_set_log_filename(log_file, _fatal);
 if (log_mask) {
 int mask;
 
diff --git a/include/qemu/log.h b/include/qemu/log.h
index df8d041..8bec6b4 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -106,7 +106,7 @@ extern const QEMULogItem qemu_log_items[];
 
 void qemu_set_log(int log_flags);
 void qemu_log_needs_buffers(void);
-void qemu_set_log_filename(const char *filename);
+void qemu_set_log_filename(const char *filename, Error **errp);
 void qemu_set_dfilter_ranges(const char *ranges, Error **errp);
 bool qemu_log_in_addr_range(uint64_t addr);
 int qemu_str_to_log_mask(const char *str);
diff --git a/linux-user/main.c b/linux-user/main.c
index f8a8764..1dd8af3 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 
+#include "qapi/error.h"
 #include "qemu.h"
 #include "qemu/path.h"
 #include "qemu/cutils.h"
@@ -3846,7 +3847,7 @@ static void handle_arg_log(const char *arg)
 
 static void handle_arg_log_filename(const char *arg)
 {
-qemu_set_log_filename(arg);
+qemu_set_log_filename(arg, _fatal);
 }
 
 static void handle_arg_set_env(const char *arg)
diff --git a/monitor.c b/monitor.c
index a27e115..29a51bf 100644
--- a/monitor.c
+++ b/monitor.c
@@ -,7 +,12 @@ void qmp_client_migrate_info(const char *protocol, const 
char *hostname,
 
 static void hmp_logfile(Monitor *mon, const QDict *qdict)
 {
-qemu_set_log_filename(qdict_get_str(qdict, "filename"));
+Error *err = NULL;
+
+qemu_set_log_filename(qdict_get_str(qdict, "filename"), );
+if (err) {
+error_report_err(err);
+}
 }
 
 static void hmp_log(Monitor *mon, const QDict *qdict)
diff --git a/tests/test-logging.c b/tests/test-logging.c
index e043adc..440e75f 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -75,49 +75,24 @@ static void test_parse_range(void)
 error_free_or_abort();
 }
 
-#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
-/* As the only real failure from a bad log filename path spec is
- * reporting to the user we have to use the g_test_trap_subprocess
- * mechanism and check no errors reported on stderr.
- */
-static void test_parse_path_subprocess(void)
-{
-/* All these should work without issue */
-qemu_set_log_filename("/tmp/qemu.log");
-qemu_set_log_filename("/tmp/qemu-%d.log");
-qemu_set_log_filename("/tmp/qemu.log.%d");
-}
 static void test_parse_path(void)
 {
-g_test_trap_subprocess ("/logging/parse_path/subprocess", 0, 0);
-g_test_trap_assert_passed();
-g_test_trap_assert_stdout("");
-g_test_trap_assert_stderr("");
+Error *err = NULL;
+
+qemu_set_log_filename("/tmp/qemu.log", _abort);
+qemu_set_log_filename("/tmp/qemu-%d.log", _abort);
+qemu_set_log_filename("/tmp/qemu.log.%d", _abort);
+
+qemu_set_log_filename("/tmp/qemu-%d%d.log", );
+error_free_or_abort();
 }
-static void test_parse_invalid_path_subprocess(void)
-{
-qemu_set_log_filename("/tmp/qemu-%d%d.log");
-}
-static void test_parse_invalid_path(void)
-{
-g_test_trap_subprocess ("/logging/parse_invalid_path/subprocess", 0, 0);
-g_test_trap_assert_passed();
-g_test_trap_assert_stdout("");
-g_test_trap_assert_stderr("Bad logfile format: /tmp/qemu-%d%d.log\n");
-}
-#endif /* CONFIG_HAS_GLIB_SUBPROCESS_TESTS */
 
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
 
 g_test_add_func("/logging/parse_range", test_parse_range);
-#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
 g_test_add_func("/logging/parse_path", test_parse_path);
-g_test_add_func("/logging/parse_path/subprocess", 
test_parse_path_subprocess);
-g_test_add_func("/logging/parse_invalid_path", 

[Qemu-devel] [PATCH 2/3] log: Fix qemu_set_dfilter_ranges() error reporting

[Markus Armbruster]

g_error() is not an acceptable way to report errors to the user:

$ qemu-system-x86_64 -dfilter 1000+0

** (process:17187): ERROR **: Failed to parse range in: 1000+0
Trace/breakpoint trap (core dumped)

g_assert() isn't, either:

$ qemu-system-x86_64 -dfilter 1000x+64
**
ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges: assertion 
failed: (e == range_op)
Aborted (core dumped)

Convert qemu_set_dfilter_ranges() to Error.  Rework its deeply nested
control flow.  Touch up the error messages.  Call it with
_fatal.

This also permits testing without a subprocess, so do that.

Signed-off-by: Markus Armbruster 
---
 include/qemu/log.h   |   2 +-
 tests/test-logging.c |  49 --
 util/log.c   | 113 ++-
 vl.c |   2 +-
 4 files changed, 75 insertions(+), 91 deletions(-)

diff --git a/include/qemu/log.h b/include/qemu/log.h
index 234fa81..df8d041 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -107,7 +107,7 @@ extern const QEMULogItem qemu_log_items[];
 void qemu_set_log(int log_flags);
 void qemu_log_needs_buffers(void);
 void qemu_set_log_filename(const char *filename);
-void qemu_set_dfilter_ranges(const char *ranges);
+void qemu_set_dfilter_ranges(const char *ranges, Error **errp);
 bool qemu_log_in_addr_range(uint64_t addr);
 int qemu_str_to_log_mask(const char *str);
 
diff --git a/tests/test-logging.c b/tests/test-logging.c
index 5ef5bb8..e043adc 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -27,11 +27,14 @@
 #include "qemu/osdep.h"
 
 #include "qemu-common.h"
-#include "include/qemu/log.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
 
 static void test_parse_range(void)
 {
-qemu_set_dfilter_ranges("0x1000+0x100");
+Error *err = NULL;
+
+qemu_set_dfilter_ranges("0x1000+0x100", _abort);
 
 g_assert_false(qemu_log_in_addr_range(0xfff));
 g_assert(qemu_log_in_addr_range(0x1000));
@@ -39,56 +42,40 @@ static void test_parse_range(void)
 g_assert(qemu_log_in_addr_range(0x10ff));
 g_assert_false(qemu_log_in_addr_range(0x1100));
 
-qemu_set_dfilter_ranges("0x1000-0x100");
+qemu_set_dfilter_ranges("0x1000-0x100", _abort);
 
 g_assert_false(qemu_log_in_addr_range(0x1001));
 g_assert(qemu_log_in_addr_range(0x1000));
 g_assert(qemu_log_in_addr_range(0x0f01));
 g_assert_false(qemu_log_in_addr_range(0x0f00));
 
-qemu_set_dfilter_ranges("0x1000..0x1100");
+qemu_set_dfilter_ranges("0x1000..0x1100", _abort);
 
 g_assert_false(qemu_log_in_addr_range(0xfff));
 g_assert(qemu_log_in_addr_range(0x1000));
 g_assert(qemu_log_in_addr_range(0x1100));
 g_assert_false(qemu_log_in_addr_range(0x1101));
 
-qemu_set_dfilter_ranges("0x1000..0x1000");
+qemu_set_dfilter_ranges("0x1000..0x1000", _abort);
 
 g_assert_false(qemu_log_in_addr_range(0xfff));
 g_assert(qemu_log_in_addr_range(0x1000));
 g_assert_false(qemu_log_in_addr_range(0x1001));
 
-qemu_set_dfilter_ranges("0x1000+0x100,0x2100-0x100,0x3000..0x3100");
+qemu_set_dfilter_ranges("0x1000+0x100,0x2100-0x100,0x3000..0x3100",
+_abort);
 g_assert(qemu_log_in_addr_range(0x1050));
 g_assert(qemu_log_in_addr_range(0x2050));
 g_assert(qemu_log_in_addr_range(0x3050));
+
+qemu_set_dfilter_ranges("0x1000+onehundred", );
+error_free_or_abort();
+
+qemu_set_dfilter_ranges("0x1000+0", );
+error_free_or_abort();
 }
 
 #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
-static void test_parse_invalid_range_subprocess(void)
-{
-qemu_set_dfilter_ranges("0x1000+onehundred");
-}
-static void test_parse_invalid_range(void)
-{
-g_test_trap_subprocess("/logging/parse_invalid_range/subprocess", 0, 0);
-g_test_trap_assert_failed();
-g_test_trap_assert_stdout("");
-g_test_trap_assert_stderr("*Failed to parse range in: 
0x1000+onehundred\n");
-}
-static void test_parse_zero_range_subprocess(void)
-{
-qemu_set_dfilter_ranges("0x1000+0");
-}
-static void test_parse_zero_range(void)
-{
-g_test_trap_subprocess("/logging/parse_zero_range/subprocess", 0, 0);
-g_test_trap_assert_failed();
-g_test_trap_assert_stdout("");
-g_test_trap_assert_stderr("*Failed to parse range in: 0x1000+0\n");
-}
-
 /* As the only real failure from a bad log filename path spec is
  * reporting to the user we have to use the g_test_trap_subprocess
  * mechanism and check no errors reported on stderr.
@@ -126,10 +113,6 @@ int main(int argc, char **argv)
 
 g_test_add_func("/logging/parse_range", test_parse_range);
 #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
-g_test_add_func("/logging/parse_invalid_range/subprocess", 
test_parse_invalid_range_subprocess);
-g_test_add_func("/logging/parse_invalid_range", test_parse_invalid_range);
-g_test_add_func("/logging/parse_zero_range/subprocess", 
test_parse_zero_range_subprocess);
-

Re: [Qemu-devel] [Bug 739785] Re: qemu-i386 user mode can't fork (bash: fork: Invalid argument)

[Justin Shafer]

../qemu-0.14.0/i386-linux-user/qemu-i386 -L ./gnemul/qemu-i386 i386/ls -l
dummyfile

0.14.0??? I tried the latest qemu and it worked.. I forget the version..
1.XX something? I was able to run wine. It could also be your ld.so in
gnemul?


On Wed, Jun 15, 2016 at 7:41 AM, T. Huth <739...@bugs.launchpad.net>
wrote:

> ** Changed in: qemu
>Status: Fix Committed => Fix Released
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/739785
>
> Title:
>   qemu-i386 user mode can't fork (bash: fork: Invalid argument)
>
> Status in QEMU:
>   Fix Released
> Status in qemu package in Debian:
>   Fix Released
>
> Bug description:
>   Good time of day everybody,
>
>   I have been trying to make usermode qemu on ARM with plugapps
>   (archlinux) with archlinux i386 chroot to work.
>
>   1. I installed arch linux in a virtuabox and created a chroot for it
> with mkarchroot. Transferred it to my pogo plug into /i386/
>   2. I comiled qemu-i386 static and put it into /i386/usr/bin/
>   ./configure --static --disable-blobs --disable-system
> --target-list=i386-linux-user
>   make
>
>   3. I also compiled linux kernel 2.6.38 with CONFIG_BINFMT_MISC=y and
> installed it.
>   uname -a
>   Linux Plugbox 2.6.38 #4 PREEMPT Fri Mar 18 22:19:10 CDT 2011 armv5tel
> Feroceon 88FR131 rev 1 (v5l) Marvell SheevaPlug Reference Board GNU/Linux
>
>   4. Added the following options into /etc/rc.local
>   /sbin/modprobe binfmt_misc
>   /bin/mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc
>   echo
> ':qemu-i386:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfb\xff\xff\xff:/usr/bin/qemu-i386:'
> >/proc/sys/fs/binfmt_misc/register
>
>   5. Also copied ld-linux.so.3 (actually ld-2.13.so because ld-
>   linux.so.3 is a link to that file) from /lib/ to /i386/lib/
>
>   6.Now i chroot into /i386 and I get this:
>   [root@Plugbox i386]# chroot .
>   [II aI hnve ao n@P /]# pacman -Suy
>   bash: fork: Invalid argument
>
>   7.I also downloaded linux-user-test-0.3 from qemu website and ran the
> test:
>   [root@Plugbox linux-user-test-0.3]# make
>   ./qemu-linux-user.sh
>   [qemu-i386]
>   ../qemu-0.14.0/i386-linux-user/qemu-i386 -L ./gnemul/qemu-i386 i386/ls
> -l dummyfile
>   BUG IN DYNAMIC LINKER ld.so: dl-version.c: 210: _dl_check_map_versions:
> Assertion `needed != ((void *)0)' failed!
>   make: *** [test] Error 127
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/qemu/+bug/739785/+subscriptions
>


-- 
Justin Shafer
Onsite Dental Systems
7704 Sagebrush Ct. S.
North Richland Hills, TX. 76182
(817) 909-4222

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

Title:
  qemu-i386 user mode can't fork (bash: fork: Invalid argument)

Status in QEMU:
  Fix Released
Status in qemu package in Debian:
  Fix Released

Bug description:
  Good time of day everybody,

  I have been trying to make usermode qemu on ARM with plugapps
  (archlinux) with archlinux i386 chroot to work.

  1. I installed arch linux in a virtuabox and created a chroot for it with 
mkarchroot. Transferred it to my pogo plug into /i386/
  2. I comiled qemu-i386 static and put it into /i386/usr/bin/
  ./configure --static --disable-blobs --disable-system 
--target-list=i386-linux-user
  make

  3. I also compiled linux kernel 2.6.38 with CONFIG_BINFMT_MISC=y and 
installed it.
  uname -a
  Linux Plugbox 2.6.38 #4 PREEMPT Fri Mar 18 22:19:10 CDT 2011 armv5tel 
Feroceon 88FR131 rev 1 (v5l) Marvell SheevaPlug Reference Board GNU/Linux

  4. Added the following options into /etc/rc.local
  /sbin/modprobe binfmt_misc
  /bin/mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc
  echo 
':qemu-i386:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfb\xff\xff\xff:/usr/bin/qemu-i386:'
 >/proc/sys/fs/binfmt_misc/register

  5. Also copied ld-linux.so.3 (actually ld-2.13.so because ld-
  linux.so.3 is a link to that file) from /lib/ to /i386/lib/

  6.Now i chroot into /i386 and I get this:
  [root@Plugbox i386]# chroot .
  [II aI hnve ao n@P /]# pacman -Suy
  bash: fork: Invalid argument

  7.I also downloaded linux-user-test-0.3 from qemu website and ran the test:
  [root@Plugbox linux-user-test-0.3]# make
  ./qemu-linux-user.sh
  [qemu-i386]
  ../qemu-0.14.0/i386-linux-user/qemu-i386 -L ./gnemul/qemu-i386 i386/ls -l 
dummyfile
  BUG IN DYNAMIC LINKER ld.so: dl-version.c: 210: _dl_check_map_versions: 
Assertion `needed != ((void *)0)' failed!
  make: *** [test] Error 127

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/739785/+subscriptions

[Qemu-devel] [Bug 739785] Re: qemu-i386 user mode can't fork (bash: fork: Invalid argument)

[T. Huth]

** Changed in: qemu
   Status: Fix Committed => Fix Released

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

Title:
  qemu-i386 user mode can't fork (bash: fork: Invalid argument)

Status in QEMU:
  Fix Released
Status in qemu package in Debian:
  Fix Released

Bug description:
  Good time of day everybody,

  I have been trying to make usermode qemu on ARM with plugapps
  (archlinux) with archlinux i386 chroot to work.

  1. I installed arch linux in a virtuabox and created a chroot for it with 
mkarchroot. Transferred it to my pogo plug into /i386/
  2. I comiled qemu-i386 static and put it into /i386/usr/bin/
  ./configure --static --disable-blobs --disable-system 
--target-list=i386-linux-user
  make

  3. I also compiled linux kernel 2.6.38 with CONFIG_BINFMT_MISC=y and 
installed it.
  uname -a
  Linux Plugbox 2.6.38 #4 PREEMPT Fri Mar 18 22:19:10 CDT 2011 armv5tel 
Feroceon 88FR131 rev 1 (v5l) Marvell SheevaPlug Reference Board GNU/Linux

  4. Added the following options into /etc/rc.local
  /sbin/modprobe binfmt_misc
  /bin/mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc
  echo 
':qemu-i386:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfb\xff\xff\xff:/usr/bin/qemu-i386:'
 >/proc/sys/fs/binfmt_misc/register

  5. Also copied ld-linux.so.3 (actually ld-2.13.so because ld-
  linux.so.3 is a link to that file) from /lib/ to /i386/lib/

  6.Now i chroot into /i386 and I get this:
  [root@Plugbox i386]# chroot .
  [II aI hnve ao n@P /]# pacman -Suy
  bash: fork: Invalid argument

  7.I also downloaded linux-user-test-0.3 from qemu website and ran the test:
  [root@Plugbox linux-user-test-0.3]# make
  ./qemu-linux-user.sh
  [qemu-i386]
  ../qemu-0.14.0/i386-linux-user/qemu-i386 -L ./gnemul/qemu-i386 i386/ls -l 
dummyfile
  BUG IN DYNAMIC LINKER ld.so: dl-version.c: 210: _dl_check_map_versions: 
Assertion `needed != ((void *)0)' failed!
  make: *** [test] Error 127

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/739785/+subscriptions