Re: [Qemu-devel] Supporting multiple CPU AddressSpaces and memory transaction attributes

[Edgar E. Iglesias]

On Thu, Sep 04, 2014 at 06:47:58PM +0100, Peter Maydell wrote:
> One of the parts of the ARM TrustZone/Security Extensions
> which the patchsets we've seen so far haven't attempted to
> tackle is the problem of Secure vs NonSecure memory accesses.
> Architecturally, every memory transaction should have
> an S/NS bit accompanying the physical address, effectively
> making an extra address bit (you could in theory have
> completely different physical memory maps for S and NS,
> though usually they're similar). We can't fake this up
> by having the device call back into the CPU to check its
> current status, because the CPU can make NS accesses
> when it is in the Secure world (controlled by page table bits).
> 
> We have some other cases where devices would really
> like to have some kind of "memory transaction attributes"
> information:
>  * watchpoints on ARM need to know whether the access
>    was userspace or system (ie which mmu_idx it was).
>    [The LDRT/STRT instructions which let the kernel
>    do userspace-privilege accesses mean that the current
>    state of the CPU isn't sufficient to determine this.]
>    They'll also want to know the S/NS info.
>  * The GIC wants to provide a different set of registers
>    to each CPU (currently we do this with a hacky use of
>    current_cpu), as do some other devices.
> 
> Paolo also mentioned that x86 SMM has some situations
> where devices need to be only visible in some cases.
> 
> (Another oddball usecase is the Cortex-M split I and D
> bus for low memory, where instruction and data accesses
> go out via different buses and might map to different things,
> but for now I think I'm happy to ignore this as more a
> theoretical question than a practical one...)
> 
> Here's one idea which deals with some of these...

Hi Peter,

Thanks for writing this down.

I was looking at some of this stuff some weeks ago and exploring something
similar to what you suggest but not exactly the same. Never fully implemented
it though. I'll comment on your suggestion and explain what I did and what
I ended up using.


> We introduce the concept of memory transaction attributes,
> which are a guest-CPU specific bunch of bits (say, a
> uint32_t). We also allow the CPU to have more than one
> AddressSpace, and have the guest CPU code provide
> a function returning the AddressSpace to use for a
> given set of transaction attributes. For ARM we'd
> put all of (S/NS, mmu_idx, CPU number) into the attributes,
> use an AddressSpace each for S and NS, and use the
> S/NS bit in the attributes to select the AddressSpace
> The default is that we have one AddressSpace and always
> use that, ie the transaction attributes are ignored.
> (Maybe the function should return an integer index
> into a cpu->as[] array?)

The new read_with_attrs (or another name) sounds good.
It allows us to incrementally change the codebase. It would be nice if the
arguments to that function be passed in a structure so we can
make future changes easier.

Maybe it could even end up going this far:
void access(BusPayload *pay);

With *pay having fields for attributes, data, slave error signaling etc.

Im a little concerned about the guest CPU defining the attribute
format. Maybe we could start by making the attributes generic
(look the same for everyone). BUS specialization and convertion
as transactions make their way through the bus could be future work.

> 
> tlb_set_page() takes an extra argument specifying the
> transaction attributes. For RAM accesses we can just
> immediately use this to get the AddressSpace to pass
> to address_space_translate_for_iotlb(). For IO accesses
> we need to stash the attributes in the iotlb[], which
> means extending that from an array of hwaddrs to an
> array of struct {hwaddr, attributes}, which is easy enough.
> Then the io_read/write glue functions in softmmu_template.h
> can fish the attributes out of the iotlb and use them to
> pick the AddressSpace to pass to iotlb_to_region().
> More importantly, we can arrange to pass them through
> to the device read/write callbacks (either directly,
> or indirectly by saving them in the CPU struct like we
> do for mem_io_vaddr; since changing the prototypes on
> every device read and write callback would be insane
> we probably want to have fields in MemoryRegionOps for
> read_with_attrs and write_with_attrs function pointers).

I think this will mostly work but could become a bit hard
to deal with when IOMMUs come into the picture that may want
to modify the attributes and AS.

Maybe we could consider having a pointer to a bundle of
AS and attributes stored in the iotlb? example:

memory.h:
typedef struct BusAttrSomething
{
    AddressSpace *as;
    MemoryTransactionAttr *attr;
} BusAttrSomthing;

So that the stuff stored in the IOTLB is not specific
to the CPU in question but can be created by any
IOMMU along the bus path. See below for more info.

> 
> We would need APIs so bus masters other than CPUs can
> specify the transaction attributes (and it's probably
> a good idea for the guest CPU to arrange its attribute
> bit definitions so that "0" means "no attribute info",
> to account for legacy bus masters).
> 
> The watchpoint read/write callbacks would just stuff
> the transaction attribute word for the access that
> triggered the watchpoint into the CPUWatchpoint struct
> so that target-specific code can look at it later.

I'll explain what I played around with.

I added a concept of TLB attribute indexes, similar to the mmu_idx. I actually
first considered reusing the mmu_idx and even spliting up the mmu_idx into
fields containing the tlb_attr_idx but didn't in the end.

To memory.h:
/* TODO: generic for now but could maybe be specialized for different
 * buses in the future.  */
typedef struct MemoryTransactionAttr
{
    unsigned int secure : 1;
    ...
} MemoryTransactionAttr;


The CPU then for example defines:
typedef struct CPUBusAttr
{
    AddressSpace *as;
    MemoryTransactionAttr *attr;
} CPUBusAttr;

IOTLB:
unsigned int attr_idx[NB_MMU_MODES][CPU_TLB_SIZE];
CPUBusAttr iotlb_attr[NB_TLB_ATTR];

cpu.h:
#define NB_TLB_ATTR 2
#define TLB_ATTR_NS 0
#define TLB_ATTR_SEC 1

At CPU reset/init we do something like the following:
    env->iotlb_attr[TLB_ATTR_NS].as = cpu->as_ns;
    env->iotlb_attr[TLB_ATTR_SEC].as = cpu->as_secure;
    env->iotlb_attr[TLB_ATTR_SEC].attr = (MemoryTransactionAttr[]) {{ .secure = 
true }};

The MMU code the CPUs selects the initial TLB attr index based on the
current CPU state. Address translators are then allowed to downgrade
the index as they see fit. tlb_set_page takes the attr_index as input.
The base attr_index could also be passed from the translators but would
need further changes (or the attr_idx encoded in mmu_idx tricks).

In practice, this looks something like:

static int get_phys_addr_lpae_common(CPUARMState *env,
                                     ...
                                     unsigned int *attr_idx)
{
    ...
    if (stage == 1) {
        if (attrs & (1 << 3)) {
            /* NS downgrade */
            *attr_idx = TLB_ATTR_NS;
        }


int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
                             int access_type, int mmu_idx)
{
    ...
    bool secure = arm_is_secure(env);
    unsigned int attr_idx = secure ? TLB_ATTR_SEC : TLB_ATTR_NS;

    ret = get_phys_addr(env, address, access_type, cur_el, &phys_addr, &prot,
                        &page_size, 2, &attr_idx);
    ...
    tlb_set_page_attr(cs, address, phys_addr, prot, mmu_idx, page_size,
                          attr_idx);

Were things got messier is when IOMMUs come into the picture.
I've got some patches that add support for CPUs to sit behind IOMMUs.

Somewhere at this point I abandonned the .access with attributes approach
and am now using AddresSpaces only. I still have the CPUBusAttr in the iotlb
so that MMU translation can select and modify the AS (i.e downgrade S to NS)
but I don't use the attributes parts of it to propagate the info to devs.

At the device level, devs export multiple mr ports that internally map to
common access functions with S/NS argsw. IOMMUs and other masters do not know
about or set specific attributes, they operate only on AddressSpaces.
My reasons for going this path were maybe not so technical, it was more
pragmatism to get something working with little changes to current QEMU.

Having mem attributes support upstream would be nice :-)

Cheers,
Edgar