[Qemu-devel] [PATCH v1] cirrus: keep vga vram pointer within bounds

[P J P]

From: Prasad J Pandit 

'dst' pointer could exceed vga vram size when writing to the
cirrus memory area; it'd lead to an OOB access issue. Add check
to avoid it.

Reported-by: Niu Guoxiang 
Signed-off-by: Prasad J Pandit 
---
 hw/display/cirrus_vga.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

Update: fixed coding style error, add space around '<<' operator.

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index afc290ab91..9f1f856679 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2047,8 +2047,11 @@ static void 
cirrus_mem_writeb_mode4and5_8bpp(CirrusVGAState * s,
}
val <<= 1;
dst++;
+if (dst >= s->vga.vram_ptr + s->vga.vram_size) {
+break;
+}
 }
-memory_region_set_dirty(>vga.vram, offset, 8);
+memory_region_set_dirty(>vga.vram, offset, x);
 }
 
 static void cirrus_mem_writeb_mode4and5_16bpp(CirrusVGAState * s,
@@ -2071,8 +2074,11 @@ static void 
cirrus_mem_writeb_mode4and5_16bpp(CirrusVGAState * s,
}
val <<= 1;
dst += 2;
+if (dst >= s->vga.vram_ptr + s->vga.vram_size) {
+break;
+}
 }
-memory_region_set_dirty(>vga.vram, offset, 16);
+memory_region_set_dirty(>vga.vram, offset, x << 1);
 }
 
 /***
-- 
2.13.6

Re: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern checking.

[no-reply]

Hi,

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

Type: series
Message-id: 1507696406-11168-1-git-send-email-jiang.bi...@zte.com.cn
Subject: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern 
checking.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5a8e815e78 checkpatch: replace ERROR with WARN for extern checking.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: replace ERROR with WARN for extern checking
ERROR: line over 90 characters
#25: FILE: scripts/checkpatch.pl:2550:
+   WARN("externs should be avoided in .c files\n" 
.  $herecurr);

WARNING: line over 80 characters
#34: FILE: scripts/checkpatch.pl:2560:
+   WARN("externs should be avoided in .c files\n" .  
$herecurr);

total: 1 errors, 1 warnings, 16 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern checking.

[Jiang Biao]

There are some rare cases which need external declarations in .c
files. patchew.org and checkpatch.pl will complain errors on
patches for these declarations.

Degrade ERROR to WARN to erase the error complaints taking
checkpatch.pl in kernel as reference.

Signed-off-by: Jiang Biao 
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3c0a28e..9123788 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2546,7 +2546,7 @@ sub process {
if ($s =~ /^\s*;/ &&
$function_name ne 'uninitialized_var')
{
-   ERROR("externs should be avoided in .c files\n" 
.  $herecurr);
+   WARN("externs should be avoided in .c files\n" 
.  $herecurr);
}
 
if ($paren_space =~ /\n/) {
@@ -2556,7 +2556,7 @@ sub process {
} elsif ($realfile =~ /\.c$/ && defined $stat &&
$stat =~ /^.\s*extern\s+/)
{
-   ERROR("externs should be avoided in .c files\n" .  
$herecurr);
+   WARN("externs should be avoided in .c files\n" .  
$herecurr);
}
 
 # check for pointless casting of g_malloc return
-- 
2.9.5

Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH

[Dong Jia Shi]

* Halil Pasic  [2017-10-10 12:06:23 +0200]:

> 
> 
> On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> > * Halil Pasic  [2017-10-04 17:41:39 +0200]:
> > 
> > [...]
> > 
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 4f47dbc8b0..b2978c3bae 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1003,12 +1003,11 @@ static void sch_handle_start_func_virtual(SubchDev 
> >> *sch)
> >>
> >>  }
> >>
> >> -static int sch_handle_start_func_passthrough(SubchDev *sch)
> >> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
> >>  {
> >>
> >>  PMCW *p = >curr_status.pmcw;
> >>  SCSW *s = >curr_status.scsw;
> >> -int ret;
> >>
> >>  ORB *orb = >orb;
> >>  if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> >> @@ -1022,31 +1021,11 @@ static int 
> >> sch_handle_start_func_passthrough(SubchDev *sch)
> >>   */
> >>  if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> >>  !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> >> -return -EINVAL;
> >> +sch_gen_unit_exception(sch);
> >> +css_inject_io_interrupt(sch);
> > Last cycle, we agreed to add some log here. Sth. like:
> > warn_report("vfio-ccw requires PFCH and C64 flags set...");
> > 
> > I promised to do a fix for this piece of code. But since this patch
> > already fixed it, I guess what I have to do is to add the log only? Or
> > you would like to add it by yourself? ;)
> > 
> 
> I think I forgot this one. Should there be a v3 I could add this too.
> Otherwise I would not mind if you do it on top.
> 
[...]

> >> @@ -1084,16 +1063,15 @@ int do_subchannel_work_passthrough(SubchDev *sch)
> >>  /* TODO: Halt handling */
> >>  sch_handle_halt_func(sch);
> >>  } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
> >> -ret = sch_handle_start_func_passthrough(sch);
> >> +return sch_handle_start_func_passthrough(sch);
> >>  }
> >> -
> >> -return ret;
> >> +return (IOInstEnding){.cc = 0};
> >>  }
> >>
> >> -static int do_subchannel_work(SubchDev *sch)
> >> +static IOInstEnding do_subchannel_work(SubchDev *sch)
> >>  {
> >>  if (!sch->do_subchannel_work) {
> >> -return -EINVAL;
> >> +return (IOInstEnding){.cc = 1};
> > This keeps the logic here as-is, so it is right.
> > 
> 
> Yep.
> 
> > Anybody agrees that also adding an assert() here?
> 
> With automated regression testing in place I'm for it, without
> I feel uncomfortable doing it myself. You could do this
> on top if you like.
Got it.

Marked. I will look back after this series.

[...]

> 
> Except for the missing warning are you OK with the rest
> of the patch? I would like to re-claim your r-b (dropped
> because changes weren't just minor).

I replied to the patch thread - the main part looks good to me.

I will save my r-b for the next round. ;)

-- 
Dong Jia Shi

[Qemu-devel] [Bug 1659901] Re: Regression: SIGSEGV running Java

[Edward Vielmetti]

Similar issue reported in two other places on the net:

https://github.com/multiarch/qemu-user-static/issues/18 "qemu-arm-static
2.8 and Java+Maven setup not working"

https://bugs.linaro.org/show_bug.cgi?id=3259#c4 Bug 3259 - Javac fails
within qemu-aarch64-static chroot on x86

** Bug watch added: github.com/multiarch/qemu-user-static/issues #18
   https://github.com/multiarch/qemu-user-static/issues/18

** Bug watch added: Linaro Bug Tracking System #3259
   https://bugs.linaro.org/show_bug.cgi?id=3259

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

Title:
  Regression: SIGSEGV running Java

Status in QEMU:
  New

Bug description:
  I have a build script that bootstraps a Debian armhf image. Part of
  the process involves running a Java program while inside a chroot. I
  am using Debian's qemu-user-static package to run the armhf Java
  binary on an amd64 system.

  qemu-user-static version 1:2.7+dfsg-3~bpo8+2 works fine. Version
  1:2.8+dfsg-1~bpo8+1 always causes Java to crash with a SIGSEGV. The
  location of the crash appears to be random and hasn't been the same
  twice.

  I am using the Azul Systems Zulu Embedded Java runtime, rather than
  the regular OpenJDK runtime, because the Zulu runtime has an arm32 JIT
  whereas OpenJDK is interpreter-only on arm32.

  I can reproduce the problem easily by mounting the image created by my
  build script and executing "java -XshowSettings -version" in a chroot.
  I can give you the image if that would help debug the problem.

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

Re: [Qemu-devel] [PATCH v2 1/6] aspeed: add support for the witherspoon-bmc board

[Andrew Jeffery]

On Tue, 2017-10-10 at 15:30 +0200, Cédric Le Goater wrote:
> On 10/09/2017 02:04 AM, Andrew Jeffery wrote:
> > On Wed, 2017-09-20 at 09:01 +0200, Cédric Le Goater wrote:
> > > The Witherspoon boards are OpenPOWER system hosting POWER9 Processors.
> > > Let's add support for their BMC including a couple of I2C devices as
> > > found on real HW.
> > >  
> > > > > > Signed-off-by: Cédric Le Goater 
> > > ---
> > >  hw/arm/aspeed.c | 49 +
> > >  1 file changed, 49 insertions(+)
> > >  
> > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > > index ab895ad490af..81f522f711ae 100644
> > > --- a/hw/arm/aspeed.c
> > > +++ b/hw/arm/aspeed.c
> > > @@ -46,6 +46,7 @@ enum {
> > >  PALMETTO_BMC,
> > >  AST2500_EVB,
> > >  ROMULUS_BMC,
> > > +WITHERSPOON_BMC,
> > >  };
> > >  
> > >  /* Palmetto hardware value: 0x120CE416 */
> > > @@ -83,8 +84,12 @@ enum {
> > >  SCU_AST2500_HW_STRAP_ACPI_ENABLE |  \
> > >  SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER))
> > >  
> > > +/* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
> > > +#define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
> > > +
> > >  static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
> > >  static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
> > > +static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
> > >  
> > >  static const AspeedBoardConfig aspeed_boards[] = {
> > >  [PALMETTO_BMC] = {
> > > @@ -110,6 +115,14 @@ static const AspeedBoardConfig aspeed_boards[] = {
> > >  .spi_model = "mx66l1g45g",
> > >  .num_cs= 2,
> > >  },
> > > +[WITHERSPOON_BMC]  = {
> > > +.soc_name  = "ast2500-a1",
> > > +.hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
> > > +.fmc_model = "mx25l25635e",
> > > +.spi_model = "mx66l1g45g",
> > > +.num_cs= 2,
> > > +.i2c_init  = witherspoon_bmc_i2c_init,
> > > +},
> > >  };
> > >  
> > >  #define FIRMWARE_ADDR 0x0
> > > @@ -337,11 +350,47 @@ static const TypeInfo romulus_bmc_type = {
> > >  .class_init = romulus_bmc_class_init,
> > >  };
> > >  
> > > +static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
> > > +{
> > > +AspeedSoCState *soc = >soc;
> > > +
> > > +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 4), "tmp423", 
> > > 0x4c);
> > > +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 5), "tmp423", 
> > > 0x4c);
> > > +
> > > +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 9), "tmp105", 
> > > 0x4a);
> > 
> > Looks like I need to track down newer versions of the schematics I have.
> 
> the device on the board is a tmp275 but the tmp105 model is compatible.

It neither device is listed in the version I have :)

Andrew

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH

[Dong Jia Shi]

* Halil Pasic  [2017-10-04 17:41:39 +0200]:

> Simplify the error handling of the SSCH and RSCH handler avoiding
> arbitrary and cryptic error codes being used to tell how the instruction
> is supposed to end.  Let the code detecting the condition tell how it's
> to be handled in a less ambiguous way.  It's best to handle SSCH and RSCH
> in one go as the emulation of the two shares a lot of code.
> 
> For passthrough this change isn't pure refactoring, but changes the
> way kernel reported EFAULT is handled. After clarifying the kernel
> interface we decided that EFAULT shall be mapped to unit exception.
> Same goes for unexpected error codes.
> 
> Signed-off-by: Halil Pasic 
> ---
> 
> AFAIR we decided in the previous round to rather do transformation
> and fixing in one patch than touch stuff twice. Hence this patch
> ain't pure transformation any more.
> ---
>  hw/s390x/css.c  | 83 
> +
>  hw/s390x/s390-ccw.c | 11 +++---
>  hw/vfio/ccw.c   | 30 
>  include/hw/s390x/css.h  | 24 +
>  include/hw/s390x/s390-ccw.h |  2 +-
>  target/s390x/ioinst.c   | 53 -
>  6 files changed, 77 insertions(+), 126 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 4f47dbc8b0..b2978c3bae 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1003,12 +1003,11 @@ static void sch_handle_start_func_virtual(SubchDev 
> *sch)
> 
>  }
> 
> -static int sch_handle_start_func_passthrough(SubchDev *sch)
> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>  {
> 
>  PMCW *p = >curr_status.pmcw;
>  SCSW *s = >curr_status.scsw;
> -int ret;
> 
>  ORB *orb = >orb;
>  if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev 
> *sch)
>   */
>  if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>  !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> -return -EINVAL;
> +sch_gen_unit_exception(sch);
> +css_inject_io_interrupt(sch);
> +return (IOInstEnding){.cc = 0};
This behavior change is not mentioned in the commit message. Right?

[...]

> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index a8baadf57a..dbb5b201de 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -23,6 +23,7 @@
>  #include "hw/s390x/s390-ccw.h"
>  #include "hw/s390x/ccw-device.h"
>  #include "qemu/error-report.h"
> +#include "cpu.h"
We need this because?

> 
>  #define TYPE_VFIO_CCW "vfio-ccw"
>  typedef struct VFIOCCWDevice {
> @@ -47,9 +48,9 @@ struct VFIODeviceOps vfio_ccw_ops = {
>  .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset,
>  };
> 
> -static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
> +static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>  {
> -S390CCWDevice *cdev = data;
> +S390CCWDevice *cdev = sch->driver_data;
>  VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
>  struct ccw_io_region *region = vcdev->io_region;
>  int ret;
> @@ -60,8 +61,8 @@ static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, 
> void *data)
> 
>  memset(region, 0, sizeof(*region));
> 
> -memcpy(region->orb_area, orb, sizeof(ORB));
> -memcpy(region->scsw_area, scsw, sizeof(SCSW));
> +memcpy(region->orb_area, >orb, sizeof(ORB));
> +memcpy(region->scsw_area, >curr_status.scsw, sizeof(SCSW));
> 
>  again:
>  ret = pwrite(vcdev->vdev.fd, region,
> @@ -71,10 +72,25 @@ again:
>  goto again;
>  }
>  error_report("vfio-ccw: wirte I/O region failed with errno=%d", 
> errno);
> -return -errno;
> +ret = -errno;
> +} else {
> +ret = region->ret_code;
> +}
> +switch (-ret) {
> +/* Currently we don't update control block and just return the cc code. 
> */
This is not true anymore? At least for the EFAULT case.

> +case 0:
> +return (IOInstEnding){.cc = 0};
> +case EBUSY:
> +return (IOInstEnding){.cc = 2};
> +case ENODEV:
> +case EACCES:
> +return (IOInstEnding){.cc = 3};
> +case EFAULT:
> +default:
> +sch_gen_unit_exception(sch);
> +css_inject_io_interrupt(sch);
> +return (IOInstEnding){.cc = 0};
>  }
> -
> -return region->ret_code;
>  }
> 
>  static void vfio_ccw_reset(DeviceState *dev)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 66916b6546..2116c6b3c7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
[...]

> @@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, 
> uint8_t ssid,
>   uint16_t schid);
>  bool css_subch_visible(SubchDev *sch);
>  void css_conditional_io_interrupt(SubchDev *sch);
> +
Extra change.

>  int css_do_stsch(SubchDev *sch, SCHIB *schib);
>  bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);

Re: [Qemu-devel] [PATCH v3] net: fix check for number of parameters to -netdev socket

[Jason Wang]

On 2017年10月04日 15:48, Jens Freimann wrote:

ping

On Wed, Sep 27, 2017 at 03:21:18PM +, Jens Freimann wrote:

Since commit 0f8c289ad "net: fix -netdev socket,fd= for UDP sockets"
we allow more than one parameter for -netdev socket. But now
we run into an assert when no parameter at all is specified


qemu-system-x86_64 -netdev socket

