* [PATCH 0/1] Skip flatview_simplify() for specific cpu vendor @ 2020-09-03 9:49 FelixCuioc 2020-09-03 9:49 ` [PATCH 1/1] " FelixCuioc 0 siblings, 1 reply; 5+ messages in thread From: FelixCuioc @ 2020-09-03 9:49 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Eduardo Habkost Cc: TonyWWang-oc, RockCui-oc, qemu-devel, CobeChen-oc The reason we want to skip flatview_simplify() is to prevent unnecessary IOVA address range mapping from being unmapped. The actual situation we encountered is: When assign EHCI device to the virtual machine, after initializing EHCI in seabios, it will continuously send dma cycles.And EHCI dma buffer is allocated from the range 0xd9000-0xexxxx belonging to zonelow. But in seabios, make_bios_readonly_intel() will modify the attributes in the range of 0xc0000-0x100000,except for the zonelow range. Before these ranges attributes are not changed,qemu will perform flatview_simplify(),and the actual address range formed in flatview is 0xc0000-0xbfffffff.When the properties of this large range are modified to readonly,qemu will unmap all the IOVA mappings in the address range 0xc0000-0xbfffffff. But EHCI device still send dma cycle. So dma cycles of the EHCI device will be blocked by the IOMMU. And we want to skip flatview_simplify(). Error log when starting the virtual machine: DMAR: [DMA Read] Request device [00:10.7] fault addr eb000 [fault reason 06] PTE Read access is not set DMAR: [DMA Read] Request device [00:10.7] fault addr eb000 [fault reason 06] PTE Read access is not set FelixCuioc (1): Skip flatview_simplify() for specific cpu vendor softmmu/memory.c | 16 +++++++++++++++- target/i386/cpu.c | 8 ++++++++ target/i386/cpu.h | 3 +++ 3 files changed, 26 insertions(+), 1 deletion(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] Skip flatview_simplify() for specific cpu vendor 2020-09-03 9:49 [PATCH 0/1] Skip flatview_simplify() for specific cpu vendor FelixCuioc @ 2020-09-03 9:49 ` FelixCuioc 2020-09-03 10:37 ` Paolo Bonzini 0 siblings, 1 reply; 5+ messages in thread From: FelixCuioc @ 2020-09-03 9:49 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Eduardo Habkost Cc: TonyWWang-oc, RockCui-oc, qemu-devel, CobeChen-oc 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> --- 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, -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] Skip flatview_simplify() for specific cpu vendor 2020-09-03 9:49 ` [PATCH 1/1] " FelixCuioc @ 2020-09-03 10:37 ` Paolo Bonzini 2020-09-03 11:24 ` 答复: " FelixCui-oc 0 siblings, 1 reply; 5+ messages in thread From: Paolo Bonzini @ 2020-09-03 10:37 UTC (permalink / raw) To: FelixCuioc, Richard Henderson, Eduardo Habkost Cc: TonyWWang-oc, RockCui-oc, qemu-devel, Peter Xu, CobeChen-oc 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, > ^ permalink raw reply [flat|nested] 5+ messages in thread
* 答复: [PATCH 1/1] Skip flatview_simplify() for specific cpu vendor 2020-09-03 10:37 ` Paolo Bonzini @ 2020-09-03 11:24 ` FelixCui-oc 2020-09-03 12:08 ` Paolo Bonzini 0 siblings, 1 reply; 5+ messages in thread From: FelixCui-oc @ 2020-09-03 11:24 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Eduardo Habkost Cc: Tony W Wang-oc, RockCui-oc, qemu-devel, Peter Xu, CobeChen-oc [-- Attachment #1: Type: text/plain, Size: 4917 bytes --] >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. VFIO MemoryListener is also included under address_space_memory. Does adding a new ioctl also apply to VFIO MemoryListener? 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, > [-- Attachment #2: Type: text/html, Size: 11603 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] Skip flatview_simplify() for specific cpu vendor 2020-09-03 11:24 ` 答复: " FelixCui-oc @ 2020-09-03 12:08 ` Paolo Bonzini 0 siblings, 0 replies; 5+ messages in thread From: Paolo Bonzini @ 2020-09-03 12:08 UTC (permalink / raw) To: FelixCui-oc Cc: CobeChen-oc, qemu-devel, Peter Xu, Tony W Wang-oc, RockCui-oc, Richard Henderson, Eduardo Habkost [-- Attachment #1: Type: text/plain, Size: 5848 bytes --] 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, > > > > [-- Attachment #2: Type: text/html, Size: 11737 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-03 13:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-03 9:49 [PATCH 0/1] Skip flatview_simplify() for specific cpu vendor FelixCuioc 2020-09-03 9:49 ` [PATCH 1/1] " FelixCuioc 2020-09-03 10:37 ` Paolo Bonzini 2020-09-03 11:24 ` 答复: " FelixCui-oc 2020-09-03 12:08 ` Paolo Bonzini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.