Re: [PATCH v5 00/18] nvme: refactoring and cleanups

[Philippe Mathieu-Daudé]

Hi Klaus,

On 5/11/20 8:25 AM, Klaus Jensen wrote:

On May  5 07:48, Klaus Jensen wrote:

From: Klaus Jensen 

Changes since v5

No functional changes, just updated Reviewed-by tags. Also, I screwed up
the CC list when sending v4.

Philippe and Keith, please add a Reviewed-by to

   * "nvme: factor out pmr setup" and
   * "do cmb/pmr init as part of pci init"

since the first one was added and the second one was changed in v4 when
rebasing on Kevins block-next tree which had the PMR work that was not
in master at the time.

With those in place, it should be ready for Kevin to merge.

  
Gentle ping on this.


Also, please see the two patches in "[PATCH 0/2] hw/block/nvme: fixes
for interrupt behavior". I think they should go in preparation to this
series.


I was going to ping Kevin last week, but then read your comment on pach 
#7 "nvme: add max_ioqpairs device parameter", so I interpreted you would 
respin.
Now it is clearer, applying in the following order you don't need to 
respin, right?


- [PATCH 0/2] hw/block/nvme: fixes for interrupt behavior"
- [PATCH v5 00/18] nvme: refactoring and cleanups

Re: [PATCH 0/7] hw/sparc/leon3: Few fixes and disable HelenOS test

[Philippe Mathieu-Daudé]

On 4/14/20 12:00 PM, KONRAD Frederic wrote:

Le 4/13/20 à 11:07 PM, Philippe Mathieu-Daudé a écrit :

[Cc'ing Peter]

On 4/13/20 12:12 PM, KONRAD Frederic wrote:

Le 4/11/20 à 7:30 PM, Philippe Mathieu-Daudé a écrit :

On 3/31/20 12:50 PM, Philippe Mathieu-Daudé wrote:

Philippe Mathieu-Daudé (7):
    hw/misc/grlib_ahb_apb_pnp: Avoid crash when writing to AHB PnP
  registers
    hw/misc/grlib_ahb_apb_pnp: Fix AHB PnP 8-bit accesses


Ping ^^^ for 5.0?


Hi Philippe,

You already have my rb tag for those one, and IMHO they should be good
candidate
for 5.0 (if it's not too late).


Yes, thanks for the reviews. I see Mark Cave-Ayland merged this file
first, but you are listed as maintainer :) I was hoping you could send a
pull request.


Yes that's usually Mark who take the patches, sorry I didn't get it.


No worries. As there are other sparc64 patches on the list, maybe Mark 
will prepare a pull request now.






$ scripts/get_maintainer.pl -f hw/misc/grlib_ahb_apb_pnp.c
Fabien Chouteau  (maintainer:Leon3)
KONRAD Frederic  (maintainer:Leon3)
qemu-devel@nongnu.org (open list:All patches CC here)




This is a bug but not 'security critical', so it might wait 5.1 and go
via qemu-trivial tree.


Well let's do that then if you're ok.


OK, then ping? :)



Best Regards,
Fred



Regards,

Phil.



Cheers,
Fred




    hw/misc/grlib_ahb_apb_pnp: Add trace events on read accesses
    hw/timer/grlib_gptimer: Display frequency in decimal
    target/sparc/int32_helper: Remove DEBUG_PCALL definition
    target/sparc/int32_helper: Extract and use excp_name_str()

   hw/misc/grlib_ahb_apb_pnp.c | 24 
++--
   target/sparc/int32_helper.c | 23 
---

   hw/misc/trace-events    |  4 
   hw/timer/trace-events   |  2 +-
   tests/acceptance/machine_sparc_leon3.py |  4 
   5 files changed, 43 insertions(+), 14 deletions(-)

[PATCH] block/replication.c: Avoid cancelling the job twice

[Lukas Straub]

If qemu in colo secondary mode is stopped, it crashes because
s->backup_job is canceled twice: First with job_cancel_sync_all()
in qemu_cleanup() and then in replication_stop().

Fix this by assigning NULL to s->backup_job when the job completes
so replication_stop() and replication_do_checkpoint() won't touch
the job.

Signed-off-by: Lukas Straub 
---
 block/replication.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/replication.c b/block/replication.c
index da013c2041..33f2f62a44 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -398,6 +398,8 @@ static void backup_job_cleanup(BlockDriverState *bs)
 BDRVReplicationState *s = bs->opaque;
 BlockDriverState *top_bs;
 
+s->backup_job = NULL;
+
 top_bs = bdrv_lookup_bs(s->top_id, s->top_id, NULL);
 if (!top_bs) {
 return;
-- 
2.20.1


pgp91i6UMsufr.pgp
Description: OpenPGP digital signature

Re: [PATCH v5 00/18] nvme: refactoring and cleanups

[Klaus Jensen]

On May 11 09:00, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 5/11/20 8:25 AM, Klaus Jensen wrote:
> > On May  5 07:48, Klaus Jensen wrote:
> > > From: Klaus Jensen 
> > > 
> > > Changes since v5
> > > 
> > > No functional changes, just updated Reviewed-by tags. Also, I screwed up
> > > the CC list when sending v4.
> > > 
> > > Philippe and Keith, please add a Reviewed-by to
> > > 
> > >* "nvme: factor out pmr setup" and
> > >* "do cmb/pmr init as part of pci init"
> > > 
> > > since the first one was added and the second one was changed in v4 when
> > > rebasing on Kevins block-next tree which had the PMR work that was not
> > > in master at the time.
> > > 
> > > With those in place, it should be ready for Kevin to merge.
> > > 
> > Gentle ping on this.
> > 
> > Also, please see the two patches in "[PATCH 0/2] hw/block/nvme: fixes
> > for interrupt behavior". I think they should go in preparation to this
> > series.
> 
> I was going to ping Kevin last week, but then read your comment on pach #7
> "nvme: add max_ioqpairs device parameter", so I interpreted you would
> respin.
> Now it is clearer, applying in the following order you don't need to respin,
> right?
> 
> - [PATCH 0/2] hw/block/nvme: fixes for interrupt behavior"
> - [PATCH v5 00/18] nvme: refactoring and cleanups
> 

Ugh. "[PATCH v5 00/18] nvme: refactoring and cleanups" doesn't apply
completely cleanly.

"[PATCH 0/2] hw/block/nvme: fixes for interrupt behavior" was intented
to go into master because it fixes a bug, that is why I split them up.

But looks like it is better to just roll it into this series. I'll
respin a v6 with the two interrupt fixes.

Re: Abort in mch_update_pciexbar

[Philippe Mathieu-Daudé]

On 5/11/20 8:19 AM, Philippe Mathieu-Daudé wrote:

On 5/11/20 6:59 AM, Alexander Bulekov wrote:

Hello,
While fuzzing, I found an input that triggers an assertion failure in
mch_update_pciexbar:

#6 0x7f38d387c55a in abort 
/build/glibc-GwnBeO/glibc-2.30/stdlib/abort.c:79:7

#7 0x55c27e94ffd0 in mch_update_pciexbar hw/pci-host/q35.c:331:9
#8 0x55c27e94db38 in mch_write_config hw/pci-host/q35.c:487:9
#9 0x55c27e9e3f4c in pci_host_config_write_common hw/pci/pci_host.c:81:5
#10 0x55c27e9e5307 in pci_data_write hw/pci/pci_host.c:118:5
#11 0x55c27e9e6601 in pci_host_data_write hw/pci/pci_host.c:165:9
#12 0x55c27ca3b17b in memory_region_write_accessor memory.c:496:5
#13 0x55c27ca3a5e4 in access_with_adjusted_size memory.c:557:18
#14 0x55c27ca38177 in memory_region_dispatch_write memory.c:1488:16
#15 0x55c27c721325 in flatview_write_continue exec.c:3174:23
#16 0x55c27c70994d in flatview_write exec.c:3214:14
#17 0x55c27c709462 in address_space_write exec.c:3305:18


These lines don't match QEMU v5.0.0.



I can reproduce it in a qemu 5.0 build using:
cat << EOF | ~/Development/qemu/build/i386-softmmu/qemu-system-i386 -M 
pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio

outl 0xcf8 0xf260
outl 0xcfc 0x8400056e


The guest shouldn't ask for a reserved bar length (grep for 
MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD). I suppose we should simply report 
it as GUEST_ERROR and ignore it.


This patch prevent the crash:

-- >8 --
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 2bbc90b28f..2b744aca93 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -36,6 +36,7 @@
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qemu/module.h"
+#include "qemu/log.h"


/
  * Q35 host
@@ -320,6 +321,9 @@ static void mch_update_pciexbar(MCHPCIState *mch)
 addr_mask |= MCH_HOST_BRIDGE_PCIEXBAR_64ADMSK;
 break;
 case MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid use of reserved 
value\n",

+   __func__);
+return;
 default:
 abort();
 }
---

But the real question is what would the real hardware do in this case.




EOF

I also uploaded the above trace, in case the formatting is broken:

curl https://paste.debian.net/plain/1146095 | qemu-system-i386 -M 
pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio


Please let me know if I can provide any further info.


It would help the community if you fill your bug reports with Launchpad, 
so they don't get lost in the high email flow, and we can track/update 
them. See for example:

https://bugs.launchpad.net/qemu/+bug/1835865 and
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06082.html 
which refers it.

Re: [PATCH 0/2] hw/block/nvme: fixes for interrupt behavior

[Philippe Mathieu-Daudé]

+ Michael & Marcel for MSIX,
and ping to Keith :)

On 5/5/20 10:36 PM, Klaus Jensen wrote:

From: Klaus Jensen 

Klaus Jensen (2):
   hw/block/nvme: fix pin-based interrupt behavior
   hw/block/nvme: allow use of any valid msix vector

  hw/block/nvme.c | 14 +-
  hw/block/nvme.h |  2 +-
  2 files changed, 10 insertions(+), 6 deletions(-)

Re: [PATCH 11/12] hw/pci-host/bonito: Set the Config register reset value with FIELD_DP32

[Aleksandar Markovic]

пон, 11. мај 2020. у 08:30 Philippe Mathieu-Daudé  је
написао/ла:
>
> On 5/11/20 8:17 AM, Aleksandar Markovic wrote:
> > нед, 10. мај 2020. у 23:01 Philippe Mathieu-Daudé  је
> > написао/ла:
> >>
> >> Describe some Config registers fields with the registerfields
> >> API. Use the FIELD_DP32() macro to set the BONGENCFG register
> >> bits at reset.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> ---
> >>   hw/pci-host/bonito.c | 21 -
> >>   1 file changed, 20 insertions(+), 1 deletion(-)
> >>
> >
> > Some Config registers? Is there any reason not to cover all Bonito
> > config registers? Or, the commit message was inprecise?
>
> The commit message is not correct English, I wanted to say "some bits of
> the Config register" (I don't want to overload the file defining bits
> we'll never use).
>
> >
> > But, in general, I salute the intent of this patch.
>
> Thanks! I'll reword the description.
>

OK, then, with that little rewording, certainly, a good patch:

Reviewed-by: Aleksandar Markovic 

> >
> > Sincerely,
> > Aleksandar
> >
> >> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> >> index 335c7787eb..86aceb333a 100644
> >> --- a/hw/pci-host/bonito.c
> >> +++ b/hw/pci-host/bonito.c
> >> @@ -50,6 +50,7 @@
> >>   #include "sysemu/runstate.h"
> >>   #include "exec/address-spaces.h"
> >>   #include "hw/misc/unimp.h"
> >> +#include "hw/registerfields.h"
> >>
> >>   /* #define DEBUG_BONITO */
> >>
> >> @@ -112,8 +113,19 @@
> >>   /* Power on register */
> >>
> >>   #define BONITO_BONPONCFG(0x00 >> 2)  /* 0x100 */
> >> +
> >> +/* PCI configuration register */
> >>   #define BONITO_BONGENCFG_OFFSET 0x4
> >>   #define BONITO_BONGENCFG(BONITO_BONGENCFG_OFFSET >> 2)   /*0x104 
> >> */
> >> +REG32(BONGENCFG,0x104)
> >> +FIELD(BONGENCFG, DEBUGMODE,  0, 1)
> >> +FIELD(BONGENCFG, SNOOP,  1, 1)
> >> +FIELD(BONGENCFG, CPUSELFRESET,   2, 1)
> >> +FIELD(BONGENCFG, BYTESWAP,   6, 1)
> >> +FIELD(BONGENCFG, UNCACHED,   7, 1)
> >> +FIELD(BONGENCFG, PREFETCH,   8, 1)
> >> +FIELD(BONGENCFG, WRITEBEHIND,9, 1)
> >> +FIELD(BONGENCFG, PCIQUEUE,  12, 1)
> >>
> >>   /* 2. IO & IDE configuration */
> >>   #define BONITO_IODEVCFG (0x08 >> 2)  /* 0x108 */
> >> @@ -577,11 +589,18 @@ static int pci_bonito_map_irq(PCIDevice *pci_dev, 
> >> int irq_num)
> >>   static void bonito_reset(void *opaque)
> >>   {
> >>   PCIBonitoState *s = opaque;
> >> +uint32_t val = 0;
> >>
> >>   /* set the default value of north bridge registers */
> >>
> >>   s->regs[BONITO_BONPONCFG] = 0xc40;
> >> -s->regs[BONITO_BONGENCFG] = 0x1384;
> >> +val = FIELD_DP32(val, BONGENCFG, PCIQUEUE, 1);
> >> +val = FIELD_DP32(val, BONGENCFG, WRITEBEHIND, 1);
> >> +val = FIELD_DP32(val, BONGENCFG, PREFETCH, 1);
> >> +val = FIELD_DP32(val, BONGENCFG, UNCACHED, 1);
> >> +val = FIELD_DP32(val, BONGENCFG, CPUSELFRESET, 1);
> >> +s->regs[BONITO_BONGENCFG] = val;
> >> +
> >>   s->regs[BONITO_IODEVCFG] = 0x2bff8010;
> >>   s->regs[BONITO_SDCFG] = 0x255e0091;
> >>
> >> --
> >> 2.21.3
> >>
> >

Re: Abort in mch_update_pciexbar

[Michael S. Tsirkin]

On Mon, May 11, 2020 at 09:10:48AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/11/20 8:19 AM, Philippe Mathieu-Daudé wrote:
> > On 5/11/20 6:59 AM, Alexander Bulekov wrote:
> > > Hello,
> > > While fuzzing, I found an input that triggers an assertion failure in
> > > mch_update_pciexbar:
> > > 
> > > #6 0x7f38d387c55a in abort
> > > /build/glibc-GwnBeO/glibc-2.30/stdlib/abort.c:79:7
> > > #7 0x55c27e94ffd0 in mch_update_pciexbar hw/pci-host/q35.c:331:9
> > > #8 0x55c27e94db38 in mch_write_config hw/pci-host/q35.c:487:9
> > > #9 0x55c27e9e3f4c in pci_host_config_write_common hw/pci/pci_host.c:81:5
> > > #10 0x55c27e9e5307 in pci_data_write hw/pci/pci_host.c:118:5
> > > #11 0x55c27e9e6601 in pci_host_data_write hw/pci/pci_host.c:165:9
> > > #12 0x55c27ca3b17b in memory_region_write_accessor memory.c:496:5
> > > #13 0x55c27ca3a5e4 in access_with_adjusted_size memory.c:557:18
> > > #14 0x55c27ca38177 in memory_region_dispatch_write memory.c:1488:16
> > > #15 0x55c27c721325 in flatview_write_continue exec.c:3174:23
> > > #16 0x55c27c70994d in flatview_write exec.c:3214:14
> > > #17 0x55c27c709462 in address_space_write exec.c:3305:18
> > 
> > These lines don't match QEMU v5.0.0.
> > 
> > > 
> > > I can reproduce it in a qemu 5.0 build using:
> > > cat << EOF | ~/Development/qemu/build/i386-softmmu/qemu-system-i386
> > > -M pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio
> > > outl 0xcf8 0xf260
> > > outl 0xcfc 0x8400056e
> > 
> > The guest shouldn't ask for a reserved bar length (grep for
> > MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD). I suppose we should simply report
> > it as GUEST_ERROR and ignore it.
> 
> This patch prevent the crash:
> 
> -- >8 --
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 2bbc90b28f..2b744aca93 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -36,6 +36,7 @@
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>  #include "qemu/module.h"
> +#include "qemu/log.h"
> 
> 
> /
>   * Q35 host
> @@ -320,6 +321,9 @@ static void mch_update_pciexbar(MCHPCIState *mch)
>  addr_mask |= MCH_HOST_BRIDGE_PCIEXBAR_64ADMSK;
>  break;
>  case MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD:
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid use of reserved
> value\n",
> +   __func__);
> +return;
>  default:
>  abort();
>  }

Maybe report the value too.

> ---
> 
> But the real question is what would the real hardware do in this case.

Spec doesn't say unfortunately. All it says is:
Designers must not rely on the absence or characteristics of any
features or instructions marked "reserved" or "undefined." Intel
reserves these for

future definition and shall have no responsibility whatsoever
for conflicts or incompatibilities arising from future changes to them.



> > 
> > > EOF
> > > 
> > > I also uploaded the above trace, in case the formatting is broken:
> > > 
> > > curl https://paste.debian.net/plain/1146095 | qemu-system-i386 -M
> > > pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio
> > > 
> > > Please let me know if I can provide any further info.
> > 
> > It would help the community if you fill your bug reports with Launchpad,
> > so they don't get lost in the high email flow, and we can track/update
> > them. See for example:
> > https://bugs.launchpad.net/qemu/+bug/1835865 and
> > https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06082.html
> > which refers it.

Re: Abort in mch_update_pciexbar

[Philippe Mathieu-Daudé]

On 5/11/20 9:39 AM, Michael S. Tsirkin wrote:

On Mon, May 11, 2020 at 09:10:48AM +0200, Philippe Mathieu-Daudé wrote:

On 5/11/20 8:19 AM, Philippe Mathieu-Daudé wrote:

On 5/11/20 6:59 AM, Alexander Bulekov wrote:

Hello,
While fuzzing, I found an input that triggers an assertion failure in
mch_update_pciexbar:

#6 0x7f38d387c55a in abort
/build/glibc-GwnBeO/glibc-2.30/stdlib/abort.c:79:7
#7 0x55c27e94ffd0 in mch_update_pciexbar hw/pci-host/q35.c:331:9
#8 0x55c27e94db38 in mch_write_config hw/pci-host/q35.c:487:9
#9 0x55c27e9e3f4c in pci_host_config_write_common hw/pci/pci_host.c:81:5
#10 0x55c27e9e5307 in pci_data_write hw/pci/pci_host.c:118:5
#11 0x55c27e9e6601 in pci_host_data_write hw/pci/pci_host.c:165:9
#12 0x55c27ca3b17b in memory_region_write_accessor memory.c:496:5
#13 0x55c27ca3a5e4 in access_with_adjusted_size memory.c:557:18
#14 0x55c27ca38177 in memory_region_dispatch_write memory.c:1488:16
#15 0x55c27c721325 in flatview_write_continue exec.c:3174:23
#16 0x55c27c70994d in flatview_write exec.c:3214:14
#17 0x55c27c709462 in address_space_write exec.c:3305:18


These lines don't match QEMU v5.0.0.



I can reproduce it in a qemu 5.0 build using:
cat << EOF | ~/Development/qemu/build/i386-softmmu/qemu-system-i386
-M pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio
outl 0xcf8 0xf260
outl 0xcfc 0x8400056e


The guest shouldn't ask for a reserved bar length (grep for
MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD). I suppose we should simply report
it as GUEST_ERROR and ignore it.


This patch prevent the crash:

-- >8 --
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 2bbc90b28f..2b744aca93 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -36,6 +36,7 @@
  #include "qapi/error.h"
  #include "qapi/visitor.h"
  #include "qemu/module.h"
+#include "qemu/log.h"


/
   * Q35 host
@@ -320,6 +321,9 @@ static void mch_update_pciexbar(MCHPCIState *mch)
  addr_mask |= MCH_HOST_BRIDGE_PCIEXBAR_64ADMSK;
  break;
  case MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid use of reserved
value\n",
+   __func__);
+return;
  default:
  abort();
  }


Maybe report the value too.


This is a 2 bit field, all are covered except 0b11 which is the RESERVED 
case, and the abort() now will never fire. It is unclear why the 
compiler doesn't notice that.





---

But the real question is what would the real hardware do in this case.


Spec doesn't say unfortunately. All it says is:
Designers must not rely on the absence or characteristics of any
features or instructions marked "reserved" or "undefined." Intel
reserves these for

future definition and shall have no responsibility whatsoever
for conflicts or incompatibilities arising from future changes to them.


OK, thanks for checking. I'll submit formal patch with proper description.






EOF

I also uploaded the above trace, in case the formatting is broken:

curl https://paste.debian.net/plain/1146095 | qemu-system-i386 -M
pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio

Please let me know if I can provide any further info.


It would help the community if you fill your bug reports with Launchpad,
so they don't get lost in the high email flow, and we can track/update
them. See for example:
https://bugs.launchpad.net/qemu/+bug/1835865 and
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06082.html
which refers it.

Re: Patches for ui/gtk and ui/sdl

[Howard Spoelstra]

On Sun, May 10, 2020 at 8:42 PM Volker Rümelin  wrote:

> It's rather difficult to test qemu patches in guests on Windows with
> important keys missing. These patches mainly fix the guest keyboard on
> Windows.
>
> With best regards,
> Volker
>

Hi Volker,

Excellent patch! I tested this on Windows with qemu-system-ppc running Mac
OS 9.2 with both SDL and GTK GUIs. Finally no more popping up of that
windows menu. Key combos are sent correctly into the guest. Also right alt
does no longer send left ctrl+alt into the guest.

A mere cosmetic difference between using a real mac keyboard and a pc
keyboard is that alt and windows key seem to have traded places. However,
the mac keyboard does have the alt where the windows key is on the pc
keyboard. The keys are, however, functionally correct.

The GTK GUI itself was and is unusable for Mac OS 9 guests in WIndows. That
OS has no tablet driver available.

Thanks,
Howard

Re: [PATCH 04/12] hw/mips/fuloong2e: Fix typo in Fuloong machine name

[Aleksandar Markovic]

пон, 11. мај 2020. у 08:52 chen huacai  је написао/ла:
>
> Hi, Philippe and Alexandar,
>
> On Mon, May 11, 2020 at 2:38 PM Philippe Mathieu-Daudé  
> wrote:
> >
> > On 5/11/20 8:21 AM, Aleksandar Markovic wrote:
> > > пон, 11. мај 2020. у 03:11 chen huacai  је 
> > > написао/ла:
> > >>
> > >> Hi, Philippe,
> > >>
> > >> On Mon, May 11, 2020 at 5:06 AM Philippe Mathieu-Daudé  
> > >> wrote:
> > >>>
> > >>> We always miswrote the Fuloong machine... Fix its name.
> > >>> Add an machine alias to the previous name for backward
> > >>> compatibility.
> > >>>
> > >>> Suggested-by: Aleksandar Markovic 
> > >>> Signed-off-by: Philippe Mathieu-Daudé 
> > >>> ---
> > >>>   docs/system/target-mips.rst  |  2 +-
> > >>>   default-configs/mips64el-softmmu.mak |  2 +-
> > >>>   hw/isa/vt82c686.c|  2 +-
> > >>>   hw/mips/{mips_fulong2e.c => fuloong2e.c} | 46 
> > >> Use mips_fuloong2e.c instead of fuloong2e.c? Other machine file names
> > >> also have a "mips_" prefix.
> > >>
> > >
> > > I would leave mips_ prefix for Fuloong, and actually add it to Boston
> > > source file, so that we are finally consistent across all MIPS
> > > machines.
> > >
> > > What do you think?
> >
> > These names were used years ago when all hardware was in the same hw/
> > directory, not sorted per target. Now new machines don't use the target
> > as prefix name. I'd clean the other way around, and dropping the 'mips_'
> > prefix. The positive side is we can 5 more characters to better describe
> > a patch while limited by the 72 chars in the subject :)
>
> All having the prefix, or all dropping the prefix, are both good for
> me, just keep consistency.
>
> Huacai
>

Philippe, Huacai,

Prefix or not, I have mixed feelings. I had consistency more in mind
than prefix.

So it seems the prevailing opinion is slightly on the side of dropping
prefix "mips_".

Philippe, if it is not too difficult, could you perhaps make dropping
that prefix for all source file names in hw/mips a part of the this
series (not to complicate situation with a separate series) in its
follow-up version (but perhaps keep that change(s) in separate
patch(es))?

Sincerely,
Aleksandar