socket.c:729: net_init_socket: Assertion `sock->has_udp' failed.

Fix this by reverting the change of the if condition done in 0f8c289ad.

Cc: Jason Wang 
Fixes: 0f8c289ad539feb5135c545bea947b310a893f4b
Reported-by: Mao Zhongyi 
Signed-off-by: Jens Freimann 

---
v2->v3:
- go back to the original check != 1. No need for more than one
 argument, putting sock->has_fd back into the calculation is enough

v1->v2:
- add check to prevent listen= and connect= being used at the same time

net/socket.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e6b471c63d..83a2a31025 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -695,8 +695,8 @@ int net_init_socket(const Netdev *netdev, const 
char *name,

    assert(netdev->type == NET_CLIENT_DRIVER_SOCKET);
    sock = >u.socket;

-    if (sock->has_listen + sock->has_connect + sock->has_mcast +
-    sock->has_udp > 1) {
+    if (sock->has_fd + sock->has_listen + sock->has_connect + 
sock->has_mcast +

+    sock->has_udp != 1) {
    error_setg(errp, "exactly one of listen=, connect=, mcast= or 
udp="

   " is required");
    return -1;
--
2.13.5






Applied, thanks.

Re: [Qemu-devel] [PATCH v16 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG

[Wei Wang]

On 10/11/2017 10:26 AM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> On 10/10/2017 09:09 PM, Tetsuo Handa wrote:
>>> Wei Wang wrote:
> And even if we could remove balloon_lock, you still cannot use
> __GFP_DIRECT_RECLAIM at xb_set_page(). I think you will need to use
> "whether it is safe to wait" flag from
> "[PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()" .
 Without the lock being held, why couldn't we use __GFP_DIRECT_RECLAIM at
 xb_set_page()?
>>> Because of dependency shown below.
>>>
>>> leak_balloon()
>>>xb_set_page()
>>>  xb_preload(GFP_KERNEL)
>>>kmalloc(GFP_KERNEL)
>>>  __alloc_pages_may_oom()
>>>Takes oom_lock
>>>out_of_memory()
>>>  blocking_notifier_call_chain()
>>>leak_balloon()
>>>  xb_set_page()
>>>xb_preload(GFP_KERNEL)
>>>  kmalloc(GFP_KERNEL)
>>>__alloc_pages_may_oom()
>>>  Fails to take oom_lock and loop forever
>> __alloc_pages_may_oom() uses mutex_trylock(_lock).
> Yes. But this mutex_trylock(_lock) is semantically mutex_lock(_lock)
> because __alloc_pages_slowpath() will continue looping until
> mutex_trylock(_lock) succeeds (or somebody releases memory).
>
>> I think the second __alloc_pages_may_oom() will not continue since the
>> first one is in progress.
> The second __alloc_pages_may_oom() will be called repeatedly because
> __alloc_pages_slowpath() will continue looping (unless somebody releases
> memory).
>

OK, I see, thanks. So, the point is that the OOM code path should not
have memory allocation, and the
old leak_balloon (without the F_SG feature) don't need xb_preload(). I
think one solution would be to let
the OOM uses the old leak_balloon() code path, and we can add one more
parameter to leak_balloon
to control that:

leak_balloon(struct virtio_balloon *vb, size_t num, bool oom)



>>> By the way, is xb_set_page() safe?
>>> Sleeping in the kernel with preemption disabled is a bug, isn't it?
>>> __radix_tree_preload() returns 0 with preemption disabled upon success.
>>> xb_preload() disables preemption if __radix_tree_preload() fails.
>>> Then, kmalloc() is called with preemption disabled, isn't it?
>>> But xb_set_page() calls xb_preload(GFP_KERNEL) which might sleep with
>>> preemption disabled.
>> Yes, I think that should not be expected, thanks.
>>
>> I plan to change it like this:
>>
>> bool xb_preload(gfp_t gfp)
>> {
>> if (!this_cpu_read(ida_bitmap)) {
>> struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);
>>
>> if (!bitmap)
>> return false;
>> bitmap = this_cpu_cmpxchg(ida_bitmap, NULL, bitmap);
>> kfree(bitmap);
>> }
> Excuse me, but you are allocating per-CPU memory when running CPU might
> change at this line? What happens if running CPU has changed at this line?
> Will it work even with new CPU's ida_bitmap == NULL ?
>


Yes, it will be detected in xb_set_bit(): when ida_bitmap = NULL on the
new CPU, xb_set_bit() will
return -EAGAIN to the caller, and the caller should restart from
xb_preload().

Best,
Wei

Re: [Qemu-devel] [RFC v2 10/33] migration: allow dst vm pause on postcopy

[Peter Xu]

On Tue, Oct 10, 2017 at 01:30:18PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Mon, Oct 09, 2017 at 07:58:13PM +0100, Dr. David Alan Gilbert wrote:

[...]

> > > We have to be careful about this; a network can fail in a way it
> > > gets stuck rather than fails - this can get stuck until a full TCP
> > > disconnection; and that takes about 30mins (from memory).
> > > The nice thing about using 'shutdown' is that you can kill the existing
> > > connection if it's hung. (Which then makes an interesting question;
> > > the rules in your migrate-incoming command become different if you
> > > want to declare it's failed!).  Having said that, you're right that at
> > > this point stuff has already failed - so do we need the shutdown?
> > > (You might want to do the shutdown as part of the recovery earlier
> > > or as a separate command to force the failure)
> > 
> > I assume if I call shutdown before the lock then we'll be good then.
> 
> The question is what happens if you only allow recovery if we're already
> in postcopy-paused state; in the case of a hung socket, since no IO has
> actually failed yet, you will still be in postcopy-active.

Hmm, but isn't that a problem of kernel rather than QEMU?  Since
sockets are after all managed by kernel.

I don't really know what is the best thing to do to detect whether a
socket is stuck.  Assume we can observed that (say, we see migration
transferred bytes keep static for 30 seconds), IIRC you mentioned
about iptable tricks to break an existing e.g. TCP connection, then we
can trigger the -EIO path.

Or do you think we should provide a way to manually trigger the paused
state?  Then it goes back to something we discussed with Dan in the
earlier post - I'd appreciate if we can postpone the manual trigger
support a bit (to make this series small, which is already not...).

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 9/9] target/arm: Implement SG instruction corner cases

[Richard Henderson]

On 10/09/2017 06:48 AM, Peter Maydell wrote:
> The common situation of the SG instruction is that it is
> executed from S memory by a CPU in NS state. That case
> is handled by v7m_handle_execute_nsc(). However the instruction
> also has defined behaviour in a couple of other cases:
>  * SG instruction in NS memory (behaves as a NOP)
>  * SG in S memory but CPU already secure (clears IT bits and
>does nothing else)
>  * SG instruction in v8M without Security Extension (NOP)
> 
> These can be implemented in translate.c.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 23 ++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH 8/9] target/arm: Support some Thumb insns being always unconditional

[Richard Henderson]

On 10/09/2017 06:48 AM, Peter Maydell wrote:
> -if (dc->condexec_mask) {
> +if (dc->condexec_mask && !thumb_insn_is_unconditional(dc, insn)) {
>  uint32_t cond = dc->condexec_cond;
>  
>  if (cond != 0x0e) { /* Skip conditional when condition is AL. */

Don't you still need to advance the condexec_mask?

For HLT it doesn't matter, clearly.
I'm not sure what happens to an IT block for debug breakpoints.

But SG behaves as NOP when in non-secure memory.
That would seem to require that

itfteq
add r1, r0, r0
sg
add r2, r0, r0
add r3, r0, r0

should modify both r1 and r2 for T and r3 should be unconditional.  If you
don't advance condexec_mask, it would seem that r2 gets F and r3 gets T.


r~

Re: [Qemu-devel] [PATCH v16 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG

[Tetsuo Handa]

Wei Wang wrote:
>On 10/10/2017 09:09 PM, Tetsuo Handa wrote:
>> Wei Wang wrote:
 And even if we could remove balloon_lock, you still cannot use
 __GFP_DIRECT_RECLAIM at xb_set_page(). I think you will need to use
 "whether it is safe to wait" flag from
 "[PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()" .
>>> Without the lock being held, why couldn't we use __GFP_DIRECT_RECLAIM at
>>> xb_set_page()?
>> Because of dependency shown below.
>>
>> leak_balloon()
>>xb_set_page()
>>  xb_preload(GFP_KERNEL)
>>kmalloc(GFP_KERNEL)
>>  __alloc_pages_may_oom()
>>Takes oom_lock
>>out_of_memory()
>>  blocking_notifier_call_chain()
>>leak_balloon()
>>  xb_set_page()
>>xb_preload(GFP_KERNEL)
>>  kmalloc(GFP_KERNEL)
>>__alloc_pages_may_oom()
>>  Fails to take oom_lock and loop forever
>
>__alloc_pages_may_oom() uses mutex_trylock(_lock).

Yes. But this mutex_trylock(_lock) is semantically mutex_lock(_lock)
because __alloc_pages_slowpath() will continue looping until
mutex_trylock(_lock) succeeds (or somebody releases memory).

>
>I think the second __alloc_pages_may_oom() will not continue since the
>first one is in progress.

The second __alloc_pages_may_oom() will be called repeatedly because
__alloc_pages_slowpath() will continue looping (unless somebody releases
memory).

>
>>
>> By the way, is xb_set_page() safe?
>> Sleeping in the kernel with preemption disabled is a bug, isn't it?
>> __radix_tree_preload() returns 0 with preemption disabled upon success.
>> xb_preload() disables preemption if __radix_tree_preload() fails.
>> Then, kmalloc() is called with preemption disabled, isn't it?
>> But xb_set_page() calls xb_preload(GFP_KERNEL) which might sleep with
>> preemption disabled.
>
>Yes, I think that should not be expected, thanks.
>
>I plan to change it like this:
>
>bool xb_preload(gfp_t gfp)
>{
> if (!this_cpu_read(ida_bitmap)) {
> struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);
>
> if (!bitmap)
> return false;
> bitmap = this_cpu_cmpxchg(ida_bitmap, NULL, bitmap);
> kfree(bitmap);
> }

Excuse me, but you are allocating per-CPU memory when running CPU might
change at this line? What happens if running CPU has changed at this line?
Will it work even with new CPU's ida_bitmap == NULL ?

>
> if (__radix_tree_preload(gfp, XB_PRELOAD_SIZE) < 0)
> return false;
>
> return true;
>}

Re: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the HDA device.

[no-reply]

Hi,

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

Type: series
Message-id: 20171010174538.25074-1-q...@martin.schrodt.org
Subject: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the 
HDA device.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
11d17311c9 Several fixes for the Pulse Audio driver, and the HDA device.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Several fixes for the Pulse Audio driver, and the HDA 
device
ERROR: open brace '{' following function declarations go on the next line
#34: FILE: audio/audio.c:2070:
+int64_t audio_get_timer_ticks(void) {

ERROR: space prohibited between function name and open parenthesis '('
#146: FILE: audio/paaudio.c:106:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#161: FILE: audio/paaudio.c:118:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#266: FILE: audio/paaudio.c:126:
+static int qpa_run_out (HWVoiceOut *hw, int live)

ERROR: space prohibited between function name and open parenthesis '('
#286: FILE: audio/paaudio.c:139:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#287: FILE: audio/paaudio.c:140:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: space prohibited between function name and open parenthesis '('
#300: FILE: audio/paaudio.c:146:
+samples = (int)(audio_MIN (avail_bytes, max_bytes)) >> hw->info.shift;

ERROR: do not use C99 // comments
#306: FILE: audio/paaudio.c:148:
+//if (avail_bytes < max_bytes) {

ERROR: unnecessary whitespace before a quoted newline
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#308: FILE: audio/paaudio.c:150:
+//}

ERROR: line over 90 characters
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: do not use C99 // comments
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: space prohibited between function name and open parenthesis '('
#324: FILE: audio/paaudio.c:157:
+int convert_samples = audio_MIN (samples, left_till_end_samples);

WARNING: line over 80 characters
#325: FILE: audio/paaudio.c:158:
+size_t convert_bytes_wanted = (size_t) convert_samples << 
hw->info.shift;

WARNING: line over 80 characters
#331: FILE: audio/paaudio.c:163:
+CHECK_SUCCESS_GOTO(pa->g, , convert_bytes == 
convert_bytes_wanted, fail);

ERROR: space prohibited between function name and open parenthesis '('
#339: FILE: audio/paaudio.c:166:
+hw->clip (pa_dst, src, convert_samples);

ERROR: line over 90 characters
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#369: FILE: audio/paaudio.c:177:
+pa_threaded_mainloop_unlock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#389: FILE: audio/paaudio.c:183:
+qpa_logerr (error, "qpa_run_out failed\n");

ERROR: space prohibited between function name and open parenthesis '('
#400: FILE: audio/paaudio.c:192:
+static int qpa_run_in (HWVoiceIn *hw)

ERROR: space prohibited between function name and open parenthesis '('
#415: FILE: audio/paaudio.c:203:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#416: FILE: audio/paaudio.c:204:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: line over 90 characters
#418: FILE: audio/paaudio.c:206:
+size_t bytes_wanted = ((unsigned int)(hw->samples - 
audio_pcm_hw_get_live_in(hw)) << 

Re: [Qemu-devel] [PATCH 7/9] target-arm: Simplify insn_crosses_page()

[Richard Henderson]

On 10/09/2017 06:48 AM, Peter Maydell wrote:
> Recent changes have left insn_crosses_page() more complicated
> than it needed to be:
>  * it's only called from thumb_tr_translate_insn() so we know
>for certain that we're looking at a Thumb insn
>  * the caller's check for dc->pc >= dc->next_page_start - 3
>means that dc->pc can't possibly be 4 aligned, so there's
>no need to check that (the check was partly there to ensure
>that we didn't treat an ARM insn as Thumb, I think)
>  * we now have thumb_insn_is_16bit() which lets us do a precise
>check of the length of the next insn, rather than opencoding
>an inaccurate check
> 
> Simplify it down to just loading the first half of the insn
> and calling thumb_insn_is_16bit() on it.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 27 ++-
>  1 file changed, 6 insertions(+), 21 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH 6/9] target/arm: Pull Thumb insn word loads up to top level

[Richard Henderson]

On 10/09/2017 06:48 AM, Peter Maydell wrote:
> +if ((insn >> 11) == 0x1e && (s->pc < s->next_page_start - 3)) {
> +/* 0b_0xxx__ : BL/BLX prefix, and the suffix
> + * is not on the next page; we merge this into a 32-bit
> + * insn.
> + */
> +return false;
> +}
> +/* 0b1110_1xxx__ : BLX suffix (or UNDEF);
> + * 0b_1xxx__ : BL suffix;
> + * 0b_0xxx__ : BL/BLX prefix on the end of a page
> + *  -- handle as single 16 bit insn
> + */

I really had to search to find docs for this.  I suspect it's quite clear in v5
manuals, but I didn't have one handy and arm.com seems to have dropped
everything before v6M.  However, I did see a footnote in v7M that explained.

But, everything appears to be in order.  It's a nice cleanup.

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH] cirrus: keep vga vram pointer within bounds

[no-reply]

Hi,

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

Type: series
Message-id: 20171010213608.31567-1-ppan...@redhat.com
Subject: [Qemu-devel] [PATCH] cirrus: keep vga vram pointer within bounds

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
62900c4b66 cirrus: keep vga vram pointer within bounds

=== OUTPUT BEGIN ===
Checking PATCH 1/1: cirrus: keep vga vram pointer within bounds...
ERROR: spaces required around that '<<' (ctx:VxV)
#40: FILE: hw/display/cirrus_vga.c:2081:
+memory_region_set_dirty(>vga.vram, offset, x<<1);
^

total: 1 errors, 0 warnings, 24 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCH v16 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG

[Wei Wang]

On 10/10/2017 09:09 PM, Tetsuo Handa wrote:

Wei Wang wrote:

And even if we could remove balloon_lock, you still cannot use
__GFP_DIRECT_RECLAIM at xb_set_page(). I think you will need to use
"whether it is safe to wait" flag from
"[PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()" .

Without the lock being held, why couldn't we use __GFP_DIRECT_RECLAIM at
xb_set_page()?

Because of dependency shown below.

leak_balloon()
   xb_set_page()
 xb_preload(GFP_KERNEL)
   kmalloc(GFP_KERNEL)
 __alloc_pages_may_oom()
   Takes oom_lock
   out_of_memory()
 blocking_notifier_call_chain()
   leak_balloon()
 xb_set_page()
   xb_preload(GFP_KERNEL)
 kmalloc(GFP_KERNEL)
   __alloc_pages_may_oom()
 Fails to take oom_lock and loop forever


__alloc_pages_may_oom() uses mutex_trylock(_lock).

I think the second __alloc_pages_may_oom() will not continue since the
first one is in progress.



By the way, is xb_set_page() safe?
Sleeping in the kernel with preemption disabled is a bug, isn't it?
__radix_tree_preload() returns 0 with preemption disabled upon success.
xb_preload() disables preemption if __radix_tree_preload() fails.
Then, kmalloc() is called with preemption disabled, isn't it?
But xb_set_page() calls xb_preload(GFP_KERNEL) which might sleep with
preemption disabled.


Yes, I think that should not be expected, thanks.

I plan to change it like this:

bool xb_preload(gfp_t gfp)
{
if (!this_cpu_read(ida_bitmap)) {
struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);

if (!bitmap)
return false;
bitmap = this_cpu_cmpxchg(ida_bitmap, NULL, bitmap);
kfree(bitmap);
}

if (__radix_tree_preload(gfp, XB_PRELOAD_SIZE) < 0)
return false;

return true;
}


Best,
Wei

Re: [Qemu-devel] ping RE: question: I found a qemu crash about migration

[wangjie (P)]

I found the real culprit, the bug begin to occur since the committed as 
following:
https://github.com/qemu/qemu/commit/e253f4b89796967d03a455d1df2ae6bda8cc7d01

since the patch committed, the mirror target bs begin to use BlockBackend, So 
the mirror target bs will be inactived in bdrv_inactivate_all


-Original Message-
From: Dr. David Alan Gilbert [mailto:dgilb...@redhat.com] 
Sent: Monday, October 09, 2017 7:56 PM
To: Kevin Wolf 
Cc: wangjie (P) ; qemu-devel@nongnu.org; 
mre...@redhat.com; quint...@redhat.com; pbonz...@redhat.com; fuweiwei (C) 
; ebl...@redhat.com; berra...@redhat.com 
kcham...@redhat.com ; f...@redhat.com
Subject: Re: ping RE: question: I found a qemu crash about migration

* Kevin Wolf (kw...@redhat.com) wrote:
> Am 29.09.2017 um 21:06 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (kw...@redhat.com) wrote:
> > > Am 28.09.2017 um 19:01 hat Dr. David Alan Gilbert geschrieben:
> > > > Hi,
> > > >   This is a 'fun' bug;  I had a good chat to kwolf about it earlier.
> > > > A proper fix really needs to be done together with libvirt so 
> > > > that we can sequence:
> > > >a) The stopping of the CPU on the source
> > > >b) The termination of the mirroring block job
> > > >c) The inactivation of the block devices on the source
> > > >(bdrv_inactivate_all)
> > > >d) The activation of the block devices on the destination
> > > >(bdrv_invalidate_cache_all)
> > > >e) The start of the CPU on the destination
> > > > 
> > > > It looks like you're hitting a race between b/c;  we've had 
> > > > races between c/d in the past and moved the bdrv_inactivate_all.
> > > > 
> > > > During the discussion we ended up with two proposed solutions; 
> > > > both of them require one extra command and one extra migration 
> > > > capability.
> > > > 
> > > > The block way
> > > > -
> > > >1) Add a new migration capability pause-at-complete
> > > >2) Add a new migration state almost-complete
> > > >3) After saving devices, if pause-at-complete is set,
> > > >   transition to almost-complete
> > > >4) Add a new command (migration-continue) that 
> > > >   causes the migration to inactivate the devices (c)
> > > >   and send the final EOF to the destination.
> > > > 
> > > > You set pause-at-complete, wait until migrate hits 
> > > > almost-complete; cleanup the mirror job, and then do 
> > > > migration-continue.  When it completes do 'cont' on the destination.
> > > 
> > > Especially since you're concerned about the exact position of the 
> > > pause, the following would be a little safer (using the 
> > > opportunity to suggest slightly different names,too):
> > > 
> > > 1) Add a new migration capability pause-at-complete
> > > 2) Add a new migration state ready
> > > 3) After migration has converged (i.e. the background phase of the
> > >migration has completed) and we are ready to actually switch to the
> > >destination, stop the VM, transition to 'ready' and emit a QMP 
> > > event
> > > 4) Continue processing QMP commands in the 'ready' phase. This is where
> > >libvirt is supposed to tear down any running non-VM activities like
> > >block jobs, NBD servers, etc.
> > > 5) Add a new command (migration-complete) that does the final part of
> > >migration, including sending the remaining dirty memory, device state
> > >and the final EOF that signals the destination to resume the VM if -S
> > >isn't given.
> > 
> > What worries me here is whether some other subsection is going to 
> > say that this pause must happen after the device state save, e.g. if 
> > the device state save causes them to push out the last 
> > block/packet/etc - and I've got no real way to say whether that 
> > point is any better or worse than the point I was suggestion.
> > And the position in the cycle when the pause happens is part of the API.
> 
> The important point here is that the VM is stopped. If device 
> emulation is doing anything by itself while the VM is stopped, that is a bug.
> In other words, that last block/packet/etc. must already have been 
> pushed out when we stopped the VM, so there is nothing left to push 
> out when we save the state.
> 
> With correctly written device code, there is no difference whether we 
> save the device state first or last, without external influence it 
> will be the same.
> 
> The obvious external thing that I can see is monitor commands, e.g. 
> the monitor allows to access I/O ports, which can change the device state.
> However, monitor commands are completely under the control of the 
> user, so they can always choose the order that they need.
> 
> In any case, saving the device state only immediately before doing the 
> switch makes sure that we consider any changes that have still been 
> made.
> 
> > > The main reason why I advocate this "block way" is that it looks a 
> > > lot 

Re: [Qemu-devel] [PULL v2 00/10] Merge tpm 2017/10/04

[Stefan Berger]

On 10/10/2017 11:08 AM, Peter Maydell wrote:

On 10 October 2017 at 02:33, Stefan Berger  wrote:

The following changes since commit d147f7e815f97cb477e223586bcb80c316ae10ea:

   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into 
staging (2017-10-03 16:27:24 +0100)

are available in the git repository at:

   git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2017-10-04-2

for you to fetch changes up to 3bfcc21bf9e437734bb3e90db7e5b5f6b8bf:

   specs: Describe the TPM support in QEMU (2017-10-05 12:50:45 -0400)


Merge tpm 2017/10/04 v2



Hi. This fails to build, I'm afraid:

On OpenBSD:


I installed OpenBSD 6.2 for i386 . I could recreate this one. PRIx64 
solves it. I don't see the EBADMSG.  We include qemu/osdep.h in this 
file, so this is odd.


Stefan




   CC  hw/tpm/tpm_emulator.o
/home/qemu/hw/tpm/tpm_emulator.c: In function 'tpm_emulator_probe_caps':
/home/qemu/hw/tpm/tpm_emulator.c:223:5: warning: format '%lx' expects
argument of type 'long unsigned int', but argument 3 has type
'ptm_cap' [-Wformat=]
  DPRINTF("capbilities : 0x%lx", tpm_emu->caps);
  ^
gmake: Leaving directory '/home/qemu/build/all'
gmake: Entering directory '/home/qemu/build/all'
   CC  hw/tpm/tpm_util.o
/home/qemu/hw/tpm/tpm_util.c: In function 'tpm_util_test':
/home/qemu/hw/tpm/tpm_util.c:94:16: error: 'EBADMSG' undeclared (first
use in this function)
  return EBADMSG;
 ^
/home/qemu/hw/tpm/tpm_util.c:94:16: note: each undeclared identifier
is reported only once for each function it appears in

On AArch32:

/home/peter.maydell/qemu/hw/tpm/tpm_emulator.c: In function
'tpm_emulator_probe_caps':
/home/peter.maydell/qemu/hw/tpm/tpm_emulator.c:53:25: error: format
'%lx' expects argument of type 'long unsigned int', but argument 3 has
type 'ptm_cap {aka long long unsigned int}' [-Werror=format=]
  fprintf(stderr, "tpm-emulator:"fmt"\n", ## __VA_ARGS__); \
  ^
/home/peter.maydell/qemu/hw/tpm/tpm_emulator.c:223:5: note: in
expansion of macro 'DPRINTF'
  DPRINTF("capbilities : 0x%lx", tpm_emu->caps);
  ^

On OSX:
/Users/pm215/src/qemu-for-merges/hw/tpm/tpm_emulator.c:223:36: error:
format specifies type 'unsigned long' but the argument has type
'ptm_cap' (aka 'unsigned long long') [-Werror,-Wformat]
 DPRINTF("capbilities : 0x%lx", tpm_emu->caps);
  ~~~   ^
  %llx
/Users/pm215/src/qemu-for-merges/hw/tpm/tpm_emulator.c:53:52: note:
expanded from macro 'DPRINTF'
 fprintf(stderr, "tpm-emulator:"fmt"\n", ## __VA_ARGS__); \
^

thanks
-- PMM

Re: [Qemu-devel] [PATCH 27/42] tpm: remove unused opened code

[Stefan Berger]

On 10/10/2017 06:27 PM, Marc-André Lureau wrote:

Hi

- Original Message -

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Signed-off-by: Marc-André Lureau 
---
   include/sysemu/tpm_backend.h | 12 
   backends/tpm.c   | 42
   --
   tpm.c|  6 --
   3 files changed, 60 deletions(-)

diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index b12ae5b625..a893e586ae 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -81,8 +81,6 @@ struct TPMBackendClass {
   
   TpmTypeOptions *(*get_tpm_options)(TPMBackend *t);
   
-void (*opened)(TPMBackend *s, Error **errp);

-
   void (*handle_request)(TPMBackend *s, TPMBackendCmd *cmd);
   };
   
@@ -172,16 +170,6 @@ bool tpm_backend_get_tpm_established_flag(TPMBackend

*s);
*/
   int tpm_backend_reset_tpm_established_flag(TPMBackend *s, uint8_t locty);
   
-/**

- * tpm_backend_open:
- * @s: the backend to open
- * @errp: a pointer to return the #Error object if an error occurs.
- *
- * This function will open the backend if it is not already open.  Calling
this
- * function on an already opened backend will not result in an error.
- */
-void tpm_backend_open(TPMBackend *s, Error **errp);
-
   /**
* tpm_backend_get_tpm_version:
* @s: the backend to call into
diff --git a/backends/tpm.c b/backends/tpm.c
index 0c48d18775..7e636fbc7a 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -159,52 +159,10 @@ TPMInfo *tpm_backend_query_tpm(TPMBackend *s)
   return info;
   }
   
-static bool tpm_backend_prop_get_opened(Object *obj, Error **errp)

-{
-TPMBackend *s = TPM_BACKEND(obj);
-
-return s->opened;
-}
-
-void tpm_backend_open(TPMBackend *s, Error **errp)
-{
-object_property_set_bool(OBJECT(s), true, "opened", errp);
-}
-
-static void tpm_backend_prop_set_opened(Object *obj, bool value, Error
**errp)
-{
-TPMBackend *s = TPM_BACKEND(obj);
-TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
-Error *local_err = NULL;
-
-if (value == s->opened) {
-return;
-}
-
-if (!value && s->opened) {
-error_setg(errp, QERR_PERMISSION_DENIED);
-return;
-}
-
-if (k->opened) {
-k->opened(s, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-}
-
-s->opened = true;
-}
-
   static void tpm_backend_instance_init(Object *obj)
   {
   TPMBackend *s = TPM_BACKEND(obj);
   
-object_property_add_bool(obj, "opened",

- tpm_backend_prop_get_opened,
- tpm_backend_prop_set_opened,
- NULL);
   s->bh = qemu_bh_new(tpm_backend_request_completed_bh, s);
   }
   
diff --git a/tpm.c b/tpm.c

index ce1543fcb4..a46ee5f144 100644
--- a/tpm.c
+++ b/tpm.c
@@ -134,12 +134,6 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts
*opts, Error **errp)
   return 1;
   }
   
-tpm_backend_open(drv, _err);

-if (local_err) {
-error_report_err(local_err);
-return 1;
-}
-
   QLIST_INSERT_HEAD(_backends, drv, list);
   
   return 0;

Since nothing is setting the "opened' anymore, would anyone notice
because this has changed or is something else doing that now? Otherwise
I don't mind removing it...

Was it ever used? I see the property was added in commit 
8f0605cc9caacbcc647a6df9ae541ed2da4b9bb0.

Code adapted from rng backend. But rng backend set "opened" on 
UserCreatableClass.complete() (TPM backend doesn't, but probably could - I have an 
experimental patch for that)

I think we can remove it that code safely.



Reviewed-by: Stefan Berger 

Re: [Qemu-devel] [PATCH 33/42] tpm-passthrough: remove error cleanup from handle_device_opts

[Stefan Berger]

On 10/10/2017 06:19 PM, Marc-André Lureau wrote:

Hi

- Original Message -

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Clean-up is handled by the create() function.

Signed-off-by: Marc-André Lureau 
---
   hw/tpm/tpm_passthrough.c | 15 ++-
   1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index aa9167e3c6..0806cf86af 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -261,27 +261,16 @@ tpm_passthrough_handle_device_opts(TPMPassthruState
*tpm_pt, QemuOpts *opts)
   if (tpm_pt->tpm_fd < 0) {
   error_report("Cannot access TPM device using '%s': %s",
tpm_pt->tpm_dev, strerror(errno));
-goto err_free_parameters;
+return -1;
   }

   if (tpm_util_test_tpmdev(tpm_pt->tpm_fd, _pt->tpm_version)) {
   error_report("'%s' is not a TPM device.",
tpm_pt->tpm_dev);
-goto err_close_tpmdev;
+return -1;
   }

I would prefer the cleanup to happen in the functions where the state is
created...

This is the role for a destructor, no need to worry about local object change 
cleanup.

I can drop the patch if you feel strongly about it, but I think it's a nice 
code simplification.


I like to see the cleanup code on the bottom...

Re: [Qemu-devel] [PATCH 14/42] tpm: add TPMBackendCmd to hold the request state

[Stefan Berger]

On 10/10/2017 12:16 PM, Marc-André Lureau wrote:

Hi

- Original Message -

On 10/09/2017 06:55 PM, Marc-André Lureau wrote:

This simplifies a bit locality handling, and argument passing, and
could pave the way to queuing requests (if that makes sense).

We won't queue requests. The TPM interfaces all send one request and
expect the driver to wait until the response comes back.

Even on different localities? (I am not familiar enough with that part)


Yes, I believe so.




Signed-off-by: Marc-André Lureau 

Reviewed-by: Stefan Berger 


---
   hw/tpm/tpm_int.h |  1 +
   include/sysemu/tpm_backend.h | 16 +---
   backends/tpm.c   |  6 +++---
   hw/tpm/tpm_emulator.c| 29 +++--
   hw/tpm/tpm_passthrough.c | 24 +---
   hw/tpm/tpm_tis.c | 18 +-
   6 files changed, 50 insertions(+), 44 deletions(-)

diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h
index f2f285b3cc..6d7b3dc850 100644
--- a/hw/tpm/tpm_int.h
+++ b/hw/tpm/tpm_int.h
@@ -26,6 +26,7 @@ struct TPMState {
   
   uint8_t locty_number;

   TPMLocality *locty_data;
+TPMBackendCmd cmd;
   
   char *backend;

   TPMBackend *be_driver;
diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 9c83a512e1..3bb90be3de 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -30,7 +30,16 @@
   typedef struct TPMBackendClass TPMBackendClass;
   typedef struct TPMBackend TPMBackend;
   
-typedef void (TPMRecvDataCB)(TPMState *, bool selftest_done);

+typedef void (TPMRecvDataCB)(TPMState *);
+
+typedef struct TPMBackendCmd {
+uint8_t locty;
+const uint8_t *in;
+uint32_t in_len;
+uint8_t *out;
+uint32_t out_len;
+bool selftest_done;
+} TPMBackendCmd;
   
   struct TPMBackend {

   Object parent;
@@ -76,7 +85,7 @@ struct TPMBackendClass {
   
   void (*opened)(TPMBackend *s, Error **errp);
   
-void (*handle_request)(TPMBackend *s);

+void (*handle_request)(TPMBackend *s, TPMBackendCmd *cmd);
   };
   
   /**

@@ -121,11 +130,12 @@ bool tpm_backend_had_startup_error(TPMBackend *s);
   /**
* tpm_backend_deliver_request:
* @s: the backend to send the request to
+ * @cmd: the command to deliver
*
* Send a request to the backend. The backend will then send the request
* to the TPM implementation.
*/
-void tpm_backend_deliver_request(TPMBackend *s);
+void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd);
   
   /**

* tpm_backend_reset:
diff --git a/backends/tpm.c b/backends/tpm.c
index 34e82085ec..dc7c831ff8 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -25,7 +25,7 @@ static void tpm_backend_worker_thread(gpointer data,
gpointer user_data)
   TPMBackendClass *k  = TPM_BACKEND_GET_CLASS(s);
   
   assert(k->handle_request != NULL);

-k->handle_request(s);
+k->handle_request(s, (TPMBackendCmd *)data);
   }
   
   static void tpm_backend_thread_end(TPMBackend *s)

@@ -76,9 +76,9 @@ bool tpm_backend_had_startup_error(TPMBackend *s)
   return s->had_startup_error;
   }
   
-void tpm_backend_deliver_request(TPMBackend *s)

+void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd)
   {
-g_thread_pool_push(s->thread_pool, NULL, NULL);
+g_thread_pool_push(s->thread_pool, cmd, NULL);
   }
   
   void tpm_backend_reset(TPMBackend *s)

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 4fe405353a..788ab9876d 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -172,28 +172,29 @@ static int tpm_emulator_set_locality(TPMEmulator
*tpm_emu, uint8_t locty_number)
   return 0;
   }
   
-static void tpm_emulator_handle_request(TPMBackend *tb)

+static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd
*cmd)
   {
   TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
-TPMLocality *locty = NULL;
-bool selftest_done = false;
   Error *err = NULL;
   
   DPRINTF("processing TPM command");
   
-locty = tb->tpm_state->locty_data;

-if (tpm_emulator_set_locality(tpm_emu,
-  tb->tpm_state->locty_number) < 0 ||
-tpm_emulator_unix_tx_bufs(tpm_emu, locty->w_buffer.buffer,
-  locty->w_offset, locty->r_buffer.buffer,
-  locty->r_buffer.size, _done,
-  ) < 0) {
-tpm_util_write_fatal_error_response(locty->r_buffer.buffer,
-locty->r_buffer.size);
-error_report_err(err);
+if (tpm_emulator_set_locality(tpm_emu, tb->tpm_state->locty_number) <
0) {
+goto error;
+}
+
+if (tpm_emulator_unix_tx_bufs(tpm_emu, cmd->in, cmd->in_len,
+  cmd->out, cmd->out_len,
+  >selftest_done, ) < 0) {
+goto error;

Re: [Qemu-devel] [PATCH 5/9] target-arm: Don't check for "Thumb2 or M profile" for not-Thumb1

[Richard Henderson]

On 10/09/2017 06:48 AM, Peter Maydell wrote:
> The code which implements the Thumb1 split BL/BLX instructions
> is guarded by a check on "not M or THUMB2". All we really need
> to check here is "not THUMB2" (and we assume that elsewhere too,
> eg in the ARCH(6T2) test that UNDEFs the Thumb2 insns).
> 
> This doesn't change behaviour because all M profile cores
> have Thumb2 and so ARM_FEATURE_M implies ARM_FEATURE_THUMB2.
> (v6M implements a very restricted subset of Thumb2, but we
> can cross that bridge when we get to it with appropriate
> feature bits.)
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH 2/9] target/arm: Implement SG instruction

[Richard Henderson]

On 10/09/2017 06:48 AM, Peter Maydell wrote:
> Implement the SG instruction, which we emulate 'by hand' in the
> exception handling code path.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/helper.c | 132 
> ++--
>  1 file changed, 127 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PULL v2 00/20] Queued TCG patches

[no-reply]

Hi,

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

Type: series
Message-id: 20171010193003.28857-1-richard.hender...@linaro.org
Subject: [Qemu-devel] [PULL v2 00/20] Queued TCG patches

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3bdf71c8cc tcg/mips: delete commented out extern keyword.
b6db70242c tcg: define TCG_HIGHWATER
8110aaa390 util: move qemu_real_host_page_size/mask to osdep.h
3b52eb07c3 tcg: take .helpers out of TCGContext
a1956949ff tci: move tci_regs to tcg_qemu_tb_exec's stack
4108e03598 exec-all: extract tb->tc_* into a separate struct tc_tb
2565c32cab translate-all: define and use DEBUG_TB_CHECK_GATE
be8d38903e translate-all: define and use DEBUG_TB_INVALIDATE_GATE
501c9fcf6e exec-all: introduce TB_PAGE_ADDR_FMT
4cc8ef863e translate-all: define and use DEBUG_TB_FLUSH_GATE
6a61ad246b exec-all: bring tb->invalid into tb->cflags
1fa8a382ef tcg: consolidate TB lookups in tb_lookup__cpu_state
b35dbd37fa tcg: remove addr argument from lookup_tb_ptr
8e09f17d21 tcg/mips: constify tcg_target_callee_save_regs
c6f520348a tcg/i386: constify tcg_target_callee_save_regs
41a8d06218 cpu-exec: rename have_tb_lock to acquired_tb_lock in tb_find
3293107b44 translate-all: make have_tb_lock static
ad863faa4a exec-all: fix typos in TranslationBlock's documentation
8f10ba2c46 tcg: fix corruption of code_time profiling counter upon tb_flush
7fb10a792f cputlb: bring back tlb_flush_count under !TLB_DEBUG

=== OUTPUT BEGIN ===
Checking PATCH 1/20: cputlb: bring back tlb_flush_count under !TLB_DEBUG...
Checking PATCH 2/20: tcg: fix corruption of code_time profiling counter upon 
tb_flush...
Checking PATCH 3/20: exec-all: fix typos in TranslationBlock's documentation...
Checking PATCH 4/20: translate-all: make have_tb_lock static...
Checking PATCH 5/20: cpu-exec: rename have_tb_lock to acquired_tb_lock in 
tb_find...
Checking PATCH 6/20: tcg/i386: constify tcg_target_callee_save_regs...
Checking PATCH 7/20: tcg/mips: constify tcg_target_callee_save_regs...
Checking PATCH 8/20: tcg: remove addr argument from lookup_tb_ptr...
Checking PATCH 9/20: tcg: consolidate TB lookups in tb_lookup__cpu_state...
Checking PATCH 10/20: exec-all: bring tb->invalid into tb->cflags...
Checking PATCH 11/20: translate-all: define and use DEBUG_TB_FLUSH_GATE...
Checking PATCH 12/20: exec-all: introduce TB_PAGE_ADDR_FMT...
Checking PATCH 13/20: translate-all: define and use DEBUG_TB_INVALIDATE_GATE...
Checking PATCH 14/20: translate-all: define and use DEBUG_TB_CHECK_GATE...
Checking PATCH 15/20: exec-all: extract tb->tc_* into a separate struct tc_tb...
Checking PATCH 16/20: tci: move tci_regs to tcg_qemu_tb_exec's stack...
Checking PATCH 17/20: tcg: take .helpers out of TCGContext...
Checking PATCH 18/20: util: move qemu_real_host_page_size/mask to osdep.h...
Checking PATCH 19/20: tcg: define TCG_HIGHWATER...
Checking PATCH 20/20: tcg/mips: delete commented out extern keyword
ERROR: externs should be avoided in .c files
#22: FILE: tcg/mips/tcg-target.inc.c:39:
+int link_error(void);

total: 1 errors, 0 warnings, 8 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCH] scripts/checkpatch.pl: fix false error ofexterns checking.

[jiang.biao2]

> On 10 October 2017 at 08:54, Jiang Biao  wrote:


> > When adding a function declaration in a .c file without extern
> >keywork decoration, checkpatch.pl will report *externs should be
> > avoided in .c files* false error. This patch fixes the bug.
> 
> I don't think this is a bug. "extern int foo(void);" and
> "int foo(void);" have the same effect: they declare a function
> prototype that can be called from outside the file.
> We don't want to permit that in QEMU. Either:
>  (1) the function is for use only within this file: the
>  declaration should be "static int foo(void);"
>  (2) the function is for use by other files too: the
>  declaration should be in a header file, not a .c file,
>  so the other files can use it
> 
> Do you have an example of code where this warning is a problem?
Hi Peter,

I understand external functions should be put in header files. But there

is some rare exceptions, take the code segment in 


tcg/mips/tcg-target.inc.c for example,

#if TCG_TARGET_REG_BITS == 32
# define LO_OFF  (MIPS_BE * 4)
# define HI_OFF  (4 - LO_OFF)
#else
/* To assert at compile-time that these values are never used
   for TCG_TARGET_REG_BITS == 64.  */
/* extern */ int link_error(void);
# define LO_OFF  link_error()
# define HI_OFF  link_error()
#endif

In this case, the external funciton is just used to assert at complie-time.


If I make a patch to delete the commented out extern before *int link_error()*,

the checkpatch.pl will complain the  *externs should be avoided in .c files* 


error.

The same usage can also be found in tcg/tcg-op.c,

/* Reduce the number of ifdefs below.  This assumes that all uses of
   TCGV_HIGH and TCGV_LOW are properly protected by a conditional that
   the compiler can eliminate.  */
#if TCG_TARGET_REG_BITS == 64
extern TCGv_i32 TCGV_LOW_link_error(TCGv_i64);
extern TCGv_i32 TCGV_HIGH_link_error(TCGv_i64);
#define TCGV_LOW  TCGV_LOW_link_error
#define TCGV_HIGH TCGV_HIGH_link_error
#endif

I'm not sure if the checkpatch.pl should take these case into consideration. :)

Thanks.





Regards.

[Qemu-devel] [PATCHv2] spapr: Correct RAM size calculation for HPT resizing

[David Gibson]

In order to prevent the guest from forcing the allocation of large amounts
of qemu memory (or host kernel memory, in the case of KVM HV), we limit
the size of Hashed Page Table (HPT) it is allowed to allocated, based on
its RAM size.

However, the current calculation is not correct: it only adds up the size
of plugged memory, ignoring the base memory size.  This patch corrects it.

While we're there, use get_plugged_memory_size() instead of directly
calling pc_existing_dimms_capacity().  The only difference is that it
will abort on failure, which is right: a failure here indicates something
wrong within qemu.

Signed-off-by: David Gibson 
---
 hw/ppc/spapr_hcall.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Changes in v2:
  * Also remove redundant initializer of current_ram_size

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8d72bb7c1c..0d59d1534d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -472,7 +472,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
 target_ulong flags = args[0];
 int shift = args[1];
 sPAPRPendingHPT *pending = spapr->pending_hpt;
-uint64_t current_ram_size = MACHINE(spapr)->ram_size;
+uint64_t current_ram_size;
 int rc;
 
 if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
@@ -494,7 +494,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
 return H_PARAMETER;
 }
 
-current_ram_size = pc_existing_dimms_capacity(_fatal);
+current_ram_size = MACHINE(spapr)->ram_size + get_plugged_memory_size();
 
 /* We only allow the guest to allocate an HPT one order above what
  * we'd normally give them (to stop a small guest claiming a huge
-- 
2.13.6

Re: [Qemu-devel] [PATCH] spapr: Correct RAM size calculation for HPT resizing

[David Gibson]

On Tue, Oct 10, 2017 at 05:21:53PM +0200, Laurent Vivier wrote:
> On 10/10/2017 16:44, Greg Kurz wrote:
> > On Wed, 11 Oct 2017 00:21:59 +1100
> > David Gibson  wrote:
> > 
> >> In order to prevent the guest from forcing the allocation of large amounts
> >> of qemu memory (or host kernel memory, in the case of KVM HV), we limit
> >> the size of Hashed Page Table (HPT) it is allowed to allocated, based on
> >> its RAM size.
> >>
> >> However, the current calculation is not correct: it only adds up the size
> >> of plugged memory, ignoring the base memory size.  This patch corrects it.
> >>
> >> While we're there, use get_plugged_memory_size() instead of directly
> >> calling pc_existing_dimms_capacity().  The only difference is that it
> >> will abort on failure, which is right: a failure here indicates something
> >> wrong within qemu.
> >>
> >> Signed-off-by: David Gibson 
> >> ---
> >>  hw/ppc/spapr_hcall.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 8d72bb7c1c..06af1b15c0 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -494,7 +494,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU 
> >> *cpu,
> >>  return H_PARAMETER;
> >>  }
> >>  
> >> -current_ram_size = pc_existing_dimms_capacity(_fatal);
> >> +current_ram_size = ram_size + get_plugged_memory_size();
> > 
> > current_ram_size is initialized earlier in this function:
> > 
> > uint64_t current_ram_size = MACHINE(spapr)->ram_size;
> > 
> > which is is initialized to ram_size in vl.c. Why not doing:
> > 
> > current_ram_size += get_plugged_memory_size();
> > 
> > ?
> 
> I agree, it seems like the original intend of the first patch...

Yes, I think so.  However, splitting the calculation like that
demonstrably misread someone reading the code (i.e. me), so I'm going
to just ditch the initializerin the new spin.

-- 
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] [Qemu-ppc] [PATCH] spapr: Correct RAM size calculation for HPT resizing

[David Gibson]

On Tue, Oct 10, 2017 at 05:10:27PM +0200, Andrea Bolognani wrote:
> On Wed, 2017-10-11 at 00:21 +1100, David Gibson wrote:
> > In order to prevent the guest from forcing the allocation of large amounts
> > of qemu memory (or host kernel memory, in the case of KVM HV), we limit
> > the size of Hashed Page Table (HPT) it is allowed to allocated, based on
> > its RAM size.
> > 
> > However, the current calculation is not correct: it only adds up the size
> > of plugged memory, ignoring the base memory size.  This patch corrects it.
> > 
> > While we're there, use get_plugged_memory_size() instead of directly
> > calling pc_existing_dimms_capacity().  The only difference is that it
> > will abort on failure, which is right: a failure here indicates something
> > wrong within qemu.
> 
> Does this change invalidate in any way the calculation performed
> by libvirt to figure out the memory locking limit for guests?

No.

-- 
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 3/3 v4] xenfb: Add [feature|request]-raw-pointer

[Stefano Stabellini]

On Mon, 2 Oct 2017, Anthony PERARD wrote:
> On Tue, Sep 26, 2017 at 02:43:39PM +, Owen Smith wrote:
> > Writes "feature-raw-pointer" during init to indicate the backend
> > can pass raw unscaled values for absolute axes to the frontend.
> > Frontends set "request-raw-pointer" to indicate the backend should
> > not attempt to scale absolute values to console size.
> > "request-raw-pointer" is only valid if "request-abs-pointer" is
> > also set. Raw unscaled pointer values are in the range [0, 0x7fff]
> > 
> > Signed-off-by: Owen Smith 
> 
> Hi Owen,
> 
> Why did you remove the following from the commit description?
> > "feature-raw-pointer" and "request-raw-pointer" added to Xen
> > header in commit 7868654ff7fe5e4a2eeae2b277644fa884a5031e
> 
> I think that with it, you could have kept stefano's reviewed-by tag.

Hi Anthony,

Have you tested this series with a few of different guests? Do you
consider it safe to merge? If so, we can send it upstream (either via
xen or via ui as Gerd kindly offered). 

Re: [Qemu-devel] [PATCH] tcg: Initialize cpu_env generically

[Emilio G. Cota]

On Tue, Oct 10, 2017 at 14:45:40 -0700, Richard Henderson wrote:
> This is identical for each target.  So, move the initialization to
> common code.  Move the variable itself out of tcg_ctx and name it
> cpu_env to minimize changes within targets.
> 
> This also means we can remove tcg_global_reg_new_{ptr,i32,i64},
> since there are no longer global-register temps created by targets.
> 
> Signed-off-by: Richard Henderson 

Reviewed-by: Emilio G. Cota 

E.

Re: [Qemu-devel] [PATCH 1/9] target/arm: Add M profile secure MMU index values to get_a32_user_mem_index()

[Richard Henderson]

On 10/09/2017 06:48 AM, Peter Maydell wrote:
> Add the M profile secure MMU index values to the switch in
> get_a32_user_mem_index() so that LDRT/STRT work correctly
> rather than asserting at translate time.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the HDA device.

[no-reply]

Hi,

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

Type: series
Message-id: 20171010174403.24660-1-mar...@schrodt.org
Subject: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the 
HDA device.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e49f94ef7b Several fixes for the Pulse Audio driver, and the HDA device.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Several fixes for the Pulse Audio driver, and the HDA 
device
ERROR: open brace '{' following function declarations go on the next line
#34: FILE: audio/audio.c:2070:
+int64_t audio_get_timer_ticks(void) {

ERROR: space prohibited between function name and open parenthesis '('
#146: FILE: audio/paaudio.c:106:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#161: FILE: audio/paaudio.c:118:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#266: FILE: audio/paaudio.c:126:
+static int qpa_run_out (HWVoiceOut *hw, int live)

ERROR: space prohibited between function name and open parenthesis '('
#286: FILE: audio/paaudio.c:139:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#287: FILE: audio/paaudio.c:140:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: space prohibited between function name and open parenthesis '('
#300: FILE: audio/paaudio.c:146:
+samples = (int)(audio_MIN (avail_bytes, max_bytes)) >> hw->info.shift;

ERROR: do not use C99 // comments
#306: FILE: audio/paaudio.c:148:
+//if (avail_bytes < max_bytes) {

ERROR: unnecessary whitespace before a quoted newline
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#308: FILE: audio/paaudio.c:150:
+//}

ERROR: line over 90 characters
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: do not use C99 // comments
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: space prohibited between function name and open parenthesis '('
#324: FILE: audio/paaudio.c:157:
+int convert_samples = audio_MIN (samples, left_till_end_samples);

WARNING: line over 80 characters
#325: FILE: audio/paaudio.c:158:
+size_t convert_bytes_wanted = (size_t) convert_samples << 
hw->info.shift;

WARNING: line over 80 characters
#331: FILE: audio/paaudio.c:163:
+CHECK_SUCCESS_GOTO(pa->g, , convert_bytes == 
convert_bytes_wanted, fail);

ERROR: space prohibited between function name and open parenthesis '('
#339: FILE: audio/paaudio.c:166:
+hw->clip (pa_dst, src, convert_samples);

ERROR: line over 90 characters
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#369: FILE: audio/paaudio.c:177:
+pa_threaded_mainloop_unlock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#389: FILE: audio/paaudio.c:183:
+qpa_logerr (error, "qpa_run_out failed\n");

ERROR: space prohibited between function name and open parenthesis '('
#400: FILE: audio/paaudio.c:192:
+static int qpa_run_in (HWVoiceIn *hw)

ERROR: space prohibited between function name and open parenthesis '('
#415: FILE: audio/paaudio.c:203:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#416: FILE: audio/paaudio.c:204:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: line over 90 characters
#418: FILE: audio/paaudio.c:206:
+size_t bytes_wanted = ((unsigned int)(hw->samples - 
audio_pcm_hw_get_live_in(hw)) << hw->info.shift);

Re: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the HDA device.

[no-reply]

Hi,

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

Type: series
Message-id: 20171010171218.14298-1-mar...@schrodt.org
Subject: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the 
HDA device.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
14f6aeab34 Several fixes for the Pulse Audio driver, and the HDA device.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Several fixes for the Pulse Audio driver, and the HDA 
device
ERROR: open brace '{' following function declarations go on the next line
#34: FILE: audio/audio.c:2070:
+int64_t audio_get_timer_ticks(void) {

ERROR: space prohibited between function name and open parenthesis '('
#146: FILE: audio/paaudio.c:106:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#161: FILE: audio/paaudio.c:118:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#266: FILE: audio/paaudio.c:126:
+static int qpa_run_out (HWVoiceOut *hw, int live)

ERROR: space prohibited between function name and open parenthesis '('
#286: FILE: audio/paaudio.c:139:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#287: FILE: audio/paaudio.c:140:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: space prohibited between function name and open parenthesis '('
#300: FILE: audio/paaudio.c:146:
+samples = (int)(audio_MIN (avail_bytes, max_bytes)) >> hw->info.shift;

ERROR: do not use C99 // comments
#306: FILE: audio/paaudio.c:148:
+//if (avail_bytes < max_bytes) {

ERROR: unnecessary whitespace before a quoted newline
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#308: FILE: audio/paaudio.c:150:
+//}

ERROR: line over 90 characters
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: do not use C99 // comments
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: space prohibited between function name and open parenthesis '('
#324: FILE: audio/paaudio.c:157:
+int convert_samples = audio_MIN (samples, left_till_end_samples);

WARNING: line over 80 characters
#325: FILE: audio/paaudio.c:158:
+size_t convert_bytes_wanted = (size_t) convert_samples << 
hw->info.shift;

WARNING: line over 80 characters
#331: FILE: audio/paaudio.c:163:
+CHECK_SUCCESS_GOTO(pa->g, , convert_bytes == 
convert_bytes_wanted, fail);

ERROR: space prohibited between function name and open parenthesis '('
#339: FILE: audio/paaudio.c:166:
+hw->clip (pa_dst, src, convert_samples);

ERROR: line over 90 characters
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#369: FILE: audio/paaudio.c:177:
+pa_threaded_mainloop_unlock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#389: FILE: audio/paaudio.c:183:
+qpa_logerr (error, "qpa_run_out failed\n");

ERROR: space prohibited between function name and open parenthesis '('
#400: FILE: audio/paaudio.c:192:
+static int qpa_run_in (HWVoiceIn *hw)

ERROR: space prohibited between function name and open parenthesis '('
#415: FILE: audio/paaudio.c:203:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#416: FILE: audio/paaudio.c:204:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: line over 90 characters
#418: FILE: audio/paaudio.c:206:
+size_t bytes_wanted = ((unsigned int)(hw->samples - 
audio_pcm_hw_get_live_in(hw)) << hw->info.shift);

Re: [Qemu-devel] [PATCH 05/34] misc: remove duplicated includes

[Anthony PERARD]

On Fri, Sep 22, 2017 at 12:39:45PM -0300, Philippe Mathieu-Daudé wrote:
> exec: housekeeping (funny since 02d0e095031)
> 
> applied using ./scripts/clean-includes
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  accel/tcg/translate-all.c  | 1 -
>  exec.c | 3 ---
>  hw/arm/spitz.c | 1 -
>  hw/char/xen_console.c  | 1 -
>  hw/core/machine.c  | 1 -
>  hw/s390x/css.c | 1 -
>  target/openrisc/exception_helper.c | 1 -
>  tests/vhost-user-test.c| 1 -
>  util/qemu-sockets.c| 1 -
>  vl.c   | 1 -
>  10 files changed, 12 deletions(-)
> 

Reviewed-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH v2 15/47] hw/i386: Replace fprintf(stderr, "*\n" with error_report()

[Anthony PERARD]

On Fri, Sep 29, 2017 at 05:15:46PM -0700, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.

[...]

> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index d9ccd5d0d6..f8e3e0507b 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -246,9 +246,10 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, 
> MemoryRegion *mr,
>  
>  if (runstate_check(RUN_STATE_INMIGRATE)) {
>  /* RAM already populated in Xen */
> -fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
> -" bytes of ram at "RAM_ADDR_FMT" when runstate is 
> INMIGRATE\n",
> -__func__, size, ram_addr); 
> +error_report("%s: do not alloc "RAM_ADDR_FMT
> + " bytes of ram at "RAM_ADDR_FMT" when runstate is "
> + " INMIGRATE",
> + __func__, size, ram_addr);
>  return;
>  }
>  
> @@ -444,8 +445,9 @@ static int xen_remove_from_physmap(XenIOState *state,
>  
>  rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, 
> XENMAPSPACE_gmfn, idx, gpfn);
>  if (rc) {
> -fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> -PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, 
> errno);
> +error_report("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> + PRI_xen_pfn" failed: %d (errno: %d)", idx,
> + gpfn, rc, errno);
>  return -rc;
>  }
>  }
> @@ -1090,11 +1092,11 @@ static void cpu_handle_ioreq(void *opaque)
>  req->data = copy.data;
>  
>  if (req->state != STATE_IOREQ_INPROCESS) {
> -fprintf(stderr, "Badness in I/O request ... not in service?!: "
> -"%x, ptr: %x, port: %"PRIx64", "
> -"data: %"PRIx64", count: %u, size: %u, type: %u\n",
> -req->state, req->data_is_ptr, req->addr,
> -req->data, req->count, req->size, req->type);
> +error_report("Badness in I/O request ... not in service?!: "
> + "%x, ptr: %x, port: %"PRIx64", "
> + "data: %"PRIx64", count: %u, size: %u, type: %u",
> + req->state, req->data_is_ptr, req->addr,
> + req->data, req->count, req->size, req->type);
>  destroy_hvm_domain(false);
>  return;
>  }
> @@ -1397,16 +1399,16 @@ void destroy_hvm_domain(bool reboot)
>  
>  xc_handle = xc_interface_open(0, 0, 0);
>  if (xc_handle == NULL) {
> -fprintf(stderr, "Cannot acquire xenctrl handle\n");
> +error_report("Cannot acquire xenctrl handle");
>  } else {
>  sts = xc_domain_shutdown(xc_handle, xen_domid,
>   reboot ? SHUTDOWN_reboot : 
> SHUTDOWN_poweroff);
>  if (sts != 0) {
> -fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
> -"sts %d, %s\n", reboot ? "reboot" : "poweroff",
> +error_report("xc_domain_shutdown failed to issue %s, "
> +"sts %d, %s", reboot ? "reboot" : "poweroff",
>  sts, strerror(errno));
>  } else {
> -fprintf(stderr, "Issued domain %d %s\n", xen_domid,
> +error_report("Issued domain %d %s", xen_domid,
>  reboot ? "reboot" : "poweroff");
>  }
>  xc_interface_close(xc_handle);
> @@ -1425,7 +1427,7 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
>  va_start(ap, fmt);
>  vfprintf(stderr, fmt, ap);

Here, we may want to replace vfprintf by error_vreport as well?
Or is it more complicated and needs its own patch to fix all call to
xen_shutdown_fatal_error?

>  va_end(ap);
> -fprintf(stderr, "Will destroy the domain.\n");
> +error_report("Will destroy the domain.");
>  /* destroy the domain */
>  qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
>  }
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index baab93b614..4062af0900 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -377,7 +377,7 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
>  }
>  }
>  if (!found) {
> -fprintf(stderr, "%s, could not find %p\n", __func__, ptr);
> +error_report("%s, could not find %p", __func__, ptr);
>  QTAILQ_FOREACH(reventry, >locked_entries, next) {
>  DPRINTF("   "TARGET_FMT_plx" -> %p is present\n", 
> reventry->paddr_index,
>  reventry->vaddr_req);
> @@ -477,9 +477,9 @@ void xen_invalidate_map_cache(void)
>  if (!reventry->dma) {
>  continue;
>  }
> -fprintf(stderr, "Locked DMA mapping while invalidating mapcache!"
> -  

Re: [Qemu-devel] [PATCH 12/34] misc: remove old i386 dependency

[Anthony PERARD]

On Fri, Sep 22, 2017 at 12:39:52PM -0300, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/audio/pcspk.c| 1 -
>  hw/i386/kvm/pci-assign.c| 1 -
>  hw/i386/pci-assign-load-rom.c   | 1 -
>  hw/i386/xen/xen_platform.c  | 1 -
>  hw/isa/vt82c686.c   | 1 -
>  hw/misc/ivshmem.c   | 1 -
>  hw/misc/sga.c   | 1 -
>  hw/pci-bridge/pci_expander_bridge.c | 1 -
>  monitor.c   | 1 -
>  9 files changed, 9 deletions(-)
> 

Reviewed-by: Anthony PERARD 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until 
after os_setup_post"):
> I'm tring to find out what does calling xen_restrict_all(0), when
> running an non-Xen guest. I think it would just lock(), then unlock()
> then there should not be any handle to restrict, and return 0; is that
> right?

Yes.  If the process has not opened any Xen control handles,
xentoolcore_restrict_all is a no-op.

Ian.

Re: [Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> On Mon, Oct 09, 2017 at 05:28:08PM +0100, Ian Jackson wrote:
> > The xentoolcore library was just committed to xen.git#staging, so at
> > least this patch (or something like it) should go into qemu RSN.
> 
> I don't think it is necessary to do anything in qemu. The linker should
> find on its own the new libxentoolcore as long as an option
> -Wl,-rpath-link= provide the right path to xentoolcore when building
> qemu from xen.git.

But, the -rpath-link= specification in libxendevicemodel refers not to
the xen.git build tree, but to the eventual installed copy.

In my tests, this does in fact break the build.

>  In other cases, the pkg-config files should be
> enough (configure doesn't need to known about a new xentoolcore.pc
> file).

In that case, yes, the .pc works.

Ian.

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new 
-runasid option"):
> Actually, a numeric UID without group name or ID could be made to work
> just fine as long as it maps to a user name.  The use case may not be
> worth the bother, though.

In libxl's use case, it wouldn't map to a name.

> Using '.' to separate user and group is suboptimal, because POSIX
> portable user and group names may contain it:
...
> Coreutils uses ':'.  Let's follow its lead.

I'm happy to change it to use `:'.

