Re: [Qemu-devel] [PATCH v2 1/2] virtio-input-host-pci: cleanup types

[Michael S. Tsirkin]

On Fri, May 10, 2019 at 12:51:36PM +0200, Gerd Hoffmann wrote:
> virtio input is virtio-1.0 only, so we don't need the -transitional and
> -non-transitional variants.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Michael S. Tsirkin 

I assume you are merging this?

> ---
>  hw/virtio/virtio-input-host-pci.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio-input-host-pci.c 
> b/hw/virtio/virtio-input-host-pci.c
> index 725a51ad30b4..124c4f344742 100644
> --- a/hw/virtio/virtio-input-host-pci.c
> +++ b/hw/virtio/virtio-input-host-pci.c
> @@ -13,7 +13,7 @@
>  
>  typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
>  
> -#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci-base"
> +#define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci"
>  #define VIRTIO_INPUT_HOST_PCI(obj) \
>  OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI)
>  
> @@ -31,10 +31,7 @@ static void virtio_host_initfn(Object *obj)
>  }
>  
>  static const VirtioPCIDeviceTypeInfo virtio_input_host_pci_info = {
> -.base_name = TYPE_VIRTIO_INPUT_HOST_PCI,
> -.generic_name  = "virtio-input-host-pci",
> -.transitional_name = "virtio-input-host-pci-transitional",
> -.non_transitional_name = "virtio-input-host-pci-non-transitional",
> +.generic_name  = TYPE_VIRTIO_INPUT_HOST_PCI,
>  .parent= TYPE_VIRTIO_INPUT_PCI,
>  .instance_size = sizeof(VirtIOInputHostPCI),
>  .instance_init = virtio_host_initfn,
> -- 
> 2.18.1

Re: [Qemu-devel] [PULL v2 00/27] tcg: Add CPUClass::tlb_fill

[Aleksandar Markovic]

On May 10, 2019 8:55 PM, "Richard Henderson" 
wrote:
>
> Changes in v2:
>
>   * Fix --disable-tcg compilation for x86 and s390x.
> I adjusted every target/ that used any CONFIG_TCG in cpu.c.
> but then afterward I see that only x86 and s390x have had
> their Makefiles adjusted to make --disable-tcg actually work.
>
>   * Fix Werror for 64-bit on 32-bit.
>
> Only re-posting changed patches.
>

Hello, Richard.

Just want to stress that there are unaddressed concerns for patch 12/27.

Thanks,
Aleksamdar

>
> r~
>
>
> The following changes since commit
efb4f3b62c69383a7308d7b739a3193e7c0ccae8:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request'
into staging (2019-05-10 14:49:36 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/rth7680/qemu.git tags/pull-tcg-20190510
>
> for you to fetch changes up to 4811e9095c0491bc6f5450e5012c9c4796b9e59d:
>
>   tcg: Use tlb_fill probe from tlb_vaddr_to_host (2019-05-10 11:12:50
-0700)
>
> 
> Add CPUClass::tlb_fill.
> Improve tlb_vaddr_to_host for use by ARM SVE no-fault loads.
>
> 
> Richard Henderson (27):
>   tcg: Add CPUClass::tlb_fill
>   target/alpha: Convert to CPUClass::tlb_fill
>   target/arm: Convert to CPUClass::tlb_fill
>   target/cris: Convert to CPUClass::tlb_fill
>   target/hppa: Convert to CPUClass::tlb_fill
>   target/i386: Convert to CPUClass::tlb_fill
>   target/lm32: Convert to CPUClass::tlb_fill
>   target/m68k: Convert to CPUClass::tlb_fill
>   target/microblaze: Convert to CPUClass::tlb_fill
>   target/mips: Pass a valid error to raise_mmu_exception for user-only
>   target/mips: Tidy control flow in mips_cpu_handle_mmu_fault
>   target/mips: Convert to CPUClass::tlb_fill
>   target/moxie: Convert to CPUClass::tlb_fill
>   target/nios2: Convert to CPUClass::tlb_fill
>   target/openrisc: Convert to CPUClass::tlb_fill
>   target/ppc: Convert to CPUClass::tlb_fill
>   target/riscv: Convert to CPUClass::tlb_fill
>   target/s390x: Convert to CPUClass::tlb_fill
>   target/sh4: Convert to CPUClass::tlb_fill
>   target/sparc: Convert to CPUClass::tlb_fill
>   target/tilegx: Convert to CPUClass::tlb_fill
>   target/tricore: Convert to CPUClass::tlb_fill
>   target/unicore32: Convert to CPUClass::tlb_fill
>   target/xtensa: Convert to CPUClass::tlb_fill
>   tcg: Use CPUClass::tlb_fill in cputlb.c
>   tcg: Remove CPUClass::handle_mmu_fault
>   tcg: Use tlb_fill probe from tlb_vaddr_to_host
>
>  include/exec/cpu_ldst.h |  50 +++
>  include/exec/exec-all.h |   9 --
>  include/qom/cpu.h   |  12 ++-
>  target/alpha/cpu.h  |   5 +-
>  target/arm/internals.h  |  10 ++-
>  target/cris/cpu.h   |   5 +-
>  target/hppa/cpu.h   |   8 +-
>  target/i386/cpu.h   |   5 +-
>  target/lm32/cpu.h   |   5 +-
>  target/m68k/cpu.h   |   5 +-
>  target/microblaze/cpu.h |   5 +-
>  target/mips/internal.h  |   5 +-
>  target/moxie/cpu.h  |   5 +-
>  target/nios2/cpu.h  |   5 +-
>  target/openrisc/cpu.h   |   5 +-
>  target/ppc/cpu.h|   7 +-
>  target/riscv/cpu.h  |   5 +-
>  target/s390x/internal.h |   5 +-
>  target/sh4/cpu.h|   5 +-
>  target/sparc/cpu.h  |   5 +-
>  target/tricore/cpu.h|   6 +-
>  target/unicore32/cpu.h  |   5 +-
>  target/xtensa/cpu.h |   5 +-
>  accel/tcg/cputlb.c  |  88 +--
>  accel/tcg/user-exec.c   |  36 ++--
>  target/alpha/cpu.c  |   5 +-
>  target/alpha/helper.c   |  24 +++--
>  target/alpha/mem_helper.c   |  16 
>  target/arm/cpu.c|  22 +
>  target/arm/helper.c |  90 +++
>  target/arm/op_helper.c  |  29 +-
>  target/arm/sve_helper.c |   6 +-
>  target/cris/cpu.c   |   5 +-
>  target/cris/helper.c|  61 ++---
>  target/cris/op_helper.c |  28 --
>  target/hppa/cpu.c   |   5 +-
>  target/hppa/mem_helper.c|  16 ++--
>  target/i386/cpu.c   |   5 +-
>  target/i386/excp_helper.c   |  53 ++-
>  target/i386/mem_helper.c|  21 -
>  target/lm32/cpu.c   |   5 +-
>  target/lm32/helper.c|   8 +-
>  target/lm32/op_helper.c |  16 
>  target/m68k/cpu.c   |   2 +-
>  target/m68k/helper.c|  89 +--
>  target/m68k/op_helper.c |  15 
>  target/microblaze/cpu.c |   5 +-
>  target/microblaze/helper.c  | 101 ++---
>  target/microblaze/op_helper.c   |  19 
>  target/mips/cpu.c

Re: [Qemu-devel] [PATCH v3 38/39] tcg/arm: Use LDRD to load tlb mask+table

[Richard Henderson]

On 5/10/19 2:08 PM, Alistair Francis wrote:
>> +if (use_armv6_instructions && TARGET_LONG_BITS == 64) {
>> +tcg_out_ldrd_8(s, COND_AL, TCG_REG_R2, TCG_REG_R1, cmp_off);
...
> 
> This is complex and I'm probably misunderstanding something but isn't
> it possible for TCG_REG_R3 to not be set if use_armv6_instructions is
> true and TARGET_LONG_BITS is 64?

No, the LDRD instruction loads data into both R2 and R2+1 = R3.


r~

Re: [Qemu-devel] [PATCH] configure: Change capstone's default state to disabled

[Programmingkid]

> On May 11, 2019, at 2:05 PM, Thomas Huth  wrote:
> 
> On 11/05/2019 19.21, Programmingkid wrote:
>> 
>>> On Apr 20, 2019, at 6:40 AM, Thomas Huth  wrote:
>>> 
>>> On 19/04/2019 15.44, G 3 wrote:
 
 On Apr 19, 2019, at 3:10 AM, Thomas Huth wrote:
 
> On 19/04/2019 00.47, John Arbuckle wrote:
>> Capstone is not necessary in order to use QEMU. Disable it by default.
>> This will save the user the pain of having to figure why QEMU isn't
>> building when this library is missing.
>> 
>> Signed-off-by: John Arbuckle 
>> ---
>> configure | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/configure b/configure
>> index 1c563a7027..77d7967f92 100755
>> --- a/configure
>> +++ b/configure
>> @@ -433,7 +433,7 @@ opengl_dmabuf="no"
>> cpuid_h="no"
>> avx2_opt=""
>> zlib="yes"
>> -capstone=""
>> +capstone="no"
>> lzo=""
>> snappy=""
>> bzip2=""
> 
> AFAIK we ship capstone as a submodule, so how can this be missing? Also,
> our philosophy is to keep everything enabled by default if possible, so
> that the code paths don't bitrot. Thus I don't think that disabling this
> by default is a good idea. ... so if you've got a problem here, there
> must be another solution (e.g. is the system capstone detection not
> working right on your system?).
> 
> Thomas
 
 Thank you for replying. Capstone comes with QEMU? Every time I try to
 compile QEMU I see an error relating to Capstone not being on my system.
 Why do you feel that disabling Capstone by default is not a good idea?
 
 Here is the error message I see when compiling QEMU:
 
 CHK version_gen.h
 make[1]: *** No rule to make target
 `/Users/John/qemu-git/capstone/libcapstone.a'.  Stop.
 make: *** [subdir-capstone] Error 2
>>> 
>>> I assume you're using a git checkout here, right? For git checkouts, the
>>> Makefile should take care of calling the scripts/git-submodule.sh script
>>> which should initialize the submodule in the capstone directory.
>>> 
>>> What's the content of your .git-submodule-status file? What does
>>> "configure" say about capstone support on your system?
>>> 
>>> Thomas
>> 
>> Yes I use a git checkout.
>> 
>> This is the contents of my .git-submodule-status file:
>> #!/bin/sh
> [...]
> 
> That were the contents of scripts/git-submodule.sh. I meant the hidden
> file .git-submodule-status in the main directory.

This is it:
 88f18909db731a627456f26d779445f84e449536 dtc (v1.4.7)
 f0da6726207b740f6101028b2992f918477a4b08 slirp (v4.0.0-rc0-25-gf0da672)
 b64af41c3276f97f0e181920400ee056b9c88037 tests/fp/berkeley-softfloat-3 
(heads/master)
 5a59dcec19327396a011a17fd924aed4fec416b3 tests/fp/berkeley-testfloat-3 
(remotes/origin/HEAD)
 6b3d716e2b6472eb7189d3220552280ef3d832ce ui/keycodemapdb 
(heads/master-4-g6b3d716)


> 
>> I did a 'make clean' followed by a 'make distclean'. Then tried building 
>> again using this command line:
>> 
>> ./configure --target-list=ppc-softmmu,i386-softmmu,x86_64-softmmu
>> make -j 4
> 
> That should normally populate the capstone directory. What happens if
> you run "make git-submodule-update" directly?

Here is the result:
$ make git-submodule-update
make[1]: Nothing to be done for `all'.
make[1]: *** No rule to make target 
`/Users/John/Documents/Development/Projects/Qemu/qemu-git/capstone/libcapstone.a'.
  Stop.
make: *** [subdir-capstone] Error 2


>> Here is the error message I see:
>> 
>> make[1]: *** No rule to make target 
>> `/Users/John/Documents/Development/Projects/Qemu/qemu-git/capstone/libcapstone.a'.
>>  Stop.
>> make: *** [subdir-capstone] Error 2
>> 
>> I took a look at the capstone folder. There is no 'make' file in this 
>> folder. Should there be one?
> 
> Yes, the capstone folder should be populated automatically. Is it
> completely empty for you?

It isn't empty. All I see are two folders: obj and docs.

Thank you.

Re: [Qemu-devel] [PATCH] configure: Change capstone's default state to disabled

[Thomas Huth]

On 11/05/2019 19.21, Programmingkid wrote:
> 
>> On Apr 20, 2019, at 6:40 AM, Thomas Huth  wrote:
>>
>> On 19/04/2019 15.44, G 3 wrote:
>>>
>>> On Apr 19, 2019, at 3:10 AM, Thomas Huth wrote:
>>>
 On 19/04/2019 00.47, John Arbuckle wrote:
> Capstone is not necessary in order to use QEMU. Disable it by default.
> This will save the user the pain of having to figure why QEMU isn't
> building when this library is missing.
>
> Signed-off-by: John Arbuckle 
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 1c563a7027..77d7967f92 100755
> --- a/configure
> +++ b/configure
> @@ -433,7 +433,7 @@ opengl_dmabuf="no"
>  cpuid_h="no"
>  avx2_opt=""
>  zlib="yes"
> -capstone=""
> +capstone="no"
>  lzo=""
>  snappy=""
>  bzip2=""

 AFAIK we ship capstone as a submodule, so how can this be missing? Also,
 our philosophy is to keep everything enabled by default if possible, so
 that the code paths don't bitrot. Thus I don't think that disabling this
 by default is a good idea. ... so if you've got a problem here, there
 must be another solution (e.g. is the system capstone detection not
 working right on your system?).

  Thomas
>>>
>>> Thank you for replying. Capstone comes with QEMU? Every time I try to
>>> compile QEMU I see an error relating to Capstone not being on my system.
>>> Why do you feel that disabling Capstone by default is not a good idea?
>>>
>>> Here is the error message I see when compiling QEMU:
>>>
>>> CHK version_gen.h
>>> make[1]: *** No rule to make target
>>> `/Users/John/qemu-git/capstone/libcapstone.a'.  Stop.
>>> make: *** [subdir-capstone] Error 2
>>
>> I assume you're using a git checkout here, right? For git checkouts, the
>> Makefile should take care of calling the scripts/git-submodule.sh script
>> which should initialize the submodule in the capstone directory.
>>
>> What's the content of your .git-submodule-status file? What does
>> "configure" say about capstone support on your system?
>>
>> Thomas
> 
> Yes I use a git checkout.
> 
> This is the contents of my .git-submodule-status file:
> #!/bin/sh
[...]

That were the contents of scripts/git-submodule.sh. I meant the hidden
file .git-submodule-status in the main directory.

> I did a 'make clean' followed by a 'make distclean'. Then tried building 
> again using this command line:
> 
> ./configure --target-list=ppc-softmmu,i386-softmmu,x86_64-softmmu
> make -j 4

That should normally populate the capstone directory. What happens if
you run "make git-submodule-update" directly?

> Here is the error message I see:
> 
> make[1]: *** No rule to make target 
> `/Users/John/Documents/Development/Projects/Qemu/qemu-git/capstone/libcapstone.a'.
>   Stop.
> make: *** [subdir-capstone] Error 2
> 
> I took a look at the capstone folder. There is no 'make' file in this folder. 
> Should there be one?

Yes, the capstone folder should be populated automatically. Is it
completely empty for you?

 Thomas

Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output

[Alex Bennée]

Peter Maydell  writes:

> On Fri, 10 May 2019 at 17:59, Alex Bennée  wrote:
>>
>>
>> Peter Maydell  writes:
>>
>> > On Thu, 9 May 2019 at 17:59, Alex Bennée  wrote:
>> >>
>> >> For running system tests we want to be able to re-direct output to a
>> >> file like we do with serial output. This does the wiring to allow us
>> >> to treat semihosting like just another character output device.
>> >>
>> >> diff --git a/qemu-options.hx b/qemu-options.hx
>> >> index 51802cbb266..6aa3a08c2fb 100644
>> >> --- a/qemu-options.hx
>> >> +++ b/qemu-options.hx
>> >> @@ -3975,12 +3975,12 @@ STEXI
>> >>  Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only).
>> >>  ETEXI
>> >>  DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
>> >> -"-semihosting-config 
>> >> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
>> >> +"-semihosting-config 
>> >> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \
>> >>  "semihosting configuration\n",
>> >>  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 |
>> >>  QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2)
>> >
>> > As you can see in the docs here, semihosting is supported on
>> > five guest architectures, so we should implement this new
>> > feature for all of them, not just arm.
>>
>> As I've introduced this for testing I see no reason not to add support
>> for other architectures. However I was hoping this is something that
>> could be done organically as other system tests get enabled.
>
> IME transitions done "organically" really means "slowly, and
> nobody ever gets round to actually completing them".
> Semihosting is a user-facing feature, so if we want to add
> the user feature of allowing output to go to a chardev we
> should add it properly, I think.

So a quick review of the current semi output:

  - MIPS
This has a fairly generalised open/read/write support with special
handling for open/close on /dev/std[out/err/in]. There is also a
UHI_plog which currently just printf's to stdout

  - xtensa
This already has support for a sim_console char device as part of
the xtensa sim platform. Otherwise the TARGET_SYS_open can open
paths directly (which I assume could include stdio) which then
read/write.

  - m68k
This has the usual open/read/write/close support directly to the
FD's as well as support for integrating with the gdbstub via
gdb_do_syscall.

  - lm32
Although based on the m68k semithosting support it lacks the gdbstub
integration. It has the usual open/read/write/close stuff.

  - NIOS2
Again based on the m68k semihosting but looks like it was taken
later because it retains the gdbsub integration support.

Generally all the other semihosting stuff looks a lot cleaner - probably
an indication of being done later and avoiding some of the warts of the
early arm semihosting code.

One difference with ARM is it has specific calls aside from the
open/read/write/close (WRITEC/WRITE0) which are specifically aimed at
"console" type logging. They don't seem to require an explicit open at
the start and assume you can write to them from the get go.

One question that would need to be answered is should the chardev
support be generalised for all semihosts that can read/write to the
stdio outputs or should we restrict it to the "console" log operations
(xtensa sim, mips plog and ARM)?

--
Alex Bennée

Re: [Qemu-devel] [PATCH] configure: Change capstone's default state to disabled

[Programmingkid]

> On Apr 20, 2019, at 6:40 AM, Thomas Huth  wrote:
> 
> On 19/04/2019 15.44, G 3 wrote:
>> 
>> On Apr 19, 2019, at 3:10 AM, Thomas Huth wrote:
>> 
>>> On 19/04/2019 00.47, John Arbuckle wrote:
 Capstone is not necessary in order to use QEMU. Disable it by default.
 This will save the user the pain of having to figure why QEMU isn't
 building when this library is missing.
 
 Signed-off-by: John Arbuckle 
 ---
  configure | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/configure b/configure
 index 1c563a7027..77d7967f92 100755
 --- a/configure
 +++ b/configure
 @@ -433,7 +433,7 @@ opengl_dmabuf="no"
  cpuid_h="no"
  avx2_opt=""
  zlib="yes"
 -capstone=""
 +capstone="no"
  lzo=""
  snappy=""
  bzip2=""
>>> 
>>> AFAIK we ship capstone as a submodule, so how can this be missing? Also,
>>> our philosophy is to keep everything enabled by default if possible, so
>>> that the code paths don't bitrot. Thus I don't think that disabling this
>>> by default is a good idea. ... so if you've got a problem here, there
>>> must be another solution (e.g. is the system capstone detection not
>>> working right on your system?).
>>> 
>>>  Thomas
>> 
>> Thank you for replying. Capstone comes with QEMU? Every time I try to
>> compile QEMU I see an error relating to Capstone not being on my system.
>> Why do you feel that disabling Capstone by default is not a good idea?
>> 
>> Here is the error message I see when compiling QEMU:
>> 
>> CHK version_gen.h
>> make[1]: *** No rule to make target
>> `/Users/John/qemu-git/capstone/libcapstone.a'.  Stop.
>> make: *** [subdir-capstone] Error 2
> 
> I assume you're using a git checkout here, right? For git checkouts, the
> Makefile should take care of calling the scripts/git-submodule.sh script
> which should initialize the submodule in the capstone directory.
> 
> What's the content of your .git-submodule-status file? What does
> "configure" say about capstone support on your system?
> 
> Thomas

Yes I use a git checkout.

This is the contents of my .git-submodule-status file:
#!/bin/sh
#
# This code is licensed under the GPL version 2 or later.  See
# the COPYING file in the top-level directory.

substat=".git-submodule-status"

command=$1
shift
maybe_modules="$@"

test -z "$GIT" && GIT=git

error() {
echo "$0: $*"
echo
echo "Unable to automatically checkout GIT submodules '$modules'."
echo "If you require use of an alternative GIT binary (for example to"
echo "enable use of a transparent proxy), then please specify it by"
echo "running configure by with the '--with-git' argument. e.g."
echo
echo " $ ./configure --with-git='tsocks git'"
echo
echo "Alternatively you may disable automatic GIT submodule checkout"
echo "with:"
echo
echo " $ ./configure --disable-git-update"
echo
echo "and then manually update submodules prior to running make, with:"
echo
echo " $ scripts/git-submodule.sh update $modules"
echo
exit 1
}

modules=""
for m in $maybe_modules
do
$GIT submodule status $m 1> /dev/null 2>&1
if test $? = 0
then
modules="$modules $m"
else
echo "warn: ignoring non-existent submodule $m"
fi
done

if test -n "$maybe_modules" && ! test -e ".git"
then
echo "$0: unexpectedly called with submodules but no git checkout exists"
exit 1
fi

case "$command" in
status)
if test -z "$maybe_modules"
then
 test -s ${substat} && exit 1 || exit 0
fi

test -f "$substat" || exit 1
CURSTATUS=$($GIT submodule status $modules)
OLDSTATUS=$(cat $substat)
test "$CURSTATUS" = "$OLDSTATUS"
exit $?
;;
update)
if test -z "$maybe_modules"
then
test -e $substat || touch $substat
exit 0
fi

$GIT submodule update --init $modules 1>/dev/null
test $? -ne 0 && error "failed to update modules"

$GIT submodule status $modules > "${substat}"
test $? -ne 0 && error "failed to save git submodule status" >&2
;;
esac

exit 0



The Configure command says:
capstone  git

I did a 'make clean' followed by a 'make distclean'. Then tried building again 
using this command line:

./configure --target-list=ppc-softmmu,i386-softmmu,x86_64-softmmu
make -j 4

Here is the error message I see:

make[1]: *** No rule to make target 
`/Users/John/Documents/Development/Projects/Qemu/qemu-git/capstone/libcapstone.a'.
  Stop.
make: *** [subdir-capstone] Error 2

I took a look at the capstone folder. There is no 'make' file in this folder. 
Should there be one?

Thank you.

Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Aleksandar Markovic]

