[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simp
From: |
Andrew Jones |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP |
Date: |
Tue, 10 Mar 2015 17:32:33 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, Mar 10, 2015 at 03:22:55PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 15:05, Andrew Jones <address@hidden> wrote:
> > Teach get_rw_prot about the simple AP format AP[2:1]. An additional
> > switch was added, as opposed to converting ap := AP[2:1] to AP[2:0]
> > with a simple shift - and then modifying cases 0,2,4,6, because the
> > resulting code is easier to read with the switch.
> >
> > Signed-off-by: Andrew Jones <address@hidden>
> > ---
> > target-arm/helper.c | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 610f305c4d661..b63ec7b7979f9 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env,
> > ARMMMUIdx mmu_idx)
> > static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> > int ap, int domain_prot)
> > {
> > + bool simple_ap = regime_using_lpae_format(env, mmu_idx)
> > + || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
>
> We should check arm_feature(env, ARM_FEATURE_V6K) && (SCTLR.AFE is set);
> that bit isn't defined til v6K.
Indeed. Will send v2 for that.
>
> > + bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
>
> Given that the lpae code path is totally separate (and not even
> calling this function yet), can't you just have it pass in a
> zero domain_prot ? Or have the callers do the domain protection
> check themselves...
domain_prot=0 is a valid access permission (no access), so I didn't
want to overload the meaning with 'not used'. I can move the check to
the callers that need it though. It would actually be nice to remove
the need for a 0 place holder from the other callers.
>
> > bool is_user = regime_is_user(env, mmu_idx);
> >
> > - if (domain_prot == 3) {
> > + if (domain_prot_valid && domain_prot == 3) {
> > return PAGE_READ | PAGE_WRITE;
> > }
> >
> > + /* ap is AP[2:1] */
> > + if (simple_ap) {
> > + switch (ap) {
> > + case 0:
> > + return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> > + case 1:
> > + return PAGE_READ | PAGE_WRITE;
> > + case 2:
> > + return is_user ? 0 : PAGE_READ;
> > + case 3:
> > + return PAGE_READ;
> > + default:
> > + g_assert_not_reached();
> > + }
> > + }
>
> I'm confused. Even if we're using the simple-permissions
> model, the ap parameter is still AP[2:0]. Shouldn't this
> switch be for cases 0, 2, 4, 6 ?
Depends on how we choose to implement the callers. Currently
I only require the caller to send in 2 bits for the simple
model. If we want to require them to send in 3, then we'll
need to shift a zero in for the lpae caller, rather than
shift a zero out for the v6 caller.
>
> > +
> > + /* ap is AP[2:0] */
> > switch (ap) {
> > case 0:
> > if (arm_feature(env, ARM_FEATURE_V7)) {
> > --
> > 1.9.3
> >
>
> -- PMM