On Fri, 16 Oct 2026 17:47:01 +0800 Jinjie Ruan <[email protected]> wrote:
> From: Jonathan Cameron <[email protected]> > > Add the mpam-msc-mem subtype for memory-side components. Its > realize assembles the identity view of MPAMF_IDR (PARTID space, > optional RIS support, HAS_ESR/HAS_EXTD_ESR, HAS_PARTID_NRW when > the internal PARTID space is smaller) plus IIDR/AIDR, and > advertises no partitioning features yet: the bandwidth controls > come with the MBW patch. > > Based on an RFC series by Jonathan Cameron. > > Signed-off-by: Jonathan Cameron <[email protected]> > Signed-off-by: Jinjie Ruan <[email protected]> Just some minor comments, mostly about comments I left lying around that I now don't see any purpose in keeping. > --- > hw/arm/mpam.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 68 insertions(+) > > diff --git a/hw/arm/mpam.c b/hw/arm/mpam.c > index b1d6472..8a85c3b 100644 > --- a/hw/arm/mpam.c > +++ b/hw/arm/mpam.c > @@ -260,8 +260,15 @@ typedef struct MPAMMSCState { > > } MPAMMSCState; > > +typedef struct MPAMMSCMemState { > + MPAMMSCState parent; > +} MPAMMSCMemState; > + > DECLARE_INSTANCE_CHECKER(MPAMMSCState, MPAM_MSC_DEVICE, TYPE_MPAM_MSC); > > +DECLARE_INSTANCE_CHECKER(MPAMMSCMemState, MPAM_MSC_MEM_DEVICE, > + TYPE_MPAM_MSC_MEM); > + > static uint64_t mpam_msc_read_reg(void *opaque, hwaddr offset, > unsigned size) > { > @@ -367,6 +374,52 @@ static void mpam_msc_realize(DeviceState *dev, Error > **errp) > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr); > } > > +static void mpam_msc_mem_realize(DeviceState *dev, Error **errp) > +{ MPAMMSCState *s = MPAM_MSC_DEVICE(dev); > + int i; > + > + mpam_msc_realize(dev, errp); > + > + for (i = 0; i < s->num_ris; i++) { for (int i = 0; .. > + Mpamf *mpamf = &s->mpamf[i]; > + > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, PART_ID_MAX, > + s->num_partid - 1); > + /* No PCMG for now */ > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, PMG_MAX, 0); > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, EXT, 1); I guess if being flexible this would emulate with and without EXT, but meh, not sure it adds much value. > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, HAS_RIS, s->num_ris > > 1); > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, RIS_MAX, > + s->num_ris > 1 ? s->num_ris - 1 : 0); > + /* Optional - test with and without */ Should drop that comment, or add the ability to test with and without - honestly I'm not sure which field that was even referring too as kind of applies to all the HAS_ ones. > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, HAS_ESR, 1); > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, HAS_EXTD_ESR, 1); > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, HAS_PARTID_NRW, > + s->num_int_partid < s->num_partid); > + > + /* We won't implement any implementation specific stuff */ > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, NO_IMPL_PART, 1); > + mpamf->idr = FIELD_DP64(mpamf->idr, MPAMF_IDR, NO_IMPL_MSMON, 1); > + > + mpamf->iidr = FIELD_DP64(mpamf->iidr, MPAMF_IIDR, IMPLEMENTER, > + MPAM_IIDR_IMPLEMENTER); > + mpamf->iidr = FIELD_DP64(mpamf->iidr, MPAMF_IIDR, REVISION, 0); > + mpamf->iidr = FIELD_DP64(mpamf->iidr, MPAMF_IIDR, VARIANT, 0); > + /* FIXME get allocation for this emulation */ That still needs fixing before this can be merged. If sticking to Hisilicon implementer, need to get one set aside so it is never real hardware - just in case we run into problems. Or... Use implementer 0 which is reserved for software use on CPUs and hence I assume is fine here? Maybe then set the product ID to 'Q' like the gicv2m emulation does? > + mpamf->iidr = FIELD_DP64(mpamf->iidr, MPAMF_IIDR, PRODUCT_ID, 42); :) I was clearly having one of those sorts of days. > + > + mpamf->aidr = FIELD_DP64(mpamf->aidr, MPAMF_AIDR, ARCH_MINOR_REV, 1); > + mpamf->aidr = FIELD_DP64(mpamf->aidr, MPAMF_AIDR, ARCH_MAJOR_REV, 1); > + > + if (s->num_int_partid < s->num_partid) { > + mpamf->partid_nrw_idr = FIELD_DP32(mpamf->partid_nrw_idr, > + MPAMF_PARTID_NRW_IDR, > + INTPARTID_MAX, > + s->num_int_partid - 1); > + } > + } > +}
