| Message ID | 20250411145655.140667-4-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | First chunk for Arm R82 and MPU support | expand |
On 11/04/2025 16:56, Luca Fancellu wrote: > Introduce few utility function to manipulate and handle the > pr_t type. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/arm/include/asm/mpu.h | 40 ++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h > index 59ff22c804c1..6971507457fb 100644 > --- a/xen/arch/arm/include/asm/mpu.h > +++ b/xen/arch/arm/include/asm/mpu.h > @@ -20,6 +20,46 @@ > #define NUM_MPU_REGIONS_MASK (NUM_MPU_REGIONS - 1) > #define MAX_MPU_REGIONS NUM_MPU_REGIONS_MASK > > +#ifndef __ASSEMBLY__ > + > +/* Set base address of MPU protection region(pr_t). */ What's the use of (pr_t) in this comment? pr_t is a data type. If at all, it would want to be ...region @pr but I think you can skip it. > +static inline void pr_set_base(pr_t *pr, paddr_t base) > +{ > + pr->prbar.reg.base = (base >> MPU_REGION_SHIFT); Looking at pr_t definition, base/limit is 46 bits wide. However the spec says that last 4bits are reserved (i.e. you should not write to them) unless FEAT_LPA is implemented. What's our plan here? > +} > + > +/* Set limit address of MPU protection region(pr_t). */ > +static inline void pr_set_limit(pr_t *pr, paddr_t limit) > +{ > + pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT); Why -1? AFAIR these registers take inclusive addresses, so is it because you want caller to pass limit as exclusive and you convert it to inclusive? I think it's quite error prone. ~Michal
Hi Michal, > On 16 Apr 2025, at 11:50, Orzel, Michal <Michal.Orzel@amd.com> wrote: > > > > On 11/04/2025 16:56, Luca Fancellu wrote: >> Introduce few utility function to manipulate and handle the >> pr_t type. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> xen/arch/arm/include/asm/mpu.h | 40 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> >> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h >> index 59ff22c804c1..6971507457fb 100644 >> --- a/xen/arch/arm/include/asm/mpu.h >> +++ b/xen/arch/arm/include/asm/mpu.h >> @@ -20,6 +20,46 @@ >> #define NUM_MPU_REGIONS_MASK (NUM_MPU_REGIONS - 1) >> #define MAX_MPU_REGIONS NUM_MPU_REGIONS_MASK >> >> +#ifndef __ASSEMBLY__ >> + >> +/* Set base address of MPU protection region(pr_t). */ > What's the use of (pr_t) in this comment? pr_t is a data type. If at all, it > would want to be ...region @pr but I think you can skip it. ok > >> +static inline void pr_set_base(pr_t *pr, paddr_t base) >> +{ >> + pr->prbar.reg.base = (base >> MPU_REGION_SHIFT); > Looking at pr_t definition, base/limit is 46 bits wide. However the spec says > that last 4bits are reserved (i.e. you should not write to them) unless FEAT_LPA > is implemented. What's our plan here? So we’re currently supporting max 1TB, so probably this one needs to be on the case when FEAT_LPA is considered not implemented, so I’ll change and if we will later support more than 42 bit we could do something? > >> +} >> + >> +/* Set limit address of MPU protection region(pr_t). */ >> +static inline void pr_set_limit(pr_t *pr, paddr_t limit) >> +{ >> + pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT); > Why -1? AFAIR these registers take inclusive addresses, so is it because you > want caller to pass limit as exclusive and you convert it to inclusive? I think > it's quite error prone. Yes it’s meant to be used with exclusive range, shall we document it or use Inclusive range instead? Cheers, Luca
On 16/04/2025 14:31, Luca Fancellu wrote: > Hi Michal, > >> On 16 Apr 2025, at 11:50, Orzel, Michal <Michal.Orzel@amd.com> wrote: >> >> >> >> On 11/04/2025 16:56, Luca Fancellu wrote: >>> Introduce few utility function to manipulate and handle the >>> pr_t type. >>> >>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >>> --- >>> xen/arch/arm/include/asm/mpu.h | 40 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 40 insertions(+) >>> >>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h >>> index 59ff22c804c1..6971507457fb 100644 >>> --- a/xen/arch/arm/include/asm/mpu.h >>> +++ b/xen/arch/arm/include/asm/mpu.h >>> @@ -20,6 +20,46 @@ >>> #define NUM_MPU_REGIONS_MASK (NUM_MPU_REGIONS - 1) >>> #define MAX_MPU_REGIONS NUM_MPU_REGIONS_MASK >>> >>> +#ifndef __ASSEMBLY__ >>> + >>> +/* Set base address of MPU protection region(pr_t). */ >> What's the use of (pr_t) in this comment? pr_t is a data type. If at all, it >> would want to be ...region @pr but I think you can skip it. > > ok > >> >>> +static inline void pr_set_base(pr_t *pr, paddr_t base) >>> +{ >>> + pr->prbar.reg.base = (base >> MPU_REGION_SHIFT); >> Looking at pr_t definition, base/limit is 46 bits wide. However the spec says >> that last 4bits are reserved (i.e. you should not write to them) unless FEAT_LPA >> is implemented. What's our plan here? > > So we’re currently supporting max 1TB, so probably this one needs to be on the > case when FEAT_LPA is considered not implemented, so I’ll change and if we will > later support more than 42 bit we could do something? I think yes. > >> >>> +} >>> + >>> +/* Set limit address of MPU protection region(pr_t). */ >>> +static inline void pr_set_limit(pr_t *pr, paddr_t limit) >>> +{ >>> + pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT); >> Why -1? AFAIR these registers take inclusive addresses, so is it because you >> want caller to pass limit as exclusive and you convert it to inclusive? I think >> it's quite error prone. > > Yes it’s meant to be used with exclusive range, shall we document it or use > Inclusive range instead? What's the expected behavior of callers? Are we going to set up protection region based on regular start+size pair (like with MMU) or start+end? If the latter for some reason (with size there are less issues), then end usually is inclusive and you would not need conversion. ~Michal
Hi Michal, >> >>> >>>> +} >>>> + >>>> +/* Set limit address of MPU protection region(pr_t). */ >>>> +static inline void pr_set_limit(pr_t *pr, paddr_t limit) >>>> +{ >>>> + pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT); >>> Why -1? AFAIR these registers take inclusive addresses, so is it because you >>> want caller to pass limit as exclusive and you convert it to inclusive? I think >>> it's quite error prone. >> >> Yes it’s meant to be used with exclusive range, shall we document it or use >> Inclusive range instead? > What's the expected behavior of callers? Are we going to set up protection > region based on regular start+size pair (like with MMU) or start+end? If the > latter for some reason (with size there are less issues), then end usually is > inclusive and you would not need conversion. I think we have a mix because for example destroy_xen_mappings and modify_xen_mappings are start and end, map_pages_to_xen instead is kind of start+size? I moved the -1 inside pr_set_limit because it was open-coded in all the places, for example when referencing linker symbols or output of mfn_to_maddr(mfn_add(smfn, nr_mfn)), for this reason I thought: let’s call this one with exclusive ranges and modify internally for inclusive.
On 16/04/2025 14:57, Luca Fancellu wrote: > Hi Michal, > >>> >>>> >>>>> +} >>>>> + >>>>> +/* Set limit address of MPU protection region(pr_t). */ >>>>> +static inline void pr_set_limit(pr_t *pr, paddr_t limit) >>>>> +{ >>>>> + pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT); >>>> Why -1? AFAIR these registers take inclusive addresses, so is it because you >>>> want caller to pass limit as exclusive and you convert it to inclusive? I think >>>> it's quite error prone. >>> >>> Yes it’s meant to be used with exclusive range, shall we document it or use >>> Inclusive range instead? >> What's the expected behavior of callers? Are we going to set up protection >> region based on regular start+size pair (like with MMU) or start+end? If the >> latter for some reason (with size there are less issues), then end usually is >> inclusive and you would not need conversion. > > I think we have a mix because for example destroy_xen_mappings and modify_xen_mappings > are start and end, map_pages_to_xen instead is kind of start+size? > > I moved the -1 inside pr_set_limit because it was open-coded in all the places, for example when > referencing linker symbols or output of mfn_to_maddr(mfn_add(smfn, nr_mfn)), for this reason I > thought: let’s call this one with exclusive ranges and modify internally for inclusive. Hmm, yes. Indeed we have a mix of places where end is inclusive or exclusive. I think we can stick with exclusive address being passed to this helper unless others have a different opinion. I know we tried to convert some places to start+size but I don't remember the future plans. ~Michal
> On 16 Apr 2025, at 14:10, Orzel, Michal <Michal.Orzel@amd.com> wrote: > > > > On 16/04/2025 14:57, Luca Fancellu wrote: >> Hi Michal, >> >>>> >>>>> >>>>>> +} >>>>>> + >>>>>> +/* Set limit address of MPU protection region(pr_t). */ >>>>>> +static inline void pr_set_limit(pr_t *pr, paddr_t limit) >>>>>> +{ >>>>>> + pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT); >>>>> Why -1? AFAIR these registers take inclusive addresses, so is it because you >>>>> want caller to pass limit as exclusive and you convert it to inclusive? I think >>>>> it's quite error prone. >>>> >>>> Yes it’s meant to be used with exclusive range, shall we document it or use >>>> Inclusive range instead? >>> What's the expected behavior of callers? Are we going to set up protection >>> region based on regular start+size pair (like with MMU) or start+end? If the >>> latter for some reason (with size there are less issues), then end usually is >>> inclusive and you would not need conversion. >> >> I think we have a mix because for example destroy_xen_mappings and modify_xen_mappings >> are start and end, map_pages_to_xen instead is kind of start+size? >> >> I moved the -1 inside pr_set_limit because it was open-coded in all the places, for example when >> referencing linker symbols or output of mfn_to_maddr(mfn_add(smfn, nr_mfn)), for this reason I >> thought: let’s call this one with exclusive ranges and modify internally for inclusive. > Hmm, yes. Indeed we have a mix of places where end is inclusive or exclusive. I > think we can stick with exclusive address being passed to this helper unless > others have a different opinion. I know we tried to convert some places to > start+size but I don't remember the future plans. Ok, I'll document on top of the helper that @limit needs to be exclusive just to be sure it will be used in this way.
diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h index 59ff22c804c1..6971507457fb 100644 --- a/xen/arch/arm/include/asm/mpu.h +++ b/xen/arch/arm/include/asm/mpu.h @@ -20,6 +20,46 @@ #define NUM_MPU_REGIONS_MASK (NUM_MPU_REGIONS - 1) #define MAX_MPU_REGIONS NUM_MPU_REGIONS_MASK +#ifndef __ASSEMBLY__ + +/* Set base address of MPU protection region(pr_t). */ +static inline void pr_set_base(pr_t *pr, paddr_t base) +{ + pr->prbar.reg.base = (base >> MPU_REGION_SHIFT); +} + +/* Set limit address of MPU protection region(pr_t). */ +static inline void pr_set_limit(pr_t *pr, paddr_t limit) +{ + pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT); +} + +/* + * Access to get base address of MPU protection region(pr_t). + * The base address shall be zero extended. + */ +static inline paddr_t pr_get_base(pr_t *pr) +{ + return (paddr_t)(pr->prbar.reg.base << MPU_REGION_SHIFT); +} + +/* + * Access to get limit address of MPU protection region(pr_t). + * The limit address shall be concatenated with 0x3f. + */ +static inline paddr_t pr_get_limit(pr_t *pr) +{ + return (paddr_t)((pr->prlar.reg.limit << MPU_REGION_SHIFT) + | ~MPU_REGION_MASK); +} + +static inline bool region_is_valid(pr_t *pr) +{ + return pr->prlar.reg.en; +} + +#endif /* __ASSEMBLY__ */ + #endif /* __ARM_MPU_H__ */ /*
Introduce few utility function to manipulate and handle the pr_t type. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/include/asm/mpu.h | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)