> >
> > >
> > > Aleksandar
> > >
> > >> Huacai
> > >>>   hw/pci-host/bonito.c |  8 ++---
> > >>>   tests/qtest/endianness-test.c|  2 +-
> > >>>   MAINTAINERS  |  4 +--
> > >>>   hw/mips/Kconfig  |  2 +-
> > >>>   hw/mips/Makefile.objs|  2 +-
> > >>>   9 files changed, 36 insertions(+), 34 deletions(-)
> > >>>   rename hw/mips/{mips_fulong2e.c => fuloong2e.c} (91%)
> > >>>
> > >>> diff --git a/docs/system/target-mips.rst b/docs/system/target-mips.rst
> > >>> index 2736fd0509..cd2a931edf 100644
> > >>> --- a/docs/system/target-mips.rst
> > >>> +++ b/docs/system/target-mips.rst
> > >>> @@ -74,7 +74,7 @@ The MIPS Magnum R4000 emulation supports:
> > >>>
> > >>>   -  G364 framebuffer
> > >>>
> > >>> -The Fulong 2E emulation supports:
> > >>> +The Fuloong 2E emulation supports:
> > >>>
> > >>>   -  Loongson 2E CPU
> > >>>
> > >>> diff --git a/default-configs/mips64el-softmmu.mak 
> > >>> b/default-configs/mips64el-softmmu.mak
> > >>> index 8b0c9b1e15..9f8a3ef156 100644
> > >>> --- a/default-configs/mips64el-softmmu.mak
> > >>> +++ b/default-configs/mips64el-softmmu.mak
> > >>> @@ -2,7 +2,7 @@
> > >>>
> > >>>   include mips-softmmu-common.mak
> > >>>   CONFIG_IDE_VIA=y
> > >>> -CONFIG_FULONG=y
> > >>> +CONFIG_FULOONG=y
> > >>>   CONFIG_ATI_VGA=y
> > >>>   CONFIG_RTL8139_PCI=y
> > >>>   CONFIG_JAZZ=y
> > >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > >>> index d9b51fce8d..fac4e56b7d 100644
> > >>> --- a/hw/isa/vt82c686.c
> > >>> +++ b/hw/isa/vt82c686.c
> > >>> @@ -503,7 +503,7 @@ static void via_class_init(ObjectClass *klass, void 
> > >>> *data)
> > >>>   dc->vmsd = &vmstate_via;
> > >>>   /*
> > >>>* Reason: part of VIA VT82C686 southbridge, needs to be wired up,
> > >>> - * e.g. by mips_fulong2e_init()
> > >>> + * e.g. by mips_fuloong2e_init()
> > >>>*/
> > >>>   dc->user_creatable = false;
> > >>>   }
> > >>> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/fuloong2e.c
> > >>> similarity index 91%
> > >>> rename from hw/mips/mips_fulong2e.c
> > >>> rename to hw/mips/fuloong2e.c
> > >>> index 4e1a3646af..624c46a4fd 100644
> > >>> --- a/hw/mips/mips_fulong2e.c
> > >>> +++ b/hw/mips/fuloong2e.c
> > >>> @@ -1,5 +1,5 @@
> > >>>   /*
> > >>> - * QEMU fulong 2e mini pc support
> > >>> + * QEMU fuloong 2e mini pc support
> > >>>*
> > >>>* Copyright (c) 2008 yajin (ya...@vm-kernel.org)
> > >>>* Copyright (c) 2009 chenming (chenm...@rdc.faw.com.cn)
> > >>> @@ -11,8 +11,8 @@
> > >>>*/
> > >>>
> > >>>   /*
> > >>> - * Fulong 2e mini pc is based on ICT/ST Loongson 2e CPU (MIPS III 
> > >>> like, 800MHz)
> > >>> - * http://www.linux-mips.org/wiki/Fulong
> > >>> + * Fuloong 2e mini pc is bas

Re: [PATCH] xen: fix build without pci passthrough

[Roger Pau Monné]

Ping?

On Mon, May 04, 2020 at 12:14:43PM +0200, Roger Pau Monne wrote:
> has_igd_gfx_passthru is only available when QEMU is built with
> CONFIG_XEN_PCI_PASSTHROUGH, and hence shouldn't be used in common
> code without checking if it's available.
> 
> Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator 
> property')
> Signed-off-by: Roger Pau Monné 
> ---
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: xen-de...@lists.xenproject.org
> ---
>  hw/xen/xen-common.c | 4 
>  hw/xen/xen_pt.h | 7 +++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index a15070f7f6..c800862419 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -127,6 +127,7 @@ static void xen_change_state_handler(void *opaque, int 
> running,
>  }
>  }
>  
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>  static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>  {
>  return has_igd_gfx_passthru;
> @@ -136,6 +137,7 @@ static void xen_set_igd_gfx_passthru(Object *obj, bool 
> value, Error **errp)
>  {
>  has_igd_gfx_passthru = value;
>  }
> +#endif
>  
>  static void xen_setup_post(MachineState *ms, AccelState *accel)
>  {
> @@ -197,11 +199,13 @@ static void xen_accel_class_init(ObjectClass *oc, void 
> *data)
>  
>  compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>  
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>  object_class_property_add_bool(oc, "igd-passthru",
>  xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
>  &error_abort);
>  object_class_property_set_description(oc, "igd-passthru",
>  "Set on/off to enable/disable igd passthrou", &error_abort);
> +#endif
>  }
>  
>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 179775db7b..660dd8a008 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -1,6 +1,7 @@
>  #ifndef XEN_PT_H
>  #define XEN_PT_H
>  
> +#include "qemu/osdep.h"
>  #include "hw/xen/xen_common.h"
>  #include "hw/pci/pci.h"
>  #include "xen-host-pci-device.h"
> @@ -322,7 +323,13 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice 
> *dev,
>  unsigned int domain,
>  unsigned int bus, unsigned int 
> slot,
>  unsigned int function);
> +
> +#ifdef CONFIG_XEN_PCI_PASSTHROUGH
>  extern bool has_igd_gfx_passthru;
> +#else
> +# define has_igd_gfx_passthru false
> +#endif
> +
>  static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
>  {
>  return (has_igd_gfx_passthru
> -- 
> 2.26.2
> 

[PATCH v2 1/4] memory: Simplify memory_region_do_writeback()

[Philippe Mathieu-Daudé]

mr->dirty_log_mask tells if dirty tracking has been enabled,
not if the page is dirty. It would always be true during live
migration and when running on TCG, but otherwise it would
always be false.
As the value of mr->dirty_log_mask does not matter, remove
the check.

Cc: Beata Michalska 
Suggested-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
---
 memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index 601b749906..fd5c3af535 100644
--- a/memory.c
+++ b/memory.c
@@ -2204,7 +2204,7 @@ void memory_region_do_writeback(MemoryRegion *mr, hwaddr 
addr, hwaddr size)
  * Might be extended case needed to cover
  * different types of memory regions
  */
-if (mr->ram_block && mr->dirty_log_mask) {
+if (mr->ram_block) {
 qemu_ram_writeback(mr->ram_block, addr, size);
 }
 }
-- 
2.21.3

[PATCH v2 0/4] memory: Add memory_region_sync() & make NVMe emulated device generic

[Philippe Mathieu-Daudé]

Remove the pointless dirty_log_mask check before msync'ing,
let the NVMe emulated device be target-agnostic.

Supersedes: <20200508062456.23344-1-phi...@redhat.com>

Philippe Mathieu-Daudé (4):
  memory: Simplify memory_region_do_writeback()
  memory: Rename memory_region_do_writeback() -> memory_region_sync()
  hw/block: Let the NVMe emulated device be target-agnostic
  exec: Rename qemu_ram_writeback() as qemu_ram_msync()

 include/exec/memory.h   | 13 +++--
 include/exec/ram_addr.h |  4 ++--
 exec.c  |  2 +-
 hw/block/nvme.c |  6 ++
 memory.c|  7 +++
 target/arm/helper.c |  2 +-
 hw/block/Makefile.objs  |  2 +-
 7 files changed, 17 insertions(+), 19 deletions(-)

-- 
2.21.3

[PATCH v2 3/4] hw/block: Let the NVMe emulated device be target-agnostic

[Philippe Mathieu-Daudé]

Now than the non-target specific memory_region_sync() function
is available, use it to make this device target-agnostic.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/nvme.c| 6 ++
 hw/block/Makefile.objs | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9b453423cf..d9d0649540 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -46,8 +46,7 @@
 #include "qapi/visitor.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/block-backend.h"
-#include "exec/ram_addr.h"
-
+#include "exec/memory.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/cutils.h"
@@ -1207,8 +1206,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, 
unsigned size)
  */
 if (addr == 0xE08 &&
 (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
-qemu_ram_writeback(n->pmrdev->mr.ram_block,
-   0, n->pmrdev->size);
+memory_region_sync(&n->pmrdev->mr, 0, n->pmrdev->size);
 }
 memcpy(&val, ptr + addr, size);
 } else {
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index 47960b5f0d..8855c22656 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -13,6 +13,6 @@ common-obj-$(CONFIG_SH4) += tc58128.o
 
 obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o
 obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
-obj-$(CONFIG_NVME_PCI) += nvme.o
+common-obj-$(CONFIG_NVME_PCI) += nvme.o
 
 obj-y += dataplane/
-- 
2.21.3

[PATCH v2 2/4] memory: Rename memory_region_do_writeback() -> memory_region_sync()

[Philippe Mathieu-Daudé]

We usually use '_do_' for internal functions. Rename
memory_region_do_writeback() as memory_region_sync()
to better reflect what it does.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/exec/memory.h | 13 +++--
 memory.c  |  2 +-
 target/arm/helper.c   |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index e000bd2f97..4fc1d85b99 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1474,14 +1474,15 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize,
   Error **errp);
 /**
- * memory_region_do_writeback: Trigger cache writeback or msync for
- * selected address range
+ * memory_region_sync: Synchronize selected address range
  *
- * @mr: the memory region to be updated
- * @addr: the initial address of the range to be written back
- * @size: the size of the range to be written back
+ * It is only meaningful for RAM regions, otherwise it is no-op.
+ *
+ * @mr: the memory region to be synchronized
+ * @addr: the initial address of the range to be sync
+ * @size: the size of the range to be sync
  */
-void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size);
+void memory_region_sync(MemoryRegion *mr, hwaddr addr, hwaddr size);
 
 /**
  * memory_region_set_log: Turn dirty logging on or off for a region.
diff --git a/memory.c b/memory.c
index fd5c3af535..73534b26f4 100644
--- a/memory.c
+++ b/memory.c
@@ -2198,7 +2198,7 @@ void memory_region_ram_resize(MemoryRegion *mr, 
ram_addr_t newsize, Error **errp
 }
 
 
-void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size)
+void memory_region_sync(MemoryRegion *mr, hwaddr addr, hwaddr size)
 {
 /*
  * Might be extended case needed to cover
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a94f650795..c2697ed7c0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6829,7 +6829,7 @@ static void dccvap_writefn(CPUARMState *env, const 
ARMCPRegInfo *opaque,
 mr = memory_region_from_host(haddr, &offset);
 
 if (mr) {
-memory_region_do_writeback(mr, offset, dline_size);
+memory_region_sync(mr, offset, dline_size);
 }
 }
 }
-- 
2.21.3

[PATCH v2 4/4] exec: Rename qemu_ram_writeback() as qemu_ram_msync()

[Philippe Mathieu-Daudé]

Rename qemu_ram_writeback() as qemu_ram_msync() to better
match what it does.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/exec/ram_addr.h | 4 ++--
 exec.c  | 2 +-
 memory.c| 3 +--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index a94809f89b..84817e19fa 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -136,12 +136,12 @@ void qemu_ram_free(RAMBlock *block);
 
 int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
 
-void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length);
+void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length);
 
 /* Clear whole block of mem */
 static inline void qemu_ram_block_writeback(RAMBlock *block)
 {
-qemu_ram_writeback(block, 0, block->used_length);
+qemu_ram_msync(block, 0, block->used_length);
 }
 
 #define DIRTY_CLIENTS_ALL ((1 << DIRTY_MEMORY_NUM) - 1)
diff --git a/exec.c b/exec.c
index 2874bb5088..f5a8cdf370 100644
--- a/exec.c
+++ b/exec.c
@@ -2127,7 +2127,7 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, 
Error **errp)
  * Otherwise no-op.
  * @Note: this is supposed to be a synchronous op.
  */
-void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
+void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length)
 {
 /* The requested range should fit in within the block range */
 g_assert((start + length) <= block->used_length);
diff --git a/memory.c b/memory.c
index 73534b26f4..5bfa1429df 100644
--- a/memory.c
+++ b/memory.c
@@ -2197,7 +2197,6 @@ void memory_region_ram_resize(MemoryRegion *mr, 
ram_addr_t newsize, Error **errp
 qemu_ram_resize(mr->ram_block, newsize, errp);
 }
 
-
 void memory_region_sync(MemoryRegion *mr, hwaddr addr, hwaddr size)
 {
 /*
@@ -2205,7 +2204,7 @@ void memory_region_sync(MemoryRegion *mr, hwaddr addr, 
hwaddr size)
  * different types of memory regions
  */
 if (mr->ram_block) {
-qemu_ram_writeback(mr->ram_block, addr, size);
+qemu_ram_msync(mr->ram_block, addr, size);
 }
 }
 
-- 
2.21.3

Re: [PATCH 04/12] hw/mips/fuloong2e: Fix typo in Fuloong machine name

[Aleksandar Markovic]

пон, 11. мај 2020. у 10:12 Aleksandar Markovic
 је написао/ла:
>
> пон, 11. мај 2020. у 08:52 chen huacai  је написао/ла:
> >
> > Hi, Philippe and Alexandar,
> >
> > On Mon, May 11, 2020 at 2:38 PM Philippe Mathieu-Daudé  
> > wrote:
> > >
> > > On 5/11/20 8:21 AM, Aleksandar Markovic wrote:
> > > > пон, 11. мај 2020. у 03:11 chen huacai  је 
> > > > написао/ла:
> > > >>
> > > >> Hi, Philippe,
> > > >>
> > > >> On Mon, May 11, 2020 at 5:06 AM Philippe Mathieu-Daudé 
> > > >>  wrote:
> > > >>>
> > > >>> We always miswrote the Fuloong machine... Fix its name.
> > > >>> Add an machine alias to the previous name for backward
> > > >>> compatibility.
> > > >>>
> > > >>> Suggested-by: Aleksandar Markovic 
> > > >>> Signed-off-by: Philippe Mathieu-Daudé 
> > > >>> ---
> > > >>>   docs/system/target-mips.rst  |  2 +-
> > > >>>   default-configs/mips64el-softmmu.mak |  2 +-
> > > >>>   hw/isa/vt82c686.c|  2 +-
> > > >>>   hw/mips/{mips_fulong2e.c => fuloong2e.c} | 46 
> > > >>> 
> > > >> Use mips_fuloong2e.c instead of fuloong2e.c? Other machine file names
> > > >> also have a "mips_" prefix.
> > > >>
> > > >
> > > > I would leave mips_ prefix for Fuloong, and actually add it to Boston
> > > > source file, so that we are finally consistent across all MIPS
> > > > machines.
> > > >
> > > > What do you think?
> > >
> > > These names were used years ago when all hardware was in the same hw/
> > > directory, not sorted per target. Now new machines don't use the target
> > > as prefix name. I'd clean the other way around, and dropping the 'mips_'
> > > prefix. The positive side is we can 5 more characters to better describe
> > > a patch while limited by the 72 chars in the subject :)
> >
> > All having the prefix, or all dropping the prefix, are both good for
> > me, just keep consistency.
> >
> > Huacai
> >
>
> Philippe, Huacai,
>
> Prefix or not, I have mixed feelings. I had consistency more in mind
> than prefix.
>
> So it seems the prevailing opinion is slightly on the side of dropping
> prefix "mips_".
>
> Philippe, if it is not too difficult, could you perhaps make dropping
> that prefix for all source file names in hw/mips a part of the this
> series (not to complicate situation with a separate series) in its
> follow-up version (but perhaps keep that change(s) in separate
> patch(es))?
>

Conveniently enough, most of involved files do not have checkpatch
warnings and errors:

$ ../../scripts/checkpatch.pl --strict -f ./mips_fulong2e.c
total: 0 errors, 0 warnings, 404 lines checked

./mips_fulong2e.c has no obvious style problems and is ready for submission.
$ ../../scripts/checkpatch.pl --strict -f ./mips_malta.c
ERROR: if this code is redundant consider removing it
#430: FILE: ./mips_malta.c:430:
+#if 0

ERROR: if this code is redundant consider removing it
#518: FILE: ./mips_malta.c:518:
+#if 0

total: 2 errors, 0 warnings, 1458 lines checked

./mips_malta.c has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
$ ../../scripts/checkpatch.pl --strict -f ./mips_mipssim.c
total: 0 errors, 0 warnings, 246 lines checked

./mips_mipssim.c has no obvious style problems and is ready for submission.
$ ../../scripts/checkpatch.pl --strict -f ./mips_r4k.c
total: 0 errors, 0 warnings, 318 lines checked

./mips_r4k.c has no obvious style problems and is ready for submission.


Maybe we should also finally get rid of these segments in mips_malta.c:

#if 0
printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n",
   addr);
#endif

and

#if 0
printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n",
   addr);
#endif

possibly replacing them with some logging?

Philippe?

Thanks,
Aleksandar



> Sincerely,
> Aleksandar
>
> > >
> > > >
> > > > Aleksandar
> > > >
> > > >> Huacai
> > > >>>   hw/pci-host/bonito.c |  8 ++---
> > > >>>   tests/qtest/endianness-test.c|  2 +-
> > > >>>   MAINTAINERS  |  4 +--
> > > >>>   hw/mips/Kconfig  |  2 +-
> > > >>>   hw/mips/Makefile.objs|  2 +-
> > > >>>   9 files changed, 36 insertions(+), 34 deletions(-)
> > > >>>   rename hw/mips/{mips_fulong2e.c => fuloong2e.c} (91%)
> > > >>>
> > > >>> diff --git a/docs/system/target-mips.rst b/docs/system/target-mips.rst
> > > >>> index 2736fd0509..cd2a931edf 100644
> > > >>> --- a/docs/system/target-mips.rst
> > > >>> +++ b/docs/system/target-mips.rst
> > > >>> @@ -74,7 +74,7 @@ The MIPS Magnum R4000 emulation supports:
> > > >>>
> > > >>>   -  G364 framebuffer
> > > >>>
> > > >>> -The Fulong 2E emulation supports:
> > > >>> +The Fuloong 2E emulation supports:
> > > >>>
> > > >>>   -  Loongson 2E CPU
> > > >>>
> > > >>> diff --git a/default-configs/mips64el-softmmu.mak 
> > > >>> b/default-configs/mips64el-softmmu.mak
> > > >>> index 8b0c9b

[PATCH] scripts/tracetool: Update maintainer email address

[Philippe Mathieu-Daudé]

There is an effort in progress to generate a QEMU Python
package. As I'm not sure this old email is still valid,
update it to not produce package with broken maintainer
email.

Patch created mechanically by running:

 $ sed -i 's,\(__email__ *= 
"\)stefa...@linux.vnet.ibm.com",\1stefa...@redhat.com",' \
 $(git grep -l 'email.*stefa...@linux.vnet.ibm.com')

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/tracetool.py | 2 +-
 scripts/tracetool/__init__.py| 2 +-
 scripts/tracetool/backend/__init__.py| 2 +-
 scripts/tracetool/backend/dtrace.py  | 2 +-
 scripts/tracetool/backend/log.py | 2 +-
 scripts/tracetool/backend/simple.py  | 2 +-
 scripts/tracetool/backend/ust.py | 2 +-
 scripts/tracetool/format/__init__.py | 2 +-
 scripts/tracetool/format/c.py| 2 +-
 scripts/tracetool/format/d.py| 2 +-
 scripts/tracetool/format/h.py| 2 +-
 scripts/tracetool/format/stap.py | 2 +-
 scripts/tracetool/format/tcg_h.py| 2 +-
 scripts/tracetool/format/tcg_helper_c.py | 2 +-
 scripts/tracetool/format/tcg_helper_h.py | 2 +-
 scripts/tracetool/format/tcg_helper_wrapper_h.py | 2 +-
 scripts/tracetool/transform.py   | 2 +-
 scripts/tracetool/vcpu.py| 2 +-
 18 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/scripts/tracetool.py b/scripts/tracetool.py
index 264cc9eecc..31146242b7 100755
--- a/scripts/tracetool.py
+++ b/scripts/tracetool.py
@@ -10,7 +10,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 import sys
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 13d29f1e42..3ccfa1e116 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -9,7 +9,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 import re
diff --git a/scripts/tracetool/backend/__init__.py 
b/scripts/tracetool/backend/__init__.py
index 54cab2c4de..7bfcc86cc5 100644
--- a/scripts/tracetool/backend/__init__.py
+++ b/scripts/tracetool/backend/__init__.py
@@ -50,7 +50,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 import os
diff --git a/scripts/tracetool/backend/dtrace.py 
b/scripts/tracetool/backend/dtrace.py
index 638990db79..5711892ba0 100644
--- a/scripts/tracetool/backend/dtrace.py
+++ b/scripts/tracetool/backend/dtrace.py
@@ -9,7 +9,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 from tracetool import out
diff --git a/scripts/tracetool/backend/log.py b/scripts/tracetool/backend/log.py
index 23b274c0fd..877222bbe9 100644
--- a/scripts/tracetool/backend/log.py
+++ b/scripts/tracetool/backend/log.py
@@ -9,7 +9,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 from tracetool import out
diff --git a/scripts/tracetool/backend/simple.py 
b/scripts/tracetool/backend/simple.py
index b650c262b5..a74d61fcd6 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -9,7 +9,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 from tracetool import out
diff --git a/scripts/tracetool/backend/ust.py b/scripts/tracetool/backend/ust.py
index a772a3b53b..6c0a5f8d68 100644
--- a/scripts/tracetool/backend/ust.py
+++ b/scripts/tracetool/backend/ust.py
@@ -9,7 +9,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 from tracetool import out
diff --git a/scripts/tracetool/format/__init__.py 
b/scripts/tracetool/format/__init__.py
index aba2f7a441..2dc46f3dd9 100644
--- a/scripts/tracetool/format/__init__.py
+++ b/scripts/tracetool/format/__init__.py
@@ -32,7 +32,7 @@
 __license__= "GPL version 2 or (at your option) any later version"
 
 __maintainer__ = "Stefan Hajnoczi"
-__email__  = "stefa...@linux.vnet.ibm.com"
+__email__  = "stefa...@redhat.com"
 
 
 import os
diff --git a/scripts/tracetool/format/c.py

RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Zhang, Chen]

> -Original Message-
> From: Lukas Straub 
> Sent: Friday, May 8, 2020 3:56 PM
> To: Zhang, Chen 
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
> 
> Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
> 
> On Fri, 8 May 2020 06:28:45 +
> "Zhang, Chen"  wrote:
> 
> > > -Original Message-
> > > From: Lukas Straub 
> > > Sent: Friday, May 8, 2020 2:08 PM
> > > To: Zhang, Chen 
> > > Cc: qemu-devel ; Li Zhijian
> > > ; Jason Wang ; Marc-
> > > André Lureau ; Paolo Bonzini
> > > 
> > > Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > > compare_chr_send
> > >
> > > On Fri, 8 May 2020 02:19:00 +
> > > "Zhang, Chen"  wrote:
> > > > > > No need to init the notify_sendco each time, because the
> > > > > > notify dev just
> > > > > an optional parameter.
> > > > > > You can use the if (s->notify_dev) here. Just Xen use the
> > > chr_notify_dev.
> > > > >
> > > > > Ok, I will change that and the code below in the next version.
> > > > >
> > > > > > Overall, make the chr_send job to coroutine is a good idea. It
> > > > > > looks good
> > > > > for me.
> > > > > > And your patch inspired me, it looks we can re-use the
> > > > > > compare_chr_send
> > > > > code on filter mirror/redirector too.
> > > > >
> > > > > I already have patch for that, but I don't think it is a good
> > > > > idea, because the guest then can send packets faster than
> > > > > colo-compare can process. This leads bufferbloat and the
> performance drops in my tests:
> > > > > Client-to-server tcp:
> > > > > without patch: ~66 Mbit/s
> > > > > with patch: ~59 Mbit/s
> > > > > Server-to-client tcp:
> > > > > without patch: ~702 Kbit/s
> > > > > with patch: ~328 Kbit/s
> > > >
> > > > Oh, a big performance drop, is that caused by memcpy/zero_copy
> parts ?
> > > >
> > > > Thanks
> > > > Zhang Chen
> > >
> > > No, there is no memcpy overhead with this patch, see below.
> >
> > I means for the filter mirror/redirector parts why coroutine will lead huge
> performance drop?
> 
> It's because having a additional buffer before the network bottleneck (colo-
> compare in our case) confuses TCP's congestion-control:
> TCP will speed up the data transfer until packets start to drop (or the
> network interface is blocked). This feedback has to be quick so TCP can select
> a suitable transfer speed. But with the patch, the guest will fill the buffer 
> as
> fast as it can (it does not "see" the slow bandwidth of colo-compare behind
> the buffer) until it it hits against the TCP congestion window. At this point 
> TCP
> drastically reduces its transfer speed and it stays low because the full 
> buffer
> delays the packets so it doesn't receive ACK's so it can't speed up the
> transfer again. Until the buffer is empty again (can take up to a second in my
> tests). Then this cycle repeats.

