From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46758) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFgkx-0002x2-W9 for qemu-devel@nongnu.org; Thu, 16 Jul 2015 06:51:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFgks-0007Cx-Rb for qemu-devel@nongnu.org; Thu, 16 Jul 2015 06:51:11 -0400 Received: from smtp.ispras.ru ([83.149.199.79]:55668) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFgks-0007Cs-Ek for qemu-devel@nongnu.org; Thu, 16 Jul 2015 06:51:06 -0400 Message-ID: <55A78C99.5040504@ispras.ru> Date: Thu, 16 Jul 2015 13:51:05 +0300 From: =?UTF-8?B?0JXRhNC40LzQvtCyINCS0LDRgdC40LvQuNC5?= MIME-Version: 1.0 References: <1437035704-11299-1-git-send-email-real@ispras.ru> <1437035704-11299-4-git-send-email-real@ispras.ru> <55A773C1.6060400@redhat.com> In-Reply-To: <55A773C1.6060400@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: "Michael S. Tsirkin" , Kirill Batuzov 16.07.2015 12:05, Paolo Bonzini =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > > 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 dir= ected 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 an= d 3 are >> well emulated but modes 1 and 2 are not. The cause is: aliases are use= d >> while more complicated logic is required. >> >> The idea is to use ROM device like memory regions for mode 1 and 2 emu= lation >> instead of aliases. Writes are directed to proper destination region b= y >> specified I/O callback. Read redirection depends on type of source reg= ion. >> 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 i= s an I/O >> region, reading is redirected to source region read callback by PAM re= gion 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 >> --- >> 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 =3D (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 =3D memory_region_get_ram_ptr(pam->mr_write_to) + addr >> + + pam->writ= e_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_of= fset, >> + size); >> + } >> +} >> + > > The idea is very good, but the implementation relies on copying a lot o= f > code from exec.c. If it is about pam_write then, for instance, I could suggest to outline corresponding part of address_space_rw to dedicated public function. The solution will require endianness conversion because of the part works with uint8_t buffer but not with uint64_t values. The rest of code looks up destination or source region or child region offset in memory sub-tree which root is PCI or RAM region provided on PAM creation. We cannon use common address_space_translate because it searches from address space root and will return current PAM region. To summarize, I suggest to move the code to exec.c. It is generic enough. > > 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". Did you mean MemoryRegion.iommu_ops ? The feature does not allow to change destination memory region. Also I has no find its using during write access from CPU. And there is: exec.c: address_space_translate_for_iotlb: assert(!section->mr->iommu_ops); > > Paolo >