Re: [Qemu-devel] [PATCH 00/14] Add memory attributes and use them in ARM

[Edgar E. Iglesias]

On Tue, Apr 07, 2015 at 09:09:46PM +0100, Peter Maydell wrote:
> Following from my previous RFC about transaction memory attributes,
> here's some code I think is good enough to drop the 'RFC' tag :-)
> (read: I would like to land this when master reopens for 2.4.)
> 
> I've included both the changes to the core memory system code
> and the target-arm changes as a usage example, but the ARM stuff
> is all at the end of the series, so if we want to split it and
> take it via separate subtrees that's fine.

Hi Peter,

More of a general comment. This is maybe follow-up patches but
at somepoint we will need to add attributes to the IOMMU
translate functions.

Thanks,
Edgar


> 
> I think I have followed the outcome of our discussions on the
> RFC; please let me know if I got confused or missed something.
> What we have here is:
>  * MemoryRegions can provide read_with_attrs and write_with_attrs
>    so they can get memory attributes and return a success/error
>    indication
>  * the attributes and error indication are plumbed through the
>    core memory system code
>  * new functions address_space_ld*/st* are provided which are
>    like the old ld/st*_phys but have extra args for MemTxAttrs
>    and MemTxResult*
>  * callers have been auto-converted from the old ld/st*_phys
>    unless they were using the CPUState::as address space
>    [those will be moved to some cpu-specific API later]
>  * TCG frontends can use tlb_set_page_with_attrs() to provide
>    attributes when they add an entry to the TLB
>  * two attributes: MEMTXATTRS_SECURE [ARM TrustZone secure access]
>    and MEMTXATTRS_USER [access is unprivileged], both implemented
>    for the ARM CPU frontend (these both correspond to AMBA/AXI
>    bus sideband signals, more or less)
> 
> 
> I believe this code contains enough changes that all the memory
> transactions issued by the ARM CPU will correctly be marked as
> S or NS. Obviously nothing currently pays attention to this, but
> the patches to make the GIC model support TrustZone can be easily
> wired up to this.
> 
> The diffstat's quite big but the biggest patch is a Coccinelle
> generated automated rename of the callers of ld/st*_phys to
> address_space_ld/st* where they don't use the CPUState::as.
> 
> thanks
> -- PMM
> 
> Peter Maydell (14):
>   memory: Define API for MemoryRegionOps to take attrs and return status
>   memory: Add MemTxAttrs, MemTxResult to io_mem_read and io_mem_write
>   Make CPU iotlb a structure rather than a plain hwaddr
>   Add MemTxAttrs to the IOTLB
>   exec.c: Convert subpage memory ops to _with_attrs
>   exec.c: Make address_space_rw take transaction attributes
>   exec.c: Add new address_space_ld*/st* functions
>   Switch non-CPU callers from ld/st*_phys to address_space_ld/st*
>   exec.c: Capture the memory attributes for a watchpoint hit
>   target-arm: Honour NS bits in page tables
>   target-arm: Use correct memory attributes for page table walks
>   target-arm: Add user-mode transaction attribute
>   target-arm: Use attribute info to handle user-only watchpoints
>   target-arm: Check watchpoints against CPU security state
> 
>  cputlb.c                          |  22 +-
>  dma-helpers.c                     |   3 +-
>  exec.c                            | 418 
> +++++++++++++++++++++++++++++---------
>  hw/alpha/dp264.c                  |   9 +-
>  hw/alpha/typhoon.c                |   3 +-
>  hw/arm/boot.c                     |   6 +-
>  hw/arm/highbank.c                 |  12 +-
>  hw/dma/pl080.c                    |  20 +-
>  hw/dma/sun4m_iommu.c              |   3 +-
>  hw/i386/intel_iommu.c             |   3 +-
>  hw/mips/mips_jazz.c               |   6 +-
>  hw/pci-host/apb.c                 |   3 +-
>  hw/pci-host/prep.c                |   6 +-
>  hw/pci/msi.c                      |   3 +-
>  hw/pci/msix.c                     |   3 +-
>  hw/s390x/css.c                    |  19 +-
>  hw/s390x/s390-pci-bus.c           |   9 +-
>  hw/s390x/s390-pci-inst.c          |   7 +-
>  hw/s390x/s390-virtio-bus.c        |  73 ++++---
>  hw/s390x/s390-virtio.c            |   4 +-
>  hw/s390x/virtio-ccw.c             |  87 +++++---
>  hw/sh4/r2d.c                      |   6 +-
>  hw/timer/hpet.c                   |   5 +-
>  hw/vfio/pci.c                     |   4 +-
>  include/exec/cpu-defs.h           |  15 +-
>  include/exec/exec-all.h           |   7 +-
>  include/exec/memattrs.h           |  40 ++++
>  include/exec/memory.h             | 128 +++++++++++-
>  include/qom/cpu.h                 |   2 +
>  include/sysemu/dma.h              |   3 +-
>  ioport.c                          |  16 +-
>  kvm-all.c                         |   3 +-
>  memory.c                          | 212 ++++++++++++-------
>  monitor.c                         |   3 +-
>  scripts/coverity-model.c          |   8 +-
>  softmmu_template.h                |  36 ++--
>  target-arm/helper.c               | 134 ++++++++++--
>  target-arm/op_helper.c            |  29 +--
>  target-i386/arch_memory_mapping.c |  15 +-
>  39 files changed, 1038 insertions(+), 347 deletions(-)
>  create mode 100644 include/exec/memattrs.h
> 
> -- 
> 1.9.1
> 
>