[PATCH 0/5] hvf: stability fixes for HVF

Cameron Esfahani via posted 5 patches 4 years, 4 months ago
Test asan passed
Test checkpatch failed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/cover.1574375668.git.dirty@apple.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Richard Henderson <rth@twiddle.net>
There is a newer version of this series
target/i386/hvf/hvf.c        | 74 +++++++++++++++++++++++++-----------
target/i386/hvf/vmx.h        | 18 +++++----
target/i386/hvf/x86_decode.c | 55 ++++++++++++++-------------
target/i386/hvf/x86_decode.h | 16 ++++----
target/i386/hvf/x86_emu.c    |  3 --
target/i386/hvf/x86hvf.c     | 26 +++++--------
6 files changed, 108 insertions(+), 84 deletions(-)
[PATCH 0/5] hvf: stability fixes for HVF
Posted by Cameron Esfahani via 4 years, 4 months ago
The following patches fix stability issues with running QEMU on Apple
Hypervisor Framework (HVF):
- non-RAM, non-ROMD areas need to trap so accesses can be correctly
  emulated.
- Current TSC synchronization implementation is insufficient: when
  running with more than 1 core, TSC values can go backwards.  Until
  a correct implementation can be written, remove calls to
  hv_vm_sync_tsc().  Pass through TSC to guest OS.
- Fix REX emulation in relation to legacy prefixes.
- More correctly match SDM when setting CR0 and PDPTE registers.
- Save away exception type as well as vector in hvf_store_events() so
  they can be correctly reinjected in hvf_inject_interrupts().  Under
  heavy loads, exceptions got misrouted.

Cameron Esfahani (5):
  hvf: non-RAM, non-ROMD memory ranges are now correctly mapped in
  hvf: remove TSC synchronization code because it isn't fully complete
  hvf: correctly handle REX prefix in relation to legacy prefixes
  hvf: more accurately match SDM when setting CR0 and PDPTE registers
  hvf: save away type as well as vector so we can reinject them

 target/i386/hvf/hvf.c        | 74 +++++++++++++++++++++++++-----------
 target/i386/hvf/vmx.h        | 18 +++++----
 target/i386/hvf/x86_decode.c | 55 ++++++++++++++-------------
 target/i386/hvf/x86_decode.h | 16 ++++----
 target/i386/hvf/x86_emu.c    |  3 --
 target/i386/hvf/x86hvf.c     | 26 +++++--------
 6 files changed, 108 insertions(+), 84 deletions(-)

-- 
2.24.0


Re: [PATCH 0/5] hvf: stability fixes for HVF
Posted by no-reply@patchew.org 4 years, 4 months ago
Patchew URL: https://patchew.org/QEMU/cover.1574375668.git.dirty@apple.com/



Hi,

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

Subject: [PATCH 0/5] hvf: stability fixes for HVF
Type: series
Message-id: cover.1574375668.git.dirty@apple.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
e1d1679 hvf: save away type as well as vector so we can reinject them
b201a1b hvf: more accurately match SDM when setting CR0 and PDPTE registers
995bd07 hvf: correctly handle REX prefix in relation to legacy prefixes
3ebb7cf hvf: remove TSC synchronization code because it isn't fully complete
5a99e4f hvf: non-RAM, non-ROMD memory ranges are now correctly mapped in

=== OUTPUT BEGIN ===
1/5 Checking commit 5a99e4fc4643 (hvf: non-RAM, non-ROMD memory ranges are now correctly mapped in)
WARNING: Block comments use a leading /* on a separate line
#72: FILE: target/i386/hvf/hvf.c:171:
+            /* If the memory device is not in romd_mode, then we actually want

WARNING: Block comments use a trailing */ on a separate line
#73: FILE: target/i386/hvf/hvf.c:172:
+             * to remove the hvf memory slot so all accesses will trap. */

ERROR: Error messages should not contain newlines
#92: FILE: target/i386/hvf/hvf.c:194:
+            error_report("Failed to reset overlapping slot\n");

WARNING: line over 80 characters
#100: FILE: target/i386/hvf/hvf.c:203:
+    if (area->readonly || (!memory_region_is_ram(area) && memory_region_is_romd(area)))

ERROR: braces {} are necessary for all arms of this statement
#100: FILE: target/i386/hvf/hvf.c:203:
+    if (area->readonly || (!memory_region_is_ram(area) && memory_region_is_romd(area)))
[...]
+    else
[...]

ERROR: Error messages should not contain newlines
#115: FILE: target/i386/hvf/hvf.c:229:
+        error_report("Error registering new memory slot\n");

ERROR: braces {} are necessary for all arms of this statement
#124: FILE: target/i386/hvf/hvf.c:357:
+    if (!slot)
[...]

ERROR: line over 90 characters
#126: FILE: target/i386/hvf/hvf.c:359:
+    if (!memory_region_is_ram(slot->region) && !(read && memory_region_is_romd(slot->region)))

ERROR: braces {} are necessary for all arms of this statement
#126: FILE: target/i386/hvf/hvf.c:359:
+    if (!memory_region_is_ram(slot->region) && !(read && memory_region_is_romd(slot->region)))
[...]

total: 6 errors, 3 warnings, 128 lines checked

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