Make sense!
After fix above issue:
Reviewed-by: Zhang Chen 

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > ---
> > >  net/filter-mirror.c | 142
> > > +---
> > >  1 file changed, 106 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > d83e815545..6bcd317502 100644
> > > --- a/net/filter-mirror.c
> > > +++ b/net/filter-mirror.c
> > > @@ -20,6 +20,8 @@
> > >  #include "chardev/char-fe.h"
> > >  #include "qemu/iov.h"
> > >  #include "qemu/sockets.h"
> > > +#include "block/aio-wait.h"
> > > +#include "qemu/coroutine.h"
> > >
> > >  #define FILTER_MIRROR(obj) \
> > >  OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) @@ -31,6
> > > +33,18 @@  #define TYPE_FILTER_REDIRECTOR "filter-redirector"
> > >  #define REDIRECTOR_MAX_LEN NET_BUFSIZE
> > >
> > > +typedef struct SendCo {
> > > +Coroutine *co;
> > > +GQueue send_list;
> > > +bool done;
> > > +int ret;
> > > +} SendCo;
> > > +
> > > +typedef struct SendEntry {
> > > +ssize_t size;
> > > +uint8_t buf[];
> > > +} SendEntry;
> > > +
> > >  typedef struct MirrorState {
> > >  NetFilterState parent_obj;
> > >  char *indev;
> > > @@ -38,59 +52,101 @@ typedef struct MirrorState {
> > >  CharBackend chr_in;
> > >  CharBackend chr_out;
> > >  SocketReadState rs;
> > > +SendCo sendco;
> > >  bool vnet_hdr;
> > >  } MirrorState;
> > >
> > > -static int filter_send(MirrorState *s,
> > > -   const struct iovec *iov,
> > > -   int iovcnt)
> > > +static void coroutine_fn _filter_send(void *opaque)
> > >  {
> > > +MirrorState *s = opaque;
> > > +SendCo *sendco = &s->sendco;
> > >  NetFilterState *nf = NETFILTER(s);
> > >  int ret = 0;
> > > -ssize_t size = 0;
> > > -uint32_t len = 0;
> > > -char *buf;
> > > -
> > > -size = iov_size(iov, iovcnt);
> > > -if (!size) {
> > > -return 0;
> > > -}
> > >
> > > -len 

RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize

[Zhang, Chen]

> -Original Message-
> From: Lukas Straub 
> Sent: Friday, May 8, 2020 12:09 AM
> To: Zhang, Chen 
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
> 
> Subject: Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> complete and finalize
> 
> On Thu, 7 May 2020 13:26:11 +
> "Zhang, Chen"  wrote:
> 
> > > -Original Message-
> > > From: Lukas Straub 
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel 
> > > Cc: Zhang, Chen ; Li Zhijian
> > > ; Jason Wang ; Marc-
> > > André Lureau ; Paolo Bonzini
> > > 
> > > Subject: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> > > complete and finalize
> > >
> > > In colo_compare_complete, insert CompareState into net_compares
> only
> > > after everything has been initialized.
> > > In colo_compare_finalize, remove CompareState from net_compares
> > > before anything is deinitialized.
> >
> > S/deinitialized/finalized
> >
> > It looks no dependences on each step on initialization and finalization.
> > Do you means we just need add/remove each colo-compare module at last
> in logic?
> 
> Yes. While I didn't see any crashes here, there is the possibility that if 
> colo-
> compare is removed during checkpoint, the destroyed event_bh is called
> from colo_notify_compares_event. Same with colo_compare_complete
> (very unlikely) if colo-compare is created while colo is running,
> colo_notify_compares_event may call the uninitialized event_bh.
> 

Reviewed-by: Zhang Chen 

Thanks
Zhang Chen

> Regards,
> Lukas Straub
> 
> > Or current code have some issue?
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Signed-off-by: Lukas Straub 
> > > ---
> > >  net/colo-compare.c | 45
> > > +++--
> > >  1 file changed, 23 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > c7572d75e9..6f80bcece6 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -1283,15 +1283,6 @@ static void
> > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > > s->vnet_hdr);
> > >  }
> > >
> > > -qemu_mutex_lock(&colo_compare_mutex);
> > > -if (!colo_compare_active) {
> > > -qemu_mutex_init(&event_mtx);
> > > -qemu_cond_init(&event_complete_cond);
> > > -colo_compare_active = true;
> > > -}
> > > -QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > -qemu_mutex_unlock(&colo_compare_mutex);
> > > -
> > >  s->out_sendco.s = s;
> > >  s->out_sendco.chr = &s->chr_out;
> > >  s->out_sendco.notify_remote_frame = false; @@ -1312,6 +1303,16
> > > @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > >
> > > connection_destroy);
> > >
> > >  colo_compare_iothread(s);
> > > +
> > > +qemu_mutex_lock(&colo_compare_mutex);
> > > +if (!colo_compare_active) {
> > > +qemu_mutex_init(&event_mtx);
> > > +qemu_cond_init(&event_complete_cond);
> > > +colo_compare_active = true;
> > > +}
> > > +QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > +qemu_mutex_unlock(&colo_compare_mutex);
> > > +
> > >  return;
> > >  }
> > >
> > > @@ -1384,19 +1385,6 @@ static void colo_compare_finalize(Object *obj)
> > >  CompareState *s = COLO_COMPARE(obj);
> > >  CompareState *tmp = NULL;
> > >
> > > -qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > -qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > -qemu_chr_fe_deinit(&s->chr_out, false);
> > > -if (s->notify_dev) {
> > > -qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > -}
> > > -
> > > -if (s->iothread) {
> > > -colo_compare_timer_del(s);
> > > -}
> > > -
> > > -qemu_bh_delete(s->event_bh);
> > > -
> > >  qemu_mutex_lock(&colo_compare_mutex);
> > >  QTAILQ_FOREACH(tmp, &net_compares, next) {
> > >  if (tmp == s) {
> > > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> > >  }
> > >  qemu_mutex_unlock(&colo_compare_mutex);
> > >
> > > +qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > +qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > +qemu_chr_fe_deinit(&s->chr_out, false);
> > > +if (s->notify_dev) {
> > > +qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > +}
> > > +
> > > +if (s->iothread) {
> > > +colo_compare_timer_del(s);
> > > +}
> > > +
> > > +qemu_bh_delete(s->event_bh);
> > > +
> > >  AioContext *ctx = iothread_get_aio_context(s->iothread);
> > >  aio_context_acquire(ctx);
> > >  AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > > --
> > > 2.20.1

RE: [EXT] [PATCH v2 2/5] virtio-iommu: Implement RESV_MEM probe request

[Bharat Bhushan]

Hi Eric,

> -Original Message-
> From: Auger Eric 
> Sent: Monday, May 11, 2020 12:26 PM
> To: Bharat Bhushan ; eric.auger@gmail.com;
> qemu-devel@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org;
> m...@redhat.com; jean-phili...@linaro.org; pet...@redhat.com;
> arm...@redhat.com; pbonz...@redhat.com
> Subject: Re: [EXT] [PATCH v2 2/5] virtio-iommu: Implement RESV_MEM probe
> request
> 
> Hi Bharat,
> On 5/11/20 8:38 AM, Bharat Bhushan wrote:
> > Hi Eric,
> >
> >> -Original Message-
> >> From: Eric Auger 
> >> Sent: Friday, May 8, 2020 11:01 PM
> >> To: eric.auger@gmail.com; eric.au...@redhat.com;
> >> qemu-devel@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org;
> >> m...@redhat.com; jean- phili...@linaro.org; Bharat Bhushan
> >> ; pet...@redhat.com; arm...@redhat.com;
> >> pbonz...@redhat.com
> >> Subject: [EXT] [PATCH v2 2/5] virtio-iommu: Implement RESV_MEM probe
> >> request
> >>
> >> External Email
> >>
> >> -
> >> - This patch implements the PROBE request. At the moment, only THE
> >> RESV_MEM property is handled. The first goal is to report iommu wide
> >> reserved regions such as the MSI regions set by the machine code. On
> >> x86 this will be the IOAPIC MSI region,
> >> [0xFEE0 - 0xFEEF], on ARM this may be the ITS doorbell.
> >>
> >> In the future we may introduce per device reserved regions.
> >> This will be useful when protecting host assigned devices which may
> >> expose their own reserved regions
> >>
> >> Signed-off-by: Eric Auger 
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - move the unlock back to the same place
> >> - remove the push label and factorize the code after the out label
> >> - fix a bunch of cpu_to_leX according to the latest spec revision
> >> - do not remove sizeof(last) from free space
> >> - check the ep exists
> >> ---
> >>  include/hw/virtio/virtio-iommu.h |  2 +
> >>  hw/virtio/virtio-iommu.c | 94 ++--
> >>  hw/virtio/trace-events   |  1 +
> >>  3 files changed, 93 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/hw/virtio/virtio-iommu.h
> >> b/include/hw/virtio/virtio-iommu.h
> >> index e653004d7c..49eb105cd8 100644
> >> --- a/include/hw/virtio/virtio-iommu.h
> >> +++ b/include/hw/virtio/virtio-iommu.h
> >> @@ -53,6 +53,8 @@ typedef struct VirtIOIOMMU {
> >>  GHashTable *as_by_busptr;
> >>  IOMMUPciBus *iommu_pcibus_by_bus_num[PCI_BUS_MAX];
> >>  PCIBus *primary_bus;
> >> +ReservedRegion *reserved_regions;
> >> +uint32_t nb_reserved_regions;
> >>  GTree *domains;
> >>  QemuMutex mutex;
> >>  GTree *endpoints;
> >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> >> index
> >> 22ba8848c2..35d772e021 100644
> >> --- a/hw/virtio/virtio-iommu.c
> >> +++ b/hw/virtio/virtio-iommu.c
> >> @@ -38,6 +38,7 @@
> >>
> >>  /* Max size */
> >>  #define VIOMMU_DEFAULT_QUEUE_SIZE 256
> >> +#define VIOMMU_PROBE_SIZE 512
> >>
> >>  typedef struct VirtIOIOMMUDomain {
> >>  uint32_t id;
> >> @@ -378,6 +379,65 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
> >>  return ret;
> >>  }
> >>
> >> +static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t 
> >> ep,
> >> +   uint8_t *buf, size_t
> >> +free) {
> >> +struct virtio_iommu_probe_resv_mem prop = {};
> >> +size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
> >> +int i;
> >> +
> >> +total = size * s->nb_reserved_regions;
> >> +
> >> +if (total > free) {
> >> +return -ENOSPC;
> >> +}
> >> +
> >> +for (i = 0; i < s->nb_reserved_regions; i++) {
> >> +prop.head.type = cpu_to_le16(VIRTIO_IOMMU_PROBE_T_RESV_MEM);
> >> +prop.head.length = cpu_to_le16(length);
> >> +prop.subtype = s->reserved_regions[i].type;
> >> +prop.start = cpu_to_le64(s->reserved_regions[i].low);
> >> +prop.end = cpu_to_le64(s->reserved_regions[i].high);
> >> +
> >> +memcpy(buf, &prop, size);
> >> +
> >> +trace_virtio_iommu_fill_resv_property(ep, prop.subtype,
> >> +  prop.start, prop.end);
> >> +buf += size;
> >> +}
> >> +return total;
> >> +}
> >> +
> >> +/**
> >> + * virtio_iommu_probe - Fill the probe request buffer with
> >> + * the properties the device is able to return and add a NONE
> >> + * property at the end.
> >> + */
> >> +static int virtio_iommu_probe(VirtIOIOMMU *s,
> >> +  struct virtio_iommu_req_probe *req,
> >> +  uint8_t *buf) {
> >> +uint32_t ep_id = le32_to_cpu(req->endpoint);
> >> +size_t free = VIOMMU_PROBE_SIZE;
> >> +ssize_t count;
> >> +
> >> +if (!virtio_iommu_mr(s, ep_id)) {
> >> +return VIRTIO_IOMMU_S_NOENT;
> >> +}
> >> +
> >> +count = virtio_iommu_fill_resv_mem_prop(s, ep_id, buf, free);
> >> +if (co

[Bug 1877716] Re: Win10 guest unusable after a few minutes

[zkrx]

** Summary changed:

- Win10 guest unsuable after a few minutes
+ Win10 guest unusable after a few minutes

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

Title:
  Win10 guest unusable after a few minutes

Status in QEMU:
  New

Bug description:
  On Arch Linux, the recent qemu package update seems to misbehave on
  some systems. In my case, my Windows 10 guest runs fine for around 5
  minutes and then start to get really sluggish, even unresponsive. It
  needs to be forced off. I could reproduce this on a minimal VM with no
  passthrough, although my current testing setup involves an nvme pcie
  passthrough.

  I bisected it to the following commit which rapidly starts to run sluggishly 
on my setup:
  https://github.com/qemu/qemu/commit/73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf

  I've ran the previous commit (
  https://github.com/qemu/qemu/commit/b321051cf48ccc2d3d832af111d688f2282f089b
  ) for the entire night without an issue so far.

  I believe this might be a duplicate of
  https://bugs.launchpad.net/qemu/+bug/1873032 , although I'm not sure.

  Linux cc 5.6.10-arch1-1 #1 SMP PREEMPT Sat, 02 May 2020 19:11:54 + x86_64 
GNU/Linux
  AMD Ryzen 7 2700X Eight-Core Processor

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

[Bug 1860742] Re: xv6 Bootloop

[Manavjeet Singh]

** Also affects: qemu (Ubuntu)
   Importance: Undecided
   Status: New

** No longer affects: qemu (Ubuntu)

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

Title:
  xv6 Bootloop

Status in QEMU:
  New

Bug description:
  Qemu Version: 4.2.0

  Launch command:
  qemu-system-x86_64 -nographic -drive 
file=fs.img,index=1,media=disk,format=raw -drive 
file=xv6.img,index=0,media=disk,format=raw -smp 2 -m 512

  How to reproduce?
  1.)  Use/install latest release of qemu (4.2.0 at time of writing)

  2.)  Download, build, and run xv6 (a simple os designed for learning
  operating systems fundamentals)

  cd /tmp
  git clone https://github.com/mit-pdos/xv6-public.git
  cd xv6-public
  make qemu-nox

  3.)  Qemu should now bootloop (seem to try to boot but then just
  repeat). This is what it looks like below before it repeats:

  SeaBIOS (version ?-20191223_100556-anatol)

  iPXE (http://ipxe.org) 00:03.0 CA00 PCI2.10 PnP PMM+1FF92A50+1FEF2A50
  CA00

  Booting from Hard Disk..

  Host: Arch Linux - Kernel version: 5.4.13
  Guest: xv6 (https://github.com/mit-pdos/xv6-public)

  Suspicion:

  When I was using qemu 2.11.1 inside an ubuntu docker container, the
  xv6 os booted with no problem. I am thinking that something changed
  between Qemu 2.11.1 and Qemu 4.2.0 which is now causing boot problems.

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

Re: [PATCH 2/4] device-core: use RCU for list of childs of a bus

[Maxim Levitsky]

On Mon, 2020-05-04 at 11:41 +0100, Stefan Hajnoczi wrote:
> On Thu, Apr 16, 2020 at 11:36:22PM +0300, Maxim Levitsky wrote:
> > @@ -90,9 +92,13 @@ static void bus_reset_child_foreach(Object *obj, 
> > ResettableChildCallback cb,
> >  BusState *bus = BUS(obj);
> >  BusChild *kid;
> >  
> > -QTAILQ_FOREACH(kid, &bus->children, sibling) {
> > +rcu_read_lock();
> > +
> > +QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
> >  cb(OBJECT(kid->child), opaque, type);
> >  }
> > +
> > +rcu_read_unlock();
> >  }
> >  
> >  static void qbus_realize(BusState *bus, DeviceState *parent, const char 
> > *name)
> > @@ -138,10 +144,15 @@ static void bus_unparent(Object *obj)
> >  /* Only the main system bus has no parent, and that bus is never freed 
> > */
> >  assert(bus->parent);
> >  
> > -while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
> > +rcu_read_lock();
> > +
> > +while ((kid = QTAILQ_FIRST_RCU(&bus->children)) != NULL) {
> >  DeviceState *dev = kid->child;
> >  object_unparent(OBJECT(dev));
> >  }
> > +
> > +rcu_read_unlock();
> 
> rcu_read_lock() is called but this looks like a list write operation.
> If I understand correctly bus->children list writes can only be called
> with the QEMU global mutex and therefore rcu_read_lock() is not required
> here?

This is indeed write operation. Paulo helped me to finally understand
what RCU guarantees are, so now I understand.

About write locking here (which I also understand now that I need for RCU),
this is very good question if that can race as well:

It looks like qdev_unplug is what does the device removal, and it first
calls the hotplug handler which is supposed to unrealize the device,
in addition to whatever hot unplug actions are needed (like informing the 
guest),
and then it does object_unparent which removes the device from the bus.
Therefore as long as the .realized store is atomic and with proper barriers,
the scsi IO thread might be able to see the device but it will be unplugged by 
then.


There are plenty of object_unparent calls through the code base and I can only 
hope
that these are done with big qemu lock held.


> 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 85f062def7..f0c87e582e 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -50,26 +50,37 @@ const VMStateDescription *qdev_get_vmsd(DeviceState 
> > *dev)
> >  return dc->vmsd;
> >  }
> >  
> > +static void bus_free_bus_child(BusChild *kid)
> > +{
> > +object_unref(OBJECT(kid->child));
> 
> Users like scsi_device_find() do not take a refcount on the child.  If
> the device is removed then bus_free_bus_child may call object_unref()
> while another thread is still accessing the child.

I agree, however this is existing bug - bus_remove_child was dropping this
reference immediatly and I delayed it to RCU callback it now sets.
So I didn't made the situation worse.


> 
> Maybe I'm missing something that prevents this scenario?
> 
> If not, then another patch is necessary first that introduces stricter
> refcount discipline across the codebase. This applies both to users who
> directly access bus->children as well as to those who call walk() and
> stash child pointers in their callback function.

In scsi_device_find, as long as RCU read lock is held, no RCU reclaim should 
happen,
thus this code shouldn't have the child disapper under it.
However once scsi_device_find returns, indeed this can happen,

so scsi_device_find should indeed take a reference to the scsi device and the 
caller should
drop it when not needed. That should be done in a separate patch, and it
might open yet another can of worms.
While at it, it should be renamed scsi_device_get()
Maybe keep both scsi_device_find and scsi_device_get(), and let the legacy 
drivers
continue using the former one, while make virtio-scsi use the later? 


> 
> > +g_free(kid);
> > +}
> > +
> >  static void bus_remove_child(BusState *bus, DeviceState *child)
> >  {
> >  BusChild *kid;
> >  
> > -QTAILQ_FOREACH(kid, &bus->children, sibling) {
> > +rcu_read_lock();
> 
> List write under rcu_read_lock().
I removed the RCU read lock here, under assumption that
bus_remove_child will be called with BQL held.
I kept the _RCU version of the list removal, under assumption that
writer still needs it to avoid race vs readers.


> 
> > @@ -82,7 +93,9 @@ static void bus_add_child(BusState *bus, DeviceState 
> > *child)
> >  kid->child = child;
> >  object_ref(OBJECT(kid->child));
> >  
> > -QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
> > +rcu_read_lock();
> > +QTAILQ_INSERT_HEAD_RCU(&bus->children, kid, sibling);
> > +rcu_read_unlock();
> 
> List write under rcu_read_lock().
Same as above.

> 
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index 472bbd233b..b0f4a35f81 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -367,12 +367,16 @@ static int virt

Re: [EXT] [PATCH v2 2/5] virtio-iommu: Implement RESV_MEM probe request

[Auger Eric]

Hi Bharat,

On 5/11/20 10:42 AM, Bharat Bhushan wrote:
> Hi Eric,
> 
>> -Original Message-
>> From: Auger Eric 
>> Sent: Monday, May 11, 2020 12:26 PM
>> To: Bharat Bhushan ; eric.auger@gmail.com;
>> qemu-devel@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org;
>> m...@redhat.com; jean-phili...@linaro.org; pet...@redhat.com;
>> arm...@redhat.com; pbonz...@redhat.com
>> Subject: Re: [EXT] [PATCH v2 2/5] virtio-iommu: Implement RESV_MEM probe
>> request
>>
>> Hi Bharat,
>> On 5/11/20 8:38 AM, Bharat Bhushan wrote:
>>> Hi Eric,
>>>
 -Original Message-
 From: Eric Auger 
 Sent: Friday, May 8, 2020 11:01 PM
 To: eric.auger@gmail.com; eric.au...@redhat.com;
 qemu-devel@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org;
 m...@redhat.com; jean- phili...@linaro.org; Bharat Bhushan
 ; pet...@redhat.com; arm...@redhat.com;
 pbonz...@redhat.com
 Subject: [EXT] [PATCH v2 2/5] virtio-iommu: Implement RESV_MEM probe
 request

 External Email

 -
 - This patch implements the PROBE request. At the moment, only THE
 RESV_MEM property is handled. The first goal is to report iommu wide
 reserved regions such as the MSI regions set by the machine code. On
 x86 this will be the IOAPIC MSI region,
 [0xFEE0 - 0xFEEF], on ARM this may be the ITS doorbell.

 In the future we may introduce per device reserved regions.
 This will be useful when protecting host assigned devices which may
 expose their own reserved regions

 Signed-off-by: Eric Auger 

 ---

 v1 -> v2:
 - move the unlock back to the same place
 - remove the push label and factorize the code after the out label
 - fix a bunch of cpu_to_leX according to the latest spec revision
 - do not remove sizeof(last) from free space
 - check the ep exists
 ---
  include/hw/virtio/virtio-iommu.h |  2 +
  hw/virtio/virtio-iommu.c | 94 ++--
  hw/virtio/trace-events   |  1 +
  3 files changed, 93 insertions(+), 4 deletions(-)

 diff --git a/include/hw/virtio/virtio-iommu.h
 b/include/hw/virtio/virtio-iommu.h
 index e653004d7c..49eb105cd8 100644
 --- a/include/hw/virtio/virtio-iommu.h
 +++ b/include/hw/virtio/virtio-iommu.h
 @@ -53,6 +53,8 @@ typedef struct VirtIOIOMMU {
  GHashTable *as_by_busptr;
  IOMMUPciBus *iommu_pcibus_by_bus_num[PCI_BUS_MAX];
  PCIBus *primary_bus;
 +ReservedRegion *reserved_regions;
 +uint32_t nb_reserved_regions;
  GTree *domains;
  QemuMutex mutex;
  GTree *endpoints;
 diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
 index
 22ba8848c2..35d772e021 100644
 --- a/hw/virtio/virtio-iommu.c
 +++ b/hw/virtio/virtio-iommu.c
 @@ -38,6 +38,7 @@

  /* Max size */
  #define VIOMMU_DEFAULT_QUEUE_SIZE 256
 +#define VIOMMU_PROBE_SIZE 512

  typedef struct VirtIOIOMMUDomain {
  uint32_t id;
 @@ -378,6 +379,65 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
  return ret;
  }

 +static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t 
 ep,
 +   uint8_t *buf, size_t
 +free) {
 +struct virtio_iommu_probe_resv_mem prop = {};
 +size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
 +int i;
 +
 +total = size * s->nb_reserved_regions;
 +
 +if (total > free) {
 +return -ENOSPC;
 +}
 +
 +for (i = 0; i < s->nb_reserved_regions; i++) {
 +prop.head.type = cpu_to_le16(VIRTIO_IOMMU_PROBE_T_RESV_MEM);
 +prop.head.length = cpu_to_le16(length);
 +prop.subtype = s->reserved_regions[i].type;
 +prop.start = cpu_to_le64(s->reserved_regions[i].low);
 +prop.end = cpu_to_le64(s->reserved_regions[i].high);
 +
 +memcpy(buf, &prop, size);
 +
 +trace_virtio_iommu_fill_resv_property(ep, prop.subtype,
 +  prop.start, prop.end);
 +buf += size;
 +}
 +return total;
 +}
 +
 +/**
 + * virtio_iommu_probe - Fill the probe request buffer with
 + * the properties the device is able to return and add a NONE
 + * property at the end.
 + */
 +static int virtio_iommu_probe(VirtIOIOMMU *s,
 +  struct virtio_iommu_req_probe *req,
 +  uint8_t *buf) {
 +uint32_t ep_id = le32_to_cpu(req->endpoint);
 +size_t free = VIOMMU_PROBE_SIZE;
 +ssize_t count;
 +
 +if (!virtio_iommu_mr(s, ep_id)) {
 +return VIRTIO_IOMMU_S_NOENT;
 +}
 +
 +count = 

RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Zhang, Chen]

> -Original Message-
> From: Lukas Straub 
> Sent: Saturday, May 9, 2020 8:21 PM
> To: Zhang, Chen 
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
> ; Dr . David Alan Gilbert 
> Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> colo-compare is active
> 
> On Fri, 8 May 2020 06:50:39 +
> "Zhang, Chen"  wrote:
> 
> > > -Original Message-
> > > From: Lukas Straub 
> > > Sent: Friday, May 8, 2020 2:11 PM
> > > To: Zhang, Chen 
> > > Cc: qemu-devel ; Li Zhijian
> > > ; Jason Wang ; Marc-
> > > André Lureau ; Paolo Bonzini
> > > 
> > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > that colo-compare is active
> > >
> > > On Fri, 8 May 2020 02:26:21 +
> > > "Zhang, Chen"  wrote:
> > >
> > > > > -Original Message-
> > > > > From: Lukas Straub 
> > > > > Sent: Thursday, May 7, 2020 11:54 PM
> > > > > To: Zhang, Chen 
> > > > > Cc: qemu-devel ; Li Zhijian
> > > > > ; Jason Wang ;
> > > > > Marc- André Lureau ; Paolo Bonzini
> > > > > 
> > > > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c:
> > > > > Check that colo-compare is active
> > > > >
> > > > > On Thu, 7 May 2020 11:38:04 + "Zhang, Chen"
> > > > >  wrote:
> > > > >
> > > > > > > -Original Message-
> > > > > > > From: Lukas Straub 
> > > > > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > > > > To: qemu-devel 
> > > > > > > Cc: Zhang, Chen ; Li Zhijian
> > > > > > > ; Jason Wang
> > > > > > > ;
> > > > > > > Marc- André Lureau ; Paolo
> > > > > > > Bonzini 
> > > > > > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c:
> > > > > > > Check that
> > > > > > > colo- compare is active
> > > > > > >
> > > > > > > If the colo-compare object is removed before failover and a
> > > > > > > checkpoint happens, qemu crashes because it tries to lock
> > > > > > > the destroyed event_mtx in colo_notify_compares_event.
> > > > > > >
> > > > > > > Fix this by checking if everything is initialized by
> > > > > > > introducing a new variable colo_compare_active which is
> > > > > > > protected by a new mutex colo_compare_mutex. The new mutex
> > > > > > > also protects against concurrent access of the net_compares
> > > > > > > list and makes sure that colo_notify_compares_event isn't
> > > > > > > active while we destroy event_mtx and event_complete_cond.
> > > > > > >
> > > > > > > With this it also is again possible to use colo without
> > > > > > > colo-compare (periodic
> > > > > > > mode) and to use multiple colo-compare for multiple network
> > > interfaces.
> > > > > > >
> > > > > >
> > > > > > Hi Lukas,
> > > > > >
> > > > > > For this case I think we don't need to touch vl.c code, we can
> > > > > > solve this
> > > > > issue from another perspective:
> > > > > > How to remove colo-compare?
> > > > > > User will use qemu-monitor or QMP command to disable an
> > > > > > object, so we just need return operation failed When user try
> > > > > > to remove colo-compare
> > > > > object while COLO is running.
> > > > >
> > > > > Yeah, but that still leaves the other problem that colo can't be
> > > > > used without colo-compare (qemu crashes then).
> > > >
> > > > Yes, the COLO-compare is necessary module in COLO original design.
> > > > At most cases, user need it do dynamic sync.
> > > > For rare cases, maybe we can add a new colo-compare parameter to
> > > bypass all the network workload.
> > >
> > > I think such an parameter would only be a workaround instead of a
> > > real solution like this patch.
> >
> > The root problem is why COLO-compare is necessary.
> > Yes, maybe someone want to use pure periodic synchronization mode, But
> > it means it will lost all guest network support(without colo-compare/filter-
> mirror/filter-redirector/filter-rewriter).
> > The secondary guest just a solid backup for the primary guest, when
> > occur failover the new build stateful connection (like TCP) will crashed,
> need userspace to handle this status. It lost the original meaning for COLO
> FT/HA solution, no need use do HA in application layer.
> > it looks like normal/remote periodic VM snapshot here.
> 
> Sure, but maybe the user doesn't need (reliable) network on failover. Also
> proper network support with periodic mode can easily be implemented by
> modifying filter-buffer to buffer packets until checkpoint. Being able to use
> colo without colo-compare gives more flexibility to the user.

OK, use the filter-buffer instead of colo-compare is a new use case here.
If other maintainers have no comments about touch the vl.c for COLO , I think 
it's OK here.

Thanks
Zhang Chen 


> 
> Regards,
> Lukas Straub
> 
> > Dave or Jason have any comments here?
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > > Thanks
> > > > Zhang Chen
> > > >
> > > > >
> > > > > Regards,
> > > > > Lukas Straub
> > > > >
> > > > > > Thanks
> > > > > > Zhang Chen
> > > > > >
> > > > > > > Signed-off-by: Lukas S

Re: [PATCH 0/3] tcg/s390: Support host vector operations

[David Hildenbrand]

On 08.05.20 17:57, Richard Henderson wrote:
> I've had this floating around on a branch for a while now
> It is able to run arm sve tests under qemu, but I have not
> been able to test it against real hardware. 

You can run qemu-system-arm under qemu-system-s390 with vx=on ;) I'll
try to give this a churn one I figure out what to apply where. thanks

> 
> 
> r~
> 
> 
> Richard Henderson (3):
>   tcg/s390: Change FACILITY representation
>   tcg/s390: Merge TCG_AREG0 and TCG_REG_CALL_STACK into TCGReg
>   tcg/s390: Implement vector operations
> 
>  tcg/s390/tcg-target.h |  92 ++--
>  tcg/s390/tcg-target.opc.h |   5 +
>  tcg/s390/tcg-target.inc.c | 862 +++---
>  3 files changed, 864 insertions(+), 95 deletions(-)
>  create mode 100644 tcg/s390/tcg-target.opc.h
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device

[Dima Stepanov]

On Mon, May 11, 2020 at 11:03:01AM +0800, Jason Wang wrote:
> 
> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >Introduce new wrappers to set/reset guest notifiers for the virtio
> >device in the vhost device module:
> >   vhost_dev_assign_guest_notifiers
> > ->set_guest_notifiers(..., ..., true);
> >   vhost_dev_drop_guest_notifiers
> > ->set_guest_notifiers(..., ..., false);
> >This is a preliminary step to refactor code,
> 
> 
> Maybe I miss something, I don't see any add-on patch to modify the new
> wrapper in this series?
Hi, in fact the next 3/5 patch:
  "[PATCH v2 3/5] vhost-user-blk: add mechanism to track the guest
notifiers init state"
is about using these wrappers. But disregard it, i decided to follow
Raphael suggestion. So we will fix the vhost-user-blk case first, so i
will not introduce these wrappers. And the code will be more easier to
read and straightforward.
I will send v3 as soon as we decide what to do with the migration fix
in this patchset.

No other comments mixed in below.

> 
> 
> >  so the set_guest_notifiers
> >methods could be called based on the vhost device state.
> >Update all vhost used devices to use these wrappers instead of direct
> >method call.
> >
> >Signed-off-by: Dima Stepanov 
> >---
> >  backends/cryptodev-vhost.c  | 26 +++---
> >  backends/vhost-user.c   | 16 +---
> >  hw/block/vhost-user-blk.c   | 15 +--
> >  hw/net/vhost_net.c  | 30 +-
> >  hw/scsi/vhost-scsi-common.c | 15 +--
> >  hw/virtio/vhost-user-fs.c   | 17 +++--
> >  hw/virtio/vhost-vsock.c | 18 --
> >  hw/virtio/vhost.c   | 38 ++
> >  hw/virtio/virtio.c  | 13 +
> >  include/hw/virtio/vhost.h   |  4 
> >  include/hw/virtio/virtio.h  |  1 +
> >  11 files changed, 118 insertions(+), 75 deletions(-)
> >
> >diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> >index 8337c9a..4522195 100644
> >--- a/backends/cryptodev-vhost.c
> >+++ b/backends/cryptodev-vhost.c
> >@@ -169,16 +169,13 @@ vhost_set_vring_enable(CryptoDevBackendClient *cc,
> >  int cryptodev_vhost_start(VirtIODevice *dev, int total_queues)
> >  {
> >  VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
> >-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
> >-VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> >  int r, e;
> >  int i;
> >  CryptoDevBackend *b = vcrypto->cryptodev;
> >  CryptoDevBackendVhost *vhost_crypto;
> >  CryptoDevBackendClient *cc;
> >-if (!k->set_guest_notifiers) {
> >+if (!virtio_device_guest_notifiers_initialized(dev)) {
> >  error_report("binding does not support guest notifiers");
> >  return -ENOSYS;
> >  }
> >@@ -198,9 +195,13 @@ int cryptodev_vhost_start(VirtIODevice *dev, int 
> >total_queues)
> >  }
> >   }
> >-r = k->set_guest_notifiers(qbus->parent, total_queues, true);
> >+/*
> >+ * Since all the states are handled by one vhost device,
> >+ * use the first one in array.
> >+ */
> >+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
> >+r = vhost_dev_assign_guest_notifiers(&vhost_crypto->dev, dev, 
> >total_queues);
> >  if (r < 0) {
> >-error_report("error binding guest notifier: %d", -r);
> >  goto err;
> >  }
> >@@ -232,7 +233,8 @@ err_start:
> >  vhost_crypto = cryptodev_get_vhost(cc, b, i);
> >  cryptodev_vhost_stop_one(vhost_crypto, dev);
> >  }
> >-e = k->set_guest_notifiers(qbus->parent, total_queues, false);
> >+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
> >+e = vhost_dev_drop_guest_notifiers(&vhost_crypto->dev, dev, 
> >total_queues);
> >  if (e < 0) {
> >  error_report("vhost guest notifier cleanup failed: %d", e);
> >  }
> >@@ -242,9 +244,6 @@ err:
> >  void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
> >  {
> >-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
> >-VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> >  VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
> >  CryptoDevBackend *b = vcrypto->cryptodev;
> >  CryptoDevBackendVhost *vhost_crypto;
> >@@ -259,7 +258,12 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int 
> >total_queues)
> >  cryptodev_vhost_stop_one(vhost_crypto, dev);
> >  }
> >-r = k->set_guest_notifiers(qbus->parent, total_queues, false);
> >+/*
> >+ * Since all the states are handled by one vhost device,
> >+ * use the first one in array.
> >+ */
> >+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
> >+r = vhost_dev_drop_guest_notifiers(&vhost_crypto->dev, dev, 
> >total_queues);
> >  if (r < 0) {
> >  error_report("vhost guest notifier cleanup failed: %d", r);
>

[PATCH] ppc/pnv: Add definitions for interrupts occurring in power-saving mode

[Cédric Le Goater]

If an interrupt occurred when the thread was in power-saving mode,
bits [42:45] of SRR1 indicate the exception that caused exit from
power-saving mode.

bits [46:47] of SRR1 indicate the power-saving mode in which the
thread was when the interrupt occured.

Signed-off-by: Cédric Le Goater 
---
 target/ppc/cpu.h | 21 +
 hw/ppc/pnv.c |  8 
 target/ppc/excp_helper.c | 16 
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index df5c30160da8..9648623677d2 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -476,6 +476,27 @@ typedef struct ppc_v3_pate_t {
 #define SRR1_PROTFAULT   DSISR_PROTFAULT
 #define SRR1_IAMRDSISR_AMR
 
+/* SRR1[42:45] wakeup fields for System Reset Interrupt */
+
+#define SRR1_WAKEMASK   0x003c /* reason for wakeup */
+
+#define SRR1_WAKEHMI0x0028 /* Hypervisor maintenance */
+#define SRR1_WAKEHVI0x0024 /* Hypervisor Virt. Interrupt (P9) 
*/
+#define SRR1_WAKEEE 0x0020 /* External interrupt */
+#define SRR1_WAKEDEC0x0018 /* Decrementer interrupt */
+#define SRR1_WAKEDBELL  0x0014 /* Privileged doorbell */
+#define SRR1_WAKERESET  0x0010 /* System reset */
+#define SRR1_WAKEHDBELL 0x000c /* Hypervisor doorbell */
+#define SRR1_WAKESCOM   0x0008 /* SCOM not in power-saving mode */
+
+/* SRR1[46:47] power-saving exit mode */
+
+#define SRR1_WAKESTATE  0x0003 /* Powersave exit mask */
+
+#define SRR1_WS_HVLOSS  0x0003 /* HV resources not maintained */
+#define SRR1_WS_GPRLOSS 0x0002 /* GPRs not maintained */
+#define SRR1_WS_NOLOSS  0x0001 /* All resources maintained */
+
 /* Facility Status and Control (FSCR) bits */
 #define FSCR_EBB(63 - 56) /* Event-Based Branch Facility */
 #define FSCR_TAR(63 - 55) /* Target Address Register */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 1b4748ce6dc3..182b62565022 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1986,15 +1986,15 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, 
run_on_cpu_data arg)
 
 cpu_synchronize_state(cs);
 ppc_cpu_do_system_reset(cs);
-if (env->spr[SPR_SRR1] & PPC_BITMASK(46, 47)) {
+if (env->spr[SPR_SRR1] & SRR1_WAKESTATE) {
 /*
 * Power-save wakeups, as indicated by non-zero SRR1[46:47] put the
 * wakeup reason in SRR1[42:45], system reset is indicated with 0b0100
 * (PPC_BIT(43)).
 */
-if (!(env->spr[SPR_SRR1] & PPC_BIT(43))) {
+if (!(env->spr[SPR_SRR1] & SRR1_WAKERESET)) {
 warn_report("ppc_cpu_do_system_reset does not set system reset 
wakeup reason");
-env->spr[SPR_SRR1] |= PPC_BIT(43);
+env->spr[SPR_SRR1] |= SRR1_WAKERESET;
 }
 } else {
 /*
@@ -2004,7 +2004,7 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, 
run_on_cpu_data arg)
 * another CPU requesting a NMI IPI) system reset exception should be
 * 0b0010 (PPC_BIT(44)).
  */
-env->spr[SPR_SRR1] |= PPC_BIT(44);
+env->spr[SPR_SRR1] |= SRR1_WAKESCOM;
 }
 }
 
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 14d39029825a..a988ba15f4f7 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -101,7 +101,7 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState 
*env, int excp,
 env->resume_as_sreset = false;
 
 /* Pretend to be returning from doze always as we don't lose state */
-*msr |= (0x1ull << (63 - 47));
+*msr |= SRR1_WS_NOLOSS;
 
 /* Machine checks are sent normally */
 if (excp == POWERPC_EXCP_MCHECK) {
@@ -109,25 +109,25 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState 
*env, int excp,
 }
 switch (excp) {
 case POWERPC_EXCP_RESET:
-*msr |= 0x4ull << (63 - 45);
+*msr |= SRR1_WAKERESET;
 break;
 case POWERPC_EXCP_EXTERNAL:
-*msr |= 0x8ull << (63 - 45);
+*msr |= SRR1_WAKEEE;
 break;
 case POWERPC_EXCP_DECR:
-*msr |= 0x6ull << (63 - 45);
+*msr |= SRR1_WAKEDEC;
 break;
 case POWERPC_EXCP_SDOOR:
-*msr |= 0x5ull << (63 - 45);
+*msr |= SRR1_WAKEDBELL;
 break;
 case POWERPC_EXCP_SDOOR_HV:
-*msr |= 0x3ull << (63 - 45);
+*msr |= SRR1_WAKEHDBELL;
 break;
 case POWERPC_EXCP_HV_MAINT:
-*msr |= 0xaull << (63 - 45);
+*msr |= SRR1_WAKEHMI;
 break;
 case POWERPC_EXCP_HVIRT:
-*msr |= 0x9ull << (63 - 45);
+*msr |= SRR1_WAKEHVI;
 break;
 default:
 cpu_abort(cs, "Unsupported exception %d in Power Save mode\n",
-- 
2.25.4

Re: [PATCH v3 2/9] qemu-img: Fix stale comments on doc location

[Max Reitz]

On 08.05.20 20:03, Eric Blake wrote:
> Missed in commit e13c59fa.
> 
> Signed-off-by: Eric Blake 
> ---
>  qemu-img.c   | 2 +-
>  qemu-img-cmds.hx | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate()

[Cédric Le Goater]

On 5/7/20 7:26 PM, Greg Kurz wrote:
> It is the job of the ppc_radix64_get_fully_qualified_addr() function
> which is called at the beginning of ppc_radix64_xlate() to set both
> lpid *and* pid. It doesn't buy us anything to initialize them first.
> 
> Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to
> set either lpid or pid, would be undetectable by static analysis tools
> like coverity.
> 
> Signed-off-by: Greg Kurz 
> ---
>  target/ppc/mmu-radix64.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index c76879f65b78..5e2d912ee346 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr 
> eaddr, int rwx,
>   bool cause_excp)
>  {
>  CPUPPCState *env = &cpu->env;
> -uint64_t lpid = 0, pid = 0;
> +uint64_t lpid, pid;
>  ppc_v3_pate_t pate;
>  int psize, prot;
>  hwaddr g_raddr;
> 

I am seeing this failure with gcc version 9.3.1 20200408 (Red Hat 9.3.1-2) 
(GCC) 

target/ppc/mmu-radix64.c: In function ‘ppc_radix64_xlate’:
target/ppc/mmu-radix64.c:314:12: error: ‘pid’ may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
  314 | offset = pid * sizeof(struct prtb_entry);
  | ~~~^
target/ppc/mmu-radix64.c:439:20: note: ‘pid’ was declared here
  439 | uint64_t lpid, pid;
  |^~~
target/ppc/mmu-radix64.c:458:14: error: ‘lpid’ may be used uninitialized in 
this function [-Werror=maybe-uninitialized]
  458 | if (!ppc64_v3_get_pate(cpu, lpid, &pate)) {
  |  ^~~
  CC  ppc64-softmmu/target/ppc/fpu_helper.o


This seems like a compiler optimization issue.

C.

Re: [PATCH] tests/acceptance/boot_linux: Skip slow Aarch64 'virt' machine TCG test

[Peter Maydell]

On Sat, 9 May 2020 at 14:18, Lukas Straub  wrote:
> Hi,
> Why not simply add slow tag to the test. Like:
> :avocado: tags=slow
>
> The slow tests can then be skipped with
> $ make check-acceptance AVOCADO_TAGS='-t -slow'

Is it possible to have the default be "do the fast stuff"
and only do the slow stuff if the user asks? That's the
way round that we do the iotests, I think.

thanks
-- PMM

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

[Dima Stepanov]

On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:
> 
> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >Since disconnect can happen at any time during initialization not all
> >vring buffers (for instance used vring) can be intialized successfully.
> >If the buffer was not initialized then vhost_memory_unmap call will lead
> >to SIGSEGV. Add checks for the vring address value before calling unmap.
> >Also add assert() in the vhost_memory_unmap() routine.
> >
> >Signed-off-by: Dima Stepanov 
> >---
> >  hw/virtio/vhost.c | 27 +--
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> >
> >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >index ddbdc53..3ee50c4 100644
> >--- a/hw/virtio/vhost.c
> >+++ b/hw/virtio/vhost.c
> >@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, 
> >void *buffer,
> > hwaddr len, int is_write,
> > hwaddr access_len)
> >  {
> >+assert(buffer);
> >+
> >  if (!vhost_dev_has_iommu(dev)) {
> >  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> >  }
> >@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev 
> >*dev,
> >  vhost_vq_index);
> >  }
> >-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
> >-   1, virtio_queue_get_used_size(vdev, idx));
> >-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, 
> >idx),
> >-   0, virtio_queue_get_avail_size(vdev, idx));
> >-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
> >-   0, virtio_queue_get_desc_size(vdev, idx));
> >+/*
> >+ * Since the vhost-user disconnect can happen during initialization
> >+ * check if vring was initialized, before making unmap.
> >+ */
> >+if (vq->used) {
> >+vhost_memory_unmap(dev, vq->used,
> >+   virtio_queue_get_used_size(vdev, idx),
> >+   1, virtio_queue_get_used_size(vdev, idx));
> >+}
> >+if (vq->avail) {
> >+vhost_memory_unmap(dev, vq->avail,
> >+   virtio_queue_get_avail_size(vdev, idx),
> >+   0, virtio_queue_get_avail_size(vdev, idx));
> >+}
> >+if (vq->desc) {
> >+vhost_memory_unmap(dev, vq->desc,
> >+   virtio_queue_get_desc_size(vdev, idx),
> >+   0, virtio_queue_get_desc_size(vdev, idx));
> >+}
> 
> 
> Any reason not checking hdev->started instead? vhost_dev_start() will set it
> to true if virtqueues were correctly mapped.
> 
> Thanks
Well i see it a little bit different:
 - vhost_dev_start() sets hdev->started to true before starting
   virtqueues
 - vhost_virtqueue_start() maps all the memory
If we hit the vhost disconnect at the start of the
vhost_virtqueue_start(), for instance for this call:
  r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
Then we will call vhost_user_blk_disconnect:
  vhost_user_blk_disconnect()->
vhost_user_blk_stop()->
  vhost_dev_stop()->
vhost_virtqueue_stop()
As a result we will come in this routine with the hdev->started still
set to true, but if used/avail/desc fields still uninitialized and set
to 0.

> 
> 
> >  }
> >  static void vhost_eventfd_add(MemoryListener *listener,
> 

Re: [Bug 1877716] [NEW] Win10 guest unsuable after a few minutes

[Stefan Hajnoczi]

On Sat, May 9, 2020 at 9:16 AM Xavier <1877...@bugs.launchpad.net> wrote:
>
> Public bug reported:
>
> On Arch Linux, the recent qemu package update seems to misbehave on some
> systems. In my case, my Windows 10 guest runs fine for around 5 minutes
> and then start to get really sluggish, even unresponsive. It needs to be
> forced off. I could reproduce this on a minimal VM with no passthrough,
> although my current testing setup involves an nvme pcie passthrough.
>
> I bisected it to the following commit which rapidly starts to run sluggishly 
> on my setup:
> https://github.com/qemu/qemu/commit/73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf

Thanks for bisecting this bug! Arch Linux can work around it in the
short term by building with ./configure --disable-linux-io-uring
and/or removing the liburing build dependency.

I will try to reproduce the issue and send a QEMU patch to fix it.

Re: [PATCH v4 04/10] tests/vm: add --boot-console switch

[Alex Bennée]

Robert Foley  writes:

> Added ability to view console during boot via
> --boot-console switch to basevm.py.  This helps debug issues that occur
> during the boot sequence.
> Also added a new special variable to vm-build:
> BOOT_CONSOLE=1 will cause this new --boot-console switch to be set.

Hmm why isn't this output covered by DEBUG=1? I'm wary of adding another
debug knob rather than just including the extra information under our
existing DEBUG output.

> Signed-off-by: Robert Foley 
> ---
>  tests/vm/Makefile.include |  4 
>  tests/vm/basevm.py| 11 +--
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> index 78a2de1f82..d921ee14cf 100644
> --- a/tests/vm/Makefile.include
> +++ b/tests/vm/Makefile.include
> @@ -40,6 +40,7 @@ endif
>   @echo 'EXTRA_CONFIGURE_OPTS="..."'
>   @echo "J=[0..9]* - Override the -jN parameter 
> for make commands"
>   @echo "DEBUG=1   - Enable verbose output on 
> host and interactive debugging"
> + @echo "BOOT_CONSOLE=1- Show the console output at 
> boot time. "
>   @echo "V=1   - Enable verbose ouput on host 
> and guest commands"
>   @echo "QEMU=/path/to/qemu- Change path to QEMU binary"
>   @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool"
> @@ -62,6 +63,7 @@ $(IMAGES_DIR)/%.img:$(SRC_PATH)/tests/vm/% \
>   $(call quiet-command, \
>   $(PYTHON) $< \
>   $(if $(V)$(DEBUG), --debug) \
> + $(if $(BOOT_CONSOLE),--boot-console) \
>   --image "$@" \
>   --force \
>   --build-image $@, \
> @@ -76,6 +78,7 @@ vm-build-%: $(IMAGES_DIR)/%.img
>   $(if $(DEBUG), --interactive) \
>   $(if $(J),--jobs $(J)) \
>   $(if $(V),--verbose) \
> + $(if $(BOOT_CONSOLE),--boot-console) \
>   --image "$<" \
>   $(if $(BUILD_TARGET),--build-target $(BUILD_TARGET)) \
>   --snapshot \
> @@ -96,6 +99,7 @@ vm-boot-ssh-%: $(IMAGES_DIR)/%.img
>   $(call quiet-command, \
>   $(PYTHON) $(SRC_PATH)/tests/vm/$* \
>   $(if $(J),--jobs $(J)) \
> + $(if $(BOOT_CONSOLE),--boot-console) \
>   --image "$<" \
>   --interactive \
>   false, \
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index dd545d3d1d..aab3d98edf 100644
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -85,8 +85,10 @@ class BaseVM(object):
>  # 4 is arbitrary, but greater than 2,
>  # since we found we need to wait more than twice as long.
>  tcg_ssh_timeout_multiplier = 4
> -def __init__(self, debug=False, vcpus=None, config=None):
> +def __init__(self, debug=False, vcpus=None, config=None,
> + boot_console=None):
>  self._guest = None
> +self._boot_console = boot_console
>  # Allow input config to override defaults.
>  self._config = DEFAULT_CONFIG.copy()
>  if config != None:
> @@ -535,6 +537,8 @@ def parse_args(vmcls):
>  parser.add_option("--config", "-c", default=None,
>help="Provide config yaml for configuration. "\
> "See config_example.yaml for example.")
> +parser.add_option("--boot-console", action="store_true",
> +  help="Show console during boot. ")
>  parser.disable_interspersed_args()
>  return parser.parse_args()
>  
> @@ -549,7 +553,8 @@ def main(vmcls, config=None):
>  config = parse_config(config, args)
>  logging.basicConfig(level=(logging.DEBUG if args.debug
> else logging.WARN))
> -vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config)
> +vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config,
> +   boot_console=args.boot_console)
>  if args.build_image:
>  if os.path.exists(args.image) and not args.force:
>  sys.stderr.writelines(["Image file exists: %s\n" % 
> args.image,
> @@ -569,6 +574,8 @@ def main(vmcls, config=None):
>  if args.snapshot:
>  img += ",snapshot=on"
>  vm.boot(img)
> +if vm._boot_console:
> +vm.wait_boot()
>  vm.wait_ssh()
>  except Exception as e:
>  if isinstance(e, SystemExit) and e.code == 0:


-- 
Alex Bennée

Re: is there any way to make qemu stop at the very first instruction of the emulation process and wait for gdb connect?

[Peter Maydell]

On Sun, 10 May 2020 at 10:18, tugouxp <13824125...@163.com> wrote:
>
> is there any way to make qemu stop at the very first instruction of the 
> emulation process  and wait for gdb connect?

Yes: use the '-S' option (as well as the usual option
to set up a gdbstub connection). See the documentation:
https://www.qemu.org/docs/master/system/gdb.html

thanks
-- PMM

Re: [PATCH v4 06/10] tests/vm: allow wait_ssh() to specify command

[Alex Bennée]

Robert Foley  writes:

> This allows for waiting for completion of arbitrary commands.
>
> Signed-off-by: Robert Foley 

Reviewed-by: Alex Bennée 


-- 
Alex Bennée

Re: [PATCH v4 00/10] tests/vm: Add support for aarch64 VMs

[Alex Bennée]

Robert Foley  writes:

> This is version 4 of the patch series to 
> add support for aarch64 VMs in the vm-build infrastructure.
>  - Ubuntu 18.04 aarch64 VM
>  - CentOS 8 aarch64 VM
>
> V3: https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg02805.html 
>
> Changes in V4.
> - Removed the target validation we had put into Makefile.include.
> - Corrected a dependency on a later patch in gen_cloud_init_iso().
> - Changed the consume console patch to make draining the console to a 
>   buffer optional.  This fixes the acceptance test issue.

Hi Robert,

Sorry I didn't get to this in the run up to 4.0. I've tried applying the
series to the current master but I ran into conflicts pretty early on. I
think the first patch conflicts with f01454ad17 because I had already
cherry picked some of the cleanups to gen_cloud_init_iso and then had to
fix it up. Skipping it caused the next patch to fail to apply so I
decided to back away from misapplying the series.

Would you be able to re-spin on current master please?

-- 
Alex Bennée

Re: [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps

[Max Reitz]

On 08.05.20 20:03, Eric Blake wrote:
> Upcoming patches will enhance bitmap support in qemu-img, but in doing
> so, it turns out to be nice to suppress output when bitmaps make no
> sense (such as on a qcow2 v2 image).  Add a hook to make this easier
> to query.
> 
> In the future, when we improve the ability to look up bitmaps through
> a filter, we will probably also want to teach the block layer to
> automatically let filters pass this request on through.
> 
> Signed-off-by: Eric Blake 
> ---
>  block/qcow2.h| 1 +
>  include/block/block_int.h| 1 +
>  include/block/dirty-bitmap.h | 1 +
>  block/dirty-bitmap.c | 9 +
>  block/qcow2-bitmap.c | 7 +++
>  block/qcow2.c| 1 +
>  6 files changed, 20 insertions(+)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index f4de0a27d5c3..fb2b2b5a7b4d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -764,6 +764,7 @@ bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState 
> *bs,
>  int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>  const char *name,
>  Error **errp);
> +bool qcow2_dirty_bitmap_supported(BlockDriverState *bs);
> 
>  ssize_t coroutine_fn
>  qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index df6d0273d679..cb1082da4c43 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -560,6 +560,7 @@ struct BlockDriver {
>   uint64_t parent_perm, uint64_t parent_shared,
>   uint64_t *nperm, uint64_t *nshared);
> 
> +bool (*bdrv_dirty_bitmap_supported)(BlockDriverState *bs);

All BDSs support bitmaps, but only some support persistent dirty
bitmaps, so I think the name should reflect that.

Conceptually, this looks reasonable.  This information might indeed be
nice to have, and I’m not sure whether we should extend any existing
interface to return it.

(The interfaces that come to my mind are:
(1) bdrv_can_store_new_dirty_bitmap() below, which we could make accept
a NULL @name to return basically the same information.  But it’s still a
bit different, because I’d expect that function to return whether any
bitmap can be stored then, not whether the node supports bitmaps at all.
 So e.g. if there are already too many bitmaps, it should return false,
even though the node itself does support bitmaps.

(2) bdrv_get_info()/BlockDriverInfo: This information would fit in very
nicely here, but do we have to put it here just because it does?  I
don’t think so.  This patch adds 20 lines of code, that shows that it’s
very simple to add a dedicated method, and it’s certainly a bit easier
to use than to invoke bdrv_get_info() and throw away all the other
information.  Perhaps this patch only shows that BlockDriverInfo doesn’t
make much sense in the first place, and most of its fields should have
been scalar return values from dedicated functions.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

[Dima Stepanov]

On Mon, May 11, 2020 at 11:15:53AM +0800, Jason Wang wrote:
> 
> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >If vhost-user daemon is used as a backend for the vhost device, then we
> >should consider a possibility of disconnect at any moment. If such
> >disconnect happened in the vhost_migration_log() routine the vhost
> >device structure will be clean up.
> >At the start of the vhost_migration_log() function there is a check:
> >   if (!dev->started) {
> >   dev->log_enabled = enable;
> >   return 0;
> >   }
> >To be consistent with this check add the same check after calling the
> >vhost_dev_set_log() routine. This in general help not to break a
> >migration due the assert() message. But it looks like that this code
> >should be revised to handle these errors more carefully.
> >
> >In case of vhost-user device backend the fail paths should consider the
> >state of the device. In this case we should skip some function calls
> >during rollback on the error paths, so not to get the NULL dereference
> >errors.
> >
> >Signed-off-by: Dima Stepanov 
> >---
> >  hw/virtio/vhost.c | 39 +++
> >  1 file changed, 35 insertions(+), 4 deletions(-)
> >
> >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >index 3ee50c4..d5ab96d 100644
> >--- a/hw/virtio/vhost.c
> >+++ b/hw/virtio/vhost.c
> >@@ -787,6 +787,17 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
> >  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >  {
> >  int r, i, idx;
> >+
> >+if (!dev->started) {
> >+/*
> >+ * If vhost-user daemon is used as a backend for the
> >+ * device and the connection is broken, then the vhost_dev
> >+ * structure will be reset all its values to 0.
> >+ * Add additional check for the device state.
> >+ */
> >+return -1;
> >+}
> >+
> >  r = vhost_dev_set_features(dev, enable_log);
> >  if (r < 0) {
> >  goto err_features;
> >@@ -801,12 +812,19 @@ static int vhost_dev_set_log(struct vhost_dev *dev, 
> >bool enable_log)
> >  }
> >  return 0;
> >  err_vq:
> >-for (; i >= 0; --i) {
> >+/*
> >+ * Disconnect with the vhost-user daemon can lead to the
> >+ * vhost_dev_cleanup() call which will clean up vhost_dev
> >+ * structure.
> >+ */
> >+for (; dev->started && (i >= 0); --i) {
> >  idx = dev->vhost_ops->vhost_get_vq_index(
> 
> 
> Why need the check of dev->started here, can started be modified outside
> mainloop? If yes, I don't get the check of !dev->started in the beginning of
> this function.
> 
No dev->started can't change outside the mainloop. The main problem is
only for the vhost_user_blk daemon. Consider the case when we
successfully pass the dev->started check at the beginning of the
function, but after it we hit the disconnect on the next call on the
second or third iteration:
 r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, enable_log);
The unix socket backend device will call the disconnect routine for this
device and reset the structure. So the structure will be reset (and
dev->started set to false) inside this set_addr() call. So
we shouldn't call the clean up calls because this virtqueues were clean
up in the disconnect call. But we should protect these calls somehow, so
it will not hit SIGSEGV and we will be able to pass migration.

Just to summarize it:
For the vhost-user-blk devices we ca hit clean up calls twice in case of
vhost disconnect:
1. The first time during the disconnect process. The clean up is called
inside it.
2. The second time during roll back clean up.
So if it is the case we should skip p2.

> 
> >dev, dev->vq_index + i);
> >  vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> >   dev->log_enabled);
> >  }
> >-vhost_dev_set_features(dev, dev->log_enabled);
> >+if (dev->started) {
> >+vhost_dev_set_features(dev, dev->log_enabled);
> >+}
> >  err_features:
> >  return r;
> >  }
> >@@ -832,7 +850,15 @@ static int vhost_migration_log(MemoryListener 
> >*listener, int enable)
> >  } else {
> >  vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> >  r = vhost_dev_set_log(dev, true);
> >-if (r < 0) {
> >+/*
> >+ * The dev log resize can fail, because of disconnect
> >+ * with the vhost-user-blk daemon. Check the device
> >+ * state before calling the vhost_dev_set_log()
> >+ * function.
> >+ * Don't return error if device isn't started to be
> >+ * consistent with the check above.
> >+ */
> >+if (dev->started && r < 0) {
> >  return r;
> >  }
> >  }
> >@@ -1739,7 +1765,12 @@ int vhost_dev_start(struct vhost_dev *hdev, 
> >VirtIODevice *vdev)
> >  fail_log:
> >  vhost_log_put(hdev, false);
> >  fail_vq:
> >-while (--i >= 0) {
> >+/*
> >+ * Disconnect with the vhost-user daemon can lead

Re: [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub

[Greg Kurz]

On Mon, 11 May 2020 11:43:48 +1000
David Gibson  wrote:

> On Thu, May 07, 2020 at 07:27:15PM +0200, Greg Kurz wrote:
> > gdbstub shouldn't silently change guest visible state when doing address
> > translation. While here drop a not very useful comment.
> > 
> > This was found while reading the code. I could verify that this affects
> > both powernv and pseries, but I failed to observe any actual bug.
> > 
> > Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped 
> > translation"
> > Signed-off-by: Greg Kurz 
> 
> It's a real fix.  But AFAICT we'll always have cause_excp ==
> cause_rc_update, and I can't see any reason we'd ever them different.

This is definitely true as of today because all memory accesses are
performed by a CPU, but POWER9 has accelerator agents (eg. NPU) that
can also issue load/store operations on the PowerBus.

I'm currently doing some experiments to model the NPU as used with
OpenCAPI (the ultimate goal being to have another user for XIVE).
This requires to be able to do EA->RA translation without a CPU
context, as done by the NestMMU in real HW. This requires quite
some code refactoring in mmu-radix64.c and I opted to keep these
flags separate as a first step... but you're right, since page
faults are always handled on behalf of a CPU, I don't see any
reason for them to be different.

Cc'ing Nick in case I've missed something.

> So I'd prefer to just rename the flag and use it for both tests.
> 
> Maybe just 'guest_visible' ?
> 

Sounds good.

> > ---
> >  target/ppc/mmu-radix64.c |   36 
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> > index ceeb3dfe2d49..bc51cd89a079 100644
> > --- a/target/ppc/mmu-radix64.c
> > +++ b/target/ppc/mmu-radix64.c
> > @@ -270,7 +270,8 @@ static int 
> > ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx,
> >ppc_v3_pate_t pate,
> >hwaddr *h_raddr, int *h_prot,
> >int *h_page_size, bool 
> > pde_addr,
> > -  bool cause_excp)
> > +  bool cause_excp,
> > +  bool cause_rc_update)
> >  {
> >  int fault_cause = 0;
> >  hwaddr pte_addr;
> > @@ -291,8 +292,9 @@ static int 
> > ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx,
> >  return 1;
> >  }
> >  
> > -/* Update Reference and Change Bits */
> > -ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
> > +if (cause_rc_update) {
> > +ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
> > +}
> >  
> >  return 0;
> >  }
> > @@ -301,7 +303,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU 
> > *cpu, int rwx,
> >  vaddr eaddr, uint64_t pid,
> >  ppc_v3_pate_t pate, hwaddr 
> > *g_raddr,
> >  int *g_prot, int *g_page_size,
> > -bool cause_excp)
> > +bool cause_excp,
> > +bool cause_rc_update)
> >  {
> >  CPUState *cs = CPU(cpu);
> >  CPUPPCState *env = &cpu->env;
> > @@ -336,7 +339,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU 
> > *cpu, int rwx,
> >  ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr,
> >   pate, &h_raddr, &h_prot,
> >   &h_page_size, true,
> > - cause_excp);
> > + cause_excp,
> > + cause_rc_update);
> >  if (ret) {
> >  return ret;
> >  }
> > @@ -376,7 +380,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU 
> > *cpu, int rwx,
> >  ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, 
> > pte_addr,
> >   pate, &h_raddr, 
> > &h_prot,
> >   &h_page_size, true,
> > - cause_excp);
> > + cause_excp,
> > + cause_rc_update);
> >  if (ret) {
> >  return ret;
> >  }
> > @@ -408,7 +413,9 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU 
> > *cpu, int rwx,
> >  return 1;
> >  }
> >  
> > -ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
> > +if (cause_rc_update) {
> > +ppc_radix64_set_rc(cpu,

Re: [PATCH 1/2] migration/rdma: fix potential nullptr access in rdma_start_incoming_migration

[Peter Maydell]

On Fri, 8 May 2020 at 08:53, Juan Quintela  wrote:
>
> Pan Nengyuan  wrote:
> > 'rdma' is NULL when taking the first error branch in 
> > rdma_start_incoming_migration.
> > And it will cause a null pointer access in label 'err'. Fix that.
> >
> > Fixes: 59c59c67ee6b0327ae932deb303caa47919aeb1e
> > Signed-off-by: Pan Nengyuan 
>
> Reviewed-by: Juan Quintela 

NB: this is CID 1428762.

thanks
-- PMM

Re: [PATCH v4 08/10] hw/core/resettable: add support for warm reset

[Damien Hedde]

Hi Philippe,

On 5/10/20 10:17 PM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
> 
> On 8/21/19 6:33 PM, Damien Hedde wrote:
>> Add the RESET_TYPE_WARM reset type.
>> Expand the actual implementation to support several types.
>>
>>     /**
>>    * ResetType:
>> - * Types of reset.
>> + * Types of reset, values can be OR'ed together.
>>    *
>>    * + Cold: reset resulting from a power cycle of the object.
>> - *
>> - * TODO: Support has to be added to handle more types. In particular,
>> - * ResetState structure needs to be expanded.
>> + * + Warm: reset without power cycling.
>>    */
>>   typedef enum ResetType {
>> -    RESET_TYPE_COLD,
>> +    RESET_TYPE_COLD = 0x1,
>> +    RESET_TYPE_WARM = 0x2,
> 
> I'm a bit lost with the various iterations, what is the plan with warm
> reset, is this blocked due to discussion, API, something else?
> 

I removed it in the last versions of the series because it was adding
complexity. There were unsolved issues and discussions, in particular
regarding the interactions and propagation along trees between the
different types.

Damien

Re: [PATCH v2] e1000e: Added ICR clearing by corresponding IMS bit.

[Andrew Melnichenko]

Yo,

> So I think we should implement the 82574l behavior?

 Well, as I understand it - its already implemented. I've added ICR
clearance if ICR & IMS(also need to add ICR_ASSERTED check, my bad, I'll
prepare new patch).

At first, I had hacks to clear 'msi_causes_pending' at
'e1000e_core_set_link_status()' before link down. It works but it's not a
solution.
Also, on Windows the bug doesn't reproduce. I've traced Windows and Linux -
the difference that Windows driver clears pending by writing to ICR, where
Linux tries to clear by reading it.
I had another possible fix - for Linux driver(writing to ICR at interrupt
routine).
I've asked intel guys, does Linux driver works with a device(I don't have
real one). Thay said that it works and suggested to check 8257x spec. I'll
forward the message to you.

On Sat, May 9, 2020 at 9:02 AM Jason Wang  wrote:

>
> On 2020/5/9 上午2:13, Andrew Melnichenko wrote:
> > Yo, I've used OpenSDM_8257x-18.pdf specification.
> > This document was recommended by Intel guys(Also, they referenced to
> > that note).
> > I've made a fast fix and it works. Before that I had a fix for Linux
> > e1000e driver.
> > Overall, the issue was in pending interrupts that can't be cleared by
> > reading ICR in Linux(Windows driver clears by writing to ICR).
> >
> > You can download spec for example from:
> >
> http://iweb.dl.sourceforge.net/project/e1000/8257x%20Developer%20Manual/Revision%201.8/OpenSDM_8257x-18.pdf
>
>
> Interesting, this spec doesn't include 82574l which is what e1000e
> claims to emulate:
>
>  c->vendor_id = PCI_VENDOR_ID_INTEL;
>  c->device_id = E1000_DEV_ID_82574L;
>
> Looking at 82574l spec (using the link mentioned in e1000e_core.c), it
> said (7.4.3):
>
> In MSI-X mode the bits in this register can be configured to auto-clear
> when the MSI-X
> interrupt message is sent, in order to minimize driver overhead, and
> when using MSI-X
> interrupt signaling.
> In systems that do not support MSI-X, reading the ICR register clears
> it's bits or writing
> 1b's clears the corresponding bits in this register.
>
> So the auto clear is under the control of EIAC (MSIX) or unconditionally
> (non MSI-X).
>
> But what has been implemented in e1000e_mac_icr_read() is something
> similar to the behavior of non 82574l card.
>
> So I think we should implement the 82574l behavior?
>
> Thanks
>
>
> >
> > On Fri, May 8, 2020 at 5:21 AM Jason Wang  > > wrote:
> >
> >
> > On 2020/5/7 上午5:26, and...@daynix.com 
> > wrote:
> > > From: Andrew Melnychenko  > >
> > >
> > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> > > Added ICR clearing if there is IMS bit - according to the note by
> > > section 13.3.27 of the 8257X developers manual.
> > >
> > > Signed-off-by: Andrew Melnychenko  > >
> > > ---
> > >   hw/net/e1000e_core.c | 9 +
> > >   hw/net/trace-events  | 1 +
> > >   2 files changed, 10 insertions(+)
> > >
> > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > > index d5676871fa..302e99ff46 100644
> > > --- a/hw/net/e1000e_core.c
> > > +++ b/hw/net/e1000e_core.c
> > > @@ -2624,6 +2624,15 @@ e1000e_mac_icr_read(E1000ECore *core, int
> > index)
> > >   e1000e_clear_ims_bits(core, core->mac[IAM]);
> > >   }
> > >
> > > +/*
> > > + * PCIe* GbE Controllers Open Source Software Developer's
> > Manual
> > > + * 13.3.27 Interrupt Cause Read Register
> > > + */
> >
> >
> > Hi Andrew:
> >
> > Which version of the manual did you use? I try to use the one
> > mentioned
> > in e1000e.c which is
> >
> http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
> .
> >
> > But I couldn't find chapter 13.3.27.
> >
> > Thanks
> >
> >
> > > +if (core->mac[ICR] & core->mac[IMS]) {
> > > + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
> > core->mac[IMS]);
> > > +core->mac[ICR] = 0;
> > > +}
> > > +
> > >   trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
> > >   e1000e_update_interrupt_state(core);
> > >   return ret;
> > > diff --git a/hw/net/trace-events b/hw/net/trace-events
> > > index e18f883cfd..46e40fcfa9 100644
> > > --- a/hw/net/trace-events
> > > +++ b/hw/net/trace-events
> > > @@ -237,6 +237,7 @@ e1000e_irq_icr_read_entry(uint32_t icr)
> > "Starting ICR read. Current ICR: 0x%x"
> > >   e1000e_irq_icr_read_exit(uint32_t icr) "Ending ICR read.
> > Current ICR: 0x%x"
> > >   e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due
> > to zero IMS"
> > >   e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to
> IAME"
> > > +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims)
> > "Clearing ICR on read due corresponding IMS b

