Re: [PATCH 1/1] Skip flatview_simplify() for specific cpu vendor

[Paolo Bonzini <pbonzini@redhat.com>]

On 03/09/20 11:49, FelixCuioc wrote:
> Flatview_simplify() will merge many address ranges
> into one range.When a part of the big range needs
> to be changed,this will cause some innocent mappings
> to be unmapped.So we want to skip flatview_simplify().
> 
> Signed-off-by: FelixCuioc <FelixCui-oc@zhaoxin.com>

This has several issues.  In no particular order:

1) you're adding host_get_vendor to target/i386/cpu.c so this does not
even build for the default "../configure && make".

2) you're adding a check for the host, but the bug applies to all hosts.
 If there is a bug on x86 hardware emulation, it should be fixed even
when emulating x86 from ARM.

3) you're not explaining what is the big range and how the bug is
manifesting.

I think you're seeing issues when a guest accesses an adjacent mapping
between the delete and add phases of the KVM MemoryListener.  We're
considering fixing that in the kernel, by adding a new ioctl that
changes the whole memory map in a single step.  I am CCing Peter Xu.

Paolo


> ---
>  softmmu/memory.c  | 16 +++++++++++++++-
>  target/i386/cpu.c |  8 ++++++++
>  target/i386/cpu.h |  3 +++
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 70b93104e8..348e9db622 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -699,6 +699,18 @@ static MemoryRegion *memory_region_get_flatview_root(MemoryRegion *mr)
>      return NULL;
>  }
>  
> +static bool skip_simplify(void)
> +{
> +    char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
> +    host_get_vendor(vendor);
> +    if (!strncmp(vendor, CPUID_VENDOR_VIA, strlen(CPUID_VENDOR_VIA))
> +        || !strncmp(vendor, CPUID_VENDOR_ZHAOXIN,
> +                    strlen(CPUID_VENDOR_ZHAOXIN))) {
> +        return true;
> +    }
> +    return false;
> +}
> +
>  /* Render a memory topology into a list of disjoint absolute ranges. */
>  static FlatView *generate_memory_topology(MemoryRegion *mr)
>  {
> @@ -712,7 +724,9 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
>                               addrrange_make(int128_zero(), int128_2_64()),
>                               false, false);
>      }
> -    flatview_simplify(view);
> +    if (!skip_simplify()) {
> +        flatview_simplify(view);
> +    }
>  
>      view->dispatch = address_space_dispatch_new(view);
>      for (i = 0; i < view->nr; i++) {
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 49d8958528..08508c8580 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1664,6 +1664,14 @@ void host_cpuid(uint32_t function, uint32_t count,
>          *edx = vec[3];
>  }
>  
> +void host_get_vendor(char *vendor)
> +{
> +    uint32_t eax, ebx, ecx, edx;
> +
> +    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> +    x86_cpu_vendor_words2str(vendor, ebx, edx, ecx);
> +}
> +
>  void host_vendor_fms(char *vendor, int *family, int *model, int *stepping)
>  {
>      uint32_t eax, ebx, ecx, edx;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index d3097be6a5..14c8b4c09f 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -832,6 +832,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  
>  #define CPUID_VENDOR_VIA   "CentaurHauls"
>  
> +#define CPUID_VENDOR_ZHAOXIN   "  Shanghai  "
> +
>  #define CPUID_VENDOR_HYGON    "HygonGenuine"
>  
>  #define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
> @@ -1917,6 +1919,7 @@ void cpu_clear_apic_feature(CPUX86State *env);
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
>  void host_vendor_fms(char *vendor, int *family, int *model, int *stepping);
> +void host_get_vendor(char *vendor);
>  
>  /* helper.c */
>  bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>