On Sat, 16 May 2026 at 19:51, Richard Henderson
<[email protected]> wrote:
>
> On 5/15/26 05:57, Peter Maydell wrote:
> > -hwaddr mb_cpu_get_phys_addr_attrs_debug(CPUState *cs, vaddr addr,
> > - MemTxAttrs *attrs)
> > +bool mb_cpu_translate_for_debug(CPUState *cs, vaddr addr,
> > + TranslateForDebugResult *result)
> > {
> > MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
> > - hwaddr paddr = 0;
> > MicroBlazeMMULookup lu;
> > int mmu_idx = cpu_mmu_index(cs, false);
> > unsigned int hit;
> >
> > - /* Caller doesn't initialize */
> > - *attrs = (MemTxAttrs) {};
> > - attrs->secure = mb_cpu_access_is_secure(cpu, MMU_DATA_LOAD);
> > + result->attrs = (MemTxAttrs) {
> > + .secure = mb_cpu_access_is_secure(cpu, MMU_DATA_LOAD),
> > + .debug = 1,
> > + };
> > +
> > + result->lg_page_size = TARGET_PAGE_SIZE;
>
> TARGET_PAGE_BITS.
How many times am I going to make this mistake? I think
it must be the _size suffix on lg_page_size that makes
me think I want the _SIZE constant, not the _BITS one.
> >
> > if (mmu_idx != MMU_NOMMU_IDX) {
> > hit = mmu_translate(cpu, &lu, addr, 0, 0);
> > if (hit) {
> > - paddr = lu.paddr + addr - lu.vaddr;
> > - } else
> > - paddr = 0; /* ???. */
> > + result->physaddr = lu.paddr + addr - lu.vaddr;
> > + } else {
> > + result->physaddr = 0; /* ???. */
> > + }
>
> Surely this is the "return false" case.
> Fixing an existing bug, obviously.
Yes, it looks like that ought to be "translate failure",
but I just wanted to retain the existing behaviour here.
Possibly somebody had a reason for it given they flagged
it ??? rather than using "return -1" failure option.
> > } else {
> > - paddr = addr;
> > + result->physaddr = addr;
> > }
> >
> > - return paddr;
> > + return true;
>
> Perhaps cleaner to finish with
>
> *result = (TranslateForDebugResult){
> .physaddr = paddr,
> .lg_page_size = TARGET_PAGE_SIZE,
> .attrs.secure = mb_cpu_access_is_secure(cpu, MMU_DATA_LOAD),
> .attrs.debug = true,
I didn't know you could partially initialize substructs like that.
> };
> return true;
>
> ?
thanks
-- PMM
