Re: [PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection

[Richard Henderson]

On 8/10/24 04:55, Deepak Gupta wrote:
> On Wed, Aug 07, 2024 at 01:19:55PM +1000, Richard Henderson wrote:
> > On 8/7/24 10:06, Deepak Gupta wrote:
> > >      int prot = 0;
> > > -    if (pte & PTE_R) {
> > > +    /*
> > > +     * If PTE has read bit in it or it's shadow stack page,
> > > +     * then reads allowed
> > > +     */
> > > +    if ((pte & PTE_R) || sstack_page) {
> > >          prot |= PAGE_READ;
> > >      }
> >
> > I feel like this logic could be simplified somehow.
> > I'll think about it.
>
> Ok let me know.
>
> > > @@ -1409,6 +1461,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> > > size,
> > >      qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
> > >                    __func__, address, access_type, mmu_idx);
> > > +    /* If shadow stack instruction initiated this access, treat it as store */
> > > +    if (mmu_idx & MMU_IDX_SS_ACCESS) {
> > > +        access_type = MMU_DATA_STORE;
> > > +    }
> >
> > I know you're trying to massage the fault type, but I think this is the wrong 
> > place.
>
> Is it okay if I add `mmu_idx` argument to `raise_mmu_exception` ?
> Inside `raise_mmu_exception`, then based on `mmu_idx == shadow stack index`, I 
> can convert
> a fault due to access_type=MMU_DATA_LOAD into store page fault.

We have other places where we miss-categorize amo instructions and raise the wrong fault, 
I think particularly without smp, when we implement amo without host atomic operations. 
We should perhaps come up with a general purpose solution.

For instance, set TARGET_INSN_START_EXTRA_WORDS to 2.  In the second extra unwind word, 
set bit 0 to 1 if the instruction should raise STORE_AMO on a read fault.  Handle this in 
raise_mmu_exception, which would then need to perform the restore and exit itself.


r~