On May 8, 2019 4:33 PM, "Richard Henderson" 
wrote:
>
> On 5/8/19 1:15 AM, Aleksandar Markovic wrote:
> >
> > On May 8, 2019 2:19 AM, "Richard Henderson" <
richard.hender...@linaro.org
> > > wrote:
> >>
> >>
> >>
> >
> > This commit message doesnˊt explain the reason for the change, and why
is this
> > an improvement. The underlyng reason for distingishing between  env_cpu
and
> > env_archcpu cases is not explained too.
>
> It's certainly explained in the preceeding patches that introduce those
functions.
>

A commit (code+message) should be as standalone as possible, and one should
not be forced to resort to reverse-engineering and perusing mailing list or
patchwork in order to reveal its true meaning in another commit message
altogether.

Thanks,
Aleksandar

> Are you suggesting that it is beneficial to copy-and-paste a common block
> explanation into 21 commit messages for each of target/foo/?
>
>
> r~

Re: [Qemu-devel] [PULL v2 12/27] target/mips: Convert to CPUClass::tlb_fill

[Aleksandar Markovic]

On May 10, 2019 8:57 PM, "Richard Henderson" 
wrote:
>

Please change the title to 'target/mips: Switch to using
mips_cpu_tlb_fill()', or something along that line.

