On Sat, 15 Oct 2022 at 14:07, Tobias Roehmel
<tobias.roeh...@rwth-aachen.de> wrote:
>
> Thank you very much for the review!
>
> I have a few questions:
>
> On 27.09.22 15:50, Peter Maydell wrote:
> > On Sat, 20 Aug 2022 at 15:19, <tobias.roeh...@rwth-aachen.de> wrote:
> >> From: Tobias Röhmel <tobias.roeh...@rwth-aachen.de>
> >>
> >> Signed-off-by: Tobias Röhmel <tobias.roeh...@rwth-aachen.de>
> >> ---
> >> target/arm/cpu.h | 10 +++
> >> target/arm/helper.c | 171 ++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 181 insertions(+)
> >>
> >> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >> index 86e06116a9..632d0d13c6 100644
> >> --- a/target/arm/cpu.h
> >> +++ b/target/arm/cpu.h
> >> @@ -726,8 +726,18 @@ typedef struct CPUArchState {
> >> */
> >> uint32_t *rbar[M_REG_NUM_BANKS];
> >> uint32_t *rlar[M_REG_NUM_BANKS];
> >> + uint32_t prbarn[255];
> >> + uint32_t prlarn[255];
> >> + uint32_t hprbarn[255];
> >> + uint32_t hprlarn[255];
> > Don't use magic constants, please. In fact, don't use
> > fixed size arrays at all here. The v8R PRBAR/PRLAR
> > register arrays are exactly the same format as the v8M
> > pmsav8.rbar[] and pmsav8.rlar[], so please use the same
> > state fields that does. (You'll need to add equivalent
> > new arrays to handle the HPRBAR/HPRLAR.)
>
> Is there a way to find out whether I'm in secure mode while accessing a
> co-processor register?
> The banks for rbar etc. are used to model the secure non-secure banks.
> The access mechanism
> here is via a device which allows the use of the mmu index to find out
> if we are in secure mode.
> I'm struggling to find a good solution in the co-processor register case.
Well, for 32-bit v8 R-profile you know you're always NonSecure.
But more generally, the right way depends on where you are.
Code in ptw.c now gets passed a bool to tell it whether it
needs to do the address translation for a secure or nonsecure
translation regime (this is a change from a recent refactoring).
Otherwise there is the arm_is_secure() function.
For the coprocessor register access functions, just do what
pmsav7 does' and hardcode that the array element you want is
the M_REG_NS one.
> >
> >> uint32_t mair0[M_REG_NUM_BANKS];
> >> uint32_t mair1[M_REG_NUM_BANKS];
> >> + uint32_t prbar;
> >> + uint32_t prlar;
> > You should not need separate prbar/prlar fields, as those
> > registers only indirectly access the state for thecurrently
> > selected region. Similarly hprbar, hprlar below.
> >
> >> + uint32_t prselr;
> > PRSELR is just a renamed PMSAv7 RGNR, for which we already
> > have a state field, pmsav7.rnr[M_REG_NS] (and indeed a cpreg
> > struct).
> I changed it to use the rnr field, but I think I can't reuse the cpreg
> since it has a different encoding.
Oops, yes, I misread the opc2 value.
> >
> >> + uint32_t hprbar;
> >> + uint32_t hprlar;
> >> + uint32_t hprselr;
> >> } pmsav8;
> > Some of this new state must be handled for migration.
> > Where state is directly accessed via a coprocessor
> > register that will be migrated for you. However, where
> > there is state that is not directly accessible, i.e. for
> > the rbar/rlar arrays, you need to add code/vmstate structs
> > to target/arm/machine.c to migrate them. vmstate_pmsav8
> > already does this for rbar and rlar, but you'll need to
> > add something similar for the hyp versions. (Watch out that
> > you maintain migration compat for the existing CPUs -- you
> > can't just add new fields to existing VMStateDescription
> > structs. Ask if you need advice.)
> I need some help here. Do I need new subsections?
That's generally the easiest way, yes. You can make it
a subsection of vmstate_pmsav8. the way subsections work is
that they appear in a .subsections field of some other
vmstate. They need a .needed function which returns true when
the new fields in the subsection must be migrated, and false
otherwise (such that any already existing CPU returns false
for the .needed function). That way the migration code will
migrate the new CPU struct fields for the new CPU, but won't
expect or send them for an old one.
As an example, look at vmstate_m_mve, which migrates a couple
of fields which are used only by M-profile CPUs with the MVE
feature. The vmstate_m_mve struct is listed in the .subsections
list of the vmstate_m struct (this is the VMStateDescription that
handles general M-profile CPU stuff), and the mve_needed() function
returns true only for CPUs with MVE. Note that the .name of
the subsection has to match up with the .name of its parent
section, so if the parent section's name is "parent/name"
then the subsection has to be "parent/name/whatever".
thanks
-- PMM
