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

["Ефимов Василий" <real@ispras.ru>]

16.07.2015 12:05, 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 <real@ispras.ru>
>> ---
>>   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.
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
>