2/5 Checking commit 3ebb7cf4ab7f (hvf: remove TSC synchronization code because it isn't fully complete)
3/5 Checking commit 995bd07ed756 (hvf: correctly handle REX prefix in relation to legacy prefixes)
WARNING: line over 80 characters
#34: FILE: target/i386/hvf/x86_decode.c:1691:
+target_ulong get_reg_ref(CPUX86State *env, int reg, int rex_present, int is_extended,

WARNING: line over 80 characters
#65: FILE: target/i386/hvf/x86_decode.c:1715:
+target_ulong get_reg_val(CPUX86State *env, int reg, int rex_present, int is_extended,

WARNING: line over 80 characters
#70: FILE: target/i386/hvf/x86_decode.c:1719:
+    memcpy(&val, (void *)get_reg_ref(env, reg, rex_present, is_extended, size), size);

WARNING: line over 80 characters
#78: FILE: target/i386/hvf/x86_decode.c:1852:
+        /* REX prefix must come after legacy prefixes.  REX before legacy is ignored.

WARNING: Block comments use a leading /* on a separate line
#78: FILE: target/i386/hvf/x86_decode.c:1852:
+        /* REX prefix must come after legacy prefixes.  REX before legacy is ignored.

WARNING: Block comments use * on subsequent lines
#79: FILE: target/i386/hvf/x86_decode.c:1853:
+        /* REX prefix must come after legacy prefixes.  REX before legacy is ignored.
+           Clear rex to simulate this. */

WARNING: Block comments use a trailing */ on a separate line
#79: FILE: target/i386/hvf/x86_decode.c:1853:
+           Clear rex to simulate this. */

WARNING: line over 80 characters
#192: FILE: target/i386/hvf/x86_decode.h:306:
+target_ulong get_reg_ref(CPUX86State *env, int reg, int rex_present, int is_extended,

WARNING: line over 80 characters
#195: FILE: target/i386/hvf/x86_decode.h:308:
+target_ulong get_reg_val(CPUX86State *env, int reg, int rex_present, int is_extended,

total: 0 errors, 9 warnings, 169 lines checked

Patch 3/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/5 Checking commit b201a1b3b822 (hvf: more accurately match SDM when setting CR0 and PDPTE registers)
5/5 Checking commit e1d16795b97a (hvf: save away type as well as vector so we can reinject them)
WARNING: line over 80 characters
#25: FILE: target/i386/hvf/hvf.c:637:
+            /* Save away the event type as well so we can inject the correct type. */

ERROR: line over 90 characters
#26: FILE: target/i386/hvf/hvf.c:638:
+            env->interrupt_injected = idtvec_info & (VMCS_IDT_VEC_TYPE | VMCS_IDT_VEC_VECNUM);

WARNING: line over 80 characters
#34: FILE: target/i386/hvf/hvf.c:645:
+            /* Save away the event type as well so we can inject the correct type. */

WARNING: line over 80 characters
#35: FILE: target/i386/hvf/hvf.c:646:
+            env->exception_nr = idtvec_info & (VMCS_IDT_VEC_TYPE | VMCS_IDT_VEC_VECNUM);

WARNING: line over 80 characters
#108: FILE: target/i386/hvf/x86hvf.c:389:
+                /* Make sure to indicate that VMCS_ENTRY_EXCEPTION_ERROR is valid */

total: 1 errors, 4 warnings, 91 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/cover.1574375668.git.dirty@apple.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH 0/5] hvf: stability fixes for HVF
Posted by Lukas Straub 4 years, 4 months ago
On Thu, 21 Nov 2019 14:54:49 -0800
Cameron Esfahani via <qemu-devel@nongnu.org> wrote:

> The following patches fix stability issues with running QEMU on Apple
> Hypervisor Framework (HVF):
> - non-RAM, non-ROMD areas need to trap so accesses can be correctly
>   emulated.
> - Current TSC synchronization implementation is insufficient: when
>   running with more than 1 core, TSC values can go backwards.  Until
>   a correct implementation can be written, remove calls to
>   hv_vm_sync_tsc().  Pass through TSC to guest OS.
> - Fix REX emulation in relation to legacy prefixes.
> - More correctly match SDM when setting CR0 and PDPTE registers.
> - Save away exception type as well as vector in hvf_store_events() so
>   they can be correctly reinjected in hvf_inject_interrupts().  Under
>   heavy loads, exceptions got misrouted.
>
> Cameron Esfahani (5):
>   hvf: non-RAM, non-ROMD memory ranges are now correctly mapped in
>   hvf: remove TSC synchronization code because it isn't fully complete
>   hvf: correctly handle REX prefix in relation to legacy prefixes
>   hvf: more accurately match SDM when setting CR0 and PDPTE registers
>   hvf: save away type as well as vector so we can reinject them
>
>  target/i386/hvf/hvf.c        | 74 +++++++++++++++++++++++++-----------
>  target/i386/hvf/vmx.h        | 18 +++++----
>  target/i386/hvf/x86_decode.c | 55 ++++++++++++++-------------
>  target/i386/hvf/x86_decode.h | 16 ++++----
>  target/i386/hvf/x86_emu.c    |  3 --
>  target/i386/hvf/x86hvf.c     | 26 +++++--------
>  6 files changed, 108 insertions(+), 84 deletions(-)
>

Hi,
I can't comment on your code, but simply resend this as v2 with the
checkpatch.pl errors fixed. You can run checkpatch.pl locally before
posting (scripts/checkpatch.pl).

Regards,
Lukas Straub