Re: [PATCH 4/4] target/ppc: Add migration support for BHRB

[Glenn Miles]

On 2023-09-14 20:20, Nicholas Piggin wrote:
> On Wed Sep 13, 2023 at 6:25 AM AEST, Glenn Miles wrote:
> > Adds migration support for Branch History Rolling
> > Buffer (BHRB) internal state.
> >
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> >  target/ppc/machine.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index b195fb4dc8..89146969c8 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -314,6 +314,7 @@ static int cpu_post_load(void *opaque, int 
> > version_id)
> >
> >      if (tcg_enabled()) {
> >          pmu_mmcr01a_updated(env);
> > +        hreg_bhrb_filter_update(env);
> >      }
> >
> >      return 0;
> > @@ -670,6 +671,27 @@ static const VMStateDescription vmstate_compat = 
> > {
> >      }
> >  };
> >
> > +#ifdef TARGET_PPC64
> > +static bool bhrb_needed(void *opaque)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +    return (cpu->env.flags & POWERPC_FLAG_BHRB) != 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_bhrb = {
> > +    .name = "cpu/bhrb",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = bhrb_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINTTL(env.bhrb_num_entries, PowerPCCPU),
>
> Maybe don't need bhrb_num_entries since target machine should have the
> same?

Removed.

> > +        VMSTATE_UINTTL(env.bhrb_offset, PowerPCCPU),
> > +        VMSTATE_UINT64_ARRAY(env.bhrb, PowerPCCPU, 
> > BHRB_MAX_NUM_ENTRIES),
>
> Is it possible to migrate only bhrb_num_entries items? Wants a VARRAY
> AFAIKS but there is no VARRAY_UINT64?
>
> Since all sizes are the same 32 now, would it be possible to turn it
> into a VARRAY sometime later if supposing a new CPU changed to a
> different size, and would the wire format for the VARRAY still be
> compatible with this fixed size array, or does a VARRAY look different
> I wonder?

I looked into this some more.  It turns out that the UINT32 in 
VARRAY_UINT32
is referring to the size of the field that holds the number of entries 
in
the array, not the size of the array elements.  So, it is possible to do 
this
with the VARRAY_UINT32 type.  I would need to change the type for 
bhrb_num_entries
to a uint32_t and also, since VARRAY_UINT32 requires the array field to 
be a
pointer to an array, I would need to store the address of the array in 
another
field.


> Thanks,
> Nick

Thank you for taking the time to review my code!

Glenn