Re: [PATCH 0/9] target/microblaze: Always use TARGET_LONG_BITS == 32

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Wed, Apr 30, 2025 at 09:29:20AM +0200, Philippe Mathieu-Daudé wrote:
> On 30/4/25 08:26, Philippe Mathieu-Daudé wrote:
> > Hi,
> > 
> > On 13/2/25 13:37, Philippe Mathieu-Daudé wrote:
> > > +AMD folks
> > > 
> > > On 12/2/25 23:01, Richard Henderson wrote:
> > > > Use out-of-line helpers to implement extended address memory ops.
> > > > With this, we can reduce TARGET_LONG_BITS to the more natural 32
> > > > for this 32-bit cpu.
> > > 
> > > I thought about something similar 2 months ago, but then realized
> > > MicroBlaze cores can be synthetized in 64-bit, and IIRC there is
> > > not much missing (I'd say effort would be to add 20% more of what
> > > we currently have). Just wanted to mention before taking the
> > > decision to restrict to 32-bit. OTOH if there are no plan for
> > > adding 64-bit support at AMD, then I'm more than happy to simplify
> > > by considering only 32-bit.
> > 
> > I gave this series another go, and figured the microblaze target
> > addition was done way before the 64-bit. C_DATA_SIZE value was fixed
> > as 32, and C_ADDR_SIZE was not mentioned. Later C_DATA_SIZE became
> > configurable as [32, 64] and C_ADDR_SIZE appeared.
> 
> FTR C_ADDR_SIZE starts to be mentioned in Vivado 2016.1 release as
> 
>   • Included description of address extension, new in version 9.6.
> 
> Commit 72e387548534 (Jun 18 2015) made explicit supported versions
> were 5.00.a up to 9.3 (per Vivado 2014.1 release).
> 
> Commit d79fcbc298b0 (Jan 11 2017) "Add CPU versions 9.4, 9.5 and 9.6",
> and commit feac83af3be6 (Jun 15 2017) "Add CPU version 10.0" (released
> in Vivado 2016.3, but MMU Physical Address Extension 'PAE' came in
> Vivado 2017.1).
> 
> Vivado 2018.3 added MicroBlaze 64-bit implementation "new in version 11.0".
> 
> IIUC current implementation is correct w.r.t. v9.5.
> 
> I'm not so sure we can announce v9.6 and v10.0 as correctly implemented.
> 

Hi Phil,

The version of a MicroBlaze CPU core is orthogonal with the 64bit support,
new cores can be used with or without 64bit support.

There may be optional features missing but I don't think it's necessary to
remove the versions.


> Looking at what our machines uses, latest is v8.40.b:
> 
> hw/microblaze/petalogix_ml605_mmu.c:88: object_property_set_str(OBJECT(cpu),
> "version", "8.10.a", &error_abort);
> hw/microblaze/petalogix_s3adsp1800_mmu.c:78:
> object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort);
> hw/microblaze/xlnx-zynqmp-pmu.c:95: object_property_set_str(OBJECT(&s->cpu),
> "version", "8.40.b",
> 
> Maybe we can deprecate / remove v9.6 & v10.0 to better add them with
> a proper microblaze64 target implementation?

IIRC, there're Xilinx internal verification suites that check for exact
versions of cores, so for the Xilinx models with platform cores
(e.g CSU, PMU, PPU's etc) we try to instantiate them with versions
matching real HW even though most of the time there's no SW visible
difference (other than the ID) and no difference to QEMU.

Cheers,
Edgar

> 
> > Indeed what this series does is correctly implement the current
> > target as C_DATA_SIZE=32 (C_ADDR_SIZE=32 implied).
> > 
> > I had a quick look at what is missing for C_DATA_SIZE > 32 and it
> > is more than the 20% I first roughly estimated. So with the current
> > implementation, this series is doing the right thing IMHO.
> > 
> > Regards,
> > 
> > Phil.
>