Re: [PATCH 00/12] hw/mips/fuloong2e: PoC to fix hang after reboot

[Aleksandar Markovic]

нед, 10. мај 2020. у 23:01 Philippe Mathieu-Daudé  је
написао/ла:
>
> The PMON firmware behave differently regarding it is run
> after a cold/warm reset. A simple bit flip fix the issue,
> however we need to know the type of reset to set it.
> Currently QEMU only supports COLD reset.
>
> This series contains various of my current Fuloong queue,
> - Welcome Huacai Chen as co-maintainer,
> - Fix typo in machine name,
> - Few cleanups in Bonito64,
> - Report various Bonito64 accesses as UNIMP,
> - Proof-of-concept fix for the reset bit.
>
> The last patch is not for merging, but is included to see
> if the Multi-phase reset mechanism can fix the problem.
>

Perhaps you can change the title of the series in its next version to:

target/mips: Fuloong2e and ither mips fixes and improvements

or similar, since its scope is wider than the current title conveys.

Regards,
Aleksandar



> Regards,
>
> Phil.
>
> Huacai Chen (1):
>   MAINTAINERS: Add Huacai Chen as fuloong2e co-maintainer
>
> Philippe Mathieu-Daudé (11):
>   hw/mips/fuloong2e: Rename PMON BIOS name
>   hw/mips/fuloong2e: Move code and update a comment
>   hw/mips/fuloong2e: Fix typo in Fuloong machine name
>   hw/pci-host: Use CONFIG_PCI_BONITO to select the Bonito North Bridge
>   hw/pci-host/bonito: Fix DPRINTF() format strings
>   hw/pci-host/bonito: Map peripheral using physical address
>   hw/pci-host/bonito: Map all the Bonito64 I/O range
>   hw/pci-host/bonito: Map the different PCI ranges more detailled
>   hw/pci-host/bonito: Better describe the I/O CS regions
>   hw/pci-host/bonito: Set the Config register reset value with
> FIELD_DP32
>   POC hw/pci-host/bonito: Fix BONGENCFG value after a warm-reset
>
>  docs/system/target-mips.rst  |  2 +-
>  default-configs/mips64el-softmmu.mak |  2 +-
>  hw/isa/vt82c686.c|  2 +-
>  hw/mips/{mips_fulong2e.c => fuloong2e.c} | 41 +--
>  hw/pci-host/bonito.c | 92 +++-
>  tests/qtest/endianness-test.c|  2 +-
>  MAINTAINERS  |  6 +-
>  hw/mips/Kconfig  |  3 +-
>  hw/mips/Makefile.objs|  2 +-
>  hw/pci-host/Kconfig  |  5 ++
>  hw/pci-host/Makefile.objs|  2 +-
>  11 files changed, 114 insertions(+), 45 deletions(-)
>  rename hw/mips/{mips_fulong2e.c => fuloong2e.c} (91%)
>
> --
> 2.21.3
>

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

