Re: [PATCH] target/hppa: Optimize ldcw/ldcd instruction translation

[Richard Henderson]

On 9/13/23 10:19, Helge Deller wrote:
> On 9/13/23 18:55, Richard Henderson wrote:
> > On 9/13/23 07:47, Helge Deller wrote:
> > > +        haddr = (uint32_t *)((uintptr_t)vaddr);
> > > +        old = *haddr;
> >
> > This is horribly incorrect, both for user-only and system mode.
>
> Richard, thank you for the review!
> But would you mind explaining why this is incorrect?
> I thought the "vaddr = probe_access()" calculates the host address, so
> shouldn't it be the right address?

The vaddr name is confusing (since it implies virtual address, which the return from 
probe_access is not) as are the casts, which are unnecessary.



> > > +        /* if already zero, do not write 0 again to reduce memory presssure */
> > > +        if (old == 0) {
> > > +            return 0;
> > > +        }
> > > +        old = qatomic_xchg(haddr, (uint32_t) 0);
> >
> > You're also dropping the required host memory barrier.
>
> ?

The path through the read+test+return, without the qatomic_xchg, has no host memory 
barrier to provide sequential consistency of the entire operation.


r~