On Tue, May 12, 2015 at 03:47:00PM +0100, Peter Maydell wrote:
> Resurrecting a six month old thread (and starting with
> a big long quote for context):
>
> On 8 September 2014 at 12:53, Peter Maydell <peter.mayd...@linaro.org> wrote:
> > On 7 September 2014 02:47, Edgar E. Iglesias <edgar.igles...@gmail.com>
> > wrote:
> >> On Thu, Sep 04, 2014 at 06:47:58PM +0100, Peter Maydell wrote:
> >>> 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.
> >
> > Mmm, we probably want to allow for IOTLBs, so more
> > flexibility than a simple index into the CPU's list
> > of address spaces does seem warranted.
>
> Now that the tx-attributes patches are in master I'm
> looking at the "multiple AddressSpaces per CPU" part. In
> the intervening time, this code has been somewhat complicated
> by Paolo's RCU patches. In particular having actual
> AddressSpace pointers in the iotlb doesn't look like it
> will work given the way we now cache the memory_dispatch
> pointer.
>
> So we could deal with this by just falling back to
> "CPUs have N AddressSpaces and when the target code
> calls tlb_set_page_with_attrs it passes in the index
> of the AddressSpace as well as the paddr" (and we then
> can stash the index in the iotlb for later use, as
> well as handing it to address_space_translate_for_iotlb).
> Internally exec.c would also maintain an array of
> AddressSpaceDispatch pointers corresponding to the
> AddressSpaces (so effectively cs->as and cs->memory_dispatch
> become arrays, though likely with some syntactic sugar
> so we don't have to change all the uses of cs->as to
> cs->as[0] for CPUs which only have 1 AS).
Your suggestion sounds good to me.
I think it would be nice if address_space_translate_for_iotlb
was allowed to modify the attributes so that an IOMMU in front
of a CPU could for example down-grade a secure to a non-secure accesse
(once we add IOMMU support in front of CPUs). If I understood correctly
the memattrs would stay as a separate field in the iotlb, so this
would be easy to implement?
The naive implementation I have keeps pointers to AS and the memattrs
in the iotlb. address_space_translate_for_iotlb walks any IOMMU
translate() fns, if it hits a RAM it returns the host addr as usual.
If it hits MMIO behind IOMMUs it returns the first memsection, i.e
the one pointing to the iommu-ops and lets the IO access helpers
deal with the access via address_space_rw (for every IO access, slow).
Cheers,
Edgar
