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

[Paolo Bonzini]

Il gio 3 set 2020, 13:24 FelixCui-oc <FelixCui-oc@zhaoxin.com> ha scritto:

>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.

hi paolo,

              What you said is very similar to my issues. My problem is happened when a EHCI device accesses an adjacent mapping between the delete and add phases of the VFIO MemoryListener.  Does adding a new ioctl also apply to VFIO MemoryListener?

It would be done separately but it's the same issue. Let's ask Alex Williamson.

Alex, the issue here is that the delete+add passes are racing against an assigned device's DMA. For KVM we were thinking of changing the whole memory map with a single ioctl, but that's much easier because KVM builds its page tables lazily. It would be possible for the IOMMU too but it would require a relatively complicated comparison of the old and new memory maps in the kernel.

Is this something that you think would be acceptable for Linux? Would it require changes at the IOMMU level or would it be confined to VFIO?

Thanks,

Paolo

Best regards

Felixcui-oc

---

发件人: Paolo Bonzini <pbonzini@redhat.com>
发送时间: 2020年9月3日 18:37:47
收件人: FelixCui-oc; Richard Henderson; Eduardo Habkost
抄送: qemu-devel@nongnu.org; RockCui-oc; Tony W Wang-oc; CobeChen-oc; Peter Xu
主题: Re: [PATCH 1/1] Skip flatview_simplify() for specific cpu vendor

 

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,
>