Re: [PATCH 00/24] target/ppc: Clean up mmu translation

[Richard Henderson <richard.henderson@linaro.org>]

On 5/19/21 3:37 PM, Richard Henderson wrote:
> On 5/18/21 9:52 PM, David Gibson wrote:
>> I've applied 1..15, still looking at the rest.
> 
> Please dequeue.  I want to create a new mmu-internal.h, which affects all the 
> patches from #1.

Alternately, don't.  I can move the function later, and it may be a while 
before I can get back to this.

Two outstanding bugs:

(1) mmu-radix64.c vs hypervisors.  You'll not see these unless you run kvm 
inside of tcg.

Basically, all usage of msr_{hv,pr,ir,dr} within ppc_*_xlate is incorrect.  We 
should be pulling these from the 3 bits of mmu_idx, as outlined in the table in 
hreg_compute_hflags_value.

When you start propagating that around, you see that the second-level 
translation for loading the pte (2 of the 3 calls to 
ppc_radix64_partition_scoped_xlate) should not be using the mmu_idx related to 
the user-mode guest access, but should be using the mmu_idx of the 
kernel/hypervisor that owns the page table.

I can't see that mmu-radix64.c has the same problem.  I'm not really sure how 
the second-level translation for hypervisors works there.  Is it by the 
hypervisor altering the hash table as it is loaded?

(2) The direct-segment accesses for 6xx and hash32 use ACCESS_* to 
conditionally reject an mmu access.  This is flawed, because we only arrive 
into these translation routines on the softtlb slow path.  If we have an 
ACCESS_INT and then an ACCESS_FLOAT, the first will load a tlb entry which the 
second will use to stay on the fast path.

There are several possible solutions:

  (A) Give tlb_set_page size == 1 for direct-segment addresses.
      This will cause cputlb.c to force all references to the page
      back through the slow path and through tlb_fill.  At which
      point env->access_type is correct for each access, and we
      can reject on type.

      This could be really slow.  But since these direct-segment
      accesses are also uncached, I would expect the guest OS to
      only be using them for i/o access.  Which is already slow,
      so perhaps the extra trip through tlb_fill isn't noticeable.

  (B) Use additional mmu_idx.  Not ideal, since we wouldn't be
      sharing softtlb entries for ACCESS_INT and ACCESS_FLOAT
      and ACCESS_RES for the normal case.  But the relevant
      mmu_models do not have hypervisor support, so we could
      still fit them in to 8 mmu_idx.  We'd probably want to
      use special code for ACCESS_CACHE and ACCESS_EXT here.

  (C) Ignore this special case entirely, dropping everything
      related to env->access_type.  The section of code that
      uses them is all for really old machine types with
      comments like

         /* Direct-store segment : absolutely *BUGGY* for now */

      which is not encouraging.  And short of writing our own
      test cases we're not likely to have any code to exercise
      this.


r~