Re: [Qemu-devel] [PATCH RFC] target-ppc: Correctly handle translation address when bus unit ID = 0x07F

[Alexander Graf]

On 13.06.2011, at 12:13, Andreas Färber wrote:

> From: Hervé Poussineau <address@hidden>
> 
> In that case, we want to access memory space instead of I/O controller
> interface address space.
> 
> Signed-off-by: Hervé Poussineau <address@hidden>
> 
> Simplify by avoiding reindentation of existing code.
> 
> Cc: Alexander Graf <address@hidden>
> Signed-off-by: Andreas Färber <address@hidden>
> ---
> Hello Alex,
> 
> This patch fixes a hang when booting 40p, please review.
> 
> The only vaguely related section in Power ISA 2.06B I could find was the
> Programming Note on p. 764 (5.7.1).

The 601 is not 2.06 compliant, so you need to dig up earlier manuals. Google 
revealed this for the 601: 

  http://www.freescale.com/files/32bit/doc/user_guide/MPC601UM.pdf

> 6xx_pem.pdf identifies the masked SR bits as Bus unit ID, says nothing
> about the 0x07f value though - might that be machine-specific?
> 
> Andreas
> 
> target-ppc/helper.c |   10 ++++++++++
> 1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index cf2a368..cdf8d15 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -949,8 +949,18 @@ static inline int get_segment(CPUState *env, mmu_ctx_t 
> *ctx,
>             ret = -3;
>         }
>     } else {
> +        target_ulong sr;
>         LOG_MMU("direct store...\n");

This is direct store, so you're in T=1

>         /* Direct-store segment : absolutely *BUGGY* for now */
> +
> +        sr = env->sr[eaddr >> 28];
> +        if ((sr & 0x1FF00000) >> 20 == 0x07f) {

This is the BUID, yes.
According to page 70 in the manual I mentioned above, the following passage 
applies:


Memory-forced I/O controller interface (BUID = x'07F')—Memory-forced I/O 
controller interface operations access memory space. They do not use the 
extensions to the memory protocol described for I/O controller interface 
accesses, and they bypass the page- and block-translation and protection 
mechanisms. The physical address is found by concatenating bits 28–31 of the 
respective segment register with bits 4–31 of the effective address. This 
address is marked as noncacheable, write- through, and global.
Because memory-forced I/O controller interface accesses address memory space, 
they are subject to the same coherency control as other memory reference 
operations. More generally, accesses to memory-forced I/O controller interface 
segments are considered to be cache-inhibited, write-through and 
memory-coherent operations with respect to the 601 cache and bus interface.


Since we don't implement any cache ourselves and MMIO is simply handled 
immediately, I don't think there's anything special that needs to be done, 
except for mapping it as EA=RA.

> +            /* Memory forced */
> +            ctx->raddr = ((sr & 0xF) << 28) | (eaddr & 0x0FFFFFFF);

This is exactly the same as ctx->raddr = eaddr, no?

So yes, in general the patch looks fine. Please add some comments to document 
what's going on though. I don't think everyone who reads the code wants to dig 
the out from the manuals :).



Alex