[Dima Stepanov]

On Sun, May 10, 2020 at 08:03:39PM -0400, Raphael Norwitz wrote:
> On Thu, May 7, 2020 at 11:35 AM Dima Stepanov  wrote:
> >
> > What do you think?
> >
> 
> Apologies - I tripped over the if (dev->started && r < 0) check.
> Never-mind my point with race conditions and failing migrations.
> 
> Rather than modifying vhost_dev_set_log(), it may be clearer to put a
> check after vhost_dev_log_resize()? Something like:
> 
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -829,11 +829,22 @@ static int vhost_migration_log(MemoryListener
> *listener, int enable)
>  vhost_log_put(dev, false);
>  } else {
>  vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> +/*
> + * A device can be stopped because of backend disconnect inside
> + * vhost_dev_log_resize(). In this case we should mark logging
> + * enabled and return without attempting to set the backend
> + * logging state.
> + */
> +if (!dev->started) {
> +goto out_success;
> +}
>  r = vhost_dev_set_log(dev, true);
>  if (r < 0) {
>  return r;
>  }
>  }
> +
> +out_success:
>  dev->log_enabled = enable;
>  return 0;
>  }
This patch will not fix all the issues. Consider the case than you will
hit disconnect inside vhost_dev_set_log. For instance for the 3rd
virtqueue, for the following call:
  vhost_virtqueue_set_addr(...)
Maybe i didn't explain very clearly the problem. The problem i've tried
to fix is only for the vhost-user-blk devices. This issue can be hit
during VHOST_USER commands "handshake". If we hit disconnect on any step
of this "handshake" then we will try to make clean up twice:
1. First during disconnect cleanup (unix socket backend).
2. Second as roll back for initialization.
If this is the case, then we shouldn't call p2, as everything was clean
up on p1. And the complicated thing is that there are several VHOST_USER
commands and we should consider the state after each. And even more,
initialization could fail because of some other reason and we hit
disconnect inside roll back clean up, in this case we should complete
clean up in the disconnect function and stop rolling back.

Hope it helps ).

> 
> This seems harmless enough to me, and I see how it fixes your
> particular crash, but I would still prefer we worked towards a more
> robust solution. In particular I think we could handle this inside
> vhost-user-blk if we let the device state persist between connections
> (i.e. call vhost_dev_cleanup() inside vhost_user_blk_connect() before
> vhost_dev_init() on reconnect). This should also fix some of the
> crashes Li Feng has hit, and probably others which haven’t been
> reported yet. What do you think?
Yes, this looks like a good direction. Because all my patches are only
workarounds and i believe there can be some other issues which haven't
been reported or will be introduced ).
I still think that these patches are good to submit and to think about
more complicated/refactoring solution as the next step.

> 
> If that’s unworkable I guess we will need to add these vhost level
> checks.
At least for now, i don't think its unworkable, i just think that it
will take some time to figure out how to refactor it properly. But the
SIGSEGV issue is real.

> In that case I would still prefer we add a “disconnected” flag
> in struct vhost_dev struct, and make sure it isn’t cleared by
> vhost_dev_cleanup(). That way we don’t conflate stopping a device with
> backend disconnect at the vhost level and potentially regress behavior
> for other device types.
It is also possible, but should be analyzed and properly tested. So as i
said it will take some time to figure out how to refactor it properly.

Re: [PATCH 07/11] target/s390x/helper: Clean ifdef'ry

[David Hildenbrand]

On 09.05.20 15:09, Philippe Mathieu-Daudé wrote:
> All this code is guarded checking CONFIG_USER_ONLY definition.
> Drop the duplicated checks.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Suspicious ifdef'ry in s390_handle_wait() from commit 83f7f32901c.

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [Bug 1877716] [NEW] Win10 guest unsuable after a few minutes

[Stefan Hajnoczi]

On Mon, May 11, 2020 at 10:12 AM Stefan Hajnoczi  wrote:
> On Sat, May 9, 2020 at 9:16 AM Xavier <1877...@bugs.launchpad.net> wrote:
> >
> > Public bug reported:
> >
> > On Arch Linux, the recent qemu package update seems to misbehave on some
> > systems. In my case, my Windows 10 guest runs fine for around 5 minutes
> > and then start to get really sluggish, even unresponsive. It needs to be
> > forced off. I could reproduce this on a minimal VM with no passthrough,
> > although my current testing setup involves an nvme pcie passthrough.
> >
> > I bisected it to the following commit which rapidly starts to run 
> > sluggishly on my setup:
> > https://github.com/qemu/qemu/commit/73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf
>
> Thanks for bisecting this bug! Arch Linux can work around it in the
> short term by building with ./configure --disable-linux-io-uring
> and/or removing the liburing build dependency.

Hmm...a brief look at the Arch Linux package source suggests QEMU is
not being built with io_uring enabled. Anatol, please confirm whether
this is correct.

If io_uring is not enabled then this bug may affect most existing
users on Linux. Initially I thought it was because Arch Linux had
enabled the new io_uring feature but I was probably mistaken.

Re: [PATCH 03/11] sysemu/tcg: Only declare tcg_allowed when TCG is available

[Edgar E. Iglesias]

On Sat, May 09, 2020 at 03:09:02PM +0200, Philippe Mathieu-Daudé wrote:
> When TCG is not available, the tcg_allowed variable does not exist.

Reviewed-by: Edgar E. Iglesias 



> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/tcg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/tcg.h b/include/sysemu/tcg.h
> index 7d116d2e80..d9d3ca8559 100644
> --- a/include/sysemu/tcg.h
> +++ b/include/sysemu/tcg.h
> @@ -8,9 +8,9 @@
>  #ifndef SYSEMU_TCG_H
>  #define SYSEMU_TCG_H
>  
> -extern bool tcg_allowed;
>  void tcg_exec_init(unsigned long tb_size);
>  #ifdef CONFIG_TCG
> +extern bool tcg_allowed;
>  #define tcg_enabled() (tcg_allowed)
>  #else
>  #define tcg_enabled() 0
> -- 
> 2.21.3
> 

Re: [PATCH 04/11] sysemu/hvf: Only declare hvf_allowed when HVF is available

[Edgar E. Iglesias]

On Sat, May 09, 2020 at 03:09:03PM +0200, Philippe Mathieu-Daudé wrote:
> When HVF is not available, the tcg_allowed variable does not exist.

Typo in commit message tcg_allowed -> hvf_allowed.

With that fixed:
Reviewed-by: Edgar E. Iglesias 


> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/hvf.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
> index d211e808e9..fe95743124 100644
> --- a/include/sysemu/hvf.h
> +++ b/include/sysemu/hvf.h
> @@ -18,7 +18,6 @@
>  #include "exec/memory.h"
>  #include "sysemu/accel.h"
>  
> -extern bool hvf_allowed;
>  #ifdef CONFIG_HVF
>  #include 
>  #include 
> @@ -26,11 +25,12 @@ extern bool hvf_allowed;
>  #include "target/i386/cpu.h"
>  uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
>   int reg);
> +extern bool hvf_allowed;
>  #define hvf_enabled() (hvf_allowed)
> -#else
> +#else /* !CONFIG_HVF */
>  #define hvf_enabled() 0
>  #define hvf_get_supported_cpuid(func, idx, reg) 0
> -#endif
> +#endif /* !CONFIG_HVF */
>  
>  /* hvf_slot flags */
>  #define HVF_SLOT_LOG (1 << 0)
> -- 
> 2.21.3
> 

Re: [PATCH v3 09/10] target/s390x: Use tcg_gen_gvec_rotl{i,s,v}

[David Hildenbrand]

On 08.05.20 17:10, Richard Henderson wrote:
> Merge VERLL and VERLLV into op_vesv and op_ves, alongside
> all of the other vector shift operations.
> 
> Cc: David Hildenbrand 
> Signed-off-by: Richard Henderson 

RHEL8 boots just fine and this survives my (still not upstream yet)

tests/tcg: target/s390x: Test VECTOR ELEMENT ROTATE LEFT LOGICAL

Reviewed-by: David Hildenbrand 

