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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4329 bytes --]

On Wed, May 19, 2021 at 05:47:05PM -0500, Richard Henderson wrote:
> 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.

Ok, I'll leave them in, since they're good cleanups even without the
rest of the series.

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

Ah, that's probably my fault.  I reworked those substantially from the
original code (closer to mmu_helper.c).  I guess I didn't (and I
suspect I still don't) really understand how the softmmu works.

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

Uh.. you started by saying mmu-radix64.c then talked about the hash
table, so I'm not sure which model you're talking about.

For hash + hypervisor, then yes, there is no hardware 2-level
translation, it all works by paravirtualizing updates to the hash
table (this is the H_ENTER etc. code in hw/ppc/spapr_softmmu.c).

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

Indeed.  Direct store segments are basically ancient history of the
POWER architecture which Linux never used, and I don't think much else
did either.  So I'm inclined to go with

  (D) Just rip out all the direct store segment code and replace with
      some LOG_UNIMPs.  Re-adding it working can be the job of the
      probably non-existent person who has an actual use case using
      them.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]