Re: [PATCH] target/ppc: Generate storage interrupts for radix RC changes

[Shawn Anastasio]

Hi Cédric,

On 7/12/23 3:27 AM, Cédric Le Goater wrote:
> Hello Shawn,
> 
> On 7/12/23 00:24, Shawn Anastasio wrote:
>> Change radix64_set_rc to always generate a storage interrupt when the
>> R/C bits are not set appropriately instead of setting the bits itself.
>> According to the ISA both behaviors are valid, but in practice this
>> change more closely matches behavior observed on the POWER9 CPU.
>>
>>  From the POWER9 Processor User's Manual, Section 4.10.13.1: "When
>> performing Radix translation, the POWER9 hardware triggers the
>> appropriate interrupt ... for the mode and type of access whenever
>> Reference (R) and Change (C) bits require setting in either the guest or
>> host page-table entry (PTE)."
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>> ---
>>   target/ppc/mmu-radix64.c | 57 ++++++++++++++++++++++++++++------------
>>   1 file changed, 40 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>> index 920084bd8f..06e1cced31 100644
>> --- a/target/ppc/mmu-radix64.c
>> +++ b/target/ppc/mmu-radix64.c
>> @@ -219,27 +219,48 @@ static bool ppc_radix64_check_prot(PowerPCCPU
>> *cpu, MMUAccessType access_type,
>>       return false;
>>   }
>>   -static void ppc_radix64_set_rc(PowerPCCPU *cpu, MMUAccessType
>> access_type,
>> -                               uint64_t pte, hwaddr pte_addr, int *prot)
>> +static int ppc_radix64_check_rc(PowerPCCPU *cpu, MMUAccessType
>> access_type,
>> +                               uint64_t pte, vaddr eaddr, bool
>> partition_scoped,
>> +                               hwaddr g_raddr)
>>   {
>> -    CPUState *cs = CPU(cpu);
>> -    uint64_t npte;
>> +    uint64_t lpid = 0;
>> +    uint64_t pid = 0;
>>   -    npte = pte | R_PTE_R; /* Always set reference bit */
>> +    switch (access_type) {
>> +    case MMU_DATA_STORE:
>> +        if (!(pte & R_PTE_C)) {
>> +            break;
>> +        }
>> +        /* fall through */
>> +    case MMU_INST_FETCH:
>> +    case MMU_DATA_LOAD:
>> +        if (!(pte & R_PTE_R)) {
>> +            break;
>> +        }
>>   -    if (access_type == MMU_DATA_STORE) { /* Store/Write */
>> -        npte |= R_PTE_C; /* Set change bit */
>> -    } else {
>> -        /*
>> -         * Treat the page as read-only for now, so that a later write
>> -         * will pass through this function again to set the C bit.
>> -         */
>> -        *prot &= ~PAGE_WRITE;
>> +        /* R/C bits are already set appropriately for this access */
>> +        return 0;
>>       }
>>   -    if (pte ^ npte) { /* If pte has changed then write it back */
>> -        stq_phys(cs->as, pte_addr, npte);
>> +    /* Obtain effLPID */
>> +    (void)ppc_radix64_get_fully_qualified_addr(&cpu->env, eaddr,
>> &lpid, &pid);
>> +
>> +    /*
>> +     * Per ISA 3.1 Book III, 7.5.3 and 7.5.5, failure to set R/C during
>> +     * partition-scoped translation when effLPID = 0 results in normal
>> +     * (non-Hypervisor) Data and Instruction Storage Interrupts
>> respectively.
>> +     *
>> +     * ISA 3.0 is ambiguous about this, but tests on POWER9 hardware
>> seem to
>> +     * exhibit the same behavior.
>> +     */
>> +    if (partition_scoped && lpid > 0) {
>> +        ppc_radix64_raise_hsi(cpu, access_type, eaddr, g_raddr,
>> +                              DSISR_ATOMIC_RC);
>> +    } else {
>> +        ppc_radix64_raise_si(cpu, access_type, eaddr, DSISR_ATOMIC_RC);
>>       }
> 
> I would raise the exception in the callers :
> 
>   ppc_radix64_partition_scoped_xlate()
>   ppc_radix64_process_scoped_xlate()
> 
> lpid could be passed to these routines also, this to avoid the call to
> ppc_radix64_get_fully_qualified_addr().
> 
> This requires a little more changes but would be cleaner I think.

Sure, I can do that. I'll send a v2 with this change made.

> Thanks,
> 
> C.

Thanks,
Shawn