> ---
>  target/s390x/helper.h   |  4 --
>  target/s390x/translate_vx.inc.c | 66 +
>  target/s390x/vec_int_helper.c   | 31 
>  target/s390x/insn-data.def  |  4 +-
>  4 files changed, 11 insertions(+), 94 deletions(-)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index b5813c2ac2..b7887b552b 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -198,10 +198,6 @@ DEF_HELPER_FLAGS_4(gvec_vmlo16, TCG_CALL_NO_RWG, void, 
> ptr, cptr, cptr, i32)
>  DEF_HELPER_FLAGS_4(gvec_vmlo32, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>  DEF_HELPER_FLAGS_3(gvec_vpopct8, TCG_CALL_NO_RWG, void, ptr, cptr, i32)
>  DEF_HELPER_FLAGS_3(gvec_vpopct16, TCG_CALL_NO_RWG, void, ptr, cptr, i32)
> -DEF_HELPER_FLAGS_4(gvec_verllv8, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
> -DEF_HELPER_FLAGS_4(gvec_verllv16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, 
> i32)
> -DEF_HELPER_FLAGS_4(gvec_verll8, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
> -DEF_HELPER_FLAGS_4(gvec_verll16, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>  DEF_HELPER_FLAGS_4(gvec_verim8, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>  DEF_HELPER_FLAGS_4(gvec_verim16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>  DEF_HELPER_FLAGS_4(gvec_vsl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
> diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
> index 12347f8a03..eb767f5288 100644
> --- a/target/s390x/translate_vx.inc.c
> +++ b/target/s390x/translate_vx.inc.c
> @@ -1825,63 +1825,6 @@ static DisasJumpType op_vpopct(DisasContext *s, 
> DisasOps *o)
>  return DISAS_NEXT;
>  }
>  
> -static void gen_rll_i32(TCGv_i32 d, TCGv_i32 a, TCGv_i32 b)
> -{
> -TCGv_i32 t0 = tcg_temp_new_i32();
> -
> -tcg_gen_andi_i32(t0, b, 31);
> -tcg_gen_rotl_i32(d, a, t0);
> -tcg_temp_free_i32(t0);
> -}
> -
> -static void gen_rll_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
> -{
> -TCGv_i64 t0 = tcg_temp_new_i64();
> -
> -tcg_gen_andi_i64(t0, b, 63);
> -tcg_gen_rotl_i64(d, a, t0);
> -tcg_temp_free_i64(t0);
> -}
> -
> -static DisasJumpType op_verllv(DisasContext *s, DisasOps *o)
> -{
> -const uint8_t es = get_field(s, m4);
> -static const GVecGen3 g[4] = {
> -{ .fno = gen_helper_gvec_verllv8, },
> -{ .fno = gen_helper_gvec_verllv16, },
> -{ .fni4 = gen_rll_i32, },
> -{ .fni8 = gen_rll_i64, },
> -};
> -
> -if (es > ES_64) {
> -gen_program_exception(s, PGM_SPECIFICATION);
> -return DISAS_NORETURN;
> -}
> -
> -gen_gvec_3(get_field(s, v1), get_field(s, v2),
> -   get_field(s, v3), &g[es]);
> -return DISAS_NEXT;
> -}
> -
> -static DisasJumpType op_verll(DisasContext *s, DisasOps *o)
> -{
> -const uint8_t es = get_field(s, m4);
> -static const GVecGen2s g[4] = {
> -{ .fno = gen_helper_gvec_verll8, },
> -{ .fno = gen_helper_gvec_verll16, },
> -{ .fni4 = gen_rll_i32, },
> -{ .fni8 = gen_rll_i64, },
> -};
> -
> -if (es > ES_64) {
> -gen_program_exception(s, PGM_SPECIFICATION);
> -return DISAS_NORETURN;
> -}
> -gen_gvec_2s(get_field(s, v1), get_field(s, v3), o->addr1,
> -&g[es]);
> -return DISAS_NEXT;
> -}
> -
>  static void gen_rim_i32(TCGv_i32 d, TCGv_i32 a, TCGv_i32 b, int32_t c)
>  {
>  TCGv_i32 t = tcg_temp_new_i32();
> @@ -1946,6 +1889,9 @@ static DisasJumpType op_vesv(DisasContext *s, DisasOps 
> *o)
>  case 0x70:
>  gen_gvec_fn_3(shlv, es, v1, v2, v3);
>  break;
> +case 0x73:
> +gen_gvec_fn_3(rotlv, es, v1, v2, v3);
> +break;
>  case 0x7a:
>  gen_gvec_fn_3(sarv, es, v1, v2, v3);
>  break;
> @@ -1977,6 +1923,9 @@ static DisasJumpType op_ves(DisasContext *s, DisasOps 
> *o)
>  case 0x30:
>  gen_gvec_fn_2i(shli, es, v1, v3, d2);
>  break;
> +case 0x33:
> +gen_gvec_fn_2i(rotli, es, v1, v3, d2);
> +break;
>  case 0x3a:
>  gen_gvec_fn_2i(sari, es, v1, v3, d2);
>  break;
> @@ -1994,6 +1943,9 @@ static DisasJumpType op_ves(DisasContext *s, DisasOps 
> *o)
>  case 0x30:
>  gen_gvec_fn_2s(shls, es, v1, v3, shift);
>  break;
> +case 0x33:
> +gen_gvec_fn_2s(rotls, es, v1, v3, shift);
> +break;
>  case 0x3a:
>  gen_gvec_fn_2s(sars, es, v1, v3, shift);
>  break;
> diff --git a/target/s390x/vec_int_helper.c b/target/s390x/vec_int_helper.c
> index 0d6bc13dd6..5561b3ed

Re: [PATCH v3 4/9] blockdev: Promote several bitmap functions to non-static

[Max Reitz]

On 08.05.20 20:03, Eric Blake wrote:
> The next patch will split blockdev.c, which will require accessing
> some previously-static functions from more than one .c file.  But part
> of promoting a function to public is picking a naming scheme that does
> not reek of exposing too many internals (two of the three functions
> were named starting with 'do_').  To make future code motion easier,
> perform the function rename and non-static promotion into its own
> patch.
> 
> Signed-off-by: Eric Blake 
> ---
>  include/block/block_int.h | 12 +++
>  blockdev.c| 45 ---
>  2 files changed, 30 insertions(+), 27 deletions(-)

Reviewed-by: Max Reitz 

> diff --git a/blockdev.c b/blockdev.c
> index b3c840ec0312..fbeb38437869 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -2504,9 +2495,10 @@ out:
>  aio_context_release(aio_context);
>  }
> 
> -static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
> -const char *node, const char *name, bool release,
> -BlockDriverState **bitmap_bs, Error **errp)
> +BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char 
> *name,
> +   bool release,
> +   BlockDriverState **bitmap_bs,

I’m not entirely sure what this parameter is even used for, but it
obviously doesn’t concern this patch.

Max

> +   Error **errp)
>  {
>  BlockDriverState *bs;
>  BdrvDirtyBitmap *bitmap;



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3] aspeed: Add boot stub for smp booting

[Peter Maydell]

On Fri, 8 May 2020 at 15:27, Cédric Le Goater  wrote:
> Indeed, with OpenBMC kernel v5.4.32-260-g7dc9442bbe7d and wfi (patch v3),
>
> [0.045214] smp: Bringing up secondary CPUs ...
> [1.178127] CPU1: failed to come online
> [1.187309] smp: Brought up 1 node, 1 CPU
> [1.187590] SMP: Total of 1 processors activated (2250.00 BogoMIPS).
> [1.187786] CPU: All CPU(s) started in HYP mode.
> [1.187850] CPU: Virtualization extensions available.
>
> When using wfe (patch v2),
>
> [0.091092] smp: Bringing up secondary CPUs ...
> [0.096628] smp: Brought up 1 node, 2 CPUs
> [0.096718] SMP: Total of 2 processors activated (4500.00 BogoMIPS).
> [0.096768] CPU: All CPU(s) started in HYP mode.
> [0.096785] CPU: Virtualization extensions available.

OK, I've applied patch v2 to target-arm.next.

v3 looked a bit odd anyway -- a wfi-based secondary loop generally
needs to prod the GIC to enable interrupts for the core before
going into its loop (compare the 32-bit smpboot[] code in hw/arm/boot.c).
But looking at the code in the kernel's arch/arm/mach-aspeed/platsmp.c,
it uses dsb_sev() to prod the secondary core, so only wfe will
work here. If you find that you are often in situations where the
secondary cores are not started and so QEMU is wasting host CPU
in this busy-loop, you could look at implementing a non-noop
wfe/sev in QEMU, though it's not completely trivial because of
all the things other than sev that are 'wfe wake-up events'.

thanks
-- PMM

Re: [PATCH v2 4/4] exec: Rename qemu_ram_writeback() as qemu_ram_msync()

[Stefan Hajnoczi]

On Mon, May 11, 2020 at 10:17:19AM +0200, Philippe Mathieu-Daudé wrote:
> Rename qemu_ram_writeback() as qemu_ram_msync() to better
> match what it does.

Based on Juan's comment in the other email thread I think this commit
description could be expanded. Let's explain the rationale for this
change:

  qemu_ram_writeback() and memory_region_do_writeback() have similar names
  but perform different functions. qemu_ram_writeback() is concerned with
  syncing changes to the underlying memory device (NVDIMM or file-backed
  RAM). memory_region_do_writeback() is concerned with dirty memory
  logging.

  Rename qemu_ram_writeback() to prevent confusion with
  memory_region_do_writeback().

> 
> Suggested-by: Stefan Hajnoczi 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/exec/ram_addr.h | 4 ++--
>  exec.c  | 2 +-
>  memory.c| 3 +--
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index a94809f89b..84817e19fa 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -136,12 +136,12 @@ void qemu_ram_free(RAMBlock *block);
>  
>  int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
>  
> -void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t 
> length);
> +void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length);
>  
>  /* Clear whole block of mem */
>  static inline void qemu_ram_block_writeback(RAMBlock *block)
>  {
> -qemu_ram_writeback(block, 0, block->used_length);
> +qemu_ram_msync(block, 0, block->used_length);
>  }
>  
>  #define DIRTY_CLIENTS_ALL ((1 << DIRTY_MEMORY_NUM) - 1)
> diff --git a/exec.c b/exec.c
> index 2874bb5088..f5a8cdf370 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2127,7 +2127,7 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t 
> newsize, Error **errp)
>   * Otherwise no-op.
>   * @Note: this is supposed to be a synchronous op.
>   */
> -void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
> +void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length)
>  {
>  /* The requested range should fit in within the block range */
>  g_assert((start + length) <= block->used_length);
> diff --git a/memory.c b/memory.c
> index 73534b26f4..5bfa1429df 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2197,7 +2197,6 @@ void memory_region_ram_resize(MemoryRegion *mr, 
> ram_addr_t newsize, Error **errp
>  qemu_ram_resize(mr->ram_block, newsize, errp);
>  }
>  
> -
>  void memory_region_sync(MemoryRegion *mr, hwaddr addr, hwaddr size)
>  {
>  /*
> @@ -2205,7 +2204,7 @@ void memory_region_sync(MemoryRegion *mr, hwaddr addr, 
> hwaddr size)
>   * different types of memory regions
>   */
>  if (mr->ram_block) {
> -qemu_ram_writeback(mr->ram_block, addr, size);
> +qemu_ram_msync(mr->ram_block, addr, size);
>  }
>  }
>  
> -- 
> 2.21.3
> 


signature.asc
Description: PGP signature

Re: [PATCH 0/3] tcg/s390: Support host vector operations

[David Hildenbrand]

On 11.05.20 10:50, David Hildenbrand wrote:
> On 08.05.20 17:57, Richard Henderson wrote:
>> I've had this floating around on a branch for a while now
>> It is able to run arm sve tests under qemu, but I have not
>> been able to test it against real hardware. 
> 
> You can run qemu-system-arm under qemu-system-s390 with vx=on

(re-reading, I assume you did exactly that already)

I'm having issues building this due to lack of HWCAP_S390_VX.

[linux1@rhkvm01 qemu]$ cat /etc/redhat-release
Red Hat Enterprise Linux Server release 7.7 (Maipo)

-- 
Thanks,

David / dhildenb

Re: [PATCH v16 QEMU 09/16] vfio: Add save state functions to SaveVMHandlers

[Kirti Wankhede]

On 5/5/2020 10:07 AM, Alex Williamson wrote:

On Tue, 5 May 2020 04:48:14 +0530
Kirti Wankhede  wrote:


On 3/26/2020 3:33 AM, Alex Williamson wrote:

On Wed, 25 Mar 2020 02:39:07 +0530
Kirti Wankhede  wrote:
   

Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy
functions. These functions handles pre-copy and stop-and-copy phase.

In _SAVING|_RUNNING device state or pre-copy phase:
- read pending_bytes. If pending_bytes > 0, go through below steps.
- read data_offset - indicates kernel driver to write data to staging
buffer.
- read data_size - amount of data in bytes written by vendor driver in
migration region.
- read data_size bytes of data from data_offset in the migration region.
- Write data packet to file stream as below:
{VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data,
VFIO_MIG_FLAG_END_OF_STATE }

In _SAVING device state or stop-and-copy phase
a. read config space of device and save to migration file stream. This
 doesn't need to be from vendor driver. Any other special config state
 from driver can be saved as data in following iteration.
b. read pending_bytes. If pending_bytes > 0, go through below steps.
c. read data_offset - indicates kernel driver to write data to staging
 buffer.
d. read data_size - amount of data in bytes written by vendor driver in
 migration region.
e. read data_size bytes of data from data_offset in the migration region.
f. Write data packet as below:
 {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data}
g. iterate through steps b to f while (pending_bytes > 0)
h. Write {VFIO_MIG_FLAG_END_OF_STATE}

When data region is mapped, its user's responsibility to read data from
data_offset of data_size before moving to next steps.

Signed-off-by: Kirti Wankhede 
Reviewed-by: Neo Jia 
---
   hw/vfio/migration.c   | 245 
+-
   hw/vfio/trace-events  |   6 ++
   include/hw/vfio/vfio-common.h |   1 +
   3 files changed, 251 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 033f76526e49..ecbeed5182c2 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, 
uint32_t mask,
   return 0;
   }
   
