sifive / freedom-metal

Bare Metal Compatibility Library for the Freedom Platform
Other
154 stars 47 forks source link

metal_dcache routines not accounting for "all" operations #208

Open TacoGrandeTX opened 4 years ago

TacoGrandeTX commented 4 years ago

I was trying to use metal_dcache_l1_flush() and metal_dcache_l1_discard() and was taking a trap. While debugging the program, it appears we mishandle the case where we want the entire dcache flushed or discarded. To do the "all" ops, I assume we pass in a 0x0 for the addresss - or how do we indicate the "all" ops?

For the ops that target the entire dcache, we need to use "x0" as the "rs1" operand, not the register holding "address" (which in my testing is picking up register a5). I've added this to the function... no trap is taken but still need to confirm that the cache operation is taking place:

if (!address) {
        /* we flush all dcache by using x0 */
        __asm__ __volatile__ (".insn i 0x73, 0, x0, x0, -0x40" : : );
        __asm__ __volatile__ ("fence.i");         // FENCE
    }
else... 
bsousi5 commented 4 years ago

Hi Ralph,

Register x0 is a special register which its contents are all tie to zero. So using another register like a5 with its content with address of 0x0, should do the same job. We can add an address check, but chose not too, to minimize the code size.

On Dec 17, 2019, at 7:51 AM, Ralph Fulchiero notifications@github.com wrote:

I was trying to use metal_dcache_l1_flush() and metal_dcache_l1_discard() and was taking a trap. While debugging the program, it appears we mishandle the case where we want the entire dcache flushed or discarded. To do the "all" ops, I assume we pass in a 0x0 for the addresss - or how do we indicate the "all" ops?

For the ops that target the entire dcache, we need to use "x0" as the "rs1" operand, not the register holding "address" (which in my testing is picking up register a5). I've added this to the function... no trap is taken but still need to confirm that the cache operation is taking place:

if (!address) { / we flush all dcache by using x0 / asm volatile (".insn i 0x73, 0, x0, x0, -0x40" : : ); asm volatile ("fence.i"); // FENCE } else... — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sifive/freedom-metal/issues/208?email_source=notifications&email_token=AIQNVX3QVT4Y2TCOV6DG6D3QZDYRVA5CNFSM4J35SYCKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IBC2GUA, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIQNVX53LDKVHEJWHKXBM43QZDYRVANCNFSM4J35SYCA.

TacoGrandeTX commented 4 years ago

Hi Bunnaroath,

So using another register like a5 with its content with address of 0x0, should do the same job.

That's not what the instruction description states from the Core Manuals: • When rs1 = x0, CFLUSH.D.L1 writes back and invalidates all lines in the L1 D$. • When rs1 != x0, CFLUSH.D.L1 writes back and invalidates the L1 D$ line containing the virtual address in integer register rs1.

If we aren't using x0 as the rs1 operand, then we're getting the single line operation. In the case of passing 0x0 as the argument, we're then flushing/invalidating only the line at address 0x0.

TacoGrandeTX commented 4 years ago

@bsousi5 Do you plan on fixing this before the 19.08p3 release? Last month I thought you said you made a PR, but not seeing it.