Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation

[Paolo Bonzini]

On 16/07/2015 10:35, Efimov Vasily wrote:
> This patch improves PAM emulation.
> 
> PAM defines 4 memory access redirection modes. In mode 1 reads are directed to
> RAM and writes are directed to PCI. In mode 2 it is contrary. In mode 0 all
> access is directed to PCI. In mode 3 it is directed to RAM. Modes 0 and 3 are
> well emulated but modes 1 and 2 are not. The cause is: aliases are used
> while more complicated logic is required.
> 
> The idea is to use ROM device like memory regions for mode 1 and 2 emulation
> instead of aliases. Writes are directed to proper destination region by
> specified I/O callback. Read redirection depends on type of source region.
> In most cases source region is RAM (or ROM), so ram_addr of PAM region is set 
> to
> ram_addr of source region with offset. Otherwise, when source region is an I/O
> region, reading is redirected to source region read callback by PAM region 
> one.
> 
> Read source and write destination regions are updated by the memory
> commit callback.
> 
> Note that we cannot use I/O region for PAM as it will violate "trying to 
> execute
> code outside RAM or ROM" assertion.
> 
> Signed-off-by: Efimov Vasily <address@hidden>
> ---
>  hw/pci-host/pam.c         | 238 
> +++++++++++++++++++++++++++++++++++++++++-----
>  include/hw/pci-host/pam.h |  10 +-
>  2 files changed, 223 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
> index 17d826c..9729b2b 100644
> --- a/hw/pci-host/pam.c
> +++ b/hw/pci-host/pam.c
> @@ -27,43 +27,233 @@
>   * THE SOFTWARE.
>   */
>  
> -#include "qom/object.h"
> -#include "sysemu/sysemu.h"
>  #include "hw/pci-host/pam.h"
> +#include "exec/address-spaces.h"
> +#include "exec/memory-internal.h"
> +#include "qemu/bswap.h"
> +
> +static void pam_write(void *opaque, hwaddr addr, uint64_t data,
> +                      unsigned size)
> +{
> +    PAMMemoryRegion *pam = (PAMMemoryRegion *) opaque;
> +    void *ptr;
> +
> +    /* Destination region can be both RAM and IO. */
> +    if (!memory_access_is_direct(pam->mr_write_to, true)) {
> +        memory_region_dispatch_write(pam->mr_write_to,
> +                                     addr + pam->write_offset, data, size,
> +                                     MEMTXATTRS_UNSPECIFIED);
> +    } else {
> +        ptr = memory_region_get_ram_ptr(pam->mr_write_to) + addr
> +                                                          + 
> pam->write_offset;
> +
> +        switch (size) {
> +        case 1:
> +            stb_p(ptr, data);
> +            break;
> +        case 2:
> +            stw_he_p(ptr, data);
> +            break;
> +        case 4:
> +            stl_he_p(ptr, data);
> +            break;
> +        case 8:
> +            stq_he_p(ptr, data);
> +            break;
> +        default:
> +            abort();
> +        }
> +
> +        invalidate_and_set_dirty(pam->mr_write_to, addr + pam->pam_offset,
> +                                 size);
> +    }
> +}
> +

The idea is very good, but the implementation relies on copying a lot of
code from exec.c.

Could you use an IOMMU memory region instead?  Then a single region can
be used to implement all four modes, and you don't hit the "trying to
execute code outside RAM or RAM".

Paolo