+static void *find_data_region(VFIORegion *region,

+  uint64_t data_offset,
+  uint64_t data_size)
+{
+void *ptr = NULL;
+int i;
+
+for (i = 0; i < region->nr_mmaps; i++) {
+if ((data_offset >= region->mmaps[i].offset) &&
+(data_offset < region->mmaps[i].offset + region->mmaps[i].size) &&
+(data_size <= region->mmaps[i].size)) {


(data_offset - region->mmaps[i].offset) can be non-zero, so this test
is invalid.  Additionally the uapi does not require that a give data
chunk fits exclusively within an mmap'd area, it may overlap one or
more mmap'd sections of the region, possibly with non-mmap'd areas
included.
   


What's the advantage of having mmap and non-mmap overlapped regions?
Isn't it better to have data section either mapped or trapped?


The spec allows for it, therefore we need to support it.  A vendor
driver might choose to include a header with sequence and checksum
information for each transaction, they might accomplish this by setting
data_offset to a trapped area backed by kernel memory followed by an
area supporting direct mmap to the device.  The target end could then
fault on writing the header if the sequence information is incorrect.
A trapped area at the end of the transaction could allow the vendor
driver to validate a checksum.



If mmap and non-mmap regions overlapped is allowed then here read() 
should be used, which means buffer is allocated, then get data in buffer 
(first memcpy) and then call qemu_put_buffer(f, buf, data_size) (second 
memcpy)


Advantage of using full mmaped region for data, qemu_put_buffer(f, buf, 
data_size) directly uses pointer to mmaped region and so we reduce one 
memcpy.



+ptr = region->mmaps[i].mmap + (data_offset -
+   region->mmaps[i].offset);
+break;
+}
+}
+return ptr;
+}
+
+static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev)
+{
+VFIOMigration *migration = vbasedev->migration;
+VFIORegion *region = &migration->region;
+uint64_t data_offset = 0, data_size = 0;
+int ret;
+
+ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset),
+region->fd_offset + offsetof(struct vfio_device_migration_info,
+ data_offset));
+if (ret != sizeof(data_offset)) {
+error_report("%s: Failed to get migration buffer data offset %d",
+ vbasedev->name, ret);
+return -EINVAL;
+}
+
+ret = pread(vbasedev->fd, &data_size, sizeof(data_size),
+region->fd_offset + offsetof(struct vfio_devic

Re: [PATCH 02/11] sysemu/accel: Restrict machine methods to system-mode

[Edgar E. Iglesias]

On Sat, May 09, 2020 at 03:09:01PM +0200, Philippe Mathieu-Daudé wrote:
> Restrict init_machine(), setup_post() and has_memory()
> to system-mode.

Reviewed-by: Edgar E. Iglesias 


> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/accel.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 47e5788530..e08b8ab8fa 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -37,10 +37,12 @@ typedef struct AccelClass {
>  /*< public >*/
>  
>  const char *name;
> +#ifndef CONFIG_USER_ONLY
>  int (*init_machine)(MachineState *ms);
>  void (*setup_post)(MachineState *ms, AccelState *accel);
>  bool (*has_memory)(MachineState *ms, AddressSpace *as,
> hwaddr start_addr, hwaddr size);
> +#endif
>  bool *allowed;
>  /*
>   * Array of global properties that would be applied when specific
> -- 
> 2.21.3
> 

Re: [PATCH 1/3] tcg/s390: Change FACILITY representation

[David Hildenbrand]

On 08.05.20 17:57, Richard Henderson wrote:
> We will shortly need to be able to check facilities
> beyond the first 64.  Instead of explicitly masking
> against s390_facilities, create a FACILITY macro
> that indexes an array.
> 
> Signed-off-by: Richard Henderson 
> ---
>  tcg/s390/tcg-target.h | 29 ---
>  tcg/s390/tcg-target.inc.c | 74 +++
>  2 files changed, 52 insertions(+), 51 deletions(-)
> 
> diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h
> index 07accabbd1..7ca48457ff 100644
> --- a/tcg/s390/tcg-target.h
> +++ b/tcg/s390/tcg-target.h
> @@ -52,16 +52,19 @@ typedef enum TCGReg {
>  /* A list of relevant facilities used by this translator.  Some of these
> are required for proper operation, and these are checked at startup.  */
>  
> -#define FACILITY_ZARCH_ACTIVE (1ULL << (63 - 2))
> -#define FACILITY_LONG_DISP(1ULL << (63 - 18))
> -#define FACILITY_EXT_IMM  (1ULL << (63 - 21))
> -#define FACILITY_GEN_INST_EXT (1ULL << (63 - 34))
> -#define FACILITY_LOAD_ON_COND (1ULL << (63 - 45))
> +#define FACILITY_ZARCH_ACTIVE 2
> +#define FACILITY_LONG_DISP18
> +#define FACILITY_EXT_IMM  21
> +#define FACILITY_GEN_INST_EXT 34
> +#define FACILITY_LOAD_ON_COND 45
>  #define FACILITY_FAST_BCR_SER FACILITY_LOAD_ON_COND
>  #define FACILITY_DISTINCT_OPS FACILITY_LOAD_ON_COND
> -#define FACILITY_LOAD_ON_COND2(1ULL << (63 - 53))
> +#define FACILITY_LOAD_ON_COND253
>  
> -extern uint64_t s390_facilities;
> +extern uint64_t s390_facilities[1];
> +
> +#define FACILITY(X) \
> +((s390_facilities[FACILITY_##X / 64] >> (63 - FACILITY_##X % 64)) & 1)

I'd have named this "HAVE_FACILITY" or similar.

Apart from that, looks good

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/3] tcg/s390: Merge TCG_AREG0 and TCG_REG_CALL_STACK into TCGReg

[David Hildenbrand]

On 08.05.20 17:57, Richard Henderson wrote:
> They are rightly values in the same enumeration.
> 
> Signed-off-by: Richard Henderson 
> ---
>  tcg/s390/tcg-target.h | 28 +++-
>  1 file changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h
> index 7ca48457ff..1e67c90ed2 100644
> --- a/tcg/s390/tcg-target.h
> +++ b/tcg/s390/tcg-target.h
> @@ -29,22 +29,13 @@
>  #define TCG_TARGET_TLB_DISPLACEMENT_BITS 19
>  
>  typedef enum TCGReg {
> -TCG_REG_R0 = 0,
> -TCG_REG_R1,
> -TCG_REG_R2,
> -TCG_REG_R3,
> -TCG_REG_R4,
> -TCG_REG_R5,
> -TCG_REG_R6,
> -TCG_REG_R7,
> -TCG_REG_R8,
> -TCG_REG_R9,
> -TCG_REG_R10,
> -TCG_REG_R11,
> -TCG_REG_R12,
> -TCG_REG_R13,
> -TCG_REG_R14,
> -TCG_REG_R15
> +TCG_REG_R0,  TCG_REG_R1,  TCG_REG_R2,  TCG_REG_R3,
> +TCG_REG_R4,  TCG_REG_R5,  TCG_REG_R6,  TCG_REG_R7,
> +TCG_REG_R8,  TCG_REG_R9,  TCG_REG_R10, TCG_REG_R11,
> +TCG_REG_R12, TCG_REG_R13, TCG_REG_R14, TCG_REG_R15,
> +
> +TCG_AREG0 = TCG_REG_R10,
> +TCG_REG_CALL_STACK = TCG_REG_R15
>  } TCGReg;
>  
>  #define TCG_TARGET_NB_REGS 16
> @@ -135,7 +126,6 @@ extern uint64_t s390_facilities[1];
>  #define TCG_TARGET_HAS_mulsh_i64  0
>  
>  /* used for function call generation */
> -#define TCG_REG_CALL_STACK   TCG_REG_R15
>  #define TCG_TARGET_STACK_ALIGN   8
>  #define TCG_TARGET_CALL_STACK_OFFSET 160
>  
> @@ -144,10 +134,6 @@ extern uint64_t s390_facilities[1];
>  
>  #define TCG_TARGET_DEFAULT_MO (TCG_MO_ALL & ~TCG_MO_ST_LD)
>  
> -enum {
> -TCG_AREG0 = TCG_REG_R10,
> -};
> -
>  static inline void flush_icache_range(uintptr_t start, uintptr_t stop)
>  {
>  }
> 

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/2] hw/block/nvme: fixes for interrupt behavior

[Klaus Jensen]

On May 11 09:09, Philippe Mathieu-Daudé wrote:
> + Michael & Marcel for MSIX,
> and ping to Keith :)
> 

I'll await some feedback here before moving these fixes into my "nvme:
refactoring and cleanups" series. Not sure if this should go directly to
master or block-next.

Re: [PATCH v3 0/1] target/arm: Remove access_el3_aa32ns_aa64any()

[Peter Maydell]

On Tue, 5 May 2020 at 15:17, Edgar E. Iglesias  wrote:
>
> From: "Edgar E. Iglesias" 
>
> Hi,
>
> Laurent reported hitting the assert in access_el3_aa32ns()
> when accessing 32-bit versions of some of the virtualization
> regs when EL3 is 64-bit.
>
> I think we got this wrong back then and it seems to me like
> we should merge access_el3_aa32ns and access_el3_aa32ns_aa64_any()
> and always call the merged function to handle both aa32-only cases
> and mixed aa32/aa64.



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] aspeed: Support AST2600A1 silicon revision

[Peter Maydell]

On Mon, 4 May 2020 at 10:37, Joel Stanley  wrote:
>
> There are minimal differences from Qemu's point of view between the A0
> and A1 silicon revisions.
>
> As the A1 exercises different code paths in u-boot it is desirable to
> emulate that instead.
>
> Signed-off-by: Joel Stanley 



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] aspeed: sdmc: Implement AST2600 locking behaviour

[Peter Maydell]

On Tue, 5 May 2020 at 10:01, Joel Stanley  wrote:
>
> The AST2600 handles this differently with the extra 'hardlock' state, so
> move the testing to the soc specific class' write callback.
>
> Signed-off-by: Joel Stanley 
> ---



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0

[Daniel P . Berrangé]

On Fri, May 08, 2020 at 08:42:22PM +0800, Li Feng wrote:
> Marc-André Lureau  于2020年5月8日周五 下午8:32写道：
> >
> > Hi
> >
> > On Fri, May 8, 2020 at 7:14 AM Li Feng  wrote:
> > >
> > > Root cause:
> > > From `man recvmsg`, the RETURN VALUE says:
> > > These  calls return the number of bytes received, or -1 if an error 
> > > occurred.
> > > In the event of an error, errno is set to indicate the error.
> > > The return value will be 0 when the peer has performed an orderly 
> > > shutdown.
> > >
> > > When an error happens, the socket will be closed, and recvmsg return 0,
> > > then error_setg will trigger a crash.
> > >
> > > This unit test could reproduce this issue:
> > > tests/test-char -p /char/socket/client/reconnect-error/unix
> >
> > Current master doesn't trigger the backtrace, it's only after your patch 2.
> Yes. However, the issue did exist in the master code base.
> The test case in patch 2 revealed this issue.
> 
> >
> > >
> > > The core file backtrace is :
> > >
> > > (gdb) bt
> > > #0  0x75ac3277 in raise () from /lib64/libc.so.6
> > > #1  0x75ac4968 in abort () from /lib64/libc.so.6
> > > #2  0x555aaa94 in error_handle_fatal (errp=, 
> > > err=0x7fffec0012d0) at util/error.c:40
> > > #3  0x555aab6d in error_setv (errp=0x55802a08 
> > > , src=0x555c4280 "io/channel.c", line=148,
> > > func=0x555c4580 <__func__.17489> "qio_channel_readv_all", 
> > > err_class=ERROR_CLASS_GENERIC_ERROR,
> > > fmt=, ap=0x7423bae0, suffix=0x0) at 
> > > util/error.c:73
> > > #4  0x555aacf0 in error_setg_internal 
> > > (errp=errp@entry=0x55802a08 ,
> > > src=src@entry=0x555c4280 "io/channel.c", line=line@entry=148,
> > > func=func@entry=0x555c4580 <__func__.17489> 
> > > "qio_channel_readv_all",
> > > fmt=fmt@entry=0x555c43a0 "Unexpected end-of-file before all 
> > > bytes were read") at util/error.c:97
> > > #5  0x5556c25c in qio_channel_readv_all (ioc=, 
> > > iov=, niov=,
> > > errp=0x55802a08 ) at io/channel.c:147
> > > #6  0x5556c29a in qio_channel_read_all (ioc=, 
> > > buf=, buflen=,
> > > errp=) at io/channel.c:247
> > > #7  0x5556ad22 in char_socket_ping_pong (ioc=0x7fffec0008c0) 
> > > at tests/test-char.c:732
> > > #8  0x5556ae12 in char_socket_client_server_thread 
> > > (data=data@entry=0x5582e350) at tests/test-char.c:891
> > > #9  0x555a95b6 in qemu_thread_start (args=) at 
> > > util/qemu-thread-posix.c:519
> > > #10 0x75e61e25 in start_thread () from /lib64/libpthread.so.0
> > > #11 0x75b8bbad in clone () from /lib64/libc.so.6
> > >
> > > Signed-off-by: Li Feng 
> > > ---
> > >  io/channel.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/io/channel.c b/io/channel.c
> > > index e4376eb0bc..1a4a505f01 100644
> > > --- a/io/channel.c
> > > +++ b/io/channel.c
> > > @@ -144,8 +144,6 @@ int qio_channel_readv_all(QIOChannel *ioc,
> > >
> > >  if (ret == 0) {
> > >  ret = -1;
> > > -error_setg(errp,
> > > -   "Unexpected end-of-file before all bytes were read");
> >
> > Nack, this code is fine.
> >
> > The problem is that the test case doesn't expect a disconnect in
> > char_socket_ping_pong().
> Yes, in the test case I try to inject an error in char_socket_ping_pong.
> Any concerns about these two patches?

I agree with Marc-André - this patch is wrong. qio_channel_readv_all
*MUST* always set 'errp' if the return value is -1. To not set 'errp'
is violating the calling convention.

The bug here is in your test case - it is passing '&error_abort' to the
'qio_channel_readv_all' call.  If you don't want the test to crash, then
don't pass &error_abort

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

Re: [PULL v2 0/1] Merge tpm 2020/05/08 v2

[Peter Maydell]

On Fri, 8 May 2020 at 21:47, Stefan Berger  wrote:
>
> This PR submits a fix that changes improperly used 'FALSE' to 'false'.
>
> The following changes since commit c88f1ffc19e38008a1c33ae039482a860aa7418c:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging 
> (2020-05-08 14:29:18 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2020-05-08-1
>
> for you to fetch changes up to 441adbb4540423c90e4628a3c553e61d38445d73:
>
>   hw/tpm: fix usage of bool in tpm-tis.c (2020-05-08 16:40:45 -0400)
>
> 
> Jafar Abdi (1):
>   hw/tpm: fix usage of bool in tpm-tis.c
>
>  hw/tpm/tpm_tis_common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Hi; the commit in this pull request is missing your signed-off-by
as the submaintainer; could you fix that and resend it, please?

thanks
-- PMM

[Bug 1877716] Re: Win10 guest unusable after a few minutes

[post-factum]

Stefan,

Arch explicitly disabled io_uring for qemu after discovering this bug.
That's why you don't see it enabled in the recent version.

5.0.0-6 doesn't have io_uring enabled.
5.0.0-5 does have it, and you can grab it here: [1].

[1]
https://archive.archlinux.org/packages/q/qemu/qemu-5.0.0-5-x86_64.pkg.tar.zst

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

Title:
  Win10 guest unusable after a few minutes

Status in QEMU:
  New

Bug description:
  On Arch Linux, the recent qemu package update seems to misbehave on
  some systems. In my case, my Windows 10 guest runs fine for around 5
  minutes and then start to get really sluggish, even unresponsive. It
  needs to be forced off. I could reproduce this on a minimal VM with no
  passthrough, although my current testing setup involves an nvme pcie
  passthrough.

  I bisected it to the following commit which rapidly starts to run sluggishly 
on my setup:
  https://github.com/qemu/qemu/commit/73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf

  I've ran the previous commit (
  https://github.com/qemu/qemu/commit/b321051cf48ccc2d3d832af111d688f2282f089b
  ) for the entire night without an issue so far.

  I believe this might be a duplicate of
  https://bugs.launchpad.net/qemu/+bug/1873032 , although I'm not sure.

  Linux cc 5.6.10-arch1-1 #1 SMP PREEMPT Sat, 02 May 2020 19:11:54 + x86_64 
GNU/Linux
  AMD Ryzen 7 2700X Eight-Core Processor

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

Re: [PATCH 1/2] When del child, next_child_index need to subtract 1. Otherwise, the index will get bigger and bigger.

[Alberto Garcia]

On Mon 11 May 2020 12:00:30 PM CEST, quwei...@huayun.com wrote:
> diff --git a/block/quorum.c b/block/quorum.c
> index 6d7a56b..a8272fe 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -1096,6 +1096,7 @@ static void quorum_del_child(BlockDriverState *bs, 
> BdrvChild *child,
>  memmove(&s->children[i], &s->children[i + 1],
>  (s->num_children - i - 1) * sizeof(BdrvChild *));
>  s->children = g_renew(BdrvChild *, s->children, --s->num_children);
> +s->next_child_index--;
>  bdrv_unref_child(bs, child);

This is not correct, quorum_del_child() allows you to remove any child
from the Quorum device, so nothing guarantees you that
next_child_index-1 is free.

Berto

Re: [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate()

[Greg Kurz]

On Mon, 11 May 2020 11:07:06 +0200
Cédric Le Goater  wrote:

> On 5/7/20 7:26 PM, Greg Kurz wrote:
> > It is the job of the ppc_radix64_get_fully_qualified_addr() function
> > which is called at the beginning of ppc_radix64_xlate() to set both
> > lpid *and* pid. It doesn't buy us anything to initialize them first.
> > 
> > Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to
> > set either lpid or pid, would be undetectable by static analysis tools
> > like coverity.
> > 
> > Signed-off-by: Greg Kurz 
> > ---
> >  target/ppc/mmu-radix64.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> > index c76879f65b78..5e2d912ee346 100644
> > --- a/target/ppc/mmu-radix64.c
> > +++ b/target/ppc/mmu-radix64.c
> > @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr 
> > eaddr, int rwx,
> >   bool cause_excp)
> >  {
> >  CPUPPCState *env = &cpu->env;
> > -uint64_t lpid = 0, pid = 0;
> > +uint64_t lpid, pid;
> >  ppc_v3_pate_t pate;
> >  int psize, prot;
> >  hwaddr g_raddr;
> > 
> 
> I am seeing this failure with gcc version 9.3.1 20200408 (Red Hat 9.3.1-2) 
> (GCC) 
> 
> target/ppc/mmu-radix64.c: In function ‘ppc_radix64_xlate’:
> target/ppc/mmu-radix64.c:314:12: error: ‘pid’ may be used uninitialized in 
> this function [-Werror=maybe-uninitialized]
>   314 | offset = pid * sizeof(struct prtb_entry);
>   | ~~~^
> target/ppc/mmu-radix64.c:439:20: note: ‘pid’ was declared here
>   439 | uint64_t lpid, pid;
>   |^~~
> target/ppc/mmu-radix64.c:458:14: error: ‘lpid’ may be used uninitialized in 
> this function [-Werror=maybe-uninitialized]
>   458 | if (!ppc64_v3_get_pate(cpu, lpid, &pate)) {
>   |  ^~~
>   CC  ppc64-softmmu/target/ppc/fpu_helper.o
> 
> 
> This seems like a compiler optimization issue.
> 

Ah... it seems that gcc is trying to be smart but it doesn't realize
that ppc_radix64_get_fully_qualified_addr() doesn't have any path
that leaves lpid or pid unset... :-\ Adding a default: case in both
switch statements is enough to silent gcc.

I guess it may be easier for David if I post a v2 of the entire series
that addresses all the comments.

Thanks!

> C.

Re: [PATCH v3 5/9] blockdev: Split off basic bitmap operations for qemu-img

[Max Reitz]

On 08.05.20 20:03, Eric Blake wrote:
> Upcoming patches want to add some basic bitmap manipulation abilities
> to qemu-img.  But blockdev.o is too heavyweight to link into qemu-img
> (among other things, it would drag in block jobs and transaction
> support - qemu-img does offline manipulation, where atomicity is less
> important because there are no concurrent modifications to compete
> with), so it's time to split off the bare bones of what we will need
> into a new file block/monitor/bitmap-qmp-cmds.o.
> 
> This is sufficient to expose 6 QMP commands for use by qemu-img (add,
> remove, clear, enable, disable, merge), as well as move the three
> helper functions touched in the previous patch.  Regarding
> MAINTAINERS, the new file is automatically part of block core, but
> also makes sense as related to other dirty bitmap files.
> 
> Signed-off-by: Eric Blake 
> ---
>  Makefile.objs   |   3 +-
>  block/monitor/bitmap-qmp-cmds.c | 323 
>  blockdev.c  | 284 
>  MAINTAINERS |   1 +
>  block/monitor/Makefile.objs |   1 +
>  5 files changed, 326 insertions(+), 286 deletions(-)
>  create mode 100644 block/monitor/bitmap-qmp-cmds.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index a7c967633acf..99774cfd2545 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -13,9 +13,8 @@ chardev-obj-y = chardev/
> 
>  authz-obj-y = authz/
> 
> -block-obj-y = nbd/
> +block-obj-y = block/ block/monitor/ nbd/ scsi/

This reads weird because it’s precisely the monitor that we don’t want
in qemu-img.

But I suppose block/monitor is the natural place for block functions
that are to be used by the monitor (and maybe other parties like
qemu-img).  And the monitor itself would never be placed under block/.
So I suppose it does make sense and I have no better suggestion.

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name

[Geert Uytterhoeven]

Hi Linus,

On Thu, Mar 26, 2020 at 10:18 PM Linus Walleij  wrote:
> On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
>  wrote:
> > Currently a GPIO lookup table can only refer to a specific GPIO by a
> > tuple, consisting of a GPIO controller label and a GPIO offset inside
> > the controller.
> >
> > However, a GPIO may also carry a line name, defined by DT or ACPI.
> > If present, the line name is the most use-centric way to refer to a
> > GPIO.  Hence add support for looking up GPIOs by line name.
> >
> > Implement this by reusing the existing gpiod_lookup infrastructure.
> > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
> > that this field can have two meanings, and update the kerneldoc and
> > GPIO_LOOKUP*() macros.
> >
> > Signed-off-by: Geert Uytterhoeven 
> > Reviewed-by: Ulrich Hecht 
> > Reviewed-by: Eugeniu Rosca 
> > Tested-by: Eugeniu Rosca 
>
> I kind of like this approach, however there are things here that
> need to be considered: the line name is in no way globally unique,
> and I think there are already quite a few GPIO chips that
> have the same line names assigned for every instance of that
> chip.
>
> gpiochip_set_desc_names() only warns if there is a line with
> the same name on the same gpio_chip.

on a _different_ gpio chip.

> I suppose we need to document that the line name look-up
> will be on a first-come-first-served basis: whatever line
> we find first with this name is what you will get a reference
> to, no matter what chip it is on, and it is possible albeit
> not recommended that some other chip has a line with the
> same name.

Agreed.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 02/11] sysemu/accel: Restrict machine methods to system-mode

[Cornelia Huck]

On Sat,  9 May 2020 15:09:01 +0200
Philippe Mathieu-Daudé  wrote:

> Restrict init_machine(), setup_post() and has_memory()
> to system-mode.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/accel.h | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Cornelia Huck 

Re: device hotplug & file handles

[Michal Privoznik]

On 5/7/20 4:49 PM, Gerd Hoffmann wrote:

   Hi,

For usb device pass-through (aka -device usb-host) it would be very
useful to pass file handles from libvirt to qemu.  The workflow would
change from ...

   (1) libvirt enables access to /dev/usb/$bus/$dev
   (2) libvirt passes $bus + $dev (using hostbus + hostaddr properties)
   to qemu.
   (3) qemu opens /dev/usb/$bus/$dev

... to ...

   (1) libvirt opens /dev/usb/$bus/$dev
   (2) libvirt passes filehandle to qemu.

Question is how can we pass the file descriptor best?  My idea would be
to simply add an fd property to usb-host:

  * Coldplug would be "-device usb-host,fd=" (cmd line).
  * Hotplug would be "device_add usb-host,fd=" (monitor).

Will that work from libvirt point of view?
Or does anyone have an better idea?

thanks,
   Gerd

PS: background: https://bugzilla.redhat.com/show_bug.cgi?id=1595525



I don't have a better idea, but a little background on why libvirt even 
invented private /dev in the first place. The reason was that 
occasionally, when udev ran its rules it would overwrite the security 
labels on /dev nodes set by libvirt and thus denying access to QEMU. See:


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

Now, I think there is the same risk with what you are proposing. This 
isn't problem for DAC where permissions are checked during open(), but 
it may be a problem for SELinux where each individual operation with the 
FD is inspected.


Having said that, I am not against this approach, in fact I'm in favour 
of it. Let's hope that people learned that having udev overwriting 
seclabels is a bad idea and the bug won't appear again.


Michal

Re: [PATCH v5 04/19] accel/tcg: Adjust probe_access call to page_check_range

[Peter Maydell]

On Fri, 8 May 2020 at 17:57, Richard Henderson
 wrote:
> On 5/8/20 9:13 AM, Peter Maydell wrote:
> > We've gone round this multiple times now so I feel like
> > I must be missing something here.
>
> While probe_access() has a size parameter, probe_access_flags() does not.
>
> For probe_access_internal(), I currently have a "fault_size" parameter that
> gets passed to tlb_fill, which is "size" for probe_access() and 0 for
> probe_access_flags().
>
> I *could* add another "check_size" parameter to probe_access_internal, to be
> passed on to page_check_range(). It would be "size" for probe_access() and 1
> for probe_access_flags().  But what's the point?  Always passing 1 to
> page_check_range() has the same effect.
>
> I feel like I'm missing something with your objection.

The thing I was missing was that probe_access_flags() doesn't
have a size to pass usefully to probe_access_internal() and
so the size is zero in that case, but that tlb_fill() and
probe_check_range() want different values for the "just
tell me about this address" case.

thanks
-- PMM

Re: [PATCH v16 QEMU 09/16] vfio: Add save state functions to SaveVMHandlers

[Kirti Wankhede]

On 5/9/2020 11:01 AM, Yan Zhao wrote:

On Wed, Mar 25, 2020 at 05:09:07AM +0800, Kirti Wankhede wrote:

Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy
functions. These functions handles pre-copy and stop-and-copy phase.

In _SAVING|_RUNNING device state or pre-copy phase:
- read pending_bytes. If pending_bytes > 0, go through below steps.
- read data_offset - indicates kernel driver to write data to staging
   buffer.
- read data_size - amount of data in bytes written by vendor driver in
   migration region.

I think we should change the sequence of reading data_size and
data_offset. see the next comment below.


- read data_size bytes of data from data_offset in the migration region.
- Write data packet to file stream as below:
{VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data,
VFIO_MIG_FLAG_END_OF_STATE }

In _SAVING device state or stop-and-copy phase
a. read config space of device and save to migration file stream. This
doesn't need to be from vendor driver. Any other special config state
from driver can be saved as data in following iteration.
b. read pending_bytes. If pending_bytes > 0, go through below steps.
c. read data_offset - indicates kernel driver to write data to staging
buffer.
d. read data_size - amount of data in bytes written by vendor driver in
migration region.
e. read data_size bytes of data from data_offset in the migration region.
f. Write data packet as below:
{VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data}
g. iterate through steps b to f while (pending_bytes > 0)
h. Write {VFIO_MIG_FLAG_END_OF_STATE}

When data region is mapped, its user's responsibility to read data from
data_offset of data_size before moving to next steps.

Signed-off-by: Kirti Wankhede 
Reviewed-by: Neo Jia 
---
  hw/vfio/migration.c   | 245 +-
  hw/vfio/trace-events  |   6 ++
  include/hw/vfio/vfio-common.h |   1 +
  3 files changed, 251 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 033f76526e49..ecbeed5182c2 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, 
uint32_t mask,
  return 0;
  }
  
+static void *find_data_region(VFIORegion *region,

+  uint64_t data_offset,
+  uint64_t data_size)
+{
+void *ptr = NULL;
+int i;
+
+for (i = 0; i < region->nr_mmaps; i++) {
+if ((data_offset >= region->mmaps[i].offset) &&
+(data_offset < region->mmaps[i].offset + region->mmaps[i].size) &&
+(data_size <= region->mmaps[i].size)) {
+ptr = region->mmaps[i].mmap + (data_offset -
+   region->mmaps[i].offset);
+break;
+}
+}
+return ptr;
+}
+
+static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev)
+{
+VFIOMigration *migration = vbasedev->migration;
+VFIORegion *region = &migration->region;
+uint64_t data_offset = 0, data_size = 0;
+int ret;
+
+ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset),
+region->fd_offset + offsetof(struct vfio_device_migration_info,
+ data_offset));
+if (ret != sizeof(data_offset)) {
+error_report("%s: Failed to get migration buffer data offset %d",
+ vbasedev->name, ret);
+return -EINVAL;
+}
+
+ret = pread(vbasedev->fd, &data_size, sizeof(data_size),
+region->fd_offset + offsetof(struct vfio_device_migration_info,
+ data_size));
+if (ret != sizeof(data_size)) {
+error_report("%s: Failed to get migration buffer data size %d",
+ vbasedev->name, ret);
+return -EINVAL;
+}

data_size should be read first, and if it's 0, data_offset will not
be read further.

the reasons are below:
1. if there's no data region provided by vendor driver, there's no
reason to get a valid data_offset, so reading/writing of data_offset
should fail. And this should not be treated as a migration error.

2. even if pending_bytes is 0, vfio_save_iterate() is still possible to be
called and therefore vfio_save_buffer() is called.



As I mentioned in reply to Alex in:
https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg02476.html

With that, vfio_save_iterate() will read pending_bytes if its 0 and then 
if pending_bytes is not 0 then call vfio_save_buffer(). With that your 
above concerns should get resolved.


Thanks,
Kirti

Re: [PATCH v8 03/74] cpu: introduce cpu_mutex_lock/unlock

[Alex Bennée]

Robert Foley  writes:

> From: "Emilio G. Cota" 
>
> The few direct users of &cpu->lock will be converted soon.
>
> The per-thread bitmap introduced here might seem unnecessary,
> since a bool could just do. However, once we complete the
> conversion to per-vCPU locks, we will need to cover the use
> case where all vCPUs are locked by the same thread, which
> explains why the bitmap is introduced here.
>
> Reviewed-by: Richard Henderson 
> Signed-off-by: Emilio G. Cota 
> Signed-off-by: Robert Foley 
> ---
>  cpus.c| 48 +--
>  include/hw/core/cpu.h | 33 +
>  stubs/Makefile.objs   |  1 +
>  stubs/cpu-lock.c  | 28 +
>  4 files changed, 108 insertions(+), 2 deletions(-)
>  create mode 100644 stubs/cpu-lock.c
>
> diff --git a/cpus.c b/cpus.c
> index 71bd2f5d55..633734fb5c 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -91,6 +91,47 @@ static unsigned int throttle_percentage;
>  #define CPU_THROTTLE_PCT_MAX 99
>  #define CPU_THROTTLE_TIMESLICE_NS 1000
>  
> +/* XXX: is this really the max number of CPUs? */
> +#define CPU_LOCK_BITMAP_SIZE 2048

I wonder if we should be asserting this somewhere? Given it's an init
time constant we can probably do it somewhere in the machine realise
stage. I think the value is set in  MachineState *ms->smp.max_cpus;


> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 45be5dc0ed..d2dd6c94cc 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -5,6 +5,7 @@ stub-obj-y += blockdev-close-all-bdrv-states.o
>  stub-obj-y += clock-warp.o
>  stub-obj-y += cpu-get-clock.o
>  stub-obj-y += cpu-get-icount.o
> +stub-obj-y += cpu-lock.o
>  stub-obj-y += dump.o
>  stub-obj-y += error-printf.o
>  stub-obj-y += fdset.o
> diff --git a/stubs/cpu-lock.c b/stubs/cpu-lock.c
> new file mode 100644
> index 00..ca2ea8a9c2
> --- /dev/null
> +++ b/stubs/cpu-lock.c
> @@ -0,0 +1,28 @@
> +#include "qemu/osdep.h"
> +#include "hw/core/cpu.h"
> +
> +void cpu_mutex_lock_impl(CPUState *cpu, const char *file, int line)
> +{
> +/* coverity gets confused by the indirect function call */
> +#ifdef __COVERITY__
> +qemu_mutex_lock_impl(&cpu->lock, file, line);
> +#else
> +QemuMutexLockFunc f = atomic_read(&qemu_mutex_lock_func);
> +f(&cpu->lock, file, line);
> +#endif
> +}
> +
> +void cpu_mutex_unlock_impl(CPUState *cpu, const char *file, int line)
> +{
> +qemu_mutex_unlock_impl(&cpu->lock, file, line);
> +}

I find this a little confusing because we clearly aren't stubbing
something out here - we are indeed doing a lock. What we seem to have is
effectively the linux-user implementation of cpu locking which currently
doesn't support qsp profiling.

> +bool cpu_mutex_locked(const CPUState *cpu)
> +{
> +return true;
> +}
> +
> +bool no_cpu_mutex_locked(void)
> +{
> +return true;
> +}

What functions care about these checks. I assume it's only system
emulation checks that are in common code. Maybe that indicates we could
achieve better separation of emulation and linux-user code. My worry is
by adding an assert in linux-user code we wouldn't actually be asserting
anything.

-- 
Alex Bennée

Re: [PATCH 03/11] sysemu/tcg: Only declare tcg_allowed when TCG is available

[Cornelia Huck]

On Sat,  9 May 2020 15:09:02 +0200
Philippe Mathieu-Daudé  wrote:

> When TCG is not available, the tcg_allowed variable does not exist.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/tcg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH 04/11] sysemu/hvf: Only declare hvf_allowed when HVF is available

[Cornelia Huck]

On Sat,  9 May 2020 15:09:03 +0200
Philippe Mathieu-Daudé  wrote:

> When HVF is not available, the tcg_allowed variable does not exist.

s/tcg_allowed/hvf_allowed/

> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/hvf.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH v8 17/74] hw/semihosting: convert to cpu_halted_set

[Alex Bennée]

Robert Foley  writes:

> Signed-off-by: Robert Foley 

Reviewed-by: Alex Bennée 

> ---
>  hw/semihosting/console.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index 6346bd7f50..f70085f3c1 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -131,7 +131,7 @@ static void console_wake_up(gpointer data, gpointer 
> user_data)
>  {
>  CPUState *cs = (CPUState *) data;
>  /* cpu_handle_halt won't know we have work so just unbung here */
> -cs->halted = 0;
> +cpu_halted_set(cs, 0);
>  qemu_cpu_kick(cs);
>  }
>  
> @@ -154,7 +154,7 @@ target_ulong qemu_semihosting_console_inc(CPUArchState 
> *env)
>  g_assert(current_cpu);
>  if (fifo8_is_empty(&c->fifo)) {
>  c->sleeping_cpus = g_slist_prepend(c->sleeping_cpus, current_cpu);
> -current_cpu->halted = 1;
> +cpu_halted_set(current_cpu, 1);
>  current_cpu->exception_index = EXCP_HALTED;
>  cpu_loop_exit(current_cpu);
>  /* never returns */


-- 
Alex Bennée

Re: [PATCH v5 00/19] target/arm: sve load/store improvements

[Peter Maydell]

On Fri, 8 May 2020 at 16:44, Richard Henderson
 wrote:
>
> Version 5 splits patch 4, as requested during review.
> The patches remaining unreviewed are:
>
> 0004-accel-tcg-Adjust-probe_access-call-to-page_check_.patch
> 0005-accel-tcg-Add-probe_access_flags.patch

Applied to target-arm.next, thanks. Sorry it took me so long
to figure out what was going on with patches 4/5.

-- PMM

Re: QEMU + HVF Fails to start OVMF.fd (hang before displaying logo)

[Philippe Mathieu-Daudé]

Hi Olivier,

Cc'ing the HVF maintainers.

On 5/11/20 12:26 PM, LAHAYE Olivier wrote:

Hi,

I’m facing a similar problem to this one, but I’m unable to find any 
solution via google.


https://www.mail-archive.com/qemu-discuss@nongnu.org/msg04372.html

I’m trying to run an EFI BIOS using qemu.

  * It works fine without acceleration
  * It hangs (display not initialized) when using -accel hvf

I’ve tested many OVMF.fd files from internet including the official one 
of course whith no differences. (always work with no acceleration and 
always fail with HVF acceleration).


  * OS: MacOS 10.14.6
  * QEMU 5.0.0 monitor - type 'help' for more information
  * (qemu) qemu-system-x86_64: warning: host doesn't support requested
feature: CPUID.8001H:ECX.svm [bit 2]
  * info roms
  * fw=genroms/kvmvapic.bin size=0x002400 name="kvmvapic.bin"
  * addr=fff0 size=0x10 mem=rom
name="/Users/ol222822/data/OVMF.fd"
  * /rom@etc/acpi/tables size=0x20 name="etc/acpi/tables"
  * /rom@etc/table-loader size=0x001000 name="etc/table-loader"
  * /rom@etc/acpi/rsdp size=0x14 name="etc/acpi/rsdp"

Is it a known issue?

Is there something I’m missing?

I’m running qemu in the following way:

qemu-system-x86_64 -machine q35 -bios ~/data/OVMF.fd -monitor stdio

If I add -accel hvf, it’ll hang before tiano core logo.

Best regards,

--

Olivier LAHAYE

Re: [PATCH 06/11] target/s390x: Only compile decode_basedisp() on system-mode

[Cornelia Huck]

On Sat,  9 May 2020 15:09:05 +0200
Philippe Mathieu-Daudé  wrote:

> The decode_basedisp*() methods are only used in ioinst.c,
> which is only build in system-mode emulation.

I/O instructions are privileged, and other S instructions are decoded
elsewhere.

> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/s390x/internal.h | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Cornelia Huck 

Re: [PATCH v8 25/74] riscv: convert to cpu_halted

[Alex Bennée]

Robert Foley  writes:

> From: "Emilio G. Cota" 
>
> Cc: Palmer Dabbelt 
> Cc: Sagar Karandikar 
> Cc: Bastian Koppelmann 
> Cc: Alistair Francis 
> Reviewed-by: Palmer Dabbelt 

You can drop Cc: lines fron patches once you have an a-b or r-b tag from
the person in question. They are basically a formalised way of saying "I
have tried to contact the maintainer/relevant party" and memorialising
it in the patches. The effect of Cc, r-b and a-b tags in the patch is
all pretty much the same in that people get Cc'ed anyway - on top of
what get_maintainers.pl will add as well!

> Reviewed-by: Richard Henderson 
> Reviewed-by: Alistair Francis 
> Signed-off-by: Emilio G. Cota 
> Signed-off-by: Robert Foley 
> ---
>  target/riscv/op_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index c6412f680c..91f8833c2e 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -182,7 +182,7 @@ void helper_wfi(CPURISCVState *env)
>  riscv_cpu_virt_enabled(env)) {
>  riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>  } else {
> -cs->halted = 1;
> +cpu_halted_set(cs, 1);
>  cs->exception_index = EXCP_HLT;
>  cpu_loop_exit(cs);
>  }