Can you confirm that this command line interface is then OK ?
We would like to stablise the corresponding code in libxl...

Ian.

Re: [Qemu-devel] [PATCH] tcg: Initialize cpu_env generically

[Philippe Mathieu-Daudé]

On 10/10/2017 06:45 PM, Richard Henderson wrote:
> This is identical for each target.  So, move the initialization to
> common code.  Move the variable itself out of tcg_ctx and name it
> cpu_env to minimize changes within targets.
> 
> This also means we can remove tcg_global_reg_new_{ptr,i32,i64},
> since there are no longer global-register temps created by targets.
> 
> Signed-off-by: Richard Henderson 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  include/exec/gen-icount.h |  8 
>  target/arm/translate.h|  1 -
>  tcg/tcg.h |  9 +
>  target/alpha/translate.c  |  4 
>  target/arm/translate.c|  4 
>  target/cris/translate.c   |  3 ---
>  target/cris/translate_v10.c   |  2 --
>  target/hppa/translate.c   |  4 
>  target/i386/translate.c   |  3 ---
>  target/lm32/translate.c   |  4 
>  target/m68k/translate.c   |  5 -
>  target/microblaze/translate.c |  4 
>  target/mips/translate.c   |  4 
>  target/moxie/translate.c  |  7 ++-
>  target/nios2/translate.c  |  3 ---
>  target/openrisc/translate.c   |  3 ---
>  target/ppc/translate.c| 10 +++---
>  target/s390x/translate.c  |  6 --
>  target/sh4/translate.c|  7 +--
>  target/sparc/translate.c  |  4 
>  target/tilegx/translate.c |  3 ---
>  target/tricore/translate.c|  6 ++
>  target/unicore32/translate.c  |  4 
>  target/xtensa/translate.c |  3 ---
>  tcg/tcg-op.c  | 30 +++---
>  tcg/tcg.c | 31 +++
>  26 files changed, 35 insertions(+), 137 deletions(-)
> 
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index 9b3cb14dfa..de52a67ee8 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -19,7 +19,7 @@ static inline void gen_tb_start(TranslationBlock *tb)
>  count = tcg_temp_new_i32();
>  }
>  
> -tcg_gen_ld_i32(count, tcg_ctx.tcg_env,
> +tcg_gen_ld_i32(count, cpu_env,
> -ENV_OFFSET + offsetof(CPUState, icount_decr.u32));
>  
>  if (tb->cflags & CF_USE_ICOUNT) {
> @@ -37,7 +37,7 @@ static inline void gen_tb_start(TranslationBlock *tb)
>  tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, exitreq_label);
>  
>  if (tb->cflags & CF_USE_ICOUNT) {
> -tcg_gen_st16_i32(count, tcg_ctx.tcg_env,
> +tcg_gen_st16_i32(count, cpu_env,
>   -ENV_OFFSET + offsetof(CPUState, 
> icount_decr.u16.low));
>  }
>  
> @@ -62,7 +62,7 @@ static inline void gen_tb_end(TranslationBlock *tb, int 
> num_insns)
>  static inline void gen_io_start(void)
>  {
>  TCGv_i32 tmp = tcg_const_i32(1);
> -tcg_gen_st_i32(tmp, tcg_ctx.tcg_env,
> +tcg_gen_st_i32(tmp, cpu_env,
> -ENV_OFFSET + offsetof(CPUState, can_do_io));
>  tcg_temp_free_i32(tmp);
>  }
> @@ -70,7 +70,7 @@ static inline void gen_io_start(void)
>  static inline void gen_io_end(void)
>  {
>  TCGv_i32 tmp = tcg_const_i32(0);
> -tcg_gen_st_i32(tmp, tcg_ctx.tcg_env,
> +tcg_gen_st_i32(tmp, cpu_env,
> -ENV_OFFSET + offsetof(CPUState, can_do_io));
>  tcg_temp_free_i32(tmp);
>  }
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index 3c96aec956..410ba79c0d 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -80,7 +80,6 @@ typedef struct DisasCompare {
>  } DisasCompare;
>  
>  /* Share the TCG temporaries common between 32 and 64 bit modes.  */
> -extern TCGv_env cpu_env;
>  extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF;
>  extern TCGv_i64 cpu_exclusive_addr;
>  extern TCGv_i64 cpu_exclusive_val;
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index b2d42e3136..da1fefd6f1 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -702,7 +702,6 @@ struct TCGContext {
>  
>  /* Track which vCPU triggers events */
>  CPUState *cpu;  /* *_trans */
> -TCGv_env tcg_env;   /* *_exec  */
>  
>  /* These structures are private to tcg-target.inc.c.  */
>  #ifdef TCG_TARGET_NEED_LDST_LABELS
> @@ -727,6 +726,7 @@ struct TCGContext {
>  };
>  
>  extern TCGContext tcg_ctx;
> +extern TCGv_env cpu_env;
>  extern bool parallel_cpus;
>  
>  static inline void tcg_set_insn_param(int op_idx, int arg, TCGArg v)
> @@ -783,9 +783,6 @@ void tcg_set_frame(TCGContext *s, TCGReg reg, intptr_t 
> start, intptr_t size);
>  
>  int tcg_global_mem_new_internal(TCGType, TCGv_ptr, intptr_t, const char *);
>  
> -TCGv_i32 tcg_global_reg_new_i32(TCGReg reg, const char *name);
> -TCGv_i64 tcg_global_reg_new_i64(TCGReg reg, const char *name);
> -
>  TCGv_i32 tcg_temp_new_internal_i32(int temp_local);
>  TCGv_i64 tcg_temp_new_internal_i64(int temp_local);
>  
> @@ -904,8 +901,6 @@ do {\
>  #define TCGV_PTR_TO_NAT(n) MAKE_TCGV_I32(GET_TCGV_PTR(n))
>  
>  #define tcg_const_ptr(V) 

Re: [Qemu-devel] [PATCH 37/42] tpm: lookup the the TPM interface instead of TIS device

[Marc-André Lureau]

Hi

- Original Message -
> On 10/10/2017 04:21 PM, Eduardo Habkost wrote:
> > On Tue, Oct 10, 2017 at 12:56:18AM +0200, Marc-André Lureau wrote:
> > [...]
> >> -static inline TPMVersion tpm_get_version(void)
> >> +static inline TPMIf *tpm_find(void)
> >>   {
> >> -#ifdef CONFIG_TPM
> >> -Object *obj = object_resolve_path_type("", TYPE_TPM_TIS, NULL);
> >> +Object *obj = object_resolve_path_type("", TYPE_TPM_IF, NULL);
> > Considering that tpm_crb_realizefn() will rely on tpm_find()
> > returning NULL if there are multiple TPM devices, I suggest
> > adding a "returns NULL unless there is exactly one TPM device"
> > comment, just like fw_cfg_find() and find_vmgenid_dev()
> 
> I wonder whether the function couldn't have a better name.
> tpm_find_single() ?
> 

As Eduardo said, there is precedence in QEMU codebase (fw_cfg_find() and 
find_vmgenid_dev())

I don't think foo_find() is a bad name - it returns NULL if there are multiple 
foo, which makes sense imho. I'll add the missing comment though.

Re: [Qemu-devel] [PATCH 27/42] tpm: remove unused opened code

[Marc-André Lureau]

Hi

- Original Message -
> On 10/09/2017 06:56 PM, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   include/sysemu/tpm_backend.h | 12 
> >   backends/tpm.c   | 42
> >   --
> >   tpm.c|  6 --
> >   3 files changed, 60 deletions(-)
> >
> > diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> > index b12ae5b625..a893e586ae 100644
> > --- a/include/sysemu/tpm_backend.h
> > +++ b/include/sysemu/tpm_backend.h
> > @@ -81,8 +81,6 @@ struct TPMBackendClass {
> >   
> >   TpmTypeOptions *(*get_tpm_options)(TPMBackend *t);
> >   
> > -void (*opened)(TPMBackend *s, Error **errp);
> > -
> >   void (*handle_request)(TPMBackend *s, TPMBackendCmd *cmd);
> >   };
> >   
> > @@ -172,16 +170,6 @@ bool tpm_backend_get_tpm_established_flag(TPMBackend
> > *s);
> >*/
> >   int tpm_backend_reset_tpm_established_flag(TPMBackend *s, uint8_t locty);
> >   
> > -/**
> > - * tpm_backend_open:
> > - * @s: the backend to open
> > - * @errp: a pointer to return the #Error object if an error occurs.
> > - *
> > - * This function will open the backend if it is not already open.  Calling
> > this
> > - * function on an already opened backend will not result in an error.
> > - */
> > -void tpm_backend_open(TPMBackend *s, Error **errp);
> > -
> >   /**
> >* tpm_backend_get_tpm_version:
> >* @s: the backend to call into
> > diff --git a/backends/tpm.c b/backends/tpm.c
> > index 0c48d18775..7e636fbc7a 100644
> > --- a/backends/tpm.c
> > +++ b/backends/tpm.c
> > @@ -159,52 +159,10 @@ TPMInfo *tpm_backend_query_tpm(TPMBackend *s)
> >   return info;
> >   }
> >   
> > -static bool tpm_backend_prop_get_opened(Object *obj, Error **errp)
> > -{
> > -TPMBackend *s = TPM_BACKEND(obj);
> > -
> > -return s->opened;
> > -}
> > -
> > -void tpm_backend_open(TPMBackend *s, Error **errp)
> > -{
> > -object_property_set_bool(OBJECT(s), true, "opened", errp);
> > -}
> > -
> > -static void tpm_backend_prop_set_opened(Object *obj, bool value, Error
> > **errp)
> > -{
> > -TPMBackend *s = TPM_BACKEND(obj);
> > -TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
> > -Error *local_err = NULL;
> > -
> > -if (value == s->opened) {
> > -return;
> > -}
> > -
> > -if (!value && s->opened) {
> > -error_setg(errp, QERR_PERMISSION_DENIED);
> > -return;
> > -}
> > -
> > -if (k->opened) {
> > -k->opened(s, _err);
> > -if (local_err) {
> > -error_propagate(errp, local_err);
> > -return;
> > -}
> > -}
> > -
> > -s->opened = true;
> > -}
> > -
> >   static void tpm_backend_instance_init(Object *obj)
> >   {
> >   TPMBackend *s = TPM_BACKEND(obj);
> >   
> > -object_property_add_bool(obj, "opened",
> > - tpm_backend_prop_get_opened,
> > - tpm_backend_prop_set_opened,
> > - NULL);
> >   s->bh = qemu_bh_new(tpm_backend_request_completed_bh, s);
> >   }
> >   
> > diff --git a/tpm.c b/tpm.c
> > index ce1543fcb4..a46ee5f144 100644
> > --- a/tpm.c
> > +++ b/tpm.c
> > @@ -134,12 +134,6 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts
> > *opts, Error **errp)
> >   return 1;
> >   }
> >   
> > -tpm_backend_open(drv, _err);
> > -if (local_err) {
> > -error_report_err(local_err);
> > -return 1;
> > -}
> > -
> >   QLIST_INSERT_HEAD(_backends, drv, list);
> >   
> >   return 0;
> 
> Since nothing is setting the "opened' anymore, would anyone notice
> because this has changed or is something else doing that now? Otherwise
> I don't mind removing it...

Was it ever used? I see the property was added in commit 
8f0605cc9caacbcc647a6df9ae541ed2da4b9bb0.

Code adapted from rng backend. But rng backend set "opened" on 
UserCreatableClass.complete() (TPM backend doesn't, but probably could - I have 
an experimental patch for that)

I think we can remove it that code safely.

Re: [Qemu-devel] [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ian Jackson]

Ross Lagerwall writes ("Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict 
improvements"):
> If no one objects, I propose adding the following calls to 
> libxendevicemodel (with underlying Xen implementations, of course) that 
> would be usable after the xendevicemodel handle has been restricted.
> 
> xendevicemodel_add_to_physmap(xendevicemodel_handle *dmod,
...
> xendevicemodel_pin_memory_cacheattr(xendevicemodel_handle *dmod,
...
> This is equivalent to xc_domain_pin_memory_cacheattr().

These seem fine to me.  If you want this in Xen 4.10 I think you will
want to make a case to Julien (not CC'd) for a release ack.

Ian.

Re: [Qemu-devel] [PATCH 33/42] tpm-passthrough: remove error cleanup from handle_device_opts

[Marc-André Lureau]

Hi

- Original Message -
> On 10/09/2017 06:56 PM, Marc-André Lureau wrote:
> > Clean-up is handled by the create() function.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   hw/tpm/tpm_passthrough.c | 15 ++-
> >   1 file changed, 2 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> > index aa9167e3c6..0806cf86af 100644
> > --- a/hw/tpm/tpm_passthrough.c
> > +++ b/hw/tpm/tpm_passthrough.c
> > @@ -261,27 +261,16 @@ tpm_passthrough_handle_device_opts(TPMPassthruState
> > *tpm_pt, QemuOpts *opts)
> >   if (tpm_pt->tpm_fd < 0) {
> >   error_report("Cannot access TPM device using '%s': %s",
> >tpm_pt->tpm_dev, strerror(errno));
> > -goto err_free_parameters;
> > +return -1;
> >   }
> >
> >   if (tpm_util_test_tpmdev(tpm_pt->tpm_fd, _pt->tpm_version)) {
> >   error_report("'%s' is not a TPM device.",
> >tpm_pt->tpm_dev);
> > -goto err_close_tpmdev;
> > +return -1;
> >   }
> 
> I would prefer the cleanup to happen in the functions where the state is
> created...

This is the role for a destructor, no need to worry about local object change 
cleanup.

I can drop the patch if you feel strongly about it, but I think it's a nice 
code simplification.

Re: [Qemu-devel] [PATCH 40/88] hw/xen: use g_new() family of functions

[Anthony PERARD]

On Fri, Oct 06, 2017 at 08:49:35PM -0300, Philippe Mathieu-Daudé wrote:
> From: Marc-André Lureau 
> 
> Signed-off-by: Marc-André Lureau 
> Signed-off-by: Philippe Mathieu-Daudé 
> [PMD: replaced g_new0() -> g_new() in xen_remap_bucket() (no bzero required),
>   renamed X86 -> hw/xen and few other changes]

Reviewed-by: Anthony PERARD 

Thanks,

-- 
Anthony PERARD

[Qemu-devel] [PATCH] tcg: Initialize cpu_env generically

[Richard Henderson]

This is identical for each target.  So, move the initialization to
common code.  Move the variable itself out of tcg_ctx and name it
cpu_env to minimize changes within targets.

This also means we can remove tcg_global_reg_new_{ptr,i32,i64},
since there are no longer global-register temps created by targets.

Signed-off-by: Richard Henderson 
---
 include/exec/gen-icount.h |  8 
 target/arm/translate.h|  1 -
 tcg/tcg.h |  9 +
 target/alpha/translate.c  |  4 
 target/arm/translate.c|  4 
 target/cris/translate.c   |  3 ---
 target/cris/translate_v10.c   |  2 --
 target/hppa/translate.c   |  4 
 target/i386/translate.c   |  3 ---
 target/lm32/translate.c   |  4 
 target/m68k/translate.c   |  5 -
 target/microblaze/translate.c |  4 
 target/mips/translate.c   |  4 
 target/moxie/translate.c  |  7 ++-
 target/nios2/translate.c  |  3 ---
 target/openrisc/translate.c   |  3 ---
 target/ppc/translate.c| 10 +++---
 target/s390x/translate.c  |  6 --
 target/sh4/translate.c|  7 +--
 target/sparc/translate.c  |  4 
 target/tilegx/translate.c |  3 ---
 target/tricore/translate.c|  6 ++
 target/unicore32/translate.c  |  4 
 target/xtensa/translate.c |  3 ---
 tcg/tcg-op.c  | 30 +++---
 tcg/tcg.c | 31 +++
 26 files changed, 35 insertions(+), 137 deletions(-)

diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 9b3cb14dfa..de52a67ee8 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -19,7 +19,7 @@ static inline void gen_tb_start(TranslationBlock *tb)
 count = tcg_temp_new_i32();
 }
 
-tcg_gen_ld_i32(count, tcg_ctx.tcg_env,
+tcg_gen_ld_i32(count, cpu_env,
-ENV_OFFSET + offsetof(CPUState, icount_decr.u32));
 
 if (tb->cflags & CF_USE_ICOUNT) {
@@ -37,7 +37,7 @@ static inline void gen_tb_start(TranslationBlock *tb)
 tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, exitreq_label);
 
 if (tb->cflags & CF_USE_ICOUNT) {
-tcg_gen_st16_i32(count, tcg_ctx.tcg_env,
+tcg_gen_st16_i32(count, cpu_env,
  -ENV_OFFSET + offsetof(CPUState, 
icount_decr.u16.low));
 }
 
@@ -62,7 +62,7 @@ static inline void gen_tb_end(TranslationBlock *tb, int 
num_insns)
 static inline void gen_io_start(void)
 {
 TCGv_i32 tmp = tcg_const_i32(1);
-tcg_gen_st_i32(tmp, tcg_ctx.tcg_env,
+tcg_gen_st_i32(tmp, cpu_env,
-ENV_OFFSET + offsetof(CPUState, can_do_io));
 tcg_temp_free_i32(tmp);
 }
@@ -70,7 +70,7 @@ static inline void gen_io_start(void)
 static inline void gen_io_end(void)
 {
 TCGv_i32 tmp = tcg_const_i32(0);
-tcg_gen_st_i32(tmp, tcg_ctx.tcg_env,
+tcg_gen_st_i32(tmp, cpu_env,
-ENV_OFFSET + offsetof(CPUState, can_do_io));
 tcg_temp_free_i32(tmp);
 }
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 3c96aec956..410ba79c0d 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -80,7 +80,6 @@ typedef struct DisasCompare {
 } DisasCompare;
 
 /* Share the TCG temporaries common between 32 and 64 bit modes.  */
-extern TCGv_env cpu_env;
 extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF;
 extern TCGv_i64 cpu_exclusive_addr;
 extern TCGv_i64 cpu_exclusive_val;
diff --git a/tcg/tcg.h b/tcg/tcg.h
index b2d42e3136..da1fefd6f1 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -702,7 +702,6 @@ struct TCGContext {
 
 /* Track which vCPU triggers events */
 CPUState *cpu;  /* *_trans */
-TCGv_env tcg_env;   /* *_exec  */
 
 /* These structures are private to tcg-target.inc.c.  */
 #ifdef TCG_TARGET_NEED_LDST_LABELS
@@ -727,6 +726,7 @@ struct TCGContext {
 };
 
 extern TCGContext tcg_ctx;
+extern TCGv_env cpu_env;
 extern bool parallel_cpus;
 
 static inline void tcg_set_insn_param(int op_idx, int arg, TCGArg v)
@@ -783,9 +783,6 @@ void tcg_set_frame(TCGContext *s, TCGReg reg, intptr_t 
start, intptr_t size);
 
 int tcg_global_mem_new_internal(TCGType, TCGv_ptr, intptr_t, const char *);
 
-TCGv_i32 tcg_global_reg_new_i32(TCGReg reg, const char *name);
-TCGv_i64 tcg_global_reg_new_i64(TCGReg reg, const char *name);
-
 TCGv_i32 tcg_temp_new_internal_i32(int temp_local);
 TCGv_i64 tcg_temp_new_internal_i64(int temp_local);
 
@@ -904,8 +901,6 @@ do {\
 #define TCGV_PTR_TO_NAT(n) MAKE_TCGV_I32(GET_TCGV_PTR(n))
 
 #define tcg_const_ptr(V) TCGV_NAT_TO_PTR(tcg_const_i32((intptr_t)(V)))
-#define tcg_global_reg_new_ptr(R, N) \
-TCGV_NAT_TO_PTR(tcg_global_reg_new_i32((R), (N)))
 #define tcg_global_mem_new_ptr(R, O, N) \
 TCGV_NAT_TO_PTR(tcg_global_mem_new_i32((R), (O), (N)))
 #define tcg_temp_new_ptr() TCGV_NAT_TO_PTR(tcg_temp_new_i32())
@@ -915,8 +910,6 @@ do {\
 #define TCGV_PTR_TO_NAT(n) 

[Qemu-devel] [PULL v2 26/27] vhost-user-scsi: use libvhost-user glib helper

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 162 +++---
 1 file changed, 16 insertions(+), 146 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index ff817d6643..615e2a76bb 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -11,7 +11,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "contrib/libvhost-user/libvhost-user.h"
+#include "contrib/libvhost-user/libvhost-user-glib.h"
 #include "standard-headers/linux/virtio_scsi.h"
 #include "iscsi/iscsi.h"
 #include "iscsi/scsi-lowlevel.h"
@@ -26,95 +26,13 @@ typedef struct VusIscsiLun {
 } VusIscsiLun;
 
 typedef struct VusDev {
-VuDev vu_dev;
+VugDev parent;
+
 int server_sock;
-GMainLoop *loop;
-GHashTable *fdmap;   /* fd -> gsource */
 VusIscsiLun lun;
+GMainLoop *loop;
 } VusDev;
 
-/** glib event loop integration for libvhost-user and misc callbacks **/
-
-QEMU_BUILD_BUG_ON((int)G_IO_IN != (int)VU_WATCH_IN);
-QEMU_BUILD_BUG_ON((int)G_IO_OUT != (int)VU_WATCH_OUT);
-QEMU_BUILD_BUG_ON((int)G_IO_PRI != (int)VU_WATCH_PRI);
-QEMU_BUILD_BUG_ON((int)G_IO_ERR != (int)VU_WATCH_ERR);
-QEMU_BUILD_BUG_ON((int)G_IO_HUP != (int)VU_WATCH_HUP);
-
-typedef struct vus_gsrc {
-GSource parent;
-VusDev *vdev_scsi;
-GPollFD gfd;
-} vus_gsrc_t;
-
-static gboolean vus_gsrc_prepare(GSource *src, gint *timeout)
-{
-assert(timeout);
-
-*timeout = -1;
-return FALSE;
-}
-
-static gboolean vus_gsrc_check(GSource *src)
-{
-vus_gsrc_t *vus_src = (vus_gsrc_t *)src;
-
-assert(vus_src);
-
-return vus_src->gfd.revents & vus_src->gfd.events;
-}
-
-static gboolean vus_gsrc_dispatch(GSource *src, GSourceFunc cb, gpointer data)
-{
-VusDev *vdev_scsi;
-vus_gsrc_t *vus_src = (vus_gsrc_t *)src;
-
-assert(vus_src);
-
-vdev_scsi = vus_src->vdev_scsi;
-
-assert(vdev_scsi);
-
-((vu_watch_cb)cb)(_scsi->vu_dev, vus_src->gfd.revents, data);
-
-return G_SOURCE_CONTINUE;
-}
-
-static GSourceFuncs vus_gsrc_funcs = {
-vus_gsrc_prepare,
-vus_gsrc_check,
-vus_gsrc_dispatch,
-NULL
-};
-
-static GSource *vus_gsrc_new(VusDev *vdev_scsi, int fd, GIOCondition cond,
- vu_watch_cb vu_cb, GSourceFunc gsrc_cb, gpointer 
data)
-{
-GSource *vus_gsrc;
-vus_gsrc_t *vus_src;
-guint id;
-
-assert(vdev_scsi);
-assert(fd >= 0);
-assert(vu_cb || gsrc_cb);
-assert(!(vu_cb && gsrc_cb));
-
-vus_gsrc = g_source_new(_gsrc_funcs, sizeof(vus_gsrc_t));
-g_source_set_callback(vus_gsrc, (GSourceFunc) vu_cb, data, NULL);
-vus_src = (vus_gsrc_t *)vus_gsrc;
-vus_src->vdev_scsi = vdev_scsi;
-vus_src->gfd.fd = fd;
-vus_src->gfd.events = cond;
-
-g_source_add_poll(vus_gsrc, _src->gfd);
-g_source_set_callback(vus_gsrc, gsrc_cb, data, NULL);
-id = g_source_attach(vus_gsrc, NULL);
-assert(id);
-g_source_unref(vus_gsrc);
-
-return vus_gsrc;
-}
-
 /** libiscsi integration **/
 
 typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq;
@@ -291,11 +209,13 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 
 static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 {
+VugDev *gdev;
 VusDev *vdev_scsi;
 
 assert(vu_dev);
 
-vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
+gdev = container_of(vu_dev, VugDev, parent);
+vdev_scsi = container_of(gdev, VusDev, parent);
 if (buf) {
 g_warning("vu_panic: %s", buf);
 }
@@ -303,40 +223,16 @@ static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 g_main_loop_quit(vdev_scsi->loop);
 }
 
-static void vus_add_watch_cb(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb cb,
- void *pvt)
-{
-GSource *src;
-VusDev *vdev_scsi;
-
-assert(vu_dev);
-assert(fd >= 0);
-assert(cb);
-
-vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
-src = vus_gsrc_new(vdev_scsi, fd, vu_evt, cb, NULL, pvt);
-g_hash_table_replace(vdev_scsi->fdmap, GINT_TO_POINTER(fd), src);
-}
-
-static void vus_del_watch_cb(VuDev *vu_dev, int fd)
-{
-VusDev *vdev_scsi;
-
-assert(vu_dev);
-assert(fd >= 0);
-
-vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
-g_hash_table_remove(vdev_scsi->fdmap, GINT_TO_POINTER(fd));
-}
-
 static void vus_proc_req(VuDev *vu_dev, int idx)
 {
+VugDev *gdev;
 VusDev *vdev_scsi;
 VuVirtq *vq;
 
 assert(vu_dev);
 
-vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
+gdev = container_of(vu_dev, VugDev, parent);
+vdev_scsi = container_of(gdev, VusDev, parent);
 if (idx < 0 || idx >= VHOST_MAX_NR_VIRTQUEUE) {
 g_warning("VQ Index out of range: %d", idx);
 vus_panic_cb(vu_dev, NULL);
@@ -420,21 +316,6 @@ static const VuDevIface vus_iface = {
 .queue_set_started = vus_queue_set_started,
 };
 
-static 

Re: [Qemu-devel] [PATCH RFC] file-posix: make lock_fd read-only

[Eric Blake]

On 10/10/2017 02:30 PM, Denis V. Lunev wrote:
> On 10/10/2017 09:50 PM, Eric Blake wrote:
>> On 10/10/2017 08:42 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> We do not reopen lock_fd on bdrv_reopen which leads to problems on
>>> reopen image RO. So, lets make lock_fd be always RO.
>>> This is correct, because qemu_lock_fd always called with exclusive=false
>>> on lock_fd.
>> How is that correct?  file-posix.c calls:
>> ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
>> where exclusive being true in turn sets:
>> .l_type   = exclusive ? F_WRLCK : F_RDLCK,
>>
>> and F_WRLCK requests fail on a RO fd with EBADF.
> this works fine for us as here we do not get the lock but just
> query it.
> 

>     ftruncate(fd, 1024);
>     fcntl(fd, F_SETLK, _rd);
>     fcntl(fd, F_GETLK, _wr);

Trying with F_OFD_SETLK (which is more important, because it matches
what we prefer to use) gives the same results as your strace (remember
to #define _GNU_SOURCE 1 before compilation, to get F_OFD_SETLK into scope).

> 
> The key is that in test cod we do not _set_ the lock, but query it! This
> is allowed even on
> RDONLY file descriptor.

Hmm, you're right: file-posix only uses qemu_lock_fd() to set locks
(with exclusive == false), and qemu_lock_fd_test() to query locks
(there, exclusive == true means to probe if a write lock can be grabbed,
but does not actually grab one so it doesn't care whether the fd is RW.
We ).

We'd need a lot more explanation in the commit message (for example, you
still haven't stated an actual backtrace or error message you got when
the lock file is left RW), but you may have just convinced me that the
conversion to always use a RO lock fd is correct, at least for
file-posix' use of fcntl locking.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PULL v2 21/27] vhost-user-scsi: don't copy iscsi/scsi-lowlevel.h

[Marc-André Lureau]

There is no need to include hw/virtio/virtio-scsi.h, then the conflict
with SCSI_XFER enum goes away.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 96 +++
 1 file changed, 9 insertions(+), 87 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index c7f9d82142..cfac1ce1fe 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -12,8 +12,9 @@
 
 #include "qemu/osdep.h"
 #include "contrib/libvhost-user/libvhost-user.h"
-#include "hw/virtio/virtio-scsi.h"
+#include "standard-headers/linux/virtio_scsi.h"
 #include "iscsi/iscsi.h"
+#include "iscsi/scsi-lowlevel.h"
 
 #include 
 
@@ -163,90 +164,11 @@ static void vus_gsrc_new(VusDev *vdev_scsi, int fd, 
GIOCondition cond,
 (gpointer)(uintptr_t)id);
 }
 
-/* from libiscsi's scsi-lowlevel.h **
- *
- * nb. We can't directly include scsi-lowlevel.h due to a namespace conflict:
- * QEMU's scsi.h also defines "SCSI_XFER_NONE".
- */
-
-#define SCSI_CDB_MAX_SIZE   16
-
-struct scsi_iovector {
-struct scsi_iovec *iov;
-int niov;
-int nalloc;
-size_t offset;
-int consumed;
-};
-
-struct scsi_allocated_memory {
-struct scsi_allocated_memory *next;
-char buf[0];
-};
-
-struct scsi_data {
-intsize;
-unsigned char *data;
-};
-
-enum scsi_sense_key {
-SCSI_SENSE_NO_SENSE= 0x00,
-SCSI_SENSE_RECOVERED_ERROR = 0x01,
-SCSI_SENSE_NOT_READY   = 0x02,
-SCSI_SENSE_MEDIUM_ERROR= 0x03,
-SCSI_SENSE_HARDWARE_ERROR  = 0x04,
-SCSI_SENSE_ILLEGAL_REQUEST = 0x05,
-SCSI_SENSE_UNIT_ATTENTION  = 0x06,
-SCSI_SENSE_DATA_PROTECTION = 0x07,
-SCSI_SENSE_BLANK_CHECK = 0x08,
-SCSI_SENSE_VENDOR_SPECIFIC = 0x09,
-SCSI_SENSE_COPY_ABORTED= 0x0a,
-SCSI_SENSE_COMMAND_ABORTED = 0x0b,
-SCSI_SENSE_OBSOLETE_ERROR_CODE = 0x0c,
-SCSI_SENSE_OVERFLOW_COMMAND= 0x0d,
-SCSI_SENSE_MISCOMPARE  = 0x0e
-};
-
-struct scsi_sense {
-unsigned char   error_type;
-enum scsi_sense_key key;
-int ascq;
-unsignedsense_specific:1;
-unsignedill_param_in_cdb:1;
-unsignedbit_pointer_valid:1;
-unsigned char   bit_pointer;
-uint16_tfield_pointer;
-};
-
-enum scsi_residual {
-SCSI_RESIDUAL_NO_RESIDUAL = 0,
-SCSI_RESIDUAL_UNDERFLOW,
-SCSI_RESIDUAL_OVERFLOW
-};
-
-struct scsi_task {
-int status;
-int cdb_size;
-int xfer_dir;
-int expxferlen;
-unsigned char cdb[SCSI_CDB_MAX_SIZE];
-enum scsi_residual residual_status;
-size_t residual;
-struct scsi_sense sense;
-struct scsi_data datain;
-struct scsi_allocated_memory *mem;
-void *ptr;
-
-uint32_t itt;
-uint32_t cmdsn;
-uint32_t lun;
-
-struct scsi_iovector iovector_in;
-struct scsi_iovector iovector_out;
-};
-
 /** libiscsi integration **/
 
+typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq;
+typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp;
+
 static int vus_iscsi_add_lun(VusIscsiLun *lun, char *iscsi_uri)
 {
 struct iscsi_url *iscsi_url;
@@ -365,12 +287,12 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 if (!out_len && !in_len) {
 dir = SCSI_XFER_NONE;
 } else if (out_len) {
-dir = SCSI_XFER_TO_DEV;
+dir = SCSI_XFER_WRITE;
 for (i = 0; i < out_len; i++) {
 len += out[i].iov_len;
 }
 } else {
-dir = SCSI_XFER_FROM_DEV;
+dir = SCSI_XFER_READ;
 for (i = 0; i < in_len; i++) {
 len += in[i].iov_len;
 }
@@ -378,10 +300,10 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 
 task = scsi_task_new(cdb_len, req->cdb, dir, len);
 
-if (dir == SCSI_XFER_TO_DEV) {
+if (dir == SCSI_XFER_WRITE) {
 task->iovector_out.iov = (struct scsi_iovec *)out;
 task->iovector_out.niov = out_len;
-} else if (dir == SCSI_XFER_FROM_DEV) {
+} else if (dir == SCSI_XFER_READ) {
 task->iovector_in.iov = (struct scsi_iovec *)in;
 task->iovector_in.niov = in_len;
 }
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 25/27] libvhost-user: add glib source helper

[Marc-André Lureau]

This file implements a bridge from the vu_init API of libvhost-user to
GSource, so that libvhost-user can be used inside a GLib main loop.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/libvhost-user/libvhost-user-glib.h |  32 ++
 contrib/libvhost-user/libvhost-user-glib.c | 154 +
 contrib/libvhost-user/Makefile.objs|   2 +-
 3 files changed, 187 insertions(+), 1 deletion(-)
 create mode 100644 contrib/libvhost-user/libvhost-user-glib.h
 create mode 100644 contrib/libvhost-user/libvhost-user-glib.c

diff --git a/contrib/libvhost-user/libvhost-user-glib.h 
b/contrib/libvhost-user/libvhost-user-glib.h
new file mode 100644
index 00..6b2110b94c
--- /dev/null
+++ b/contrib/libvhost-user/libvhost-user-glib.h
@@ -0,0 +1,32 @@
+/*
+ * Vhost User library
+ *
+ * Copyright (c) 2016 Nutanix Inc. All rights reserved.
+ * Copyright (c) 2017 Red Hat, Inc.
+ *
+ * Authors:
+ *  Marc-André Lureau 
+ *  Felipe Franciosi 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#ifndef LIBVHOST_USER_GLIB_H
+#define LIBVHOST_USER_GLIB_H
+
+#include 
+#include "libvhost-user.h"
+
+typedef struct VugDev {
+VuDev parent;
+
+GHashTable *fdmap; /* fd -> gsource */
+GSource *src;
+} VugDev;
+
+void vug_init(VugDev *dev, int socket,
+  vu_panic_cb panic, const VuDevIface *iface);
+void vug_deinit(VugDev *dev);
+
+#endif /* LIBVHOST_USER_GLIB_H */
diff --git a/contrib/libvhost-user/libvhost-user-glib.c 
b/contrib/libvhost-user/libvhost-user-glib.c
new file mode 100644
index 00..545f089587
--- /dev/null
+++ b/contrib/libvhost-user/libvhost-user-glib.c
@@ -0,0 +1,154 @@
+/*
+ * Vhost User library
+ *
+ * Copyright (c) 2016 Nutanix Inc. All rights reserved.
+ * Copyright (c) 2017 Red Hat, Inc.
+ *
+ * Authors:
+ *  Marc-André Lureau 
+ *  Felipe Franciosi 
+ *
+ * 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 "libvhost-user-glib.h"
+
+/* glib event loop integration for libvhost-user and misc callbacks */
+
+G_STATIC_ASSERT((int)G_IO_IN == (int)VU_WATCH_IN);
+G_STATIC_ASSERT((int)G_IO_OUT == (int)VU_WATCH_OUT);
+G_STATIC_ASSERT((int)G_IO_PRI == (int)VU_WATCH_PRI);
+G_STATIC_ASSERT((int)G_IO_ERR == (int)VU_WATCH_ERR);
+G_STATIC_ASSERT((int)G_IO_HUP == (int)VU_WATCH_HUP);
+
+typedef struct VugSrc {
+GSource parent;
+VuDev *dev;
+GPollFD gfd;
+} VugSrc;
+
+static gboolean
+vug_src_prepare(GSource *gsrc, gint *timeout)
+{
+g_assert(timeout);
+
+*timeout = -1;
+return FALSE;
+}
+
+static gboolean
+vug_src_check(GSource *gsrc)
+{
+VugSrc *src = (VugSrc *)gsrc;
+
+g_assert(src);
+
+return src->gfd.revents & src->gfd.events;
+}
+
+static gboolean
+vug_src_dispatch(GSource *gsrc, GSourceFunc cb, gpointer data)
+{
+VugSrc *src = (VugSrc *)gsrc;
+
+g_assert(src);
+
+((vu_watch_cb)cb)(src->dev, src->gfd.revents, data);
+
+return G_SOURCE_CONTINUE;
+}
+
+static GSourceFuncs vug_src_funcs = {
+vug_src_prepare,
+vug_src_check,
+vug_src_dispatch,
+NULL
+};
+
+static GSource *
+vug_source_new(VuDev *dev, int fd, GIOCondition cond,
+   vu_watch_cb vu_cb, gpointer data)
+{
+GSource *gsrc;
+VugSrc *src;
+guint id;
+
+g_assert(dev);
+g_assert(fd >= 0);
+g_assert(vu_cb);
+
+gsrc = g_source_new(_src_funcs, sizeof(VugSrc));
+g_source_set_callback(gsrc, (GSourceFunc)vu_cb, data, NULL);
+src = (VugSrc *)gsrc;
+src->dev = dev;
+src->gfd.fd = fd;
+src->gfd.events = cond;
+
+g_source_add_poll(gsrc, >gfd);
+id = g_source_attach(gsrc, NULL);
+g_assert(id);
+g_source_unref(gsrc);
+
+return gsrc;
+}
+
+static void
+set_watch(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb cb, void *pvt)
+{
+GSource *src;
+VugDev *dev;
+
+g_assert(vu_dev);
+g_assert(fd >= 0);
+g_assert(cb);
+
+dev = container_of(vu_dev, VugDev, parent);
+src = vug_source_new(vu_dev, fd, vu_evt, cb, pvt);
+g_hash_table_replace(dev->fdmap, GINT_TO_POINTER(fd), src);
+}
+
+static void
+remove_watch(VuDev *vu_dev, int fd)
+{
+VugDev *dev;
+
+g_assert(vu_dev);
+g_assert(fd >= 0);
+
+dev = container_of(vu_dev, VugDev, parent);
+g_hash_table_remove(dev->fdmap, GINT_TO_POINTER(fd));
+}
+
+
+static void vug_watch(VuDev *dev, int condition, void *data)
+{
+if (!vu_dispatch(dev) != 0) {
+dev->panic(dev, "Error processing vhost message");
+}
+}
+
+void
+vug_init(VugDev *dev, int socket,
+ vu_panic_cb panic, const VuDevIface *iface)
+{
+g_assert(dev);
+g_assert(iface);
+
+vu_init(>parent, socket, panic, set_watch, remove_watch, iface);
+

[Qemu-devel] [PULL v2 19/27] vhost-user-scsi: rename VUS types

[Marc-André Lureau]

- use Vus prefix consistently
- use CamelCase, since that's glib & libvhost-user style
- avoid _t postfix, usually for system headers

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 46 +++
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 26b17f348b..fdd6014712 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -55,18 +55,18 @@
 
 #define VUS_ISCSI_INITIATOR "iqn.2016-11.com.nutanix:vhost-user-scsi"
 
-typedef struct iscsi_lun {
+typedef struct VusIscsiLun {
 struct iscsi_context *iscsi_ctx;
 int iscsi_lun;
-} iscsi_lun_t;
+} VusIscsiLun;
 
-typedef struct vhost_scsi_dev {
+typedef struct VusDev {
 VuDev vu_dev;
 int server_sock;
 GMainLoop *loop;
 GTree *fdmap;   /* fd -> gsource context id */
-iscsi_lun_t lun;
-} vhost_scsi_dev_t;
+VusIscsiLun lun;
+} VusDev;
 
 /** glib event loop integration for libvhost-user and misc callbacks **/
 
@@ -78,7 +78,7 @@ QEMU_BUILD_BUG_ON((int)G_IO_HUP != (int)VU_WATCH_HUP);
 
 typedef struct vus_gsrc {
 GSource parent;
-vhost_scsi_dev_t *vdev_scsi;
+VusDev *vdev_scsi;
 GPollFD gfd;
 vu_watch_cb vu_cb;
 } vus_gsrc_t;
@@ -107,7 +107,7 @@ static gboolean vus_gsrc_check(GSource *src)
 
 static gboolean vus_gsrc_dispatch(GSource *src, GSourceFunc cb, gpointer data)
 {
-vhost_scsi_dev_t *vdev_scsi;
+VusDev *vdev_scsi;
 vus_gsrc_t *vus_src = (vus_gsrc_t *)src;
 
 assert(vus_src);
@@ -133,7 +133,7 @@ static GSourceFuncs vus_gsrc_funcs = {
 NULL
 };
 
-static void vus_gsrc_new(vhost_scsi_dev_t *vdev_scsi, int fd, GIOCondition 
cond,
+static void vus_gsrc_new(VusDev *vdev_scsi, int fd, GIOCondition cond,
  vu_watch_cb vu_cb, GSourceFunc gsrc_cb, gpointer data)
 {
 GSource *vus_gsrc;
@@ -247,7 +247,7 @@ struct scsi_task {
 
 /** libiscsi integration **/
 
-static int iscsi_add_lun(iscsi_lun_t *lun, char *iscsi_uri)
+static int iscsi_add_lun(VusIscsiLun *lun, char *iscsi_uri)
 {
 struct iscsi_url *iscsi_url;
 struct iscsi_context *iscsi_ctx;
@@ -417,11 +417,11 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 
 static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 {
-vhost_scsi_dev_t *vdev_scsi;
+VusDev *vdev_scsi;
 
 assert(vu_dev);
 
-vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
+vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
 if (buf) {
 PERR("vu_panic: %s", buf);
 }
@@ -432,14 +432,14 @@ static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 static void vus_add_watch_cb(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb cb,
  void *pvt)
 {
-vhost_scsi_dev_t *vdev_scsi;
+VusDev *vdev_scsi;
 guint id;
 
 assert(vu_dev);
 assert(fd >= 0);
 assert(cb);
 
-vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
+vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
 id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
  (gpointer)(uintptr_t)fd);
 if (id) {
@@ -454,13 +454,13 @@ static void vus_add_watch_cb(VuDev *vu_dev, int fd, int 
vu_evt, vu_watch_cb cb,
 
 static void vus_del_watch_cb(VuDev *vu_dev, int fd)
 {
-vhost_scsi_dev_t *vdev_scsi;
+VusDev *vdev_scsi;
 guint id;
 
 assert(vu_dev);
 assert(fd >= 0);
 
-vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
+vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
 id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
  (gpointer)(uintptr_t)fd);
 if (id) {
@@ -473,12 +473,12 @@ static void vus_del_watch_cb(VuDev *vu_dev, int fd)
 
 static void vus_proc_req(VuDev *vu_dev, int idx)
 {
-vhost_scsi_dev_t *vdev_scsi;
+VusDev *vdev_scsi;
 VuVirtq *vq;
 
 assert(vu_dev);
 
-vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
+vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
 if (idx < 0 || idx >= VHOST_MAX_NR_VIRTQUEUE) {
 PERR("VQ Index out of range: %d", idx);
 vus_panic_cb(vu_dev, NULL);
@@ -618,7 +618,7 @@ fail:
 
 /** vhost-user-scsi **/
 
-static void vdev_scsi_free(vhost_scsi_dev_t *vdev_scsi)
+static void vdev_scsi_free(VusDev *vdev_scsi)
 {
 if (vdev_scsi->server_sock >= 0) {
 close(vdev_scsi->server_sock);
@@ -628,11 +628,11 @@ static void vdev_scsi_free(vhost_scsi_dev_t *vdev_scsi)
 g_free(vdev_scsi);
 }
 
-static vhost_scsi_dev_t *vdev_scsi_new(int server_sock)
+static VusDev *vdev_scsi_new(int server_sock)
 {
-vhost_scsi_dev_t *vdev_scsi;
+VusDev *vdev_scsi;
 
-vdev_scsi = g_new0(vhost_scsi_dev_t, 1);
+vdev_scsi = g_new0(VusDev, 1);
 vdev_scsi->server_sock = server_sock;

[Qemu-devel] [PULL v2 27/27] vhost-user-scsi: remove server_sock from VusDev

[Marc-André Lureau]

It is unneeded in the VusDev device structure, and also simplify a bit
the code.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 77 ++-
 1 file changed, 24 insertions(+), 53 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 615e2a76bb..54c1191db0 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -28,7 +28,6 @@ typedef struct VusIscsiLun {
 typedef struct VusDev {
 VugDev parent;
 
-int server_sock;
 VusIscsiLun lun;
 GMainLoop *loop;
 } VusDev;
@@ -357,57 +356,12 @@ fail:
 
 /** vhost-user-scsi **/
 
-static void vdev_scsi_free(VusDev *vdev_scsi)
-{
-if (vdev_scsi->server_sock >= 0) {
-close(vdev_scsi->server_sock);
-}
-g_main_loop_unref(vdev_scsi->loop);
-g_free(vdev_scsi);
-}
-
-static VusDev *vdev_scsi_new(int server_sock)
-{
-VusDev *vdev_scsi;
-
-vdev_scsi = g_new0(VusDev, 1);
-vdev_scsi->server_sock = server_sock;
-vdev_scsi->loop = g_main_loop_new(NULL, FALSE);
-
-return vdev_scsi;
-}
-
-static int vdev_scsi_run(VusDev *vdev_scsi)
-{
-int cli_sock;
-
-assert(vdev_scsi);
-assert(vdev_scsi->server_sock >= 0);
-
-cli_sock = accept(vdev_scsi->server_sock, NULL, NULL);
-if (cli_sock < 0) {
-perror("accept");
-return -1;
-}
-
-vug_init(_scsi->parent,
- cli_sock,
- vus_panic_cb,
- _iface);
-
-g_main_loop_run(vdev_scsi->loop);
-
-vug_deinit(_scsi->parent);
-
-return 0;
-}
-
 int main(int argc, char **argv)
 {
 VusDev *vdev_scsi = NULL;
 char *unix_fn = NULL;
 char *iscsi_uri = NULL;
-int sock, opt, err = EXIT_SUCCESS;
+int lsock = -1, csock = -1, opt, err = EXIT_SUCCESS;
 
 while ((opt = getopt(argc, argv, "u:i:")) != -1) {
 switch (opt) {
@@ -427,25 +381,42 @@ int main(int argc, char **argv)
 goto help;
 }
 
-sock = unix_sock_new(unix_fn);
-if (sock < 0) {
+lsock = unix_sock_new(unix_fn);
+if (lsock < 0) {
 goto err;
 }
-vdev_scsi = vdev_scsi_new(sock);
 
-if (vus_iscsi_add_lun(_scsi->lun, iscsi_uri) != 0) {
+csock = accept(lsock, NULL, NULL);
+if (csock < 0) {
+perror("accept");
 goto err;
 }
 
-if (vdev_scsi_run(vdev_scsi) != 0) {
+vdev_scsi = g_new0(VusDev, 1);
+vdev_scsi->loop = g_main_loop_new(NULL, FALSE);
+
+if (vus_iscsi_add_lun(_scsi->lun, iscsi_uri) != 0) {
 goto err;
 }
 
+vug_init(_scsi->parent, csock, vus_panic_cb, _iface);
+
+g_main_loop_run(vdev_scsi->loop);
+
+vug_deinit(_scsi->parent);
+
 out:
 if (vdev_scsi) {
-vdev_scsi_free(vdev_scsi);
+g_main_loop_unref(vdev_scsi->loop);
+g_free(vdev_scsi);
 unlink(unix_fn);
 }
+if (csock >= 0) {
+close(csock);
+}
+if (lsock >= 0) {
+close(lsock);
+}
 g_free(unix_fn);
 g_free(iscsi_uri);
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 23/27] vhost-user-scsi: simplify source handling

[Marc-André Lureau]

Using a hashtable.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 45 +--
 1 file changed, 12 insertions(+), 33 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 0e6784187c..2e1100dea3 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -65,7 +65,7 @@ typedef struct VusDev {
 VuDev vu_dev;
 int server_sock;
 GMainLoop *loop;
-GTree *fdmap;   /* fd -> gsource context id */
+GHashTable *fdmap;   /* fd -> gsource */
 VusIscsiLun lun;
 } VusDev;
 
@@ -83,11 +83,6 @@ typedef struct vus_gsrc {
 GPollFD gfd;
 } vus_gsrc_t;
 
-static gint vus_fdmap_compare(gconstpointer a, gconstpointer b)
-{
-return (b > a) - (b < a);
-}
-
 static gboolean vus_gsrc_prepare(GSource *src, gint *timeout)
 {
 assert(timeout);
@@ -128,8 +123,8 @@ static GSourceFuncs vus_gsrc_funcs = {
 NULL
 };
 
-static void vus_gsrc_new(VusDev *vdev_scsi, int fd, GIOCondition cond,
- vu_watch_cb vu_cb, GSourceFunc gsrc_cb, gpointer data)
+static GSource *vus_gsrc_new(VusDev *vdev_scsi, int fd, GIOCondition cond,
+ vu_watch_cb vu_cb, GSourceFunc gsrc_cb, gpointer 
data)
 {
 GSource *vus_gsrc;
 vus_gsrc_t *vus_src;
@@ -143,7 +138,6 @@ static void vus_gsrc_new(VusDev *vdev_scsi, int fd, 
GIOCondition cond,
 vus_gsrc = g_source_new(_gsrc_funcs, sizeof(vus_gsrc_t));
 g_source_set_callback(vus_gsrc, (GSourceFunc) vu_cb, data, NULL);
 vus_src = (vus_gsrc_t *)vus_gsrc;
-
 vus_src->vdev_scsi = vdev_scsi;
 vus_src->gfd.fd = fd;
 vus_src->gfd.events = cond;
@@ -154,8 +148,7 @@ static void vus_gsrc_new(VusDev *vdev_scsi, int fd, 
GIOCondition cond,
 assert(id);
 g_source_unref(vus_gsrc);
 
-g_tree_insert(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd,
-(gpointer)(uintptr_t)id);
+return vus_gsrc;
 }
 
 /** libiscsi integration **/
@@ -348,43 +341,27 @@ static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 static void vus_add_watch_cb(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb cb,
  void *pvt)
 {
+GSource *src;
 VusDev *vdev_scsi;
-guint id;
 
 assert(vu_dev);
 assert(fd >= 0);
 assert(cb);
 
 vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
-id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
- (gpointer)(uintptr_t)fd);
-if (id) {
-GSource *vus_src = g_main_context_find_source_by_id(NULL, id);
-assert(vus_src);
-g_source_destroy(vus_src);
-(void)g_tree_remove(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd);
-}
-
-vus_gsrc_new(vdev_scsi, fd, vu_evt, cb, NULL, pvt);
+src = vus_gsrc_new(vdev_scsi, fd, vu_evt, cb, NULL, pvt);
+g_hash_table_replace(vdev_scsi->fdmap, GINT_TO_POINTER(fd), src);
 }
 
 static void vus_del_watch_cb(VuDev *vu_dev, int fd)
 {
 VusDev *vdev_scsi;
-guint id;
 
 assert(vu_dev);
 assert(fd >= 0);
 
 vdev_scsi = container_of(vu_dev, VusDev, vu_dev);
-id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
- (gpointer)(uintptr_t)fd);
-if (id) {
-GSource *vus_src = g_main_context_find_source_by_id(NULL, id);
-assert(vus_src);
-g_source_destroy(vus_src);
-(void)g_tree_remove(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd);
-}
+g_hash_table_remove(vdev_scsi->fdmap, GINT_TO_POINTER(fd));
 }
 
 static void vus_proc_req(VuDev *vu_dev, int idx)
@@ -540,7 +517,7 @@ static void vdev_scsi_free(VusDev *vdev_scsi)
 close(vdev_scsi->server_sock);
 }
 g_main_loop_unref(vdev_scsi->loop);
-g_tree_destroy(vdev_scsi->fdmap);
+g_hash_table_unref(vdev_scsi->fdmap);
 g_free(vdev_scsi);
 }
 
@@ -551,7 +528,9 @@ static VusDev *vdev_scsi_new(int server_sock)
 vdev_scsi = g_new0(VusDev, 1);
 vdev_scsi->server_sock = server_sock;
 vdev_scsi->loop = g_main_loop_new(NULL, FALSE);
-vdev_scsi->fdmap = g_tree_new(vus_fdmap_compare);
+vdev_scsi->fdmap =
+g_hash_table_new_full(NULL, NULL, NULL,
+  (GDestroyNotify) g_source_destroy);
 
 return vdev_scsi;
 }
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 16/27] vhost-user-scsi: remove vdev_scsi_add_iscsi_lun()

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 0573ae794e..38fe451619 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -657,20 +657,6 @@ static vhost_scsi_dev_t *vdev_scsi_new(int server_sock)
 return vdev_scsi;
 }
 
-static int vdev_scsi_add_iscsi_lun(vhost_scsi_dev_t *vdev_scsi,
-   char *iscsi_uri, uint32_t lun)
-{
-assert(vdev_scsi);
-assert(iscsi_uri);
-assert(lun < VUS_MAX_LUNS);
-
-if (iscsi_add_lun(_scsi->luns[lun], iscsi_uri) != 0) {
-return -1;
-}
-
-return 0;
-}
-
 static int vdev_scsi_run(vhost_scsi_dev_t *vdev_scsi)
 {
 int cli_sock;
@@ -734,7 +720,7 @@ int main(int argc, char **argv)
 }
 vdev_scsi = vdev_scsi_new(sock);
 
-if (vdev_scsi_add_iscsi_lun(vdev_scsi, iscsi_uri, 0) != 0) {
+if (iscsi_add_lun(_scsi->luns[0], iscsi_uri) != 0) {
 goto err;
 }
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 24/27] vhost-user-scsi: use glib logging

[Marc-André Lureau]

- PLOG is unused
- code is compiled out unless debug is enabled
- logging is too verbose
- you can pipe to ts to have timestamp if needed, or use structured
  logging with more recent glib

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 77 +--
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 2e1100dea3..ff817d6643 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -18,42 +18,6 @@
 
 #include 
 
-/* #define VUS_DEBUG 1 */
-
-/** Log helpers **/
-
-#define PPRE  \
-struct timespec ts;   \
-char   timebuf[64];   \
-struct tm tm; \
-(void)clock_gettime(CLOCK_REALTIME, ); \
-(void)strftime(timebuf, 64, "%Y%m%d %T", gmtime_r(_sec, ))
-
-#define PEXT(lvl, msg, ...) do {  \
-PPRE; \
-fprintf(stderr, "%s.%06ld " lvl ": %s:%s():%d: " msg "\n",\
-timebuf, ts.tv_nsec / 1000,   \
-__FILE__, __func__, __LINE__, ## __VA_ARGS__);\
-} while (0)
-
-#define PNOR(lvl, msg, ...) do {  \
-PPRE; \
-fprintf(stderr, "%s.%06ld " lvl ": " msg "\n",\
-timebuf, ts.tv_nsec / 1000, ## __VA_ARGS__);  \
-} while (0)
-
-#ifdef VUS_DEBUG
-#define PDBG(msg, ...) PEXT("DBG", msg, ## __VA_ARGS__)
-#define PERR(msg, ...) PEXT("ERR", msg, ## __VA_ARGS__)
-#define PLOG(msg, ...) PEXT("LOG", msg, ## __VA_ARGS__)
-#else
-#define PDBG(msg, ...) { }
-#define PERR(msg, ...) PNOR("ERR", msg, ## __VA_ARGS__)
-#define PLOG(msg, ...) PNOR("LOG", msg, ## __VA_ARGS__)
-#endif
-
-/** vhost-user-scsi specific definitions **/
-
 #define VUS_ISCSI_INITIATOR "iqn.2016-11.com.nutanix:vhost-user-scsi"
 
 typedef struct VusIscsiLun {
@@ -168,27 +132,28 @@ static int vus_iscsi_add_lun(VusIscsiLun *lun, char 
*iscsi_uri)
 
 iscsi_ctx = iscsi_create_context(VUS_ISCSI_INITIATOR);
 if (!iscsi_ctx) {
-PERR("Unable to create iSCSI context");
+g_warning("Unable to create iSCSI context");
 return -1;
 }
 
 iscsi_url = iscsi_parse_full_url(iscsi_ctx, iscsi_uri);
 if (!iscsi_url) {
-PERR("Unable to parse iSCSI URL: %s", iscsi_get_error(iscsi_ctx));
+g_warning("Unable to parse iSCSI URL: %s", iscsi_get_error(iscsi_ctx));
 goto fail;
 }
 
 iscsi_set_session_type(iscsi_ctx, ISCSI_SESSION_NORMAL);
 iscsi_set_header_digest(iscsi_ctx, ISCSI_HEADER_DIGEST_NONE_CRC32C);
 if (iscsi_full_connect_sync(iscsi_ctx, iscsi_url->portal, iscsi_url->lun)) 
{
-PERR("Unable to login to iSCSI portal: %s", 
iscsi_get_error(iscsi_ctx));
+g_warning("Unable to login to iSCSI portal: %s",
+  iscsi_get_error(iscsi_ctx));
 goto fail;
 }
 
 lun->iscsi_ctx = iscsi_ctx;
 lun->iscsi_lun = iscsi_url->lun;
 
-PDBG("Context %p created for lun 0: %s", iscsi_ctx, iscsi_uri);
+g_debug("Context %p created for lun 0: %s", iscsi_ctx, iscsi_uri);
 
 out:
 if (iscsi_url) {
@@ -230,7 +195,7 @@ static int get_cdb_len(uint8_t *cdb)
 case 4: return 16;
 case 5: return 12;
 }
-PERR("Unable to determine cdb len (0x%02hhX)", cdb[0] >> 5);
+g_warning("Unable to determine cdb len (0x%02hhX)", cdb[0] >> 5);
 return -1;
 }
 
@@ -252,7 +217,7 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 
 if (!(!req->lun[1] && req->lun[2] == 0x40 && !req->lun[3])) {
 /* Ignore anything different than target=0, lun=0 */
-PDBG("Ignoring unconnected lun (0x%hhX, 0x%hhX)",
+g_debug("Ignoring unconnected lun (0x%hhX, 0x%hhX)",
  req->lun[1], req->lun[3]);
 rsp->status = SCSI_STATUS_CHECK_CONDITION;
 memset(rsp->sense, 0, sizeof(rsp->sense));
@@ -295,10 +260,10 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 task->iovector_in.niov = in_len;
 }
 
-PDBG("Sending iscsi cmd (cdb_len=%d, dir=%d, task=%p)",
+g_debug("Sending iscsi cmd (cdb_len=%d, dir=%d, task=%p)",
  cdb_len, dir, task);
 if (!iscsi_scsi_command_sync(ctx, 0, task, NULL)) {
-PERR("Error serving SCSI command");
+g_warning("Error serving SCSI command");
 g_free(task);
 return -1;
 }
@@ -316,7 +281,7 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 
 g_free(task);
 
-PDBG("Filled in rsp: status=%hhX, 

[Qemu-devel] [PULL v2 22/27] vhost-user-scsi: drop extra callback pointer

[Marc-André Lureau]

Use the one from the source with casting, like any other glib source.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index cfac1ce1fe..0e6784187c 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -81,7 +81,6 @@ typedef struct vus_gsrc {
 GSource parent;
 VusDev *vdev_scsi;
 GPollFD gfd;
-vu_watch_cb vu_cb;
 } vus_gsrc_t;
 
 static gint vus_fdmap_compare(gconstpointer a, gconstpointer b)
@@ -112,18 +111,13 @@ static gboolean vus_gsrc_dispatch(GSource *src, 
GSourceFunc cb, gpointer data)
 vus_gsrc_t *vus_src = (vus_gsrc_t *)src;
 
 assert(vus_src);
-assert(!(vus_src->vu_cb && cb));
 
 vdev_scsi = vus_src->vdev_scsi;
 
 assert(vdev_scsi);
 
-if (cb) {
-return cb(data);
-}
-if (vus_src->vu_cb) {
-vus_src->vu_cb(_scsi->vu_dev, vus_src->gfd.revents, data);
-}
+((vu_watch_cb)cb)(_scsi->vu_dev, vus_src->gfd.revents, data);
+
 return G_SOURCE_CONTINUE;
 }
 
@@ -147,12 +141,12 @@ static void vus_gsrc_new(VusDev *vdev_scsi, int fd, 
GIOCondition cond,
 assert(!(vu_cb && gsrc_cb));
 
 vus_gsrc = g_source_new(_gsrc_funcs, sizeof(vus_gsrc_t));
+g_source_set_callback(vus_gsrc, (GSourceFunc) vu_cb, data, NULL);
 vus_src = (vus_gsrc_t *)vus_gsrc;
 
 vus_src->vdev_scsi = vdev_scsi;
 vus_src->gfd.fd = fd;
 vus_src->gfd.events = cond;
-vus_src->vu_cb = vu_cb;
 
 g_source_add_poll(vus_gsrc, _src->gfd);
 g_source_set_callback(vus_gsrc, gsrc_cb, data, NULL);
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 13/27] vhost-user-scsi: simplify unix path cleanup

[Marc-André Lureau]

Always remove the unix path when leaving the program (instead of when
freeing scsi_dev). Note that unix_sock_new() also unlink() exisiting
path before creating the socket.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 82624a0f17..a9a4066eeb 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -636,24 +636,9 @@ fail:
 
 static void vdev_scsi_free(vhost_scsi_dev_t *vdev_scsi)
 {
-if (!vdev_scsi) {
-return;
-}
-
 if (vdev_scsi->server_sock >= 0) {
-struct sockaddr_storage ss;
-socklen_t sslen = sizeof(ss);
-
-if (getsockname(vdev_scsi->server_sock, (struct sockaddr *),
-) == 0) {
-struct sockaddr_un *su = (struct sockaddr_un *)
-(void)unlink(su->sun_path);
-}
-
-(void)close(vdev_scsi->server_sock);
-vdev_scsi->server_sock = -1;
+close(vdev_scsi->server_sock);
 }
-
 g_main_loop_unref(vdev_scsi->loop);
 g_tree_destroy(vdev_scsi->fdmap);
 g_free(vdev_scsi);
@@ -762,7 +747,10 @@ int main(int argc, char **argv)
 }
 
 out:
-vdev_scsi_free(vdev_scsi);
+if (vdev_scsi) {
+vdev_scsi_free(vdev_scsi);
+unlink(unix_fn);
+}
 g_free(unix_fn);
 g_free(iscsi_uri);
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 20/27] vhost-user-scsi: avoid use of iscsi_ namespace

[Marc-André Lureau]

It is confusing and could easily conflict with future versions.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index fdd6014712..c7f9d82142 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -247,7 +247,7 @@ struct scsi_task {
 
 /** libiscsi integration **/
 
-static int iscsi_add_lun(VusIscsiLun *lun, char *iscsi_uri)
+static int vus_iscsi_add_lun(VusIscsiLun *lun, char *iscsi_uri)
 {
 struct iscsi_url *iscsi_url;
 struct iscsi_context *iscsi_ctx;
@@ -703,7 +703,7 @@ int main(int argc, char **argv)
 }
 vdev_scsi = vdev_scsi_new(sock);
 
-if (iscsi_add_lun(_scsi->lun, iscsi_uri) != 0) {
+if (vus_iscsi_add_lun(_scsi->lun, iscsi_uri) != 0) {
 goto err;
 }
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 09/27] vhost-user-scsi: use glib allocation

[Marc-André Lureau]

Use g_new/g_free instead of plain malloc. This simplify a bit memory
handling since glib will abort if it cannot allocate.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 35 ---
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index fe567452cd..cf2793ef02 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -312,12 +312,7 @@ static struct scsi_task *scsi_task_new(int cdb_len, 
uint8_t *cdb, int dir,
 assert(cdb_len > 0);
 assert(cdb);
 
-task = calloc(1, sizeof(struct scsi_task));
-if (!task) {
-PERR("Error allocating task: %s", strerror(errno));
-return NULL;
-}
-
+task = g_new0(struct scsi_task, 1);
 memcpy(task->cdb, cdb, cdb_len);
 task->cdb_size = cdb_len;
 task->xfer_dir = dir;
@@ -393,10 +388,6 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 }
 
 task = scsi_task_new(cdb_len, req->cdb, dir, len);
-if (!task) {
-PERR("Unable to create iscsi task");
-return -1;
-}
 
 if (dir == SCSI_XFER_TO_DEV) {
 task->iovector_out.iov = (struct scsi_iovec *)out;
@@ -410,7 +401,7 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
  cdb_len, dir, task);
 if (!iscsi_scsi_command_sync(ctx, 0, task, NULL)) {
 PERR("Error serving SCSI command");
-free(task);
+g_free(task);
 return -1;
 }
 
@@ -425,7 +416,7 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 memcpy(rsp->sense, >datain.data[2], rsp->sense_len);
 }
 
-free(task);
+g_free(task);
 
 PDBG("Filled in rsp: status=%hhX, resid=%u, response=%hhX, sense_len=%u",
  rsp->status, rsp->resid, rsp->response, rsp->sense_len);
@@ -692,7 +683,7 @@ static vhost_scsi_dev_t *vdev_scsi_find_by_vu(VuDev *vu_dev)
 return NULL;
 }
 
-static void vdev_scsi_deinit(vhost_scsi_dev_t *vdev_scsi)
+static void vdev_scsi_free(vhost_scsi_dev_t *vdev_scsi)
 {
 if (!vdev_scsi) {
 return;
@@ -716,18 +707,14 @@ static void vdev_scsi_deinit(vhost_scsi_dev_t *vdev_scsi)
 g_main_loop_unref(vdev_scsi->loop);
 vdev_scsi->loop = NULL;
 }
+g_free(vdev_scsi);
 }
 
 static vhost_scsi_dev_t *vdev_scsi_new(int server_sock)
 {
-vhost_scsi_dev_t *vdev_scsi = NULL;
-
-vdev_scsi = calloc(1, sizeof(vhost_scsi_dev_t));
-if (!vdev_scsi) {
-PERR("calloc: %s", strerror(errno));
-return NULL;
-}
+vhost_scsi_dev_t *vdev_scsi;
 
+vdev_scsi = g_new0(vhost_scsi_dev_t, 1);
 vdev_scsi->server_sock = server_sock;
 vdev_scsi->loop = g_main_loop_new(NULL, FALSE);
 if (!vdev_scsi->loop) {
@@ -744,8 +731,7 @@ static vhost_scsi_dev_t *vdev_scsi_new(int server_sock)
 return vdev_scsi;
 
 err:
-vdev_scsi_deinit(vdev_scsi);
-free(vdev_scsi);
+vdev_scsi_free(vdev_scsi);
 
 return NULL;
 }
@@ -852,10 +838,7 @@ int main(int argc, char **argv)
 }
 
 out:
-if (vdev_scsi) {
-vdev_scsi_deinit(vdev_scsi);
-free(vdev_scsi);
-}
+vdev_scsi_free(vdev_scsi);
 g_free(unix_fn);
 g_free(iscsi_uri);
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 15/27] vhost-user-scsi: assert() in iscsi_add_lun()

[Marc-André Lureau]

Instead of a preliminary check, add an assert to the function that has
the pre-condition.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index dd0de1fc89..0573ae794e 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -257,6 +257,7 @@ static int iscsi_add_lun(iscsi_lun_t *lun, char *iscsi_uri)
 
 assert(lun);
 assert(iscsi_uri);
+assert(!lun->iscsi_ctx);
 
 iscsi_ctx = iscsi_create_context(VUS_ISCSI_INITIATOR);
 if (!iscsi_ctx) {
@@ -663,11 +664,6 @@ static int vdev_scsi_add_iscsi_lun(vhost_scsi_dev_t 
*vdev_scsi,
 assert(iscsi_uri);
 assert(lun < VUS_MAX_LUNS);
 
-if (vdev_scsi->luns[lun].iscsi_ctx) {
-PERR("Lun %d already configured", lun);
-return -1;
-}
-
 if (iscsi_add_lun(_scsi->luns[lun], iscsi_uri) != 0) {
 return -1;
 }
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PATCH] cirrus: keep vga vram pointer within bounds

[P J P]

From: Prasad J Pandit 

'dst' pointer could exceed vga vram size when writing to the
cirrus memory area; it'd lead to an OOB access issue. Add check
to avoid it.

Reported-by: Niu Guoxiang 
Signed-off-by: Prasad J Pandit 
---
 hw/display/cirrus_vga.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index afc290ab91..451a736262 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2047,8 +2047,11 @@ static void 
cirrus_mem_writeb_mode4and5_8bpp(CirrusVGAState * s,
}
val <<= 1;
dst++;
+if (dst >= s->vga.vram_ptr + s->vga.vram_size) {
+break;
+}
 }
-memory_region_set_dirty(>vga.vram, offset, 8);
+memory_region_set_dirty(>vga.vram, offset, x);
 }
 
 static void cirrus_mem_writeb_mode4and5_16bpp(CirrusVGAState * s,
@@ -2071,8 +2074,11 @@ static void 
cirrus_mem_writeb_mode4and5_16bpp(CirrusVGAState * s,
}
val <<= 1;
dst += 2;
+if (dst >= s->vga.vram_ptr + s->vga.vram_size) {
+break;
+}
 }
-memory_region_set_dirty(>vga.vram, offset, 16);
+memory_region_set_dirty(>vga.vram, offset, x<<1);
 }
 
 /***
-- 
2.13.6

[Qemu-devel] [PULL v2 18/27] vhost-user-scsi: remove unimplemented functions

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 21 +++--
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 6b55cc71d3..26b17f348b 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -471,16 +471,6 @@ static void vus_del_watch_cb(VuDev *vu_dev, int fd)
 }
 }
 
-static void vus_proc_ctl(VuDev *vu_dev, int idx)
-{
-/* Control VQ not implemented */
-}
-
-static void vus_proc_evt(VuDev *vu_dev, int idx)
-{
-/* Event VQ not implemented */
-}
-
 static void vus_proc_req(VuDev *vu_dev, int idx)
 {
 vhost_scsi_dev_t *vdev_scsi;
@@ -561,14 +551,9 @@ static void vus_queue_set_started(VuDev *vu_dev, int idx, 
bool started)
 
 vq = vu_get_queue(vu_dev, idx);
 
-switch (idx) {
-case 0:
-vu_set_queue_handler(vu_dev, vq, started ? vus_proc_ctl : NULL);
-break;
-case 1:
-vu_set_queue_handler(vu_dev, vq, started ? vus_proc_evt : NULL);
-break;
-default:
+if (idx == 0 || idx == 1) {
+PDBG("queue %d unimplemented", idx);
+} else {
 vu_set_queue_handler(vu_dev, vq, started ? vus_proc_req : NULL);
 }
 }
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 14/27] vhost-user-scsi: use NULL pointer

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index a9a4066eeb..dd0de1fc89 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -684,7 +684,7 @@ static int vdev_scsi_run(vhost_scsi_dev_t *vdev_scsi)
 assert(vdev_scsi->server_sock >= 0);
 assert(vdev_scsi->loop);
 
-cli_sock = accept(vdev_scsi->server_sock, (void *)0, (void *)0);
+cli_sock = accept(vdev_scsi->server_sock, NULL, NULL);
 if (cli_sock < 0) {
 perror("accept");
 return -1;
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 10/27] vhost-user-scsi: glib calls that allocate don't return NULL

[Marc-André Lureau]

They abort instead, so get rid of failure conditions.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 52 +--
 1 file changed, 7 insertions(+), 45 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index cf2793ef02..e3ae2bbd72 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -139,8 +139,8 @@ static GSourceFuncs vus_gsrc_funcs = {
 NULL
 };
 
-static int vus_gsrc_new(vhost_scsi_dev_t *vdev_scsi, int fd, GIOCondition cond,
-vu_watch_cb vu_cb, GSourceFunc gsrc_cb, gpointer data)
+static void vus_gsrc_new(vhost_scsi_dev_t *vdev_scsi, int fd, GIOCondition 
cond,
+ vu_watch_cb vu_cb, GSourceFunc gsrc_cb, gpointer data)
 {
 GSource *vus_gsrc;
 vus_gsrc_t *vus_src;
@@ -152,10 +152,6 @@ static int vus_gsrc_new(vhost_scsi_dev_t *vdev_scsi, int 
fd, GIOCondition cond,
 assert(!(vu_cb && gsrc_cb));
 
 vus_gsrc = g_source_new(_gsrc_funcs, sizeof(vus_gsrc_t));
-if (!vus_gsrc) {
-PERR("Error creating GSource for new watch");
-return -1;
-}
 vus_src = (vus_gsrc_t *)vus_gsrc;
 
 vus_src->vdev_scsi = vdev_scsi;
@@ -171,8 +167,6 @@ static int vus_gsrc_new(vhost_scsi_dev_t *vdev_scsi, int 
fd, GIOCondition cond,
 
 g_tree_insert(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd,
 (gpointer)(uintptr_t)id);
-
-return 0;
 }
 
 /* from libiscsi's scsi-lowlevel.h **
@@ -440,10 +434,7 @@ static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 PERR("vu_panic: %s", buf);
 }
 
-if (vdev_scsi) {
-assert(vdev_scsi->loop);
-g_main_loop_quit(vdev_scsi->loop);
-}
+g_main_loop_quit(vdev_scsi->loop);
 }
 
 static void vus_add_watch_cb(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb cb,
@@ -471,9 +462,7 @@ static void vus_add_watch_cb(VuDev *vu_dev, int fd, int 
vu_evt, vu_watch_cb cb,
 (void)g_tree_remove(vdev_scsi->fdmap, (gpointer)(uintptr_t)fd);
 }
 
-if (vus_gsrc_new(vdev_scsi, fd, vu_evt, cb, NULL, pvt)) {
-vus_panic_cb(vu_dev, NULL);
-}
+vus_gsrc_new(vdev_scsi, fd, vu_evt, cb, NULL, pvt);
 }
 
 static void vus_del_watch_cb(VuDev *vu_dev, int fd)
@@ -703,10 +692,7 @@ static void vdev_scsi_free(vhost_scsi_dev_t *vdev_scsi)
 vdev_scsi->server_sock = -1;
 }
 
-if (vdev_scsi->loop) {
-g_main_loop_unref(vdev_scsi->loop);
-vdev_scsi->loop = NULL;
-}
+g_main_loop_unref(vdev_scsi->loop);
 g_free(vdev_scsi);
 }
 
@@ -717,23 +703,9 @@ static vhost_scsi_dev_t *vdev_scsi_new(int server_sock)
 vdev_scsi = g_new0(vhost_scsi_dev_t, 1);
 vdev_scsi->server_sock = server_sock;
 vdev_scsi->loop = g_main_loop_new(NULL, FALSE);
-if (!vdev_scsi->loop) {
-PERR("Error creating glib event loop");
-goto err;
-}
-
 vdev_scsi->fdmap = g_tree_new(vus_fdmap_compare);
-if (!vdev_scsi->fdmap) {
-PERR("Error creating glib tree for fdmap");
-goto err;
-}
 
 return vdev_scsi;
-
-err:
-vdev_scsi_free(vdev_scsi);
-
-return NULL;
 }
 
 static int vdev_scsi_add_iscsi_lun(vhost_scsi_dev_t *vdev_scsi,
@@ -777,21 +749,14 @@ static int vdev_scsi_run(vhost_scsi_dev_t *vdev_scsi)
 vus_del_watch_cb,
 _iface);
 
-if (vus_gsrc_new(vdev_scsi, cli_sock, G_IO_IN, NULL, vus_vhost_cb,
- _scsi->vu_dev)) {
-goto fail;
-}
+vus_gsrc_new(vdev_scsi, cli_sock, G_IO_IN, NULL, vus_vhost_cb,
+ _scsi->vu_dev);
 
 g_main_loop_run(vdev_scsi->loop);
 
-out:
 vu_deinit(_scsi->vu_dev);
 
 return ret;
-
-fail:
-ret = -1;
-goto out;
 }
 
 int main(int argc, char **argv)
@@ -824,9 +789,6 @@ int main(int argc, char **argv)
 goto err;
 }
 vdev_scsi = vdev_scsi_new(sock);
-if (!vdev_scsi) {
-goto err;
-}
 vhost_scsi_devs[0] = vdev_scsi;
 
 if (vdev_scsi_add_iscsi_lun(vdev_scsi, iscsi_uri, 0) != 0) {
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 17/27] vhost-user-scsi: remove VUS_MAX_LUNS

[Marc-André Lureau]

There is no code to support more than 1 yet, no need for that today.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 38fe451619..6b55cc71d3 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -53,8 +53,6 @@
 
 /** vhost-user-scsi specific definitions **/
 
- /* Only 1 LUN and device supported today */
-#define VUS_MAX_LUNS 1
 #define VUS_ISCSI_INITIATOR "iqn.2016-11.com.nutanix:vhost-user-scsi"
 
 typedef struct iscsi_lun {
@@ -67,7 +65,7 @@ typedef struct vhost_scsi_dev {
 int server_sock;
 GMainLoop *loop;
 GTree *fdmap;   /* fd -> gsource context id */
-iscsi_lun_t luns[VUS_MAX_LUNS];
+iscsi_lun_t lun;
 } vhost_scsi_dev_t;
 
 /** glib event loop integration for libvhost-user and misc callbacks **/
@@ -535,7 +533,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
 }
 rsp = (VirtIOSCSICmdResp *)elem->in_sg[0].iov_base;
 
-if (handle_cmd_sync(vdev_scsi->luns[0].iscsi_ctx,
+if (handle_cmd_sync(vdev_scsi->lun.iscsi_ctx,
 req, >out_sg[1], elem->out_num - 1,
 rsp, >in_sg[1], elem->in_num - 1) != 0) {
 vus_panic_cb(vu_dev, NULL);
@@ -720,7 +718,7 @@ int main(int argc, char **argv)
 }
 vdev_scsi = vdev_scsi_new(sock);
 
-if (iscsi_add_lun(_scsi->luns[0], iscsi_uri) != 0) {
+if (iscsi_add_lun(_scsi->lun, iscsi_uri) != 0) {
 goto err;
 }
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 08/27] vhost-user-scsi: code style fixes

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index e01bf31296..fe567452cd 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -305,7 +305,8 @@ fail:
 }
 
 static struct scsi_task *scsi_task_new(int cdb_len, uint8_t *cdb, int dir,
-   int xfer_len) {
+   int xfer_len)
+{
 struct scsi_task *task;
 
 assert(cdb_len > 0);
@@ -344,7 +345,8 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
VirtIOSCSICmdReq *req,
struct iovec *out, unsigned int out_len,
VirtIOSCSICmdResp *rsp,
-   struct iovec *in, unsigned int in_len) {
+   struct iovec *in, unsigned int in_len)
+{
 struct scsi_task *task;
 uint32_t dir;
 uint32_t len;
@@ -454,7 +456,8 @@ static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 }
 
 static void vus_add_watch_cb(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb cb,
- void *pvt) {
+ void *pvt)
+{
 vhost_scsi_dev_t *vdev_scsi;
 guint id;
 
@@ -529,7 +532,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
 return;
 }
 
-if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
+if (idx < 0 || idx >= VHOST_MAX_NR_VIRTQUEUE) {
 PERR("VQ Index out of range: %d", idx);
 vus_panic_cb(vu_dev, NULL);
 return;
@@ -556,8 +559,8 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
 }
 PDBG("Popped elem@%p", elem);
 
-assert(!((elem->out_num > 1) && (elem->in_num > 1)));
-assert((elem->out_num > 0) && (elem->in_num > 0));
+assert(!(elem->out_num > 1 && elem->in_num > 1));
+assert(elem->out_num > 0 && elem->in_num > 0);
 
 if (elem->out_sg[0].iov_len < sizeof(VirtIOSCSICmdReq)) {
 PERR("Invalid virtio-scsi req header");
@@ -593,7 +596,7 @@ static void vus_queue_set_started(VuDev *vu_dev, int idx, 
bool started)
 
 assert(vu_dev);
 
-if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
+if (idx < 0 || idx >= VHOST_MAX_NR_VIRTQUEUE) {
 PERR("VQ Index out of range: %d", idx);
 vus_panic_cb(vu_dev, NULL);
 return;
@@ -748,7 +751,8 @@ err:
 }
 
 static int vdev_scsi_add_iscsi_lun(vhost_scsi_dev_t *vdev_scsi,
-   char *iscsi_uri, uint32_t lun) {
+   char *iscsi_uri, uint32_t lun)
+{
 assert(vdev_scsi);
 assert(iscsi_uri);
 assert(lun < VUS_MAX_LUNS);
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 11/27] vhost-user-scsi: also free the gtree

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index e3ae2bbd72..17f676bf00 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -693,6 +693,7 @@ static void vdev_scsi_free(vhost_scsi_dev_t *vdev_scsi)
 }
 
 g_main_loop_unref(vdev_scsi->loop);
+g_tree_destroy(vdev_scsi->fdmap);
 g_free(vdev_scsi);
 }
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 06/27] vhost-user-scsi: use g_strdup()

[Marc-André Lureau]

Since vhost-user-scsi uses glib.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 78bcc65f5a..1fb57da2da 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -822,10 +822,10 @@ int main(int argc, char **argv)
 case 'h':
 goto help;
 case 'u':
-unix_fn = strdup(optarg);
+unix_fn = g_strdup(optarg);
 break;
 case 'i':
-iscsi_uri = strdup(optarg);
+iscsi_uri = g_strdup(optarg);
 break;
 default:
 goto help;
@@ -854,12 +854,8 @@ out:
 vdev_scsi_deinit(vdev_scsi);
 free(vdev_scsi);
 }
-if (unix_fn) {
-free(unix_fn);
-}
-if (iscsi_uri) {
-free(iscsi_uri);
-}
+g_free(unix_fn);
+g_free(iscsi_uri);
 
 return err;
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 07/27] vhost-user-scsi: connect unix socket before allocating

[Marc-André Lureau]

This simplify a little bit memory management in the following patches.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 1fb57da2da..e01bf31296 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -715,23 +715,17 @@ static void vdev_scsi_deinit(vhost_scsi_dev_t *vdev_scsi)
 }
 }
 
-static vhost_scsi_dev_t *vdev_scsi_new(char *unix_fn)
+static vhost_scsi_dev_t *vdev_scsi_new(int server_sock)
 {
 vhost_scsi_dev_t *vdev_scsi = NULL;
 
-assert(unix_fn);
-
 vdev_scsi = calloc(1, sizeof(vhost_scsi_dev_t));
 if (!vdev_scsi) {
 PERR("calloc: %s", strerror(errno));
 return NULL;
 }
 
-vdev_scsi->server_sock = unix_sock_new(unix_fn);
-if (vdev_scsi->server_sock < 0) {
-goto err;
-}
-
+vdev_scsi->server_sock = server_sock;
 vdev_scsi->loop = g_main_loop_new(NULL, FALSE);
 if (!vdev_scsi->loop) {
 PERR("Error creating glib event loop");
@@ -815,7 +809,7 @@ int main(int argc, char **argv)
 vhost_scsi_dev_t *vdev_scsi = NULL;
 char *unix_fn = NULL;
 char *iscsi_uri = NULL;
-int opt, err = EXIT_SUCCESS;
+int sock, opt, err = EXIT_SUCCESS;
 
 while ((opt = getopt(argc, argv, "u:i:")) != -1) {
 switch (opt) {
@@ -835,7 +829,11 @@ int main(int argc, char **argv)
 goto help;
 }
 
-vdev_scsi = vdev_scsi_new(unix_fn);
+sock = unix_sock_new(unix_fn);
+if (sock < 0) {
+goto err;
+}
+vdev_scsi = vdev_scsi_new(sock);
 if (!vdev_scsi) {
 goto err;
 }
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 05/27] libvhost-user: improve vu_queue_pop() doc

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/libvhost-user/libvhost-user.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index 4021f1124e..5825b66880 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -358,7 +358,8 @@ void vu_queue_notify(VuDev *dev, VuVirtq *vq);
  * @vq: a VuVirtq queue
  * @sz: the size of struct to return (must be >= VuVirtqElement)
  *
- * Returns: a VuVirtqElement filled from the queue or NULL.
+ * Returns: a VuVirtqElement filled from the queue or NULL. The
+ * returned element must be free()-d by the caller.
  */
 void *vu_queue_pop(VuDev *dev, VuVirtq *vq, size_t sz);
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 12/27] vhost-user-scsi: remove vdev_scsi_find_by_vu()

[Marc-André Lureau]

The *dev pointer belongs to the vhost_scsi_dev_t parent.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 47 +++
 1 file changed, 4 insertions(+), 43 deletions(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 17f676bf00..82624a0f17 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -55,8 +55,6 @@
 
  /* Only 1 LUN and device supported today */
 #define VUS_MAX_LUNS 1
-#define VUS_MAX_DEVS 1
-
 #define VUS_ISCSI_INITIATOR "iqn.2016-11.com.nutanix:vhost-user-scsi"
 
 typedef struct iscsi_lun {
@@ -72,8 +70,6 @@ typedef struct vhost_scsi_dev {
 iscsi_lun_t luns[VUS_MAX_LUNS];
 } vhost_scsi_dev_t;
 
-static vhost_scsi_dev_t *vhost_scsi_devs[VUS_MAX_DEVS];
-
 /** glib event loop integration for libvhost-user and misc callbacks **/
 
 QEMU_BUILD_BUG_ON((int)G_IO_IN != (int)VU_WATCH_IN);
@@ -420,16 +416,13 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 
 /** libvhost-user callbacks **/
 
-static vhost_scsi_dev_t *vdev_scsi_find_by_vu(VuDev *vu_dev);
-
 static void vus_panic_cb(VuDev *vu_dev, const char *buf)
 {
 vhost_scsi_dev_t *vdev_scsi;
 
 assert(vu_dev);
 
-vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
-
+vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
 if (buf) {
 PERR("vu_panic: %s", buf);
 }
@@ -447,12 +440,7 @@ static void vus_add_watch_cb(VuDev *vu_dev, int fd, int 
vu_evt, vu_watch_cb cb,
 assert(fd >= 0);
 assert(cb);
 
-vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
-if (!vdev_scsi) {
-vus_panic_cb(vu_dev, NULL);
-return;
-}
-
+vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
 id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
  (gpointer)(uintptr_t)fd);
 if (id) {
@@ -473,12 +461,7 @@ static void vus_del_watch_cb(VuDev *vu_dev, int fd)
 assert(vu_dev);
 assert(fd >= 0);
 
-vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
-if (!vdev_scsi) {
-vus_panic_cb(vu_dev, NULL);
-return;
-}
-
+vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
 id = (guint)(uintptr_t)g_tree_lookup(vdev_scsi->fdmap,
  (gpointer)(uintptr_t)fd);
 if (id) {
@@ -506,12 +489,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
 
 assert(vu_dev);
 
-vdev_scsi = vdev_scsi_find_by_vu(vu_dev);
-if (!vdev_scsi) {
-vus_panic_cb(vu_dev, NULL);
-return;
-}
-
+vdev_scsi = container_of(vu_dev, vhost_scsi_dev_t, vu_dev);
 if (idx < 0 || idx >= VHOST_MAX_NR_VIRTQUEUE) {
 PERR("VQ Index out of range: %d", idx);
 vus_panic_cb(vu_dev, NULL);
@@ -656,22 +634,6 @@ fail:
 
 /** vhost-user-scsi **/
 
-static vhost_scsi_dev_t *vdev_scsi_find_by_vu(VuDev *vu_dev)
-{
-int i;
-
-assert(vu_dev);
-
-for (i = 0; i < VUS_MAX_DEVS; i++) {
-if (_scsi_devs[i]->vu_dev == vu_dev) {
-return vhost_scsi_devs[i];
-}
-}
-
-PERR("Unknown VuDev %p", vu_dev);
-return NULL;
-}
-
 static void vdev_scsi_free(vhost_scsi_dev_t *vdev_scsi)
 {
 if (!vdev_scsi) {
@@ -790,7 +752,6 @@ int main(int argc, char **argv)
 goto err;
 }
 vdev_scsi = vdev_scsi_new(sock);
-vhost_scsi_devs[0] = vdev_scsi;
 
 if (vdev_scsi_add_iscsi_lun(vdev_scsi, iscsi_uri, 0) != 0) {
 goto err;
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 04/27] libvhost-user: drop dependency on glib

[Marc-André Lureau]

libvhost-user is meant to be free of glib dependency. Make sure it is
by droping qemu/osdep.h (which included glib.h)

This fixes a bad malloc()/g_free() pair.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 contrib/libvhost-user/libvhost-user.c | 29 ++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index d27d6303db..a0e0da4ccb 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -13,14 +13,35 @@
  * later.  See the COPYING file in the top-level directory.
  */
 
-#include 
+/* this code avoids GLib dependency */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
 #include 
+#include 
 #include 
 
+#include "qemu/compiler.h"
 #include "qemu/atomic.h"
 
 #include "libvhost-user.h"
 
+/* usually provided by GLib */
+#ifndef MIN
+#define MIN(x, y) ({\
+typeof(x) _min1 = (x);  \
+typeof(y) _min2 = (y);  \
+(void) (&_min1 == &_min2);  \
+_min1 < _min2 ? _min1 : _min2; })
+#endif
+
 #define VHOST_USER_HDR_SIZE offsetof(VhostUserMsg, payload.u64)
 
 /* The version of the protocol we support */
@@ -81,7 +102,9 @@ vu_panic(VuDev *dev, const char *msg, ...)
 va_list ap;
 
 va_start(ap, msg);
-buf = g_strdup_vprintf(msg, ap);
+if (vasprintf(, msg, ap) < 0) {
+buf = NULL;
+}
 va_end(ap);
 
 dev->broken = true;
@@ -853,7 +876,7 @@ vu_dispatch(VuDev *dev)
 success = true;
 
 end:
-g_free(vmsg.data);
+free(vmsg.data);
 return success;
 }
 
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 03/27] build-sys: make vhost-user-scsi depend on libvhost-user.a

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Paolo Bonzini 
---
 Makefile  | 2 +-
 Makefile.objs | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 64d3a9e244..97b4508d7d 100644
--- a/Makefile
+++ b/Makefile
@@ -480,7 +480,7 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) 
$(COMMON_LDADDS)
 ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS)
$(call LINK, $^)
 endif
-vhost-user-scsi$(EXESUF): $(vhost-user-scsi-obj-y)
+vhost-user-scsi$(EXESUF): $(vhost-user-scsi-obj-y) libvhost-user.a
$(call LINK, $^)
 
 module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
diff --git a/Makefile.objs b/Makefile.objs
index bdfa3b6177..d4f973a8fc 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -115,7 +115,6 @@ libvhost-user-obj-y = contrib/libvhost-user/
 vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS)
 vhost-user-scsi.o-libs := $(LIBISCSI_LIBS)
 vhost-user-scsi-obj-y = contrib/vhost-user-scsi/
-vhost-user-scsi-obj-y += contrib/libvhost-user/libvhost-user.o
 
 ##
 trace-events-subdirs =
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 02/27] build-sys: fix libvhost-user.a build

[Marc-André Lureau]

And actually link to it from vhost-user-bridge.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 Makefile   | 1 +
 tests/Makefile.include | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index cee6e28659..64d3a9e244 100644
--- a/Makefile
+++ b/Makefile
@@ -357,6 +357,7 @@ Makefile: $(version-obj-y)
 # Build libraries
 
 libqemuutil.a: $(util-obj-y) $(trace-obj-y) $(stub-obj-y)
+libvhost-user.a: $(libvhost-user-obj-y)
 
 ##
 
diff --git a/tests/Makefile.include b/tests/Makefile.include
index abc6707ef2..4ca15e6817 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -795,7 +795,7 @@ tests/test-filter-redirector$(EXESUF): 
tests/test-filter-redirector.o $(qtest-ob
 tests/test-x86-cpuid-compat$(EXESUF): tests/test-x86-cpuid-compat.o 
$(qtest-obj-y)
 tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o 
contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y) $(libqos-spapr-obj-y)
 tests/megasas-test$(EXESUF): tests/megasas-test.o $(libqos-spapr-obj-y) 
$(libqos-pc-obj-y)
-tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o 
contrib/libvhost-user/libvhost-user.o $(test-util-obj-y)
+tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o $(test-util-obj-y) 
libvhost-user.a
 tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
 tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
 tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 00/27] vhost-user patches

[Marc-André Lureau]

The following changes since commit 567d0a19c7998fa366598b83d5a6e5f0759d3ea9:

  Merge remote-tracking branch 
'remotes/ehabkost/tags/x86-and-machine-pull-request' into staging (2017-10-10 
13:25:46 +0100)

are available in the Git repository at:

  https://github.com/elmarco/qemu.git tags/vus-pull-request

for you to fetch changes up to 53a2e1b523e9015423583e431f4229a0ad35e6cd:

  vhost-user-scsi: remove server_sock from VusDev (2017-10-10 23:31:09 +0200)





Marc-André Lureau (27):
  glib-compat: move G_SOURCE_CONTINUE/REMOVE there
  build-sys: fix libvhost-user.a build
  build-sys: make vhost-user-scsi depend on libvhost-user.a
  libvhost-user: drop dependency on glib
  libvhost-user: improve vu_queue_pop() doc
  vhost-user-scsi: use g_strdup()
  vhost-user-scsi: connect unix socket before allocating
  vhost-user-scsi: code style fixes
  vhost-user-scsi: use glib allocation
  vhost-user-scsi: glib calls that allocate don't return NULL
  vhost-user-scsi: also free the gtree
  vhost-user-scsi: remove vdev_scsi_find_by_vu()
  vhost-user-scsi: simplify unix path cleanup
  vhost-user-scsi: use NULL pointer
  vhost-user-scsi: assert() in iscsi_add_lun()
  vhost-user-scsi: remove vdev_scsi_add_iscsi_lun()
  vhost-user-scsi: remove VUS_MAX_LUNS
  vhost-user-scsi: remove unimplemented functions
  vhost-user-scsi: rename VUS types
  vhost-user-scsi: avoid use of iscsi_ namespace
  vhost-user-scsi: don't copy iscsi/scsi-lowlevel.h
  vhost-user-scsi: drop extra callback pointer
  vhost-user-scsi: simplify source handling
  vhost-user-scsi: use glib logging
  libvhost-user: add glib source helper
  vhost-user-scsi: use libvhost-user glib helper
  vhost-user-scsi: remove server_sock from VusDev

 contrib/libvhost-user/libvhost-user-glib.h |  32 ++
 contrib/libvhost-user/libvhost-user.h  |   3 +-
 include/glib-compat.h  |   7 +
 contrib/libvhost-user/libvhost-user-glib.c | 154 +++
 contrib/libvhost-user/libvhost-user.c  |  29 +-
 contrib/vhost-user-scsi/vhost-user-scsi.c  | 629 +
 Makefile   |   3 +-
 Makefile.objs  |   1 -
 contrib/libvhost-user/Makefile.objs|   2 +-
 tests/Makefile.include |   2 +-
 10 files changed, 315 insertions(+), 547 deletions(-)
 create mode 100644 contrib/libvhost-user/libvhost-user-glib.h
 create mode 100644 contrib/libvhost-user/libvhost-user-glib.c

-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PULL v2 01/27] glib-compat: move G_SOURCE_CONTINUE/REMOVE there

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Acked-by: Philippe Mathieu-Daudé 
---
 include/glib-compat.h | 7 +++
 contrib/vhost-user-scsi/vhost-user-scsi.c | 8 
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index e15aca2d40..c49cf87196 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -356,5 +356,12 @@ g_test_add_data_func_full(const char *path,
 }
 #endif
 
+/* Small compat shim from glib 2.32 */
+#ifndef G_SOURCE_CONTINUE
+#define G_SOURCE_CONTINUE TRUE
+#endif
+#ifndef G_SOURCE_REMOVE
+#define G_SOURCE_REMOVE FALSE
+#endif
 
 #endif
diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index b5ae02c96f..78bcc65f5a 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -17,14 +17,6 @@
 
 #include 
 
-/* Small compat shim from glib 2.32 */
-#ifndef G_SOURCE_CONTINUE
-#define G_SOURCE_CONTINUE TRUE
-#endif
-#ifndef G_SOURCE_REMOVE
-#define G_SOURCE_REMOVE FALSE
-#endif
-
 /* #define VUS_DEBUG 1 */
 
 /** Log helpers **/
-- 
2.15.0.rc0.40.gaefcc5f6f

Re: [Qemu-devel] [PATCH v2 42/47] hw/xen*: Replace fprintf(stderr, "*\n" with error_report()

[Anthony PERARD]

On Fri, Sep 29, 2017 at 05:17:12PM -0700, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.


> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 632a938dcc..42ada2ac05 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "hw/xen/xen_backend.h"
>  #include "qmp-commands.h"
>  #include "chardev/char.h"
> @@ -47,19 +48,19 @@ static int store_dev_info(int domid, Chardev *cs, const 
> char *string)
>  /* We now have everything we need to set the xenstore entry. */
>  xs = xs_open(0);
>  if (xs == NULL) {
> -fprintf(stderr, "Could not contact XenStore\n");
> +error_report("Could not contact XenStore");
>  goto out;
>  }
>  
>  path = xs_get_domain_path(xs, domid);
>  if (path == NULL) {
> -fprintf(stderr, "xs_get_domain_path() error\n");
> +error_report("xs_get_domain_path() error");
>  goto out;
>  }
>  newpath = realloc(path, (strlen(path) + strlen(string) +
>  strlen("/tty") + 1));
>  if (newpath == NULL) {
> -fprintf(stderr, "realloc error\n");
> +error_report("realloc error");
>  goto out;
>  }
>  path = newpath;
> @@ -96,13 +97,13 @@ static void xenstore_record_dm_state(struct xs_handle 
> *xs, const char *state)
>  char path[50];
>  
>  if (xs == NULL) {
> -fprintf(stderr, "xenstore connection not initialized\n");
> +error_report("xenstore connection not initialized");
>  exit(1);
>  }
>  
>  snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
>  if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
> -fprintf(stderr, "error recording dm state\n");
> +error_report("error recording dm state");
>  exit(1);
>  }
>  }

> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 375efa68f6..e86d380d02 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -73,7 +73,7 @@ void xen_pt_log(const PCIDevice *d, const char *f, ...)
>  
>  va_start(ap, f);
>  if (d) {
> -fprintf(stderr, "[%02x:%02x.%d] ", pci_bus_num(d->bus),
> +error_report("[%02x:%02x.%d] ", pci_bus_num(d->bus),

This is only part of a line of a comment, this is going to be more
complecated that just calling error_repport, right?

>  PCI_SLOT(d->devfn), PCI_FUNC(d->devfn));
>  }
>  vfprintf(stderr, f, ap);

The second part of the line is printed here.

> @@ -87,7 +87,7 @@ static int xen_pt_pci_config_access_check(PCIDevice *d, 
> uint32_t addr, int len)
>  /* check offset range */
>  if (addr > 0xFF) {
>  XEN_PT_ERR(d, "Failed to access register with offset exceeding 0xFF. 
> "
> -   "(addr: 0x%02x, len: %d)\n", addr, len);
> +   "(addr: 0x%02x, len: %d)", addr, len);

If xen_pt_log is changed, there would be many more of the XEN_PT_ERR
call to change. (as well as XEN_PT_LOG and XEN_PT_WARN.)

>  return -1;
>  }
>  
> diff --git a/hw/xenpv/xen_domainbuild.c b/hw/xenpv/xen_domainbuild.c
> index 027f76fad1..f5514ffec2 100644
> --- a/hw/xenpv/xen_domainbuild.c
> +++ b/hw/xenpv/xen_domainbuild.c
> @@ -25,22 +25,22 @@ static int xenstore_domain_mkdir(char *path)
>  int i;
>  
>  if (!xs_mkdir(xenstore, 0, path)) {
> -fprintf(stderr, "%s: xs_mkdir %s: failed\n", __func__, path);
> - return -1;
> +error_report("%s: xs_mkdir %s: failed", __func__, path);
> +return -1;
>  }
>  if (!xs_set_permissions(xenstore, 0, path, perms_ro, 2)) {
> -fprintf(stderr, "%s: xs_set_permissions failed\n", __func__);
> - return -1;
> +error_report("%s: xs_set_permissions failed", __func__);
> +return -1;
>  }
>  
>  for (i = 0; writable[i]; i++) {
>  snprintf(subpath, sizeof(subpath), "%s/%s", path, writable[i]);
>  if (!xs_mkdir(xenstore, 0, subpath)) {
> -fprintf(stderr, "%s: xs_mkdir %s: failed\n", __func__, subpath);
> +error_report("%s: xs_mkdir %s: failed", __func__, subpath);
>  return -1;
>  }
>  if (!xs_set_permissions(xenstore, 0, subpath, perms_rw, 2)) {
> -fprintf(stderr, "%s: xs_set_permissions failed\n", __func__);
> +error_report("%s: xs_set_permissions failed", __func__);
>  return -1;
>  }
>  }
> @@ -235,7 +235,7 @@ int xen_domain_build_pv(const char *kernel, const char 
> *ramdisk,
>  memcpy(uuid, _uuid, sizeof(uuid));
>  rc = xen_domain_create(xen_xc, ssidref, uuid, flags, _domid);
>  if (rc < 0) {
> -fprintf(stderr, "xen: xc_domain_create() failed\n");
> +error_report("xen: xc_domain_create() failed");
>  goto err;
>  }
>   

Re: [Qemu-devel] [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ross Lagerwall]

On 10/06/2017 02:19 PM, Paul Durrant wrote:

-Original Message-
From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
Ross Lagerwall
Sent: 06 October 2017 13:58
To: Ian Jackson ; qemu-devel@nongnu.org
Cc: Anthony Perard ; xen-
de...@lists.xenproject.org; Stefano Stabellini ;
Juergen Gross 
Subject: Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict
improvements

On 10/04/2017 05:18 PM, Ian Jackson wrote:

(Resending this because 1. I got the CC for xen-devel wrong; 2. I got
the subject wrong: there are actually 8 patches; 3. I mangled
Anthony's name in theheaders.  Sorry for the noise.)

I have been working on trying to get qemu, when running as a Xen
device model, to _actually_ not have power equivalent to root.

I think I have achieved this, with some limitations (which will be
discussed in my series against xen.git.

However, there are changes to qemu needed.  In particular

   * The -xen-domid-restrict option does not work properly right now.
 It only restricts a small subset of the descriptors qemu has open.
 I am introducing a new library call in the Xen libraries for this,
 xentoolcore_restrict_all.



Hi Ian,

I'm testing your QEMU and Xen patch series and found that after being
restricted, QEMU fails to setup up the VGA memory properly which causes
a complete stall with stdvga. With cirrus it mostly works although it
seems to have reduced performance.

I think it happens when the VM sets up the BAR some time after
xen_restrict() has been called. The failure comes from QEMU calling
xc_domain_add_to_physmap() which calls do_memory_op() and finally
xencall2(). But the underlying xencall fd has been replaced with /dev/null.


Oh, that's a PITA. This is because of the slightly hacky way that the VRAM is 
dealt with (it needing to be moved into the BAR of the PCI VGA device at the 
point where guest firmware programs it). Maybe we need a new dm_op for this?

If no one objects, I propose adding the following calls to 
libxendevicemodel (with underlying Xen implementations, of course) that 
would be usable after the xendevicemodel handle has been restricted.


xendevicemodel_add_to_physmap(xendevicemodel_handle *dmod,
  domid_t domid,
  unsigned long idx,
  xen_pfn_t gpfn);

This is equivalent to xc_domain_add_to_physmap() with
space==XENMAPSPACE_gmfn.

xendevicemodel_pin_memory_cacheattr(xendevicemodel_handle *dmod,
domid_t domid,
uint64_t start,
uint64_t end,
uint32_t type);

This is equivalent to xc_domain_pin_memory_cacheattr().

--
Ross Lagerwall

Re: [Qemu-devel] [PATCH 27/42] tpm: remove unused opened code

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Signed-off-by: Marc-André Lureau 
---
  include/sysemu/tpm_backend.h | 12 
  backends/tpm.c   | 42 --
  tpm.c|  6 --
  3 files changed, 60 deletions(-)

diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index b12ae5b625..a893e586ae 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -81,8 +81,6 @@ struct TPMBackendClass {
  
  TpmTypeOptions *(*get_tpm_options)(TPMBackend *t);
  
-void (*opened)(TPMBackend *s, Error **errp);

-
  void (*handle_request)(TPMBackend *s, TPMBackendCmd *cmd);
  };
  
@@ -172,16 +170,6 @@ bool tpm_backend_get_tpm_established_flag(TPMBackend *s);

   */
  int tpm_backend_reset_tpm_established_flag(TPMBackend *s, uint8_t locty);
  
-/**

- * tpm_backend_open:
- * @s: the backend to open
- * @errp: a pointer to return the #Error object if an error occurs.
- *
- * This function will open the backend if it is not already open.  Calling this
- * function on an already opened backend will not result in an error.
- */
-void tpm_backend_open(TPMBackend *s, Error **errp);
-
  /**
   * tpm_backend_get_tpm_version:
   * @s: the backend to call into
diff --git a/backends/tpm.c b/backends/tpm.c
index 0c48d18775..7e636fbc7a 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -159,52 +159,10 @@ TPMInfo *tpm_backend_query_tpm(TPMBackend *s)
  return info;
  }
  
-static bool tpm_backend_prop_get_opened(Object *obj, Error **errp)

-{
-TPMBackend *s = TPM_BACKEND(obj);
-
-return s->opened;
-}
-
-void tpm_backend_open(TPMBackend *s, Error **errp)
-{
-object_property_set_bool(OBJECT(s), true, "opened", errp);
-}
-
-static void tpm_backend_prop_set_opened(Object *obj, bool value, Error **errp)
-{
-TPMBackend *s = TPM_BACKEND(obj);
-TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
-Error *local_err = NULL;
-
-if (value == s->opened) {
-return;
-}
-
-if (!value && s->opened) {
-error_setg(errp, QERR_PERMISSION_DENIED);
-return;
-}
-
-if (k->opened) {
-k->opened(s, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-}
-
-s->opened = true;
-}
-
  static void tpm_backend_instance_init(Object *obj)
  {
  TPMBackend *s = TPM_BACKEND(obj);
  
-object_property_add_bool(obj, "opened",

- tpm_backend_prop_get_opened,
- tpm_backend_prop_set_opened,
- NULL);
  s->bh = qemu_bh_new(tpm_backend_request_completed_bh, s);
  }
  
diff --git a/tpm.c b/tpm.c

index ce1543fcb4..a46ee5f144 100644
--- a/tpm.c
+++ b/tpm.c
@@ -134,12 +134,6 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
Error **errp)
  return 1;
  }
  
-tpm_backend_open(drv, _err);

-if (local_err) {
-error_report_err(local_err);
-return 1;
-}
-
  QLIST_INSERT_HEAD(_backends, drv, list);
  
  return 0;


Since nothing is setting the "opened' anymore, would anyone notice 
because this has changed or is something else doing that now? Otherwise 
I don't mind removing it...


 Stefan

Re: [Qemu-devel] [PATCH 37/42] tpm: lookup the the TPM interface instead of TIS device

[Stefan Berger]

On 10/10/2017 04:21 PM, Eduardo Habkost wrote:

On Tue, Oct 10, 2017 at 12:56:18AM +0200, Marc-André Lureau wrote:
[...]

-static inline TPMVersion tpm_get_version(void)
+static inline TPMIf *tpm_find(void)
  {
-#ifdef CONFIG_TPM
-Object *obj = object_resolve_path_type("", TYPE_TPM_TIS, NULL);
+Object *obj = object_resolve_path_type("", TYPE_TPM_IF, NULL);

Considering that tpm_crb_realizefn() will rely on tpm_find()
returning NULL if there are multiple TPM devices, I suggest
adding a "returns NULL unless there is exactly one TPM device"
comment, just like fw_cfg_find() and find_vmgenid_dev()


I wonder whether the function couldn't have a better name. 
tpm_find_single() ?


   Stefan




+
+return TPM_IF(obj);
+}

[...]

Re: [Qemu-devel] [PATCH 38/42] tpm: add TPM interface to lookup TPM version

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Do not hardcode TPM device model to lookup version, use an interface
instead.

Signed-off-by: Marc-André Lureau 
---
  include/sysemu/tpm.h | 5 ++---
  hw/tpm/tpm_tis.c | 5 +++--
  2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index dbd2b0cc7a..9439330cf1 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -38,6 +38,7 @@ typedef struct TPMIfClass {

  enum TpmModel model;
  void (*request_completed)(TPMIf *obj);
+enum TPMVersion (*get_version)(TPMIf *obj);
  } TPMIfClass;

  int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
@@ -53,15 +54,13 @@ static inline TPMIf *tpm_find(void)
  return TPM_IF(obj);
  }

-TPMVersion tpm_tis_get_tpm_version(Object *obj);
-
  static inline TPMVersion tpm_get_version(TPMIf *ti)
  {
  if (!ti) {
  return TPM_VERSION_UNSPEC;
  }

-return tpm_tis_get_tpm_version(OBJECT(ti));
+return TPM_IF_GET_CLASS(ti)->get_version(ti);
  }

  #endif /* QEMU_TPM_H */
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 954b7b0e5d..6aac9bfe6b 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -989,9 +989,9 @@ static void tpm_tis_realloc_buffer(TPMSizedBuffer *sb)
  /*
   * Get the TPMVersion of the backend device being used
   */
-TPMVersion tpm_tis_get_tpm_version(Object *obj)
+static enum TPMVersion tpm_tis_get_tpm_version(TPMIf *ti)
  {
-TPMState *s = TPM(obj);
+TPMState *s = TPM(ti);

  return tpm_backend_get_tpm_version(s->be_driver);
  }
@@ -1097,6 +1097,7 @@ static void tpm_tis_class_init(ObjectClass *klass, void 
*data)
  dc->reset = tpm_tis_reset;
  dc->vmsd  = _tpm_tis;
  tc->model = TPM_MODEL_TPM_TIS;
+tc->get_version = tpm_tis_get_tpm_version;
  tc->request_completed = tpm_tis_request_completed;
  }



Reviewed-by: Stefan Berger 

Re: [Qemu-devel] [PATCH 37/42] tpm: lookup the the TPM interface instead of TIS device

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

This will allow to introduce new devices implementing TPM.

Signed-off-by: Marc-André Lureau 
---
  hw/tpm/tpm_int.h | 19 ---
  include/sysemu/tpm.h | 52 ++--
  hw/i386/acpi-build.c |  2 +-
  3 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h
index 90e97b9170..86fadc16d3 100644
--- a/hw/tpm/tpm_int.h
+++ b/hw/tpm/tpm_int.h
@@ -15,25 +15,6 @@
  #include "qemu/osdep.h"
  #include "qom/object.h"

-#define TYPE_TPM_IF "tpm-if"
-#define TPM_IF_CLASS(klass) \
-OBJECT_CLASS_CHECK(TPMIfClass, (klass), TYPE_TPM_IF)
-#define TPM_IF_GET_CLASS(obj) \
-OBJECT_GET_CLASS(TPMIfClass, (obj), TYPE_TPM_IF)
-#define TPM_IF(obj) \
-INTERFACE_CHECK(TPMIf, (obj), TYPE_TPM_IF)
-
-typedef struct TPMIf {
-Object parent_obj;
-} TPMIf;
-
-typedef struct TPMIfClass {
-InterfaceClass parent_class;
-
-enum TpmModel model;
-void (*request_completed)(TPMIf *obj);
-} TPMIfClass;
-
  #define TPM_STANDARD_CMDLINE_OPTS   \
  { \
  .name = "type", \
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 62b073beeb..dbd2b0cc7a 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -12,32 +12,56 @@
  #ifndef QEMU_TPM_H
  #define QEMU_TPM_H

-#include "qemu/option.h"
+#include "qom/object.h"
+#include "qapi-types.h"

-int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
-int tpm_init(void);
-void tpm_cleanup(void);
-
-typedef enum  TPMVersion {
+typedef enum TPMVersion {
  TPM_VERSION_UNSPEC = 0,
  TPM_VERSION_1_2 = 1,
  TPM_VERSION_2_0 = 2,
  } TPMVersion;

-TPMVersion tpm_tis_get_tpm_version(Object *obj);
+#define TYPE_TPM_IF "tpm-if"
+#define TPM_IF_CLASS(klass) \
+OBJECT_CLASS_CHECK(TPMIfClass, (klass), TYPE_TPM_IF)
+#define TPM_IF_GET_CLASS(obj)   \
+OBJECT_GET_CLASS(TPMIfClass, (obj), TYPE_TPM_IF)
+#define TPM_IF(obj) \
+INTERFACE_CHECK(TPMIf, (obj), TYPE_TPM_IF)
+
+typedef struct TPMIf {
+Object parent_obj;
+} TPMIf;
+
+typedef struct TPMIfClass {
+InterfaceClass parent_class;
+
+enum TpmModel model;
+void (*request_completed)(TPMIf *obj);
+} TPMIfClass;
+
+int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
+int tpm_init(void);
+void tpm_cleanup(void);

  #define TYPE_TPM_TIS"tpm-tis"

-static inline TPMVersion tpm_get_version(void)
+static inline TPMIf *tpm_find(void)
  {
-#ifdef CONFIG_TPM
-Object *obj = object_resolve_path_type("", TYPE_TPM_TIS, NULL);
+Object *obj = object_resolve_path_type("", TYPE_TPM_IF, NULL);
+
+return TPM_IF(obj);
+}

-if (obj) {
-return tpm_tis_get_tpm_version(obj);
+TPMVersion tpm_tis_get_tpm_version(Object *obj);
+
+static inline TPMVersion tpm_get_version(TPMIf *ti)
+{
+if (!ti) {
+return TPM_VERSION_UNSPEC;
  }
-#endif
-return TPM_VERSION_UNSPEC;
+
+return tpm_tis_get_tpm_version(OBJECT(ti));
  }

  #endif /* QEMU_TPM_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2af37a9129..40371b6f75 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -208,7 +208,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
  }

  info->has_hpet = hpet_find();
-info->tpm_version = tpm_get_version();
+info->tpm_version = tpm_get_version(tpm_find());
  info->pvpanic_port = pvpanic_port();
  info->applesmc_io_base = applesmc_port();
  }



Reviewed-by: Stefan Berger 

Re: [Qemu-devel] [PATCH 35/42] tpm-tis: simplify header inclusion

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 



---
  hw/tpm/tpm_tis.c | 13 -
  1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 3c8d246ac8..e7e8b112e8 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -24,17 +24,12 @@

  #include "qemu/osdep.h"
  #include "hw/isa/isa.h"
-#include "sysemu/tpm_backend.h"
-#include "tpm_int.h"
-#include "sysemu/block-backend.h"
-#include "exec/address-spaces.h"
-#include "hw/hw.h"
-#include "hw/i386/pc.h"
-#include "hw/pci/pci_ids.h"
  #include "qapi/error.h"
-#include "qemu-common.h"
-#include "qemu/main-loop.h"
+
  #include "hw/acpi/tpm.h"
+#include "hw/pci/pci_ids.h"
+#include "sysemu/tpm_backend.h"
+#include "tpm_int.h"

  #define TPM_TIS_NUM_LOCALITIES  5 /* per spec */
  #define TPM_TIS_LOCALITY_SHIFT  12

Re: [Qemu-devel] [PATCH 36/42] tpm: rename qemu_find_tpm() -> qemu_find_tpm_be()

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

find_tpm() will be introduced to lookup the TPM device.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 



---
  include/sysemu/tpm_backend.h | 2 +-
  hw/tpm/tpm_tis.c | 2 +-
  tpm.c| 2 +-
  3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 881be97ee3..d02067e631 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -193,7 +193,7 @@ TPMVersion tpm_backend_get_tpm_version(TPMBackend *s);
   */
  TPMInfo *tpm_backend_query_tpm(TPMBackend *s);

-TPMBackend *qemu_find_tpm(const char *id);
+TPMBackend *qemu_find_tpm_be(const char *id);

  void tpm_register_model(enum TpmModel model);

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index e7e8b112e8..954b7b0e5d 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -1055,7 +1055,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error 
**errp)
  {
  TPMState *s = TPM(dev);

-s->be_driver = qemu_find_tpm(s->backend);
+s->be_driver = qemu_find_tpm_be(s->backend);
  if (!s->be_driver) {
  error_setg(errp, "tpm_tis: backend driver with id %s could not be "
 "found", s->backend);
diff --git a/tpm.c b/tpm.c
index 37298f3f03..4d6c45a4f2 100644
--- a/tpm.c
+++ b/tpm.c
@@ -71,7 +71,7 @@ static void tpm_display_backend_drivers(void)
  /*
   * Find the TPM with the given Id
   */
-TPMBackend *qemu_find_tpm(const char *id)
+TPMBackend *qemu_find_tpm_be(const char *id)
  {
  TPMBackend *drv;

Re: [Qemu-devel] [PATCH 34/42] tpm-passthrough: workaround a possible race

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

The TPM backend processing thread has common shared variable race
issues. (they should not be so easy to reach since guest interaction
with the device is slow compared to host emulation)

An obvious one is setting op_cancelled from device thread after
calling write(cancel_fd). The backend thread may return before the
device thread has set the variable. Instead set it before
cancellation. Even if the write() failed, the end result is command
get possibly cancelled (even if cancellation came from external
sources it doesn't matter much).

It's worth to consider removing the backend processing thread for now.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 



---
  hw/tpm/tpm_passthrough.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 0806cf86af..d71d64e8aa 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -89,6 +89,7 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState 
*tpm_pt,
  bool is_selftest;
  const struct tpm_resp_hdr *hdr;

+/* FIXME: protect shared variables or use other sync mechanism */
  tpm_pt->tpm_op_canceled = false;
  tpm_pt->tpm_executing = true;
  *selftest_done = false;
@@ -178,12 +179,11 @@ static void tpm_passthrough_cancel_cmd(TPMBackend *tb)
   */
  if (tpm_pt->tpm_executing) {
  if (tpm_pt->cancel_fd >= 0) {
+tpm_pt->tpm_op_canceled = true;
  n = write(tpm_pt->cancel_fd, "-", 1);
  if (n != 1) {
  error_report("Canceling TPM command failed: %s",
   strerror(errno));


Shouldn't we set it to false here ?



-} else {
-tpm_pt->tpm_op_canceled = true;
  }
  } else {
  error_report("Cannot cancel TPM command due to missing "

Re: [Qemu-devel] [PATCH 33/42] tpm-passthrough: remove error cleanup from handle_device_opts

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Clean-up is handled by the create() function.

Signed-off-by: Marc-André Lureau 
---
  hw/tpm/tpm_passthrough.c | 15 ++-
  1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index aa9167e3c6..0806cf86af 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -261,27 +261,16 @@ tpm_passthrough_handle_device_opts(TPMPassthruState 
*tpm_pt, QemuOpts *opts)
  if (tpm_pt->tpm_fd < 0) {
  error_report("Cannot access TPM device using '%s': %s",
   tpm_pt->tpm_dev, strerror(errno));
-goto err_free_parameters;
+return -1;
  }

  if (tpm_util_test_tpmdev(tpm_pt->tpm_fd, _pt->tpm_version)) {
  error_report("'%s' is not a TPM device.",
   tpm_pt->tpm_dev);
-goto err_close_tpmdev;
+return -1;
  }


I would prefer the cleanup to happen in the functions where the state is 
created...


   Stefan



  return 0;
-
- err_close_tpmdev:
-qemu_close(tpm_pt->tpm_fd);
-tpm_pt->tpm_fd = -1;
-
- err_free_parameters:
-qapi_free_TPMPassthroughOptions(tpm_pt->options);
-tpm_pt->options = NULL;
-tpm_pt->tpm_dev = NULL;
-
-return 1;
  }

  static TPMBackend *tpm_passthrough_create(QemuOpts *opts)

Re: [Qemu-devel] [PATCH 31/42] tpm-backend: move set 'id' to common code

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Signed-off-by: Marc-André Lureau 

Reviewed-by: Stefan Berger 


---
  include/sysemu/tpm_backend.h |  2 +-
  hw/tpm/tpm_emulator.c| 12 +++-
  hw/tpm/tpm_passthrough.c |  9 +++--
  tpm.c|  3 ++-
  4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 594bb50782..881be97ee3 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -64,7 +64,7 @@ struct TPMBackendClass {
  /* get a descriptive text of the backend to display to the user */
  const char *desc;

-TPMBackend *(*create)(QemuOpts *opts, const char *id);
+TPMBackend *(*create)(QemuOpts *opts);

  /* start up the TPM on the backend - optional */
  int (*startup_tpm)(TPMBackend *t);
diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 36454837b3..315819329b 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -453,22 +453,16 @@ err:
  return -1;
  }

-static TPMBackend *tpm_emulator_create(QemuOpts *opts, const char *id)
+static TPMBackend *tpm_emulator_create(QemuOpts *opts)
  {
  TPMBackend *tb = TPM_BACKEND(object_new(TYPE_TPM_EMULATOR));

-tb->id = g_strdup(id);
-
  if (tpm_emulator_handle_device_opts(TPM_EMULATOR(tb), opts)) {
-goto err_exit;
+object_unref(OBJECT(tb));
+return NULL;
  }

  return tb;
-
-err_exit:
-object_unref(OBJECT(tb));
-
-return NULL;
  }

  static TpmTypeOptions *tpm_emulator_get_tpm_options(TPMBackend *tb)
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 9326cbfdc9..7371d50739 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -284,13 +284,10 @@ tpm_passthrough_handle_device_opts(TPMPassthruState 
*tpm_pt, QemuOpts *opts)
  return 1;
  }

-static TPMBackend *tpm_passthrough_create(QemuOpts *opts, const char *id)
+static TPMBackend *tpm_passthrough_create(QemuOpts *opts)
  {
  Object *obj = object_new(TYPE_TPM_PASSTHROUGH);
-TPMBackend *tb = TPM_BACKEND(obj);
-TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
-
-tb->id = g_strdup(id);
+TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);

  if (tpm_passthrough_handle_device_opts(tpm_pt, opts)) {
  goto err_exit;
@@ -301,7 +298,7 @@ static TPMBackend *tpm_passthrough_create(QemuOpts *opts, 
const char *id)
  goto err_exit;
  }

-return tb;
+return TPM_BACKEND(obj);

  err_exit:
  object_unref(obj);
diff --git a/tpm.c b/tpm.c
index a46ee5f144..37298f3f03 100644
--- a/tpm.c
+++ b/tpm.c
@@ -129,11 +129,12 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
Error **errp)
  return 1;
  }

-drv = be->create(opts, id);
+drv = be->create(opts);
  if (!drv) {
  return 1;
  }

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

  return 0;

Re: [Qemu-devel] [PATCH 30/42] tpm-passthrough: pass TPMPassthruState to handle_device_opts

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

It doesn't need TPMBackend. Also reorder arguments for consistency.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 


---
  hw/tpm/tpm_passthrough.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 048edb1a1a..9326cbfdc9 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -239,9 +239,9 @@ static int 
tpm_passthrough_open_sysfs_cancel(TPMPassthruState *tpm_pt)
  return fd;
  }

-static int tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)
+static int
+tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt, QemuOpts *opts)
  {
-TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
  const char *value;

  value = qemu_opt_get(opts, "cancel-path");
@@ -292,7 +292,7 @@ static TPMBackend *tpm_passthrough_create(QemuOpts *opts, 
const char *id)

  tb->id = g_strdup(id);

-if (tpm_passthrough_handle_device_opts(opts, tb)) {
+if (tpm_passthrough_handle_device_opts(tpm_pt, opts)) {
  goto err_exit;
  }

Re: [Qemu-devel] [PATCH 29/42] tpm-be: update optional function pointers

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

QEMU code doesn't generally have assert() for mandatory
callbacks/function pointers, probably because the crash is pretty
obvious. Document the methods instead of going into the code.

Make get_tpm_options() mandatory to implement (since all
backend implementation have it).

Signed-off-by: Marc-André Lureau 

Reviewed-by: Stefan Berger 


---
  include/sysemu/tpm_backend.h | 5 -
  backends/tpm.c   | 9 +
  2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index a893e586ae..594bb50782 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -66,15 +66,18 @@ struct TPMBackendClass {

  TPMBackend *(*create)(QemuOpts *opts, const char *id);

-/* start up the TPM on the backend */
+/* start up the TPM on the backend - optional */
  int (*startup_tpm)(TPMBackend *t);

+/* optional */
  void (*reset)(TPMBackend *t);

  void (*cancel_cmd)(TPMBackend *t);

+/* optional */
  bool (*get_tpm_established_flag)(TPMBackend *t);

+/* optional */
  int (*reset_tpm_established_flag)(TPMBackend *t, uint8_t locty);

  TPMVersion (*get_tpm_version)(TPMBackend *t);
diff --git a/backends/tpm.c b/backends/tpm.c
index 7e636fbc7a..467c44 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -33,7 +33,6 @@ static void tpm_backend_worker_thread(gpointer data, gpointer 
user_data)
  TPMBackend *s = TPM_BACKEND(user_data);
  TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);

-assert(k->handle_request != NULL);
  k->handle_request(s, (TPMBackendCmd *)data);

  qemu_bh_schedule(s->bh);
@@ -114,8 +113,6 @@ void tpm_backend_cancel_cmd(TPMBackend *s)
  {
  TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);

-assert(k->cancel_cmd);
-
  k->cancel_cmd(s);
  }

@@ -139,8 +136,6 @@ TPMVersion tpm_backend_get_tpm_version(TPMBackend *s)
  {
  TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);

-assert(k->get_tpm_version);
-
  return k->get_tpm_version(s);
  }

@@ -152,9 +147,7 @@ TPMInfo *tpm_backend_query_tpm(TPMBackend *s)

  info->id = g_strdup(s->id);
  info->model = tic->model;
-if (k->get_tpm_options) {
-info->options = k->get_tpm_options(s);
-}
+info->options = k->get_tpm_options(s);

  return info;
  }

Re: [Qemu-devel] [RfC PATCH 6/6] vfio/display: add dmabuf support (v15)

[Alex Williamson]

On Tue, 10 Oct 2017 16:03:34 +0200
Gerd Hoffmann  wrote:

> Wire up dma-buf based display.
> 
> TODO: drop debug code and messages.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/vfio/pci.h |  12 
>  hw/vfio/display.c | 183 
> +-
>  2 files changed, 193 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index c03c4b3eb0..1ab6f6abde 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -19,6 +19,7 @@
>  #include "qemu/event_notifier.h"
>  #include "qemu/queue.h"
>  #include "qemu/timer.h"
> +#include "ui/console.h"
>  
>  #define PCI_ANY_ID (~0)
>  
> @@ -98,6 +99,14 @@ typedef struct VFIOMSIXInfo {
>  unsigned long *pending;
>  } VFIOMSIXInfo;
>  
> +typedef struct VFIODMABuf VFIODMABuf;
> +struct VFIODMABuf {
> +QemuDmaBuf  buf;
> +uint32_t pos_x, pos_y;
> +int dmabuf_id;
> +QTAILQ_ENTRY(VFIODMABuf) next;
> +};

Not really pci specific, maybe common.h?

> +
>  typedef struct VFIOPCIDevice {
>  PCIDevice pdev;
>  VFIODevice vbasedev;
> @@ -152,6 +161,9 @@ typedef struct VFIOPCIDevice {
>  uint32_t region_size;
>  void *region_mmap;
>  DisplaySurface *region_surface;
> +QTAILQ_HEAD(, VFIODMABuf) dmabufs;
> +VFIODMABuf *primary;
> +VFIODMABuf *cursor;

All of these part of a VFIODisplay struct, dynamically allocated?

>  } VFIOPCIDevice;
>  
>  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 8211bcc6d4..f45fd1fb98 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -18,6 +18,186 @@
>  #include "ui/console.h"
>  #include "pci.h"
>  
> +/* FIXME */
> +#ifndef DRM_PLANE_TYPE_PRIMARY
> +# define DRM_PLANE_TYPE_PRIMARY 1
> +# define DRM_PLANE_TYPE_CURSOR  2
> +#endif
> +
> +static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> +   uint32_t plane_type)
> +{
> +struct vfio_device_gfx_plane_info plane;
> +struct vfio_device_gfx_dmabuf_fd gfd;
> +VFIODMABuf *dmabuf;
> +static int errcnt;
> +int ret;
> +
> +memset(, 0, sizeof(plane));
> +plane.argsz = sizeof(plane);
> +plane.flags = VFIO_GFX_PLANE_TYPE_DMABUF;
> +plane.drm_plane_type = plane_type;
> +ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, );
> +if (ret < 0) {
> +fprintf(stderr, "(%d) ioctl VFIO_DEVICE_QUERY_GFX_PLANE(%s): %s\r",
> +++errcnt,
> +(plane_type == DRM_PLANE_TYPE_PRIMARY) ? "primary" : 
> "cursor",
> +strerror(errno));
> +fflush(stderr);
> +return NULL;
> +}
> +if (!plane.drm_format || !plane.size) {
> +fprintf(stderr, "(%d) %s plane not initialized by guest\r",
> +++errcnt,
> +(plane_type == DRM_PLANE_TYPE_PRIMARY) ? "primary" : 
> "cursor");
> +fflush(stderr);
> +return NULL;
> +}

Assuming stderr is part of the to-be-removed debugging...

Looks pretty straight forward otherwise.  I like the LRU dmabuf
freeing, but is it mostly for validating the interface?  My impression
is that we'd reach a steady state with a single plane and single
cursor so I wonder if keeping that cache really provides a noticeable
benefit.  Perhaps for a Linux guest switching between vt and graphics
mode?  It also seems like the primary and cursor will be fighting for
the head of the list, so if we keep lists, maybe they should be per
plane type.

This uses the x-display option for now, how do we move past that to get
automatic configuration?  Thanks,

Alex

> +
> +QTAILQ_FOREACH(dmabuf, >dmabufs, next) {
> +if (dmabuf->dmabuf_id == plane.dmabuf_id) {
> +/* found in list, move to head, return it */
> +QTAILQ_REMOVE(>dmabufs, dmabuf, next);
> +QTAILQ_INSERT_HEAD(>dmabufs, dmabuf, next);
> +if (plane_type == DRM_PLANE_TYPE_CURSOR) {
> +dmabuf->pos_x  = plane.x_pos;
> +dmabuf->pos_y  = plane.y_pos;
> +}
> +#if 1
> +if (plane.width != dmabuf->buf.width ||
> +plane.height != dmabuf->buf.height) {
> +fprintf(stderr, "%s: cached dmabuf mismatch: id %d, "
> +"kernel %dx%d, cached %dx%d, plane %s\n",
> +__func__, plane.dmabuf_id,
> +plane.width, plane.height,
> +dmabuf->buf.width, dmabuf->buf.height,
> +(plane_type == DRM_PLANE_TYPE_PRIMARY)
> +? "primary" : "cursor");
> +abort();
> +}
> +#endif
> +return dmabuf;
> +}
> +}
> +
> +memset(, 0, sizeof(gfd));
> +gfd.argsz = sizeof(gfd);
> +gfd.dmabuf_id = plane.dmabuf_id;
> +ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_GFX_DMABUF, );
> +if (ret < 0) {

Re: [Qemu-devel] [RfC PATCH 5/6] vfio/display: adding region support

[Alex Williamson]

[cc +Kirti]

On Tue, 10 Oct 2017 16:03:33 +0200
Gerd Hoffmann  wrote:

> Wire up region-based display.  UNTESTED.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/vfio/pci.h |  6 
>  hw/vfio/display.c | 96 
> +--
>  2 files changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 3c1b0a33ab..c03c4b3eb0 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -146,6 +146,12 @@ typedef struct VFIOPCIDevice {
>  bool no_kvm_intx;
>  bool no_kvm_msi;
>  bool no_kvm_msix;
> +/* vgpu local display */
> +QemuConsole *display_con;
> +uint32_t region_index;
> +uint32_t region_size;
> +void *region_mmap;
> +DisplaySurface *region_surface;

A structure for these might be nice.

>  } VFIOPCIDevice;
>  
>  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 064ec95f3e..8211bcc6d4 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -18,6 +18,99 @@
>  #include "ui/console.h"
>  #include "pci.h"
>  
> +/* -- */
> +
> +static void vfio_display_region_update(void *opaque)
> +{
> +VFIOPCIDevice *vdev = opaque;
> +struct vfio_device_gfx_plane_info plane;
> +struct vfio_region_info *region = NULL;
> +pixman_format_code_t format = PIXMAN_x8r8g8b8;
> +int ret;
> +
> +memset(, 0, sizeof(plane));
> +plane.argsz = sizeof(plane);
> +plane.flags = VFIO_GFX_PLANE_TYPE_REGION;

Let the compiler zero it?

struct vfio_device_gfx_plane_info plane = { .argsz = sizeof(plane), .flags = 
VFIO_GFX_PLANE_TYPE_REGION };

> +ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, );
> +if (ret < 0) {
> +fprintf(stderr, "ioctl VFIO_DEVICE_QUERY_GFX_PLANE: %s\n",
> +strerror(errno));

%m?

> +return;
> +}
> +if (!plane.drm_format || !plane.size) {
> +return;
> +}
> +format = qemu_drm_format_to_pixman(plane.drm_format);
> +
> +if (vdev->region_mmap && vdev->region_index != plane.region_index) {
> +/* region changed */
> +munmap(vdev->region_mmap, vdev->region_size);
> +vdev->region_mmap = NULL;
> +vdev->region_surface = NULL;
> +}
> +
> +if (vdev->region_surface &&
> +(surface_width(vdev->region_surface) != plane.width ||
> + surface_height(vdev->region_surface) != plane.height ||
> + surface_format(vdev->region_surface) != format)) {
> +/* size changed */
> +vdev->region_surface = NULL;
> +}
> +
> +if (vdev->region_mmap == NULL) {
> +/* mmap region */
> +ret = vfio_get_region_info(>vbasedev, plane.region_index,
> +   );
> +if (ret != 0) {
> +fprintf(stderr, "%s: vfio_get_region_info(%d): %s\n",
> +__func__, plane.region_index, strerror(-ret));
> +return;
> +}
> +vdev->region_size = region->size;
> +vdev->region_mmap = mmap(NULL, region->size,
> + PROT_READ, MAP_SHARED,
> + vdev->vbasedev.fd,
> + region->offset);
> +if (vdev->region_mmap == MAP_FAILED) {
> +fprintf(stderr, "%s: mmap region %d: %s\n", __func__,
> +plane.region_index, strerror(errno));
> +vdev->region_mmap = NULL;
> +g_free(region);
> +return;
> +}

Seems like we should really try to use a VFIORegion for this.

> +g_free(region);
> +}
> +
> +if (vdev->region_surface == NULL) {
> +/* create surface */
> +vdev->region_surface = qemu_create_displaysurface_from
> +(plane.width, plane.height, format,
> + plane.stride, vdev->region_mmap);
> +dpy_gfx_replace_surface(vdev->display_con, vdev->region_surface);
> +}
> +
> +/* full screen update */
> +dpy_gfx_update(vdev->display_con, 0, 0,
> +   surface_width(vdev->region_surface),
> +   surface_height(vdev->region_surface));
> +
> +}
> +
> +static const GraphicHwOps vfio_display_region_ops = {
> +.gfx_update = vfio_display_region_update,
> +};
> +
> +static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
> +{
> +vdev->display_con = graphic_console_init(DEVICE(vdev), 0,
> + _display_region_ops,
> + vdev);
> +/* TODO: disable hotplug (there is no graphic_console_close) */
> +return 0;
> +}
> +
> +/* -- */
> +
>  int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
>  {
>  struct vfio_device_gfx_plane_info probe;
> @@ -37,8 +130,7 @@ 

Re: [Qemu-devel] [PATCH 28/42] tpm-passthrough: don't save guessed cancel_path in options

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

The value is later unneeded, and may leak if the free visitor doesn't
consider it since has_cancel_path is false. And for consistency with
"path" it shouldn't be returned in get_tpm_options().

Signed-off-by: Marc-André Lureau 

Reviewed-by: Stefan Berger 



---
  hw/tpm/tpm_passthrough.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 8c002e4da6..048edb1a1a 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -226,9 +226,7 @@ static int 
tpm_passthrough_open_sysfs_cancel(TPMPassthruState *tpm_pt)
  if (snprintf(path, sizeof(path), "/sys/class/misc/%s/device/cancel",
   dev) < sizeof(path)) {
  fd = qemu_open(path, O_WRONLY);
-if (fd >= 0) {
-tpm_pt->options->cancel_path = g_strdup(path);
-} else {
+if (fd < 0) {
  error_report("tpm_passthrough: Could not open TPM cancel "
   "path %s : %s", path, strerror(errno));
  }

Re: [Qemu-devel] [PATCH 26/42] tpm-be: ask model to the TPM interface

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

No need to store the mode in the backend, or to let the frontend set
it itself.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 


---
  hw/tpm/tpm_int.h | 1 +
  include/sysemu/tpm_backend.h | 1 -
  backends/tpm.c   | 4 ++--
  hw/tpm/tpm_tis.c | 3 +--
  tpm.c| 3 ++-
  5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h
index 9c49325f03..90e97b9170 100644
--- a/hw/tpm/tpm_int.h
+++ b/hw/tpm/tpm_int.h
@@ -30,6 +30,7 @@ typedef struct TPMIf {
  typedef struct TPMIfClass {
  InterfaceClass parent_class;

+enum TpmModel model;
  void (*request_completed)(TPMIf *obj);
  } TPMIfClass;

diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 51dfc0de9c..b12ae5b625 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -52,7 +52,6 @@ struct TPMBackend {

  /*  */
  char *id;
-enum TpmModel fe_model;

  QLIST_ENTRY(TPMBackend) list;
  };
diff --git a/backends/tpm.c b/backends/tpm.c
index 7b108bd5d8..0c48d18775 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -148,9 +148,10 @@ TPMInfo *tpm_backend_query_tpm(TPMBackend *s)
  {
  TPMInfo *info = g_new0(TPMInfo, 1);
  TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
+TPMIfClass *tic = TPM_IF_GET_CLASS(s->tpmif);

  info->id = g_strdup(s->id);
-info->model = s->fe_model;
+info->model = tic->model;
  if (k->get_tpm_options) {
  info->options = k->get_tpm_options(s);
  }
@@ -204,7 +205,6 @@ static void tpm_backend_instance_init(Object *obj)
   tpm_backend_prop_get_opened,
   tpm_backend_prop_set_opened,
   NULL);
-s->fe_model = -1;
  s->bh = qemu_bh_new(tpm_backend_request_completed_bh, s);
  }

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 2fbc760730..3c8d246ac8 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -1067,8 +1067,6 @@ static void tpm_tis_realizefn(DeviceState *dev, Error 
**errp)
  return;
  }

-s->be_driver->fe_model = TPM_MODEL_TPM_TIS;
-
  if (tpm_backend_init(s->be_driver, TPM_IF(s), errp)) {
  return;
  }
@@ -1103,6 +1101,7 @@ static void tpm_tis_class_init(ObjectClass *klass, void 
*data)
  dc->props = tpm_tis_properties;
  dc->reset = tpm_tis_reset;
  dc->vmsd  = _tpm_tis;
+tc->model = TPM_MODEL_TPM_TIS;
  tc->request_completed = tpm_tis_request_completed;
  }

diff --git a/tpm.c b/tpm.c
index 45520f555d..ce1543fcb4 100644
--- a/tpm.c
+++ b/tpm.c
@@ -204,9 +204,10 @@ TPMInfoList *qmp_query_tpm(Error **errp)
  TPMInfoList *info, *head = NULL, *cur_item = NULL;

  QLIST_FOREACH(drv, _backends, list) {
-if (!tpm_models[drv->fe_model]) {
+if (!drv->tpmif) {
  continue;
  }
+
  info = g_new0(TPMInfoList, 1);
  info->value = tpm_backend_query_tpm(drv);

Re: [Qemu-devel] [PATCH 37/42] tpm: lookup the the TPM interface instead of TIS device

[Eduardo Habkost]

On Tue, Oct 10, 2017 at 12:56:18AM +0200, Marc-André Lureau wrote:
[...]
> -static inline TPMVersion tpm_get_version(void)
> +static inline TPMIf *tpm_find(void)
>  {
> -#ifdef CONFIG_TPM
> -Object *obj = object_resolve_path_type("", TYPE_TPM_TIS, NULL);
> +Object *obj = object_resolve_path_type("", TYPE_TPM_IF, NULL);

Considering that tpm_crb_realizefn() will rely on tpm_find()
returning NULL if there are multiple TPM devices, I suggest
adding a "returns NULL unless there is exactly one TPM device"
comment, just like fw_cfg_find() and find_vmgenid_dev()

> +
> +return TPM_IF(obj);
> +}
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH 25/42] tpm-be: report error instead of front-end

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Backend can give more accurate error description, and lift out the job
from the frontend.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 


---
  include/sysemu/tpm_backend.h | 3 ++-
  backends/tpm.c   | 3 ++-
  hw/tpm/tpm_tis.c | 4 +---
  3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index dd4fb288ea..51dfc0de9c 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -100,12 +100,13 @@ enum TpmType tpm_backend_get_type(TPMBackend *s);
   * @s: the backend to initialized
   * @tpmif: TPM interface
   * @datacb: callback for sending data to frontend
+ * @errp: a pointer to return the #Error object if an error occurs.
   *
   * Initialize the backend with the given variables.
   *
   * Returns 0 on success.
   */
-int tpm_backend_init(TPMBackend *s, TPMIf *tpmif);
+int tpm_backend_init(TPMBackend *s, TPMIf *tpmif, Error **errp);

  /**
   * tpm_backend_startup_tpm:
diff --git a/backends/tpm.c b/backends/tpm.c
index 58f823d54c..7b108bd5d8 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -54,9 +54,10 @@ enum TpmType tpm_backend_get_type(TPMBackend *s)
  return k->type;
  }

-int tpm_backend_init(TPMBackend *s, TPMIf *tpmif)
+int tpm_backend_init(TPMBackend *s, TPMIf *tpmif, Error **errp)
  {
  if (s->tpmif) {
+error_setg(errp, "TPM backend '%s' is already initialized", s->id);
  return -1;
  }

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 355427ab29..2fbc760730 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -1069,9 +1069,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error 
**errp)

  s->be_driver->fe_model = TPM_MODEL_TPM_TIS;

-if (tpm_backend_init(s->be_driver, TPM_IF(s))) {
-error_setg(errp, "tpm_tis: backend driver with id %s could not be "
-   "initialized", s->backend);
+if (tpm_backend_init(s->be_driver, TPM_IF(s), errp)) {
  return;
  }

Re: [Qemu-devel] [PATCH 42/42] WIP: add TPM CRB device

[Eduardo Habkost]

On Tue, Oct 10, 2017 at 02:28:19PM -0400, Stefan Berger wrote:
> On 10/09/2017 06:56 PM, Marc-André Lureau wrote:
> > +
> > +static void tpm_crb_realizefn(DeviceState *dev, Error **errp)
> > +{
> > +CRBState *s = CRB(dev);
> > +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > +
> > +if (!tpm_find()) {
> > +error_setg(errp, "at most one TPM device is permitted");
> 
> 
> Shouldn't that be the case if tpm_find() != NULL ?

It's tricky: due to the way it's implemented (using
object_resolve_path_type()) tpm_find() will be able to find @dev
itself, because it is already attached to the device tree, but it
will return NULL if there are multiple TPM devices.

You can see the same pattern being used at
find_vmgenid_dev()/vmgenid_realize() and
fw_cfg_find()/fw_cfg_common_realize().  The difference is that
find_vmgenid_dev() and fw_cfg_find() are explicitly documented
with a "returns NULL unless there is exactly one device" comment,
while tpm_find() is not.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 23/42] tpm-tis: no longer expose TPMState

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Now that there is an interface instead.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 


---
  include/sysemu/tpm.h | 2 --
  hw/tpm/tpm_tis.c | 4 ++--
  2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index c8afa179e5..62b073beeb 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -14,8 +14,6 @@

  #include "qemu/option.h"

-typedef struct TPMState TPMState;
-
  int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
  int tpm_init(void);
  void tpm_cleanup(void);
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index c24be57136..b3757bfbda 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -72,7 +72,7 @@ typedef struct TPMLocality {
  TPMSizedBuffer r_buffer;
  } TPMLocality;

-struct TPMState {
+typedef struct TPMState {
  ISADevice busdev;
  MemoryRegion mmio;

@@ -95,7 +95,7 @@ struct TPMState {
  char *backend;
  TPMBackend *be_driver;
  TPMVersion be_tpm_version;
-};
+} TPMState;

  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)

Re: [Qemu-devel] [PATCH 21/42] tpm: move recv_data_callback to TPM interface

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Simplify the TPM backend setup, move callback to TPM interface.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Stefan Berger 


---
  hw/tpm/tpm_int.h |  3 +++
  include/sysemu/tpm_backend.h |  6 +-
  backends/tpm.c   |  4 +---
  hw/tpm/tpm_emulator.c|  3 ++-
  hw/tpm/tpm_passthrough.c |  3 ++-
  hw/tpm/tpm_tis.c | 11 ++-
  6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h
index eb02e7760c..9c045b6691 100644
--- a/hw/tpm/tpm_int.h
+++ b/hw/tpm/tpm_int.h
@@ -29,6 +29,9 @@ typedef struct TPMIf {

  typedef struct TPMIfClass {
  InterfaceClass parent_class;
+
+/* run in thread pool by backend */
+void (*request_completed)(TPMIf *obj);
  } TPMIfClass;

  #define TPM_STANDARD_CMDLINE_OPTS   \
diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 3bb90be3de..03ea5a3400 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -30,8 +30,6 @@
  typedef struct TPMBackendClass TPMBackendClass;
  typedef struct TPMBackend TPMBackend;

-typedef void (TPMRecvDataCB)(TPMState *);
-
  typedef struct TPMBackendCmd {
  uint8_t locty;
  const uint8_t *in;
@@ -48,7 +46,6 @@ struct TPMBackend {
  bool opened;
  TPMState *tpm_state;
  GThreadPool *thread_pool;
-TPMRecvDataCB *recv_data_callback;
  bool had_startup_error;

  /*  */
@@ -106,8 +103,7 @@ enum TpmType tpm_backend_get_type(TPMBackend *s);
   *
   * Returns 0 on success.
   */
-int tpm_backend_init(TPMBackend *s, TPMState *state,
- TPMRecvDataCB *datacb);
+int tpm_backend_init(TPMBackend *s, TPMState *state);

  /**
   * tpm_backend_startup_tpm:
diff --git a/backends/tpm.c b/backends/tpm.c
index 87c5c09179..5763f6f369 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -44,11 +44,9 @@ enum TpmType tpm_backend_get_type(TPMBackend *s)
  return k->type;
  }

-int tpm_backend_init(TPMBackend *s, TPMState *state,
- TPMRecvDataCB *datacb)
+int tpm_backend_init(TPMBackend *s, TPMState *state)
  {
  s->tpm_state = state;
-s->recv_data_callback = datacb;
  s->had_startup_error = false;

  return 0;
diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 407ac97651..f04f4e0830 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -176,6 +176,7 @@ static int tpm_emulator_set_locality(TPMEmulator *tpm_emu, 
uint8_t locty_number,
  static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd *cmd)
  {
  TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+TPMIfClass *tic = TPM_IF_GET_CLASS(tb->tpm_state);
  Error *err = NULL;

  DPRINTF("processing TPM command");
@@ -190,7 +191,7 @@ static void tpm_emulator_handle_request(TPMBackend *tb, 
TPMBackendCmd *cmd)
  goto error;
  }

-tb->recv_data_callback(tb->tpm_state);
+tic->request_completed(TPM_IF(tb->tpm_state));
  return;

  error:
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 4274164a61..c440aff4b2 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -139,13 +139,14 @@ err_exit:
  static void tpm_passthrough_handle_request(TPMBackend *tb, TPMBackendCmd *cmd)
  {
  TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
+TPMIfClass *tic = TPM_IF_GET_CLASS(tb->tpm_state);

  DPRINTF("tpm_passthrough: processing command %p\n", cmd);

  tpm_passthrough_unix_tx_bufs(tpm_pt, cmd->in, cmd->in_len,
   cmd->out, cmd->out_len, >selftest_done);

-tb->recv_data_callback(tb->tpm_state);
+tic->request_completed(TPM_IF(tb->tpm_state));
  }

  static void tpm_passthrough_reset(TPMBackend *tb)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index dbb50043ac..8c5cac5fa5 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -430,11 +430,10 @@ static void tpm_tis_receive_bh(void *opaque)
TPM_TIS_INT_DATA_AVAILABLE | TPM_TIS_INT_STS_VALID);
  }

-/*
- * Callback from the TPM to indicate that the response was received.
- */
-static void tpm_tis_receive_cb(TPMState *s)
+static void tpm_tis_request_completed(TPMIf *ti)
  {
+TPMState *s = TPM(ti);
+
  bool is_selftest_done = s->cmd.selftest_done;
  uint8_t locty = s->cmd.locty;
  uint8_t l;
@@ -1078,7 +1077,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error 
**errp)

  s->be_driver->fe_model = TPM_MODEL_TPM_TIS;

-if (tpm_backend_init(s->be_driver, s, tpm_tis_receive_cb)) {
+if (tpm_backend_init(s->be_driver, s)) {
  error_setg(errp, "tpm_tis: backend driver with id %s could not be "
 "initialized", s->backend);
  return;
@@ -1110,11 +1109,13 @@ static void tpm_tis_initfn(Object *obj)
  static void tpm_tis_class_init(ObjectClass *klass, void *data)
  {
  DeviceClass *dc =