Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

[Peter Maydell]

On Tue, 21 Mar 2023 at 13:23, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Mostafa,
>
> On 3/21/23 14:06, Mostafa Saleh wrote:
> > Hi Eric,
> >
> >>> +     * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
> >>> address
> >>> +     * size.
> >>> +     */
> >>> +    ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> >>> +    uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> >>> PARANGE);
> >> is this working in accelerated mode?
> > I didn't try with accel, I will give it a try, but from what I see, that
> > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > accel/kvm-all.c, so it seems this would work for accelerated mode.
>
> yeah I ma not familiar enough with that code but it is worth to be checked.

I'm a bit unsure about fishing around in the CPU ID registers for this.
That's not what you would do in real hardware, you'd just say "the
system is supposed to configure the CPU and the SMMU correctly".

Also, there is no guarantee that CPU 0 is related to this SMMU in
particular.

> >
> >>> +
> >>>      /**
> >>>       * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> >>>       *       multi-level stream table
> >>> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> >>>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> >>>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> >>>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> >>> -    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> >>> bits */
> >>> +    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> >> I am not sure you can change that easily. In case of migration this is
> >> going to change the behavior of the device, no?
> > I see IDR registers are not migrated. I guess we can add them in a
> > subsection and if they were not passed (old instances) we set OAS to
> > 44.
> > Maybe this should be another change outside of this series.
> Indeed tehy are not migrated so it can lead to inconsistent behavior in
> both source and dest. This deserves more analysis to me. In case you
> would decide to migrate IDR regs this would need to be done in that
> series I think. Migration must not be broken by this series.

Jumping in here without having read much of the context, but why
would we need to migrate the ID registers? They are constant, read-only,
so they will be the same value on both source and destination.

thanks
-- PMM