-- 
Alex Bennée

Re: [PATCH 07/11] target/s390x/helper: Clean ifdef'ry

[Cornelia Huck]

On Sat,  9 May 2020 15:09:06 +0200
Philippe Mathieu-Daudé  wrote:

> All this code is guarded checking CONFIG_USER_ONLY definition.
> Drop the duplicated checks.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Suspicious ifdef'ry in s390_handle_wait() from commit 83f7f32901c.
> ---
>  target/s390x/helper.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 09f60406aa..26e3b366e8 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -33,6 +33,7 @@
>  #endif
>  
>  #ifndef CONFIG_USER_ONLY
> +

I'd probably not have added the whitespace here...

>  void s390x_tod_timer(void *opaque)
>  {
>  cpu_inject_clock_comparator((S390CPU *) opaque);

(...)

> @@ -328,6 +324,7 @@ int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, 
> hwaddr len)
>  cpu_physical_memory_unmap(sa, len, 1, len);
>  return 0;
>  }
> +

...and here, but I don't feel strongly about it.

>  #endif /* CONFIG_USER_ONLY */
>  
>  void s390_cpu_dump_state(CPUState *cs, FILE *f, int flags)

Reviewed-by: Cornelia Huck 

Re: [PATCH 0/4] hw/arm/nrf51: Extend tracing

[Peter Maydell]

On Mon, 4 May 2020 at 08:28, Philippe Mathieu-Daudé  wrote:
>
> Few patches while playing with the Zephyr Project.
>
> - better display of unimplemented peripheral accesses,
> - better display of timers use.
>
> Philippe Mathieu-Daudé (4):
>   hw/arm/nrf51: Add NRF51_PERIPHERAL_SIZE definition
>   hw/arm/nrf51_soc: Mark some peripherals as unimplemented
>   hw/timer/nrf51_timer: Display timer ID in trace events
>   hw/timer/nrf51_timer: Add trace event of counter value update

Hi; I've put patches 1, 3 and 4 into target-arm.next as they
have been reviewed and are independent of the unimp-peripherals
patch.

thanks
-- PMM

Re: [PATCH v3 0/5] target/arm: Restrict TCG cpus to TCG accel

[Peter Maydell]

On Mon, 4 May 2020 at 18:24, Philippe Mathieu-Daudé  wrote:
>
> These are the uncontroversial patches from "Support disabling
> TCG on ARM (part 2)"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg689168.html
>
> The other patches are blocked by the "accel: Allow targets to
> use Kconfig" series:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg689024.html
>
> Patch #4 is new "Restrict v8M IDAU interface to Aarch32 CPUs".
>
> Since v2:
> - Fixed set_feature() clash trying to KISS
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg697523.html
> - Fixed aarch64-linux-user build failure reported by Peter:
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg699319.html
>
> Since v1:
> - Dropped 'Make set_feature() available for other files' patch
>   which fails to build with KVM only, see:
> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03843.html
>
> Many thanks to Richard Henderson for his patience (again...)!



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH 08/11] target/s390x: Restrict system-mode declarations

[Cornelia Huck]

On Sat,  9 May 2020 15:09:07 +0200
Philippe Mathieu-Daudé  wrote:

> As these declarations are restricted to !CONFIG_USER_ONLY in
> helper.c, only declare them when system-mode emulation is used.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/s390x/internal.h | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index c1678dc6bc..ddc276cdf4 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -236,7 +236,6 @@ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>  
>  /* cc_helper.c */
>  const char *cc_name(enum cc_op cc_op);
> -void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
>  uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t 
> dst,
>   uint64_t vr);
>  
> @@ -303,18 +302,20 @@ void s390_cpu_gdb_init(CPUState *cs);
>  
>  /* helper.c */
>  void s390_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> -hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> -hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
> +void do_restart_interrupt(CPUS390XState *env);
> +
> +#ifndef CONFIG_USER_ONLY
> +void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);

load_psw() is in cc_helper.c (and not in helper.c). Rather add the
#ifndef above, even if it is a bit awkward? Otherwise, the wrong
comment makes it confusing.

>  uint64_t get_psw_mask(CPUS390XState *env);
>  void s390_cpu_recompute_watchpoints(CPUState *cs);
>  void s390x_tod_timer(void *opaque);
>  void s390x_cpu_timer(void *opaque);
> -void do_restart_interrupt(CPUS390XState *env);
>  void s390_handle_wait(S390CPU *cpu);
> +hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> +hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
>  #define S390_STORE_STATUS_DEF_ADDR offsetof(LowCore, floating_pt_save_area)
>  int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch);
>  int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len);
> -#ifndef CONFIG_USER_ONLY
>  LowCore *cpu_map_lowcore(CPUS390XState *env);
>  void cpu_unmap_lowcore(LowCore *lowcore);
>  #endif /* CONFIG_USER_ONLY */

Re: [RFC PATCH] hw/arm/musicpal: Map the UART devices unconditionally

[Peter Maydell]

On Tue, 5 May 2020 at 10:59, Philippe Mathieu-Daudé  wrote:
>
> I can't find proper documentation or datasheet, but it is likely
> a MMIO mapped serial device mapped in the 0x8000..0x8000
> range belongs to the SoC address space, thus is always mapped in
> the memory bus.
> Map the devices on the bus regardless a chardev is attached to it.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> from 2019... found while doing housekeeping
> ---




Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH v3] aspeed: Add support for the sonorapass-bmc board

[Peter Maydell]

On Wed, 6 May 2020 at 19:32, Patrick Williams  wrote:
>
> Sonora Pass is a 2 socket x86 motherboard designed by Facebook
> and supported by OpenBMC.  Strapping configuration was obtained
> from hardware and i2c configuration is based on dts found at:
>
> https://github.com/facebook/openbmc-linux/blob/1633c87b8ba7c162095787c988979b748ba65dc8/arch/arm/boot/dts/aspeed-bmc-facebook-sonorapass.dts
>
> Booted a test image of http://github.com/facebook/openbmc to login
> prompt.
>
> Signed-off-by: Patrick Williams 
> Reviewed-by: Amithash Prasad 
> Reviewed-by: Cédric Le Goater 

Looking up through the thread I can't find the email where
Amithash gave his reviewed-by tag -- did I miss it?

PS: for the future, v2/v3 etc patches should be sent as
fresh emails, not as followups/replies to the v1.

thanks
-- PMM

Re: [PATCH 3/4] device-core: use atomic_set on .realized property

[Maxim Levitsky]

On Mon, 2020-05-04 at 13:22 +0200, Paolo Bonzini wrote:
> On 04/05/20 12:45, Stefan Hajnoczi wrote:
> > > @@ -983,7 +983,7 @@ static void device_set_realized(Object *obj, bool 
> > > value, Error **errp)
> > >  }
> > >  
> > >  assert(local_err == NULL);
> > > -dev->realized = value;
> > > +atomic_set(&dev->realized, value);
> > 
> > A memory barrier is probably needed so that the atomic_read() thread
> > sees up-to-date dev fields.
> 
> Yes, it should be a store-release for the false->true case.  The
> true->false case probably doesn't matter as much.

On second thought, I think both cases matter, after I examined the device 
removal case.
In device removal case, the device is first un-realized and then removed from 
the bus,
so just like in device hotplug case, the scsi_device_find can give you an 
unrealized device.

I will change this patch to set .realized to false at the start (if needed) of 
the function and to true at the end (also if needed)
Will atomic_rcu_set work? or atomic_store_release?
(Both are the same thing, but former documents the purpose of using with RCU.

Best regards,
Maxim Levitsky



> 
> Paolo
> 

Re: [PATCH v2 0/4] target/arm: Misc cleanups

[Peter Maydell]

On Thu, 7 May 2020 at 18:23, Richard Henderson
 wrote:
>
> Version 2 adds a fix to a just merged patch.
>
>
> r~
>
>
> Richard Henderson (4):
>   target/arm: Use tcg_gen_gvec_5_ptr for sve FMLA/FCMLA
>   target/arm: Use tcg_gen_gvec_mov for clear_vec_high
>   target/arm: Use clear_vec_high more effectively
>   target/arm: Fix tcg_gen_gvec_dup_imm vs DUP (indexed)



Applied to target-arm.next, thanks.

-- PMM

Re: [virtio-dev] Re: Fwd: Qemu Support for Virtio Video V4L2 driver

[Saket Sinha]

Hi Keiichi,

I do not support the approach of  QEMU implementation forwarding
requests to the host's vicodec module since  this can limit the scope
of the virtio-video device only for testing, which instead can be used
with multiple use cases such as -

1. VM gets access to paravirtualized  camera devices which shares the
video frames input through actual HW camera attached to Host.

2. If Host has multiple video devices (especially in ARM SOCs over
MIPI interfaces or USB), different VM can be started or hotplugged
with selective video streams from actual HW video devices.

Also instead of using libraries like Gstreamer in Host userspace, they
can also be used inside the VM userspace after getting access to
paravirtualized HW camera devices .

Regards,
Saket Sinha

On Mon, May 11, 2020 at 12:20 PM Keiichi Watanabe  wrote:
>
> Hi Dmitry,
>
> On Mon, May 11, 2020 at 6:40 PM Dmitry Sepp  
> wrote:
> >
> > Hi Saket and all,
> >
> > As we are working with automotive platforms, unfortunately we don't plan any
> > Qemu reference implementation so far.
> >
> > Of course we are ready to support the community if any help is needed. Is
> > there interest in support for the FWHT format only for testing purpose or 
> > you
> > want a full-featured implementation on the QEMU side?
>
> I guess we don't need to implement the codec algorithm in QEMU.
> Rather, QEMU forwards virtio-video requests to the host video device
> or a software library such as GStreamer or ffmpeg.
> So, what we need to implement in QEMU is a kind of API translation,
> which shouldn't care about actual video formats so much.
>
> Regarding the FWHT format discussed in the patch thread [1], in my
> understanding, Hans suggested to have QEMU implementation forwarding
> requests to the host's vicodec module [2].
> Then, we'll be able to test the virtio-video driver on QEMU on Linux
> even if the host Linux has no hardware video decoder.
> (Please correct me if I'm wrong.)
>
> Let me add Hans and Linux media ML in CC.
>
> [1]  https://patchwork.linuxtv.org/patch/61717/
> [2] https://lwn.net/Articles/760650/
>
> Best regards,
> Keiichi
>
> >
> > Please note that the spec is not finalized yet and a major update is now
> > discussed with upstream and the Chrome OS team, which is also interested and
> > deeply involved in the process. The update mostly implies some rewording and
> > reorganization of data structures, but for sure will require a driver 
> > rework.
> >
> > Best regards,
> > Dmitry.
> >
> > On Samstag, 9. Mai 2020 16:11:43 CEST Saket Sinha wrote:
> > > Hi,
> > >
> > > As suggested on #qemu-devel IRC channel, I am including virtio-dev, Gerd 
> > > and
> > > Michael to point in the right direction how to move forward with Qemu
> > > support for Virtio Video V4L2 driver
> > > posted in [1].
> > >
> > > [1]: https://patchwork.linuxtv.org/patch/61717/
> > >
> > > Regards,
> > > Saket Sinha
> > >
> > > On Sat, May 9, 2020 at 1:09 AM Saket Sinha  
> > > wrote:
> > > > Hi ,
> > > >
> > > > This is to inquire about Qemu support for Virtio Video V4L2 driver
> > > > posted in [1].
> > > > I am currently not aware of any upstream effort for Qemu reference
> > > > implementation and would like to discuss how to proceed with the same.
> > > >
> > > > [1]: https://patchwork.linuxtv.org/patch/61717/
> > > >
> > > > Regards,
> > > > Saket Sinha
> >
> >
> >
> > -
> > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
> >

Re: [PATCH v3 6/9] qemu-img: Add bitmap sub-command

[Max Reitz]

On 08.05.20 20:03, Eric Blake wrote:
> Include actions for --add, --remove, --clear, --enable, --disable, and
> --merge (note that --clear is a bit of fluff, because the same can be
> accomplished by removing a bitmap and then adding a new one in its
> place, but it matches what QMP commands exist).  Listing is omitted,
> because it does not require a bitmap name and because it was already
> possible with 'qemu-img info'.  A single command line can play one or
> more bitmap commands in sequence on the same bitmap name (although all
> added bitmaps share the same granularity, and and all merged bitmaps
> come from the same source file).  Merge defaults to other bitmaps in
> the primary image, but can also be told to merge bitmaps from a
> distinct image.

For the record: Yes, my comment was mostly about my confusion around the
{}.  So just replacing them by () would have pacified me.

But this is more fun, of course.

> While this supports --image-opts for the file being modified, I did
> not think it worth the extra complexity to support that for the source
> file in a cross-file merges.  Likewise, I chose to have --merge only
> take a single source rather than following the QMP support for
> multiple merges in one go (although you can still use more than one
> --merge in the command line); in part because qemu-img is offline and
> therefore atomicity is not an issue.
> 
> Upcoming patches will add iotest coverage of these commands while
> also testing other features.
> 
> Signed-off-by: Eric Blake 
> ---
>  docs/tools/qemu-img.rst |  23 
>  qemu-img.c  | 254 
>  qemu-img-cmds.hx|   7 ++
>  3 files changed, 284 insertions(+)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 7d08c48d308f..68393c357386 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -281,6 +281,29 @@ Command description:

[...]

> +  Additional options ``-g`` set a non-default *GRANULARITY* for

sets?

With that fixed (or maybe not, you know that better than me):

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[PATCH 0/6] colo: migration related bugfixes

[Lukas Straub]

Hello Everyone,
Here are fixes for bugs that I found in my tests.

Regards,
Lukas Straub

Lukas Straub (6):
  migration/colo.c: Use event instead of semaphore
  migration/colo.c: Use cpu_synchronize_all_states()
  migration/colo.c: Flush ram cache only after receiving device state
  migration/colo.c: Relaunch failover even if there was an error
  migration/qemu-file.c: Don't ratelimit a shutdown fd
  migration/colo.c: Move colo_notify_compares_event to the right place

 migration/colo.c  | 39 ---
 migration/migration.h |  4 ++--
 migration/qemu-file.c |  2 +-
 migration/ram.c   |  5 +
 migration/ram.h   |  1 +
 5 files changed, 29 insertions(+), 22 deletions(-)

-- 
2.20.1


pgpkdHASstYKd.pgp
Description: OpenPGP digital signature

Re: [PATCH 3/4] device-core: use atomic_set on .realized property

[Paolo Bonzini]

On 11/05/20 13:00, Maxim Levitsky wrote:
> On second thought, I think both cases matter, after I examined the device 
> removal case.
> In device removal case, the device is first un-realized and then removed from 
> the bus,
> so just like in device hotplug case, the scsi_device_find can give you an 
> unrealized device.
> 
> I will change this patch to set .realized to false at the start (if needed) 
> of the function and to true at the end (also if needed)
> Will atomic_rcu_set work? or atomic_store_release?
> (Both are the same thing, but former documents the purpose of using with RCU.

atomic_rcu_set is more to store pointers, in this case you want to store
the value after any other change to the struct so atomic_store_release
is more appropriate.

Paolo

[PATCH 1/6] migration/colo.c: Use event instead of semaphore

[Lukas Straub]

If multiple packets miscompare in a short timeframe, the semaphore
value will be increased multiple times. This causes multiple
checkpoints even if one would be sufficient.

Fix this by using a event instead of a semaphore for triggering
checkpoints. Now, checkpoint requests will be ignored until the
checkpoint event is sent to colo-compare (which releases the
miscompared packets).

Benchmark results (iperf3):
Client-to-server tcp:
without patch: ~66 Mbit/s
with patch: ~61 Mbit/s
Server-to-client tcp:
without patch: ~702 Kbit/s
with patch: ~16 Mbit/s

Signed-off-by: Lukas Straub 
---
 migration/colo.c  | 9 +
 migration/migration.h | 4 ++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index a54ac84f41..09168627bc 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -430,6 +430,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
 goto out;
 }
 
+qemu_event_reset(&s->colo_checkpoint_event);
 colo_notify_compares_event(NULL, COLO_EVENT_CHECKPOINT, &local_err);
 if (local_err) {
 goto out;
@@ -580,7 +581,7 @@ static void colo_process_checkpoint(MigrationState *s)
 goto out;
 }
 
-qemu_sem_wait(&s->colo_checkpoint_sem);
+qemu_event_wait(&s->colo_checkpoint_event);
 
 if (s->state != MIGRATION_STATUS_COLO) {
 goto out;
@@ -628,7 +629,7 @@ out:
 colo_compare_unregister_notifier(&packets_compare_notifier);
 timer_del(s->colo_delay_timer);
 timer_free(s->colo_delay_timer);
-qemu_sem_destroy(&s->colo_checkpoint_sem);
+qemu_event_destroy(&s->colo_checkpoint_event);
 
 /*
  * Must be called after failover BH is completed,
@@ -645,7 +646,7 @@ void colo_checkpoint_notify(void *opaque)
 MigrationState *s = opaque;
 int64_t next_notify_time;
 
-qemu_sem_post(&s->colo_checkpoint_sem);
+qemu_event_set(&s->colo_checkpoint_event);
 s->colo_checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
 next_notify_time = s->colo_checkpoint_time +
 s->parameters.x_checkpoint_delay;
@@ -655,7 +656,7 @@ void colo_checkpoint_notify(void *opaque)
 void migrate_start_colo_process(MigrationState *s)
 {
 qemu_mutex_unlock_iothread();
-qemu_sem_init(&s->colo_checkpoint_sem, 0);
+qemu_event_init(&s->colo_checkpoint_event, false);
 s->colo_delay_timer =  timer_new_ms(QEMU_CLOCK_HOST,
 colo_checkpoint_notify, s);
 
diff --git a/migration/migration.h b/migration/migration.h
index 507284e563..f617960522 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -215,8 +215,8 @@ struct MigrationState
 /* The semaphore is used to notify COLO thread that failover is finished */
 QemuSemaphore colo_exit_sem;
 
-/* The semaphore is used to notify COLO thread to do checkpoint */
-QemuSemaphore colo_checkpoint_sem;
+/* The event is used to notify COLO thread to do checkpoint */
+QemuEvent colo_checkpoint_event;
 int64_t colo_checkpoint_time;
 QEMUTimer *colo_delay_timer;
 
-- 
2.20.1



pgprW5en_om8x.pgp
Description: OpenPGP digital signature

Re: [PATCH v1 0/7] various tcg and linux-user updates

[Alex Bennée]

Alex Bennée  writes:

> Hi,
>
> Cleaning up my queues into more focused trees these are all tweaks to
> TCG related stuff. The guest_base changes where posted before but
> where a little radical for 5.0 but I think are worth getting in early
> as it enables the sanitizer builds for a range of linux-user targets
> we couldn't run before. Finally there is a little tweak made to the
> out_asm handling which makes it a bit easier to see which guest
> instructions are being emulated by which host code.
>
> The following need review:
>
>  - translate-all: include guest address in out_asm output
>  - disas: add optional note support to cap_disas
>  - disas: include an optional note for the start of disassembly
>  - accel/tcg: don't disable exec_tb trace events
>  - linux-user: completely re-write init_guest_space

Gentle ping,

I would especially like some feed-back on the guest base updates from
the linux-user maintainers so we can get the sanitizers more widely
used.

If your happy for me to include them in my next PR I'll just take some
Acked-by's ;-)

-- 
Alex Bennée

[PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu

[Lukas Straub]

Hello Everyone,
In many cases, if qemu has a network connection (qmp, migration, chardev, etc.)
to some other server and that server dies or hangs, qemu hangs too.
These patches introduce the new 'yank' out-of-band qmp command to recover from
these kinds of hangs. The different subsystems register callbacks which get
executed with the yank command. For example the callback can shutdown() a
socket. This is intended for the colo use-case, but it can be used for other
things too of course.

Regards,
Lukas Straub

Lukas Straub (5):
  Introduce yank feature
  io/channel.c,io/channel-socket.c: Add yank feature
  block/nbd.c: Add yank feature
  chardev/char-socket.c: Add yank feature
  migration: Add yank feature

 Makefile.objs   |  2 +
 block/nbd.c | 68 -
 chardev/char-socket.c   |  8 
 chardev/char.c  |  3 ++
 include/io/channel-socket.h |  1 +
 include/io/channel.h| 12 ++
 io/channel-socket.c | 29 ++
 io/channel.c|  9 +
 migration/channel.c |  2 +
 migration/migration.c   | 11 ++
 qapi/block-core.json|  5 ++-
 qapi/char.json  |  5 ++-
 qapi/migration.json | 17 +++--
 qapi/misc.json  | 15 
 softmmu/vl.c|  2 +
 yank.c  | 75 +
 yank.h  | 12 ++
 17 files changed, 254 insertions(+), 22 deletions(-)
 create mode 100644 yank.c
 create mode 100644 yank.h

-- 
2.20.1


pgpWxvIVluKlN.pgp
Description: OpenPGP digital signature

[PATCH 1/5] Introduce yank feature

[Lukas Straub]

The yank feature allows to recover from hanging qemu by "yanking"
at various parts. Other qemu systems can register yank functions
which will be called by the 'yank' out-of-band qmp command.

Signed-off-by: Lukas Straub 
---
 qapi/misc.json | 15 ++
 softmmu/vl.c   |  2 ++
 yank.c | 75 ++
 yank.h | 12 
 4 files changed, 104 insertions(+)
 create mode 100644 yank.c
 create mode 100644 yank.h

diff --git a/qapi/misc.json b/qapi/misc.json
index 99b90ac80b..de1ee494ae 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1550,3 +1550,18 @@
 ##
 { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
 
+##
+# @yank:
+#
+# Recover from hanging qemu by calling yank functions.
+#
+# Returns: nothing.
+#
+# Example:
+#
+# -> { "execute": "yank" }
+# <- { "return": {} }
+#
+# Since: 5.1
+##
+{ 'command': 'yank', 'allow-oob': true }
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 32c0047889..5d99749d29 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -112,6 +112,7 @@
 #include "qapi/qmp/qerror.h"
 #include "sysemu/iothread.h"
 #include "qemu/guest-random.h"
+#include "yank.h"
 
 #define MAX_VIRTIO_CONSOLES 1
 
@@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char **envp)
 precopy_infrastructure_init();
 postcopy_infrastructure_init();
 monitor_init_globals();
+yank_init();
 
 if (qcrypto_init(&err) < 0) {
 error_reportf_err(err, "cannot initialize crypto: ");
diff --git a/yank.c b/yank.c
new file mode 100644
index 00..cefbfd8ab5
--- /dev/null
+++ b/yank.c
@@ -0,0 +1,75 @@
+/*
+ * QEMU yank feature
+ *
+ * Copyright (c) Lukas Straub 
+ *
+ * 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 "qapi/error.h"
+#include "qemu/thread.h"
+#include "qemu/queue.h"
+#include "yank.h"
+
+struct YankFuncAndParam {
+YankFn *func;
+void *opaque;
+QLIST_ENTRY(YankFuncAndParam) next;
+};
+
+static QemuMutex lock;
+static QLIST_HEAD(qlisthead, YankFuncAndParam) head
+= QLIST_HEAD_INITIALIZER(head);
+
+void yank_register_function(YankFn *func, void *opaque)
+{
+struct YankFuncAndParam *tmp = g_malloc(sizeof(struct YankFuncAndParam));
+tmp->func = func;
+tmp->opaque = opaque;
+
+qemu_mutex_lock(&lock);
+QLIST_INSERT_HEAD(&head, tmp, next);
+qemu_mutex_unlock(&lock);
+}
+
+void yank_unregister_function(YankFn *func, void *opaque)
+{
+qemu_mutex_lock(&lock);
+
+struct YankFuncAndParam *tmp;
+QLIST_FOREACH(tmp, &head, next) {
+if (tmp->func == func && tmp->opaque == opaque) {
+QLIST_REMOVE(tmp, next);
+g_free(tmp);
+qemu_mutex_unlock(&lock);
+return;
+}
+}
+
+abort();
+}
+
+void yank_call_functions(void)
+{
+qemu_mutex_lock(&lock);
+
+struct YankFuncAndParam *tmp;
+QLIST_FOREACH(tmp, &head, next) {
+tmp->func(tmp->opaque);
+}
+
+qemu_mutex_unlock(&lock);
+}
+
+void qmp_yank(Error **errp)
+{
+yank_call_functions();
+}
+
+void yank_init(void)
+{
+qemu_mutex_init(&lock);
+QLIST_INIT(&head);
+}
diff --git a/yank.h b/yank.h
new file mode 100644
index 00..7376224219
--- /dev/null
+++ b/yank.h
@@ -0,0 +1,12 @@
+
+#ifndef YANK_H
+#define YANK_H
+
+typedef void (YankFn) (void *opaque);
+
+void yank_register_function(YankFn *func, void *opaque);
+void yank_unregister_function(YankFn *func, void *opaque);
+void yank_call_functions(void);
+void yank_init(void);
+void qmp_yank(Error **errp);
+#endif
-- 
2.20.1



pgpWL0xCpizRu.pgp
Description: OpenPGP digital signature

[PATCH 2/6] migration/colo.c: Use cpu_synchronize_all_states()

[Lukas Straub]

cpu_synchronize_all_pre_loadvm() marks all vcpus as dirty, so the
registers are loaded from CPUState before we continue running
the vm. However if we failover during checkpoint, CPUState is not
initialized and the registers are loaded with garbage. This causes
guest hangs and crashes.

Fix this by using cpu_synchronize_all_states(), which initializes
CPUState from the current cpu registers additionally to marking
the vcpus as dirty.

Signed-off-by: Lukas Straub 
---
 migration/colo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/colo.c b/migration/colo.c
index 09168627bc..6b2ad35aa4 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -696,7 +696,7 @@ static void 
colo_incoming_process_checkpoint(MigrationIncomingState *mis,
 }
 
 qemu_mutex_lock_iothread();
-cpu_synchronize_all_pre_loadvm();
+cpu_synchronize_all_states();
 ret = qemu_loadvm_state_main(mis->from_src_file, mis);
 qemu_mutex_unlock_iothread();
 
-- 
2.20.1



pgpXE_2g3hpUN.pgp
Description: OpenPGP digital signature