| Message ID | 20200207223520.735523-1-peterx@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: MIPS: Provide arch-specific kvm_flush_remote_tlbs() | expand |
On Fri, Feb 07, 2020 at 05:35:16PM -0500, Peter Xu wrote: > [This series is RFC because I don't have MIPS to compile and test] > > kvm_flush_remote_tlbs() can be arch-specific, by either: > > - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the > only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far > > - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ > support, however still wants to have the common tlb flush to be part > of the process. Could refer to kvm_vz_flush_shadow_all(). Then in > MIPS it's awkward to flush remote TLBs: we'll need to call the mips > hooks. > > It's awkward to have different ways to specialize this procedure, > especially MIPS cannot use the genenal interface which is quite a > pity. It's good to make it a common interface. > > This patch series removes the 2nd MIPS usage above, and let it also > use the common kvm_flush_remote_tlbs() interface. It should be > suggested that we always keep kvm_flush_remote_tlbs() be a common > entrance for tlb flushing on all archs. > > This idea comes from the reading of Sean's patchset on dynamic memslot > allocation, where a new dirty log specific hook is added for flushing > TLBs only for the MIPS code [1]. With this patchset, logically the [1] https://lore.kernel.org/kvm/20200207194532.GK2401@linux.intel.com/T/#m2da733d75dab5e54e2ae68de94fe8411166d6274 > new hook in that patch can be dropped so we can directly use > kvm_flush_remote_tlbs(). > > TODO: We can even extend another common interface for ranged TLB, but > let's see how we think about this series first. > > Any comment is welcomed, thanks. > > Peter Xu (4): > KVM: Provide kvm_flush_remote_tlbs_common() > KVM: MIPS: Drop flush_shadow_memslot() callback > KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references > KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs() > > arch/mips/include/asm/kvm_host.h | 7 ------- > arch/mips/kvm/Kconfig | 1 + > arch/mips/kvm/mips.c | 22 ++++++++++------------ > arch/mips/kvm/trap_emul.c | 15 +-------------- > arch/mips/kvm/vz.c | 14 ++------------ > include/linux/kvm_host.h | 1 + > virt/kvm/kvm_main.c | 10 ++++++++-- > 7 files changed, 23 insertions(+), 47 deletions(-) > > -- > 2.24.1 >
On 07/02/20 23:35, Peter Xu wrote: > [This series is RFC because I don't have MIPS to compile and test] > > kvm_flush_remote_tlbs() can be arch-specific, by either: > > - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the > only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far > > - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ > support, however still wants to have the common tlb flush to be part > of the process. Could refer to kvm_vz_flush_shadow_all(). Then in > MIPS it's awkward to flush remote TLBs: we'll need to call the mips > hooks. > > It's awkward to have different ways to specialize this procedure, > especially MIPS cannot use the genenal interface which is quite a > pity. It's good to make it a common interface. > > This patch series removes the 2nd MIPS usage above, and let it also > use the common kvm_flush_remote_tlbs() interface. It should be > suggested that we always keep kvm_flush_remote_tlbs() be a common > entrance for tlb flushing on all archs. > > This idea comes from the reading of Sean's patchset on dynamic memslot > allocation, where a new dirty log specific hook is added for flushing > TLBs only for the MIPS code [1]. With this patchset, logically the > new hook in that patch can be dropped so we can directly use > kvm_flush_remote_tlbs(). > > TODO: We can even extend another common interface for ranged TLB, but > let's see how we think about this series first. > > Any comment is welcomed, thanks. > > Peter Xu (4): > KVM: Provide kvm_flush_remote_tlbs_common() > KVM: MIPS: Drop flush_shadow_memslot() callback > KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references > KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs() > > arch/mips/include/asm/kvm_host.h | 7 ------- > arch/mips/kvm/Kconfig | 1 + > arch/mips/kvm/mips.c | 22 ++++++++++------------ > arch/mips/kvm/trap_emul.c | 15 +-------------- > arch/mips/kvm/vz.c | 14 ++------------ > include/linux/kvm_host.h | 1 + > virt/kvm/kvm_main.c | 10 ++++++++-- > 7 files changed, 23 insertions(+), 47 deletions(-) > Compile-tested and queued. MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to arch/mips/Kconfig? Paolo
Hi Paolo, On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote: > MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not > clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or > it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to > arch/mips/Kconfig? I'm no expert on this bit of code, but I'm pretty sure the systems KVM/VZ has been used on the most internally had PGTABLE_LEVEL=3. I suspect this is actually a regression from commit 31168f033e37 ("mips: drop __pXd_offset() macros that duplicate pXd_index() ones"). Whilst that commit is correct that pud_index() & __pud_offset() are the same when pud_index() is actually provided, it doesn't take into account the __PAGETABLE_PUD_FOLDED case. There __pud_offset() was available but would always evaluate to zero, whereas pud_index() isn't defined... Thanks, Paul
On 12/02/20 17:30, Paul Burton wrote: > Hi Paolo, > > On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote: >> MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not >> clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or >> it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to >> arch/mips/Kconfig? > > I'm no expert on this bit of code, but I'm pretty sure the systems > KVM/VZ has been used on the most internally had PGTABLE_LEVEL=3. > > I suspect this is actually a regression from commit 31168f033e37 ("mips: > drop __pXd_offset() macros that duplicate pXd_index() ones"). Whilst > that commit is correct that pud_index() & __pud_offset() are the same > when pud_index() is actually provided, it doesn't take into account the > __PAGETABLE_PUD_FOLDED case. There __pud_offset() was available but > would always evaluate to zero, whereas pud_index() isn't defined... Ok, I'll try to whip out a patch that handles __PAGETABLE_PUD_FOLDED. On the other hand this makes me worry about how much KVM is being tested by people that care about MIPS (even just compile-tested). Paolo
On 2/12/20 5:40 PM, Paolo Bonzini wrote: > On 12/02/20 17:30, Paul Burton wrote: >> Hi Paolo, >> >> On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote: >>> MIPS folks, I see that arch/mips/kvm/mmu.c uses pud_index, so it's not >>> clear to me if it's meant to only work if CONFIG_PGTABLE_LEVELS=4 or >>> it's just bit rot. Should I add a "depends on PGTABLE_LEVEL=4" to >>> arch/mips/Kconfig? >> >> I'm no expert on this bit of code, but I'm pretty sure the systems >> KVM/VZ has been used on the most internally had PGTABLE_LEVEL=3. >> >> I suspect this is actually a regression from commit 31168f033e37 ("mips: >> drop __pXd_offset() macros that duplicate pXd_index() ones"). Whilst >> that commit is correct that pud_index() & __pud_offset() are the same >> when pud_index() is actually provided, it doesn't take into account the >> __PAGETABLE_PUD_FOLDED case. There __pud_offset() was available but >> would always evaluate to zero, whereas pud_index() isn't defined... > > Ok, I'll try to whip out a patch that handles __PAGETABLE_PUD_FOLDED. > On the other hand this makes me worry about how much KVM is being tested > by people that care about MIPS (even just compile-tested). FYI last time James confirmed he tested QEMU was in 2017: https://www.mail-archive.com/qemu-devel@nongnu.org/msg477133.html At the end of 2019 he orphaned the QEMU part: https://www.mail-archive.com/qemu-devel@nongnu.org/msg667240.html and dropped the kernel maintainance: https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/commit/?id=9c48c48cd499
On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote: > On 07/02/20 23:35, Peter Xu wrote: > > [This series is RFC because I don't have MIPS to compile and test] > > > > kvm_flush_remote_tlbs() can be arch-specific, by either: > > > > - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the > > only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far > > > > - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ > > support, however still wants to have the common tlb flush to be part > > of the process. Could refer to kvm_vz_flush_shadow_all(). Then in > > MIPS it's awkward to flush remote TLBs: we'll need to call the mips > > hooks. > > > > It's awkward to have different ways to specialize this procedure, > > especially MIPS cannot use the genenal interface which is quite a > > pity. It's good to make it a common interface. > > > > This patch series removes the 2nd MIPS usage above, and let it also > > use the common kvm_flush_remote_tlbs() interface. It should be > > suggested that we always keep kvm_flush_remote_tlbs() be a common > > entrance for tlb flushing on all archs. > > > > This idea comes from the reading of Sean's patchset on dynamic memslot > > allocation, where a new dirty log specific hook is added for flushing > > TLBs only for the MIPS code [1]. With this patchset, logically the > > new hook in that patch can be dropped so we can directly use > > kvm_flush_remote_tlbs(). > > > > TODO: We can even extend another common interface for ranged TLB, but > > let's see how we think about this series first. > > > > Any comment is welcomed, thanks. > > > > Peter Xu (4): > > KVM: Provide kvm_flush_remote_tlbs_common() > > KVM: MIPS: Drop flush_shadow_memslot() callback > > KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references > > KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs() > > > > arch/mips/include/asm/kvm_host.h | 7 ------- > > arch/mips/kvm/Kconfig | 1 + > > arch/mips/kvm/mips.c | 22 ++++++++++------------ > > arch/mips/kvm/trap_emul.c | 15 +-------------- > > arch/mips/kvm/vz.c | 14 ++------------ > > include/linux/kvm_host.h | 1 + > > virt/kvm/kvm_main.c | 10 ++++++++-- > > 7 files changed, 23 insertions(+), 47 deletions(-) > > > > Compile-tested and queued. Just in case it fells through the crach - Paolo, do you still have plan to queue this again? Thanks,
On 11/03/20 19:32, Peter Xu wrote: > On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote: >> On 07/02/20 23:35, Peter Xu wrote: >>> [This series is RFC because I don't have MIPS to compile and test] >>> >>> kvm_flush_remote_tlbs() can be arch-specific, by either: >>> >>> - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the >>> only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far >>> >>> - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ >>> support, however still wants to have the common tlb flush to be part >>> of the process. Could refer to kvm_vz_flush_shadow_all(). Then in >>> MIPS it's awkward to flush remote TLBs: we'll need to call the mips >>> hooks. >>> >>> It's awkward to have different ways to specialize this procedure, >>> especially MIPS cannot use the genenal interface which is quite a >>> pity. It's good to make it a common interface. >>> >>> This patch series removes the 2nd MIPS usage above, and let it also >>> use the common kvm_flush_remote_tlbs() interface. It should be >>> suggested that we always keep kvm_flush_remote_tlbs() be a common >>> entrance for tlb flushing on all archs. >>> >>> This idea comes from the reading of Sean's patchset on dynamic memslot >>> allocation, where a new dirty log specific hook is added for flushing >>> TLBs only for the MIPS code [1]. With this patchset, logically the >>> new hook in that patch can be dropped so we can directly use >>> kvm_flush_remote_tlbs(). >>> >>> TODO: We can even extend another common interface for ranged TLB, but >>> let's see how we think about this series first. >>> >>> Any comment is welcomed, thanks. >>> >>> Peter Xu (4): >>> KVM: Provide kvm_flush_remote_tlbs_common() >>> KVM: MIPS: Drop flush_shadow_memslot() callback >>> KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references >>> KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs() >>> >>> arch/mips/include/asm/kvm_host.h | 7 ------- >>> arch/mips/kvm/Kconfig | 1 + >>> arch/mips/kvm/mips.c | 22 ++++++++++------------ >>> arch/mips/kvm/trap_emul.c | 15 +-------------- >>> arch/mips/kvm/vz.c | 14 ++------------ >>> include/linux/kvm_host.h | 1 + >>> virt/kvm/kvm_main.c | 10 ++++++++-- >>> 7 files changed, 23 insertions(+), 47 deletions(-) >>> >> >> Compile-tested and queued. > > Just in case it fells through the crach - Paolo, do you still have > plan to queue this again? Yes, I wanted to make it compile first though. I'm undecided between queuing your series and killing KVM MIPS honestly. Paolo
On Tue, Mar 17, 2020 at 02:33:00PM +0100, Paolo Bonzini wrote: > On 11/03/20 19:32, Peter Xu wrote: > > On Wed, Feb 12, 2020 at 01:25:30PM +0100, Paolo Bonzini wrote: > >> On 07/02/20 23:35, Peter Xu wrote: > >>> [This series is RFC because I don't have MIPS to compile and test] > >>> > >>> kvm_flush_remote_tlbs() can be arch-specific, by either: > >>> > >>> - Completely replace kvm_flush_remote_tlbs(), like ARM, who is the > >>> only user of CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL so far > >>> > >>> - Doing something extra before kvm_flush_remote_tlbs(), like MIPS VZ > >>> support, however still wants to have the common tlb flush to be part > >>> of the process. Could refer to kvm_vz_flush_shadow_all(). Then in > >>> MIPS it's awkward to flush remote TLBs: we'll need to call the mips > >>> hooks. > >>> > >>> It's awkward to have different ways to specialize this procedure, > >>> especially MIPS cannot use the genenal interface which is quite a > >>> pity. It's good to make it a common interface. > >>> > >>> This patch series removes the 2nd MIPS usage above, and let it also > >>> use the common kvm_flush_remote_tlbs() interface. It should be > >>> suggested that we always keep kvm_flush_remote_tlbs() be a common > >>> entrance for tlb flushing on all archs. > >>> > >>> This idea comes from the reading of Sean's patchset on dynamic memslot > >>> allocation, where a new dirty log specific hook is added for flushing > >>> TLBs only for the MIPS code [1]. With this patchset, logically the > >>> new hook in that patch can be dropped so we can directly use > >>> kvm_flush_remote_tlbs(). > >>> > >>> TODO: We can even extend another common interface for ranged TLB, but > >>> let's see how we think about this series first. > >>> > >>> Any comment is welcomed, thanks. > >>> > >>> Peter Xu (4): > >>> KVM: Provide kvm_flush_remote_tlbs_common() > >>> KVM: MIPS: Drop flush_shadow_memslot() callback > >>> KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references > >>> KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs() > >>> > >>> arch/mips/include/asm/kvm_host.h | 7 ------- > >>> arch/mips/kvm/Kconfig | 1 + > >>> arch/mips/kvm/mips.c | 22 ++++++++++------------ > >>> arch/mips/kvm/trap_emul.c | 15 +-------------- > >>> arch/mips/kvm/vz.c | 14 ++------------ > >>> include/linux/kvm_host.h | 1 + > >>> virt/kvm/kvm_main.c | 10 ++++++++-- > >>> 7 files changed, 23 insertions(+), 47 deletions(-) > >>> > >> > >> Compile-tested and queued. > > > > Just in case it fells through the crach - Paolo, do you still have > > plan to queue this again? > > Yes, I wanted to make it compile first though. I'm undecided between > queuing your series and killing KVM MIPS honestly. Understood. Yep killing that will provide the same thing too as what the series wanted to do anyways, as we'll remove the only outlier of kvm_flush_remote_tlbs(). Thanks,