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

From: Hervé Poussineau <hpoussin@reactos.org>

In that case, we want to access memory space instead of I/O controller
interface address space.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

Simplify by avoiding reindentation of existing code.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 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).
 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(-)

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

> From: Hervé Poussineau <hpoussin@reactos.org>
> 
> In that case, we want to access memory space instead of I/O controller
> interface address space.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> 
> Simplify by avoiding reindentation of existing code.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
> 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

Am 13.06.2011 um 15:31 schrieb Alexander Graf:

> On 13.06.2011, at 12:13, Andreas Färber wrote:
>
>> +            /* Memory forced */
>> +            ctx->raddr = ((sr & 0xF) << 28) | (eaddr & 0x0FFFFFFF);
>
> This is exactly the same as ctx->raddr = eaddr, no?

No, not that I see. The manual is explicit about this calculation:

"the processor [...] generates a memory access
with the physical address specified by the lowest-order four bits in  
the segment register
(SR[28–31]) concatenated with LA4–LA31." (6-63 / 6.10.4)

That matches figure 6-26 on the following page, and I've added a  
similar comment in v2.

I see no compelling reason why the lower nibble of the SR should  
always match the SR#, the upper nibble of eaddr.

Andreas

On 14.06.2011, at 23:49, Andreas Färber wrote:

> Am 13.06.2011 um 15:31 schrieb Alexander Graf:
> 
>> On 13.06.2011, at 12:13, Andreas Färber wrote:
>> 
>>> +            /* Memory forced */
>>> +            ctx->raddr = ((sr & 0xF) << 28) | (eaddr & 0x0FFFFFFF);
>> 
>> This is exactly the same as ctx->raddr = eaddr, no?
> 
> No, not that I see. The manual is explicit about this calculation:
> 
> "the processor [...] generates a memory access
> with the physical address specified by the lowest-order four bits in the segment register
> (SR[28–31]) concatenated with LA4–LA31." (6-63 / 6.10.4)
> 
> That matches figure 6-26 on the following page, and I've added a similar comment in v2.
> 
> I see no compelling reason why the lower nibble of the SR should always match the SR#, the upper nibble of eaddr.

Ah, sr is not the segment register number, but the content of the segement register. Then it's ok :).


Alex