qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH] arm: implement cache/shareability attribute


From: Andrew Baumann
Subject: Re: [Qemu-devel] [RFC PATCH] arm: implement cache/shareability attribute bits for PAR registers
Date: Thu, 19 Oct 2017 17:04:46 +0000

> From: Peter Maydell [mailto:address@hidden
> Sent: Thursday, 19 October 2017 09:51
> 
> On 18 October 2017 at 01:16, Andrew Baumann
> <address@hidden> wrote:
> Hi; thanks for this patch. Looks like you forgot to add your
> signed-off-by line.

Thanks for the review! 

I didn’t sign the patch, because it wasn't ready to go in, and my (limited) 
understanding of the protocol is that that's one of the things you're 
certifying in a signature.

> > 1. Is adding these entirely ARM-specific fields to the generic
> >    MemTxAttrs bitfield the right approach, or would you prefer a new
> >    return field from get_phys_addr() and friends?
> 
> MemTxAttrs is for attributes of a memory transaction as they
> go out on a bus (conceptually speaking). While some of the
> cacheability info does go out on a hardware AXI bus, it
> obviously isn't formatted as an index into the MAIR registers,
> since those are entirely CPU internal. I'm not sure the
> shareability stuff goes outside the CPU at all, but I haven't
> looked closely at the AXI spec.
> 
> While you could wade into the complexities of figuring out what
> attributes we want to expose to devices via the MemTxAttrs fields,
> I would suggest keeping things internal to the CPU implementation,
> by adding another parameter to get_phys_addr() &c.

Ok, will do.

> > 2. Is it acceptable to implement this piecemeal for the special LPAE
> >    case we care about, and defer the messy special cases for later? (I
> >    hope so!)
> 
> Depends whether I look at the other parts and decide on your
> behalf that they're easy ;-)

I think I will take a stab at the stage 2 translations, but grovelling through 
the details of the various short PTE formats and their remapping mechanisms 
wasn't my idea of a fun time :)

> > 3. Is my implementation of mair0 and mair1 (with the XXX comments)
> >    sane? I suspect it's not. I had a hard time mapping what the ARM
> >    ARM describes (e.g. in the pseudocode for AArch32.S1AttrDecode and
> >    AArch64.S1AttrDecode) to the union interpreting mair_el[0] and
> >    mair_el[1].
> 
> Where the AArch64.S1AttrDecode() pseudocode says MAIR[] this is
> MAIR[S1TranslationRegime()], which in QEMU terms is
> env->cp15.mair_el[regime_el(env, mmu_idx)].
> 
> For AArch32 things are a little trickier. NS MAIR0/MAIR1 are always
> in env->cp15.mair0_ns and env->cp15.mair1_ns (and if you happen to
> want the combined 64 bits MAIR1:MAIR0 then env->cp15.mair_el[1]
> will give you that). But where Secure MAIR0/MAIR1 live depends
> on whether EL3 is AArch32 or AArch64. If EL3 is AArch32 then the
> MAIR0/MAIR1 are really banked registers, and the secure copies
> are in env->cp15.mair0_s and env->cp15.mair1_s (with the MAIR1:MAIR0
> combo being in env->cp15.mair_el[3])). But if EL3 is AArch64 then
> these registers are architecturally not banked, and a secure EL1
> will be using env->cp15.mair0_ns and mair1_ns just like NS EL1.
> 
> Helpfully, regime_el() abstracts away most of this, so if all
> you want is a MAIR1:MAIR0 pair you can just say
>  env->cp15.mair_el[regime_el(env, mmu_idx)];
> the same as for AArch64.
> 
> We don't implement AArch32 EL2 yet, so the datastructure isn't
> quite right for it yet, but HMAIR1:HMAIR0 is in mair_el[2]
> (and the 32-bit part of the union should have uint32_t for
> hmair0 and hmair1 where it currently has _unused_mair_1).

Got it. Thanks.

[...]
> > @@ -2173,7 +2224,9 @@ static uint64_t do_ats_write(CPUARMState *env,
> uint64_t value,
> >              if (!attrs.secure) {
> >                  par64 |= (1 << 9); /* NS */
> >              }
> > -            /* We don't set the ATTR or SH fields in the PAR. */
> > +            par64 |= (uint64_t)get_memattrs(env, mmu_idx,
> > +                                            attrs.arm_attrindx) << 56; 
> > /*ATTR*/
> 
> I think this is the wrong place to try to do the lookup from
> AttrIndex[] pagetable bits to memory attributes via the MAIR*,
> because not every kind of page table does memory attributes
> that way. In particular for short descriptors with TEX remap
> disabled, the attributes are directly in the page descriptors,
> and this is also true for stage 2 translation tables.
> 
> So you want the page table walk functions to directly fill in
> the information about cacheability and shareability, including
> doing lookups in MAIR registers (or PRRR/NMRR registers, which
> QEMU stores in the same CPUState fields, but they have a different
> format.)

I agree. FWIW, the original reason for doing it here was that I didn't want to 
slow down the normal page-table walk with additional logic to compute the 
attributes, which isn't necessary for a TLB fill. If we're adding extra out 
parameters to the walker, then I can make those optional and skip computing 
them when the parameter is NULL.

Andrew

reply via email to

[Prev in Thread] Current Thread [Next in Thread]