Also, the reason for changing the field access_type to mips_access type
should be explained in the commit message.

This commit message is generally poor, as it explains relatively
unimportant logging issue, while not explaining the core of the change.

Thanks,
Aleksandar

> Note that env->active_tc.PC is removed from the qemu_log as that value
> is garbage.  The PC isn't recovered until cpu_restore_state, called from
> cpu_loop_exit_restore, called from do_raise_exception_err.
>
> Cc: Aleksandar Markovic 
> Cc: Aleksandar Rikalo 
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Richard Henderson 
> ---
>  target/mips/internal.h  |  5 +++--
>  target/mips/cpu.c   |  5 ++---
>  target/mips/helper.c| 45 ++---
>  target/mips/op_helper.c | 15 --
>  4 files changed, 29 insertions(+), 41 deletions(-)
>
> diff --git a/target/mips/internal.h b/target/mips/internal.h
> index 286e3888ab..b2b41a51ab 100644
> --- a/target/mips/internal.h
> +++ b/target/mips/internal.h
> @@ -202,8 +202,9 @@ void cpu_mips_start_count(CPUMIPSState *env);
>  void cpu_mips_stop_count(CPUMIPSState *env);
>
>  /* helper.c */
> -int mips_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size,
int rw,
> -  int mmu_idx);
> +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +   MMUAccessType access_type, int mmu_idx,
> +   bool probe, uintptr_t retaddr);
>
>  /* op_helper.c */
>  uint32_t float_class_s(uint32_t arg, float_status *fst);
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index e217fb3e36..a33058609a 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -197,9 +197,7 @@ static void mips_cpu_class_init(ObjectClass *c, void
*data)
>  cc->synchronize_from_tb = mips_cpu_synchronize_from_tb;
>  cc->gdb_read_register = mips_cpu_gdb_read_register;
>  cc->gdb_write_register = mips_cpu_gdb_write_register;
> -#ifdef CONFIG_USER_ONLY
> -cc->handle_mmu_fault = mips_cpu_handle_mmu_fault;
> -#else
> +#ifndef CONFIG_USER_ONLY
>  cc->do_unassigned_access = mips_cpu_unassigned_access;
>  cc->do_unaligned_access = mips_cpu_do_unaligned_access;
>  cc->get_phys_page_debug = mips_cpu_get_phys_page_debug;
> @@ -208,6 +206,7 @@ static void mips_cpu_class_init(ObjectClass *c, void
*data)
>  cc->disas_set_info = mips_cpu_disas_set_info;
>  #ifdef CONFIG_TCG
>  cc->tcg_initialize = mips_tcg_init;
> +cc->tlb_fill = mips_cpu_tlb_fill;
>  #endif
>
>  cc->gdb_num_core_regs = 73;
> diff --git a/target/mips/helper.c b/target/mips/helper.c
> index 86e622efb8..3a4917ce7b 100644
> --- a/target/mips/helper.c
> +++ b/target/mips/helper.c
> @@ -874,31 +874,25 @@ refill:
>  #endif
>  #endif
>
> -int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int
rw,
> -  int mmu_idx)
> +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +   MMUAccessType access_type, int mmu_idx,
> +   bool probe, uintptr_t retaddr)
>  {
>  MIPSCPU *cpu = MIPS_CPU(cs);
>  CPUMIPSState *env = >env;
>  #if !defined(CONFIG_USER_ONLY)
>  hwaddr physical;
>  int prot;
> -int access_type;
> +int mips_access_type;
>  #endif
>  int ret = TLBRET_BADADDR;
>
> -#if 0
> -log_cpu_state(cs, 0);
> -#endif
> -qemu_log_mask(CPU_LOG_MMU,
> -  "%s pc " TARGET_FMT_lx " ad %" VADDR_PRIx " rw %d mmu_idx
%d\n",
> -  __func__, env->active_tc.PC, address, rw, mmu_idx);
> -
>  /* data access */
>  #if !defined(CONFIG_USER_ONLY)
>  /* XXX: put correct access by using cpu_restore_state() correctly */
> -access_type = ACCESS_INT;
> -ret = get_physical_address(env, , ,
> -   address, rw, access_type, mmu_idx);
> +mips_access_type = ACCESS_INT;
> +ret = get_physical_address(env, , , address,
> +   access_type, mips_access_type, mmu_idx);
>  switch (ret) {
>  case TLBRET_MATCH:
>  qemu_log_mask(CPU_LOG_MMU,
> @@ -915,7 +909,7 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr
address, int size, int rw,
>  tlb_set_page(cs, address & TARGET_PAGE_MASK,
>   physical & TARGET_PAGE_MASK, prot | PAGE_EXEC,
>   mmu_idx, TARGET_PAGE_SIZE);
> -return 0;
> +return true;
>  }
>  #if !defined(TARGET_MIPS64)
>  if ((ret == TLBRET_NOMATCH) && (env->tlb->nb_tlb > 1)) {
> @@ -926,27 +920,36 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr
address, int size, int rw,
>  int mode = (env->hflags & MIPS_HFLAG_KSU);
>  bool ret_walker;
>  env->hflags &= ~MIPS_HFLAG_KSU;
> -ret_walker = page_table_walk_refill(env, address, rw, 

Re: [Qemu-devel] [PATCH v3 36/39] cpu: Remove CPU_COMMON

[Aleksandar Markovic]

On May 8, 2019 2:32 AM, "Richard Henderson" 
wrote:
>
> This macro is now always empty, so remove it.  This leaves the
> entire contents of CPUArchState under the control of the guest
> architecture.
>
> Reviewed-by: Peter Maydell 
> Signed-off-by: Richard Henderson 
> ---
>  include/exec/cpu-defs.h | 2 --
>  target/alpha/cpu.h  | 3 ---
>  target/arm/cpu.h| 4 +---
>  target/cris/cpu.h   | 2 --
>  target/hppa/cpu.h   | 3 ---
>  target/i386/cpu.h   | 4 +---
>  target/lm32/cpu.h   | 2 --
>  target/m68k/cpu.h   | 2 --
>  target/microblaze/cpu.h | 2 --
>  target/mips/cpu.h   | 2 --
>  target/moxie/cpu.h  | 3 ---
>  target/nios2/cpu.h  | 2 --
>  target/openrisc/cpu.h   | 2 --
>  target/ppc/cpu.h| 2 --
>  target/riscv/cpu.h  | 4 
>  target/s390x/cpu.h  | 2 --
>  target/sh4/cpu.h| 2 --
>  target/sparc/cpu.h  | 2 --
>  target/tilegx/cpu.h | 2 --
>  target/tricore/cpu.h| 2 --
>  target/unicore32/cpu.h  | 2 --
>  target/xtensa/cpu.h | 2 --
>  22 files changed, 2 insertions(+), 51 deletions(-)
>
> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
> index 4cde7d611c..1f75a97701 100644
> --- a/include/exec/cpu-defs.h
> +++ b/include/exec/cpu-defs.h
> @@ -233,8 +233,6 @@ typedef struct CPUTLB { } CPUTLB;
>
>  #endif  /* !CONFIG_USER_ONLY && CONFIG_TCG */
>
> -#define CPU_COMMON  /* Nothing */
> -
>  /*
>   * This structure must be placed in ArchCPU immedately
>   * before CPUArchState, as a field named "neg".
> diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
> index f4bb67c101..5bd90b7ce5 100644
> --- a/target/alpha/cpu.h
> +++ b/target/alpha/cpu.h
> @@ -246,9 +246,6 @@ struct CPUAlphaState {
>  /* This alarm doesn't exist in real hardware; we wish it did.  */
>  uint64_t alarm_expire;
>
> -/* Those resources are used only in QEMU core */
> -CPU_COMMON
> -
>  int error_code;
>
>  uint32_t features;
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 909cb4604d..135a16a351 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -643,9 +643,7 @@ typedef struct CPUARMState {
>  /* Fields up to this point are cleared by a CPU reset */
>  struct {} end_reset_fields;
>
> -CPU_COMMON
> -
> -/* Fields after CPU_COMMON are preserved across CPU reset. */
> +/* Fields after this point are preserved across CPU reset. */
>
>  /* Internal CPU feature flags.  */
>  uint64_t features;
> diff --git a/target/cris/cpu.h b/target/cris/cpu.h
> index eeab483dba..6dc4502e9a 100644
> --- a/target/cris/cpu.h
> +++ b/target/cris/cpu.h
> @@ -163,8 +163,6 @@ typedef struct CPUCRISState {
>  /* Fields up to this point are cleared by a CPU reset */
>  struct {} end_reset_fields;
>
> -CPU_COMMON
> -
>  /* Members from load_info on are preserved across resets.  */
>  void *load_info;
>  } CPUCRISState;
> diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
> index 0661ff60c1..3ed2ac3c25 100644
> --- a/target/hppa/cpu.h
> +++ b/target/hppa/cpu.h
> @@ -197,9 +197,6 @@ struct CPUHPPAState {
>  target_ureg cr_back[2];  /* back of cr17/cr18 */
>  target_ureg shadow[7];   /* shadow registers */
>
> -/* Those resources are used only in QEMU core */
> -CPU_COMMON
> -
>  /* ??? The number of entries isn't specified by the architecture.  */
>  /* ??? Implement a unified itlb/dtlb for the moment.  */
>  /* ??? We should use a more intelligent data structure.  */
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 43bb6ab841..8069e5b19d 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1288,9 +1288,7 @@ typedef struct CPUX86State {
>  /* Fields up to this point are cleared by a CPU reset */
>  struct {} end_reset_fields;
>
> -CPU_COMMON
> -
> -/* Fields after CPU_COMMON are preserved across CPU reset. */
> +/* Fields after this point are preserved across CPU reset. */
>
>  /* processor features (e.g. for CPUID insn) */
>  /* Minimum level/xlevel/xlevel2, based on CPU model + features */
> diff --git a/target/lm32/cpu.h b/target/lm32/cpu.h
> index 0ec898eb1d..563600a30a 100644
> --- a/target/lm32/cpu.h
> +++ b/target/lm32/cpu.h
> @@ -159,8 +159,6 @@ struct CPULM32State {
>  /* Fields up to this point are cleared by a CPU reset */
>  struct {} end_reset_fields;
>
> -CPU_COMMON
> -
>  /* Fields from here on are preserved across CPU reset. */
>  uint32_t eba;   /* exception base address */
>  uint32_t deba;  /* debug exception base address */
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index 096d1eb588..4e27ff677f 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -143,8 +143,6 @@ typedef struct CPUM68KState {
>  /* Fields up to this point are cleared by a CPU reset */
>  struct {} end_reset_fields;
>
> -CPU_COMMON
> -
>  /* Fields from here on are preserved across CPU reset. */
>  uint32_t features;
>  } 

Re: [Qemu-devel] [PATCH] configure: Disable slirp if --disable-system

[Aleksandar Markovic]

On May 10, 2019 10:36 PM, "Richard Henderson" 
wrote:
>
> For linux-user, there is no need to add slirp to the set of
> git modules checked out, nor build it.
>
> This also avoids a makefile bug wrt insufficient dependencies
> on subdir-slirp.  If slirp/ is not initially present, the
> dependencies that check it out are associated with softmmu,
> which then generates a build error on slirp/ not present.
>

Hi,

Does this work if only user mode targets are specified via ˊ--target-listˊ
switch? If no, the patch shoud be amended. If yes, the commit message
should be extended.

Thanks,
Aleksandar

> Signed-off-by: Richard Henderson 
> ---
>  configure | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/configure b/configure
> index 63f312bd1f..9de7e43a80 100755
> --- a/configure
> +++ b/configure
> @@ -5878,6 +5878,10 @@ fi
>  ##
>  # check for slirp
>
> +if test "$softmmu" = "no"; then
> +slirp=no
> +fi
> +
>  case "$slirp" in
>"" | yes)
>  if $pkg_config slirp; then
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCH] pflash: Only read non-zero parts of backend image

[Xiang Zheng]

On 2019/5/10 23:16, Markus Armbruster wrote:
> Xiang Zheng  writes:
> 
>> On 2019/5/9 19:59, Markus Armbruster wrote:
>>> Xiang Zheng  writes:
>>>
 On 2019/5/8 21:20, Markus Armbruster wrote:
> Laszlo Ersek  writes:
>
>> Hi Markus,
>>
>> On 05/07/19 20:01, Markus Armbruster wrote:
>>> The subject is slightly misleading.  Holes read as zero.  So do
>>> non-holes full of zeroes.  The patch avoids reading the former, but
>>> still reads the latter.
>>>
>>> Xiang Zheng  writes:
>>>
 Currently we fill the memory space with two 64MB NOR images when
 using persistent UEFI variables on virt board. Actually we only use
 a very small(non-zero) part of the memory while the rest significant
 large(zero) part of memory is wasted.
>>>
>>> Neglects to mention that the "virt board" is ARM.
>>>
 So this patch checks the block status and only writes the non-zero part
 into memory. This requires pflash devices to use sparse files for
 backends.
>>>
>>> I started to draft an improved commit message, but then I realized this
>>> patch can't work.
>>>
>>> The pflash_cfi01 device allocates its device memory like this:
>>>
>>> memory_region_init_rom_device(
>>> >mem, OBJECT(dev),
>>> _cfi01_ops,
>>> pfl,
>>> pfl->name, total_len, _err);
>>>
>>> pflash_cfi02 is similar.
>>>
>>> memory_region_init_rom_device() calls
>>> memory_region_init_rom_device_nomigrate() calls qemu_ram_alloc() calls
>>> qemu_ram_alloc_internal() calls g_malloc0().  Thus, all the device
>>> memory gets written to even with this patch.
>>
>> As far as I can see, qemu_ram_alloc_internal() calls g_malloc0() only to
>> allocate the the new RAMBlock object called "new_block". The actual
>> guest RAM allocation occurs inside ram_block_add(), which is also called
>> by qemu_ram_alloc_internal().
>
> You're right.  I should've read more attentively.
>
>> One frame outwards the stack, qemu_ram_alloc() passes NULL to
>> qemu_ram_alloc_internal(), for the 4th ("host") parameter. Therefore, in
>> qemu_ram_alloc_internal(), we set "new_block->host" to NULL as well.
>>
>> Then in ram_block_add(), we take the (!new_block->host) branch, and call
>> phys_mem_alloc().
>>
>> Unfortunately, "phys_mem_alloc" is a function pointer, set with
>> phys_mem_set_alloc(). The phys_mem_set_alloc() function is called from
>> "target/s390x/kvm.c" (setting the function pointer to
>> legacy_s390_alloc()), so it doesn't apply in this case. Therefore we end
>> up calling the default qemu_anon_ram_alloc() function, through the
>> funcptr. (I think anyway.)
>>
>> And qemu_anon_ram_alloc() boils down to mmap() + MAP_ANONYMOUS, in
>> qemu_ram_mmap(). (Even on PPC64 hosts, because qemu_anon_ram_alloc()
>> passes (-1) for "fd".)
>>
>> I may have missed something, of course -- I obviously didn't test it,
>> just speculated from the source.
>
> Thanks for your sleuthing!
>
>>> I'm afraid you neglected to test.
>
> Accusation actually unsupported.  I apologize, and replace it by a
> question: have you observed the improvement you're trying to achieve,
> and if yes, how?
>

 Yes, we need to create sparse files as the backing images for pflash 
 device.
 To create sparse files like:

dd of="QEMU_EFI-pflash.raw" if="/dev/zero" bs=1M seek=64 count=0
dd of="QEMU_EFI-pflash.raw" if="QEMU_EFI.fd" conv=notrunc
>>>
>>> This creates a copy of firmware binary QEMU_EFI.fd padded with a hole to
>>> 64MiB.
>>>
dd of="empty_VARS.fd" if="/dev/zero" bs=1M seek=64 count=0
>>>
>>> This creates the varstore as a 64MiB hole.  As far as I know (very
>>> little), you should use the varstore template that comes with the
>>> firmware binary.
>>>
>>> I use
>>>
>>> cp --sparse=always bld/pc-bios/edk2-arm-vars.fd .
>>> cp --sparse=always bld/pc-bios/edk2-aarch64-code.fd .
>>>
>>> These guys are already zero-padded, and I use cp to sparsify.
>>>
 Start a VM with below commandline:

 -drive 
 file=/usr/share/edk2/aarch64/QEMU_EFI-pflash.raw,if=pflash,format=raw,unit=0,readonly=on\
 -drive 
 file=/usr/share/edk2/aarch64/empty_VARS.fd,if=pflash,format=raw,unit=1 \

 Then observe the memory usage of the qemu process (THP is on).

 1) Without this patch:
 # cat /proc/`pidof qemu-system-aarch64`/smaps | grep AnonHugePages: | grep 
 -v ' 0 kB'
 AnonHugePages:706560 kB
 AnonHugePages:  2048 kB
 AnonHugePages: 65536 kB// pflash memory device
 AnonHugePages: 65536 kB// pflash memory device
 AnonHugePages:  2048 kB

 # ps aux | grep qemu-system-aarch64
 RSS: 879684

 2) After applying this patch:
 

Re: [Qemu-devel] [qemu-s390x] [PULL SUBSYSTEM s390x 2/3] s390-bios: Skip bootmap signature entries

[Thomas Huth]

On 10/05/2019 15.59, Christian Borntraeger wrote:
> Shall we cc stable this?

I think I'd rather not do it unless someone really speaks up that they
urgently need it. If we could use the binary from the master branch, I'd
say go for it, but in this case we'd need to build a separate
s390-ccw.img for this (without the DASD passthrough patches), and since
the stable branch does not get that much testing attention from all the
s390x developers, you'd end up with a firmware binary in the stable
branch that is not very well tested... This does not sound very
appealing to me.

 Thomas

Re: [Qemu-devel] [PATCH] configure: Disable slirp if --disable-system

[Thomas Huth]

On 10/05/2019 22.34, Richard Henderson wrote:
> For linux-user, there is no need to add slirp to the set of
> git modules checked out, nor build it.
> 
> This also avoids a makefile bug wrt insufficient dependencies
> on subdir-slirp.  If slirp/ is not initially present, the
> dependencies that check it out are associated with softmmu,
> which then generates a build error on slirp/ not present.
> 
> Signed-off-by: Richard Henderson 
> ---
>  configure | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure b/configure
> index 63f312bd1f..9de7e43a80 100755
> --- a/configure
> +++ b/configure
> @@ -5878,6 +5878,10 @@ fi
>  ##
>  # check for slirp
>  
> +if test "$softmmu" = "no"; then
> +slirp=no
> +fi

Maybe also check that the user did not try to run configure with both,
--disable-system and --enable-slirp? I.e. that $slirp != "yes" ?

 Thomas

[Qemu-devel] [Bug 1828507] Re: qemu-system-ppc64 smp crash on manual reset

[Thomas Huth]

** Tags added: ppc

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

Title:
  qemu-system-ppc64 smp crash on manual reset

Status in QEMU:
  New

Bug description:
  Host Environment:
 x86_64 Linux v5.0.2
 QEMU emulator version 4.0.50 (v4.0.0-354-g812b835fb4)
 SLOF:
 Build Date = Jan 14 2019 18:00:39
 FW Version = git-a5b428e1c1eae703

  Problem: Qemu crash immediately after a manual reset
   (this is not the initial reset which launches the guest).

  Steps:

  1. Download Debian ppc64el mini.iso:
 
http://ftp.debian.org/debian/dists/sid/main/installer-ppc64el/current/images/netboot/mini.iso
  2. Run qemu on the host. Ensure that it runs with more than one CPUs. With a 
single CPU, I was unable
 to reproduce the crash.
 qemu-system-ppc64 -M pseries -cpu power9 -smp 2 -m 512 -cdrom mini.iso
  3. SLOF prints the version info on the serial device, and proceeds to boot.
  4. After a few seconds, the GRUB menu appears on the VGA screen.
  5. Select one of the install options (I have tested with Default and Expert), 
and wait
 for the Debian's text-mode installer (blue-gray-red) screen to appear.
  6. Click Machine->Reset (or enter system_reset on the qemu monitor).
  7. Notice that, on the serial device, SLOF has printed the version info. That 
is, the system
 has reset and is attempting to boot again.
  8. On the host cmd prompt, qemu dies after printing this fatal error and 
spewing the
 contents of the CPU registers:

 qemu: fatal: Trying to deliver HV exception (MSR) 70 with no HV support
  (See attached out.txt for details)
 Aborted (core dumped)

  
  The HV exception is either
 (a) 70 = HISI, which occurs when NIP contains an outright bogus or 
inaccessible value, or
 (b) 69 = HDSI, which occurs when NIP happens to contain a somewhat saner 
value, and
 the cpu attempts to run the instruction at that address.

  The exception can occur on either of the CPUs. It occurs when qemu is running 
the SLOF
  code.

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