Re: [Qemu-devel] [RFC 0/5] Memory transaction attributes API

[Edgar E. Iglesias]

On Mon, Mar 16, 2015 at 05:20:17PM +0000, Peter Maydell wrote:
> This is an RFC patchset aimed at getting comment on
> some memory API changes to support "transaction attributes",
> ie sideband information that goes along with a memory read
> or write access to define things like ARM secure/nonsecure,
> CPU/transaction master ID, privileged/nonprivileged.

Hi Peter,

Generally I think the direction of this looks very good, thanks!

A few comments:

1. Would it make sense to instead of having the MemTxAttrs be
a uint64_t instead have them be a struct with bitfields?
We could still pass the struct by value I think and as long
as it doesn't grow large the compiler will emit similar or
the same code IIUC.

2. The series introduces the read and write _with_attrs. Another
approach could be to add a (for example) single .access callback.
The callback could take a structure pointer as input, e.g:

struct MemTx {
    bool rw;
    hwaddr addr;
    int size;
    uint64_t data;
    MemTxAttrs attrs;
}

MemTxResult access(MemTx *tx)

The benefit I see is that this will make it easier for us in the
future to add new fields if needed.

Best regards,
Edgar


> 
> (See also previous discussion:
> https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg01026.html )
> 
> Patch 1 is the API changes themselves:
> 1) new read_with_attrs and write_with_attrs fields in the
>    MemoryRegionOps struct; the old read/write still exist
>    for backwards compatibility, but devices that care about
>    attributes can register with these function pointers instead
> 2) the functions return a success/failure status, so a device
>    can actually fail bad transactions rather than merely
>    returning bogus data. (This isn't wired up in this patchset
>    but I include it to avoid revving the memory API twice.)
> 
> Patches 2, 3 and 4 then plumb the memory attribute parameters
> through the various functions, working upwards to being able
> to put them in the iotlb. Patch 5 implements the target-arm
> changes to provide a secure/nonsecure tx attribute based on
> the page table walk, as a demonstration.
> 
> There are obviously more APIs within QEMU for memory access
> functions which need to change to either always take a tx
> attribute, or to have extra with-tx-attribute versions of the
> functions. For the moment things are stubbed out with passing
> in "no attributes specified" values.
> 
> I've modelled the transaction attributes as a (typedefed)
> uint64_t, whose bits will be defined as we find requirements
> for them (the meaning will not be per-architecture). When
> we originally discussed this on-list, Edgar suggested making
> the attributes be a (pointer to a) struct; however I found
> the ownership/copying semantics on this too awkward, because
> the access path needs to take attributes set up in the TLB
> and then modify them according to details of the access
> actually being made before passing them to the device, so
> took the simpler implementation route.
> 
> I intend to continue working on this (filling in the gaps,
> etc), but wanted to send this series out early for comment
> on the memory API changes in particular.
> 
> thanks
> -- PMM
> 
> Peter Maydell (5):
>   memory: Define API for MemoryRegionOps to take attrs and return status
>   memory: Add MemTxAttrs argument to io_mem_read and io_mem_write
>   Make CPU iotlb a structure rather than a plain hwaddr
>   Add MemTxAttrs to the IOTLB
>   target-arm: Honour NS bits in page tables
> 
>  cputlb.c                 |  22 +++++++---
>  exec.c                   |  29 +++++++-------
>  hw/s390x/s390-pci-inst.c |   7 ++--
>  hw/vfio/pci.c            |   4 +-
>  include/exec/cpu-defs.h  |  15 ++++++-
>  include/exec/exec-all.h  |   8 +++-
>  include/exec/memattrs.h  |  37 +++++++++++++++++
>  include/exec/memory.h    |  22 ++++++++++
>  memory.c                 | 102 
> +++++++++++++++++++++++++++++++++++++----------
>  softmmu_template.h       |  36 +++++++++--------
>  target-arm/helper.c      |  83 ++++++++++++++++++++++++++++++++------
>  11 files changed, 288 insertions(+), 77 deletions(-)
>  create mode 100644 include/exec/memattrs.h
> 
> -- 
> 1.9.1
>