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