[RFC] memory: Don't use memcpy for ram marked as skip_dump

With a vfio assigned device we lay down a base MemoryRegion registered
as an IO region, giving us read & write accessors.  If the region
supports mmap, we lay down a higher priority sub-region MemoryRegion
on top of the base layer initialized as a RAM pointer to the mmap.
Finally, if we have any quirks for the device (ie. address ranges that
need additional virtualization support), we put another IO sub-region
on top of the mmap MemoryRegion.  When this is flattened, we now
potentially have sub-page mmap MemoryRegions exposed which cannot be
directly mapped through KVM.

This is as expected, but a subtle detail of this is that we end up
with two different access mechanisms through QEMU.  If we disable the
mmap MemoryRegion, we make use of the IO MemoryRegion and service
accesses using pread and pwrite to the vfio device file descriptor.
If the mmap MemoryRegion is enabled and we end up in one of these
sub-page gaps, QEMU handles the access as RAM, using memcpy to the
mmap.  Using the mmap through QEMU is a subtle difference, but it's
fine, the problem is the memcpy.  My assumption is that memcpy makes
no guarantees about access width and potentially uses all sorts of
optimized memory transfers that are not intended for talking to device
MMIO.  It turns out that this has been a problem for Realtek NIC
assignment, which has such a quirk that creates a sub-page mmap
MemoryRegion access.

My proposal to fix this is to leverage the skip_dump flag that we
already use for special handling of these device-backed MMIO ranges.
When skip_dump is set for a MemoryRegion, we mark memory access as
non-direct and automatically insert MemoryRegionOps with basic
semantics to handle accesses.  Note that we only enable dword
accesses because some devices don't particularly like qword accesses
(Realtek NICs are such a device).  This actually also fixes memory
inspection via the xp command in the QEMU monitor as well.

Please comment.  Is this the best way to solve this problem?  Thanks

Reported-by: Thorsten Kohfeldt <thorsten.kohfeldt@gmx.de>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 include/exec/memory.h |    6 ++++--
 memory.c              |   44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

Hi *,

this came to my mind when browsing the sources in the patch's vicinity.

It is just a collection of thoughts, so please don't feel offended
about how I phrased certain statements.


Questions

Is mr->opaque always unused ?
i.e. should we assert NULL before assignment ?

mr->ops vs. mr->iommu_ops
i.e. can we set mr->opaque if mr->iommu_ops is not NULL ?
or should we even assert mr->iommu_ops NULL, because a
skip_dump mr is not supposed to be addr-translated again ?

There is a _shared_ 'io_mem_unassigned' mr.
Are we in danger to modify it ?
Would that hurt ?

Are we generally switching mrops "back and forth",
or is this a first ?

Can we afford not to implement size 8 or should we rather
force 8 -> 2*4 by setting specific mrop flags if possible ?
Or just hard code case 8: handle longword[1]; fallthru 4:

When/where is memory_region_set_skip_dump() (supposed to be) called ?


Recommendations

Add comment in skip_dump_mem_read/write NOT to support 64b,
because an error will not be recognised unless specific HW is present
(maybe even give examples of specific HW combinations).

Add comments at more code locations that are break-subpage/mmap-sensible.
For example default vfio slow path mrops should also not support 64b ?

Add a trace message for each mrop.


Additional patch suggestion(s)

During former investigations I found it not easy to
identify runtime active/current mrops per mr, so:
Add .name to mr->ops/iommu_ops
     to be able to mon-list them together with mr names
OR
(this questions flag reuse/overlay)
skip_dump_flag should rather get a brother
     so (unamed) ops can be easily concluded for listing ?
     But is this the only mr<->mrop ambiguosity ?


Regards,

Thorsten


Am 21.10.2016 um 19:11 schrieb Alex Williamson:
> With a vfio assigned device we lay down a base MemoryRegion registered
> as an IO region, giving us read & write accessors.  If the region
> supports mmap, we lay down a higher priority sub-region MemoryRegion
> on top of the base layer initialized as a RAM pointer to the mmap.
> Finally, if we have any quirks for the device (ie. address ranges that
> need additional virtualization support), we put another IO sub-region
> on top of the mmap MemoryRegion.  When this is flattened, we now
> potentially have sub-page mmap MemoryRegions exposed which cannot be
> directly mapped through KVM.
>
> This is as expected, but a subtle detail of this is that we end up
> with two different access mechanisms through QEMU.  If we disable the
> mmap MemoryRegion, we make use of the IO MemoryRegion and service
> accesses using pread and pwrite to the vfio device file descriptor.
> If the mmap MemoryRegion is enabled and we end up in one of these
> sub-page gaps, QEMU handles the access as RAM, using memcpy to the
> mmap.  Using the mmap through QEMU is a subtle difference, but it's
> fine, the problem is the memcpy.  My assumption is that memcpy makes
> no guarantees about access width and potentially uses all sorts of
> optimized memory transfers that are not intended for talking to device
> MMIO.  It turns out that this has been a problem for Realtek NIC
> assignment, which has such a quirk that creates a sub-page mmap
> MemoryRegion access.
>
> My proposal to fix this is to leverage the skip_dump flag that we
> already use for special handling of these device-backed MMIO ranges.
> When skip_dump is set for a MemoryRegion, we mark memory access as
> non-direct and automatically insert MemoryRegionOps with basic
> semantics to handle accesses.  Note that we only enable dword
> accesses because some devices don't particularly like qword accesses
> (Realtek NICs are such a device).  This actually also fixes memory
> inspection via the xp command in the QEMU monitor as well.
>
> Please comment.  Is this the best way to solve this problem?  Thanks
>
> Reported-by: Thorsten Kohfeldt <thorsten.kohfeldt@gmx.de>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  include/exec/memory.h |    6 ++++--
>  memory.c              |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 10d7eac..a4c3acf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1464,9 +1464,11 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
>  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  {
>      if (is_write) {
> -        return memory_region_is_ram(mr) && !mr->readonly;
> +        return memory_region_is_ram(mr) &&
> +               !mr->readonly && !memory_region_is_skip_dump(mr);
>      } else {
> -        return memory_region_is_ram(mr) || memory_region_is_romd(mr);
> +        return (memory_region_is_ram(mr) && !memory_region_is_skip_dump(mr)) ||
> +               memory_region_is_romd(mr);
>      }
>  }
>
> diff --git a/memory.c b/memory.c
> index 58f9269..7ed7ca9 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1136,6 +1136,46 @@ const MemoryRegionOps unassigned_mem_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static uint64_t skip_dump_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    uint64_t val = (uint64_t)~0;
> +
> +    switch (size) {
> +    case 1:
> +        val = *(uint8_t *)(opaque + addr);
> +        break;
> +    case 2:
> +        val = *(uint16_t *)(opaque + addr);
> +        break;
> +    case 4:
> +        val = *(uint32_t *)(opaque + addr);
> +        break;
> +    }
> +
> +    return val;
> +}
> +
> +static void skip_dump_mem_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)
> +{
> +    switch (size) {
> +    case 1:
> +        *(uint8_t *)(opaque + addr) = (uint8_t)data;
> +        break;
> +    case 2:
> +        *(uint16_t *)(opaque + addr) = (uint16_t)data;
> +        break;
> +    case 4:
> +        *(uint32_t *)(opaque + addr) = (uint32_t)data;
> +        break;
> +    }
> +}
> +
> +const MemoryRegionOps skip_dump_mem_ops = {
> +    .read = skip_dump_mem_read,
> +    .write = skip_dump_mem_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  bool memory_region_access_valid(MemoryRegion *mr,
>                                  hwaddr addr,
>                                  unsigned size,
> @@ -1366,6 +1406,10 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>  void memory_region_set_skip_dump(MemoryRegion *mr)
>  {
>      mr->skip_dump = true;
> +    if (mr->ram && mr->ops == &unassigned_mem_ops) {
> +        mr->ops = &skip_dump_mem_ops;
> +        mr->opaque = mr->ram_block->host;
> +    }
>  }
>
>  void memory_region_init_alias(MemoryRegion *mr,
>
>

----- Original Message -----
> From: "Alex Williamson" <alex.williamson@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: pbonzini@redhat.com, "thorsten kohfeldt" <thorsten.kohfeldt@gmx.de>
> Sent: Friday, October 21, 2016 7:11:44 PM
> Subject: [RFC PATCH] memory: Don't use memcpy for ram marked as skip_dump
> 
> With a vfio assigned device we lay down a base MemoryRegion registered
> as an IO region, giving us read & write accessors.  If the region
> supports mmap, we lay down a higher priority sub-region MemoryRegion
> on top of the base layer initialized as a RAM pointer to the mmap.
> Finally, if we have any quirks for the device (ie. address ranges that
> need additional virtualization support), we put another IO sub-region
> on top of the mmap MemoryRegion.  When this is flattened, we now
> potentially have sub-page mmap MemoryRegions exposed which cannot be
> directly mapped through KVM.
> 
> This is as expected, but a subtle detail of this is that we end up
> with two different access mechanisms through QEMU.  If we disable the
> mmap MemoryRegion, we make use of the IO MemoryRegion and service
> accesses using pread and pwrite to the vfio device file descriptor.
> If the mmap MemoryRegion is enabled and we end up in one of these
> sub-page gaps, QEMU handles the access as RAM, using memcpy to the
> mmap.  Using the mmap through QEMU is a subtle difference, but it's
> fine, the problem is the memcpy.  My assumption is that memcpy makes
> no guarantees about access width and potentially uses all sorts of
> optimized memory transfers that are not intended for talking to device
> MMIO.  It turns out that this has been a problem for Realtek NIC
> assignment, which has such a quirk that creates a sub-page mmap
> MemoryRegion access.
> 
> My proposal to fix this is to leverage the skip_dump flag that we
> already use for special handling of these device-backed MMIO ranges.
> When skip_dump is set for a MemoryRegion, we mark memory access as
> non-direct and automatically insert MemoryRegionOps with basic
> semantics to handle accesses.  Note that we only enable dword
> accesses because some devices don't particularly like qword accesses
> (Realtek NICs are such a device).  This actually also fixes memory
> inspection via the xp command in the QEMU monitor as well.
> 
> Please comment.  Is this the best way to solve this problem?  Thanks

Looks good to me.

Paolo

> 
> Reported-by: Thorsten Kohfeldt <thorsten.kohfeldt@gmx.de>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  include/exec/memory.h |    6 ++++--
>  memory.c              |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 10d7eac..a4c3acf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1464,9 +1464,11 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t
> addr);
>  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  {
>      if (is_write) {
> -        return memory_region_is_ram(mr) && !mr->readonly;
> +        return memory_region_is_ram(mr) &&
> +               !mr->readonly && !memory_region_is_skip_dump(mr);
>      } else {
> -        return memory_region_is_ram(mr) || memory_region_is_romd(mr);
> +        return (memory_region_is_ram(mr) && !memory_region_is_skip_dump(mr))
> ||
> +               memory_region_is_romd(mr);
>      }
>  }
>  
> diff --git a/memory.c b/memory.c
> index 58f9269..7ed7ca9 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1136,6 +1136,46 @@ const MemoryRegionOps unassigned_mem_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +static uint64_t skip_dump_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    uint64_t val = (uint64_t)~0;
> +
> +    switch (size) {
> +    case 1:
> +        val = *(uint8_t *)(opaque + addr);
> +        break;
> +    case 2:
> +        val = *(uint16_t *)(opaque + addr);
> +        break;
> +    case 4:
> +        val = *(uint32_t *)(opaque + addr);
> +        break;
> +    }
> +
> +    return val;
> +}
> +
> +static void skip_dump_mem_write(void *opaque, hwaddr addr, uint64_t data,
> unsigned size)
> +{
> +    switch (size) {
> +    case 1:
> +        *(uint8_t *)(opaque + addr) = (uint8_t)data;
> +        break;
> +    case 2:
> +        *(uint16_t *)(opaque + addr) = (uint16_t)data;
> +        break;
> +    case 4:
> +        *(uint32_t *)(opaque + addr) = (uint32_t)data;
> +        break;
> +    }
> +}
> +
> +const MemoryRegionOps skip_dump_mem_ops = {
> +    .read = skip_dump_mem_read,
> +    .write = skip_dump_mem_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  bool memory_region_access_valid(MemoryRegion *mr,
>                                  hwaddr addr,
>                                  unsigned size,
> @@ -1366,6 +1406,10 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>  void memory_region_set_skip_dump(MemoryRegion *mr)
>  {
>      mr->skip_dump = true;
> +    if (mr->ram && mr->ops == &unassigned_mem_ops) {
> +        mr->ops = &skip_dump_mem_ops;
> +        mr->opaque = mr->ram_block->host;
> +    }
>  }
>  
>  void memory_region_init_alias(MemoryRegion *mr,
> 
>

On Sat, 22 Oct 2016 11:10:59 +0200
Thorsten Kohfeldt <thorsten.kohfeldt@gmx.de> wrote:

> Hi *,
> 
> this came to my mind when browsing the sources in the patch's vicinity.
> 
> It is just a collection of thoughts, so please don't feel offended
> about how I phrased certain statements.
> 
> 
> Questions
> 
> Is mr->opaque always unused ?
> i.e. should we assert NULL before assignment ?

Really I think it's probably unnecessary even to check the mr->ops
pointer.  I don't see a path where we can have both mr->ram true and
either mr->ops or mr->opaque set to anything other than defaults.
However, mr->opaque is consumed by mr->ops, so if mr->ops is set to
&unassigned_mem_ops, then we know opaque is unused.
 
> mr->ops vs. mr->iommu_ops
> i.e. can we set mr->opaque if mr->iommu_ops is not NULL ?
> or should we even assert mr->iommu_ops NULL, because a
> skip_dump mr is not supposed to be addr-translated again ?

Show me where a MemoryRegion with iommu_ops can be mr->ram.
 
> There is a _shared_ 'io_mem_unassigned' mr.
> Are we in danger to modify it ?
> Would that hurt ?

No.

> Are we generally switching mrops "back and forth",
> or is this a first ?

mr->ram is expected to have mr->ops set to the default unassigned
handler via memory_region_initfn().  All MemoryRegions start this way
and have their ops reassigned for certain initialization types.
 
> Can we afford not to implement size 8 or should we rather
> force 8 -> 2*4 by setting specific mrop flags if possible ?
> Or just hard code case 8: handle longword[1]; fallthru 4:

The memory API code will automatically split a qword into multiple
dword accesses.

> When/where is memory_region_set_skip_dump() (supposed to be) called ?

Use the source, it's called by vfio code after initializing the
MemoryRegion to indicate that memory dumps should skip this region as
it's backed by a physical device.
 
> Recommendations
> 
> Add comment in skip_dump_mem_read/write NOT to support 64b,
> because an error will not be recognised unless specific HW is present
> (maybe even give examples of specific HW combinations).

It's not clear to me that this is the correct long term behavior.  RTL
does not support qword access, but other devices might.  The
expectation would be that a guest driver does not use accesses beyond
the capabilities of the device.  It's convenient that limiting to dword
accesses fixes xp memory inspection in the monitor, but that's not a
sufficient reason not to implement qword should we want it for other
devices.
 
> Add comments at more code locations that are break-subpage/mmap-sensible.
> For example default vfio slow path mrops should also not support 64b ?

Nope, same.

> Add a trace message for each mrop.

Yes, this is on my todo list post-RFC.

> Additional patch suggestion(s)
> 
> During former investigations I found it not easy to
> identify runtime active/current mrops per mr, so:
> Add .name to mr->ops/iommu_ops
>      to be able to mon-list them together with mr names
> OR
> (this questions flag reuse/overlay)
> skip_dump_flag should rather get a brother
>      so (unamed) ops can be easily concluded for listing ?
>      But is this the only mr<->mrop ambiguosity ?

This is beyond the scope of this patch, you're welcome to pursue.

Most importantly, you haven't indicated whether this patch resolves the
issues you've been having.  Thanks,

Alex

On 22/10/2016 17:09, Alex Williamson wrote:
> > Add a trace message for each mrop.
> 
> Yes, this is on my todo list post-RFC.

Another thing to think about:

1) rename all the skip_dump occurrences in the API to device_memory.
The new ops make it much more specific than just skipping the region
during dumps.

2) get rid of mr->skip_dump and check mr->ops == &skip_dump_ops (or
device_memory_ops) instead.

Paolo

On Mon, 24 Oct 2016 13:05:56 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 22/10/2016 17:09, Alex Williamson wrote:
> > > Add a trace message for each mrop.  
> > 
> > Yes, this is on my todo list post-RFC.  
> 
> Another thing to think about:
> 
> 1) rename all the skip_dump occurrences in the API to device_memory.
> The new ops make it much more specific than just skipping the region
> during dumps.
> 
> 2) get rid of mr->skip_dump and check mr->ops == &skip_dump_ops (or
> device_memory_ops) instead.

Yep, I like those ideas.  Thanks,

Alex

Am 22.10.2016 um 17:09 schrieb Alex Williamson:
> On Sat, 22 Oct 2016 11:10:59 +0200
> Thorsten Kohfeldt <thorsten.kohfeldt@gmx.de> wrote:
>
>> Hi *,
>>
>> this came to my mind when browsing the sources in the patch's vicinity.
>>
>> It is just a collection of thoughts, so please don't feel offended
>> about how I phrased certain statements.
>>
>>
>> Questions
>>
>> Is mr->opaque always unused ?
>> i.e. should we assert NULL before assignment ?
>
> Really I think it's probably unnecessary even to check the mr->ops
> pointer.  I don't see a path where we can have both mr->ram true and
> either mr->ops or mr->opaque set to anything other than defaults.
> However, mr->opaque is consumed by mr->ops, so if mr->ops is set to
> &unassigned_mem_ops, then we know opaque is unused.
>
>> mr->ops vs. mr->iommu_ops
>> i.e. can we set mr->opaque if mr->iommu_ops is not NULL ?
>> or should we even assert mr->iommu_ops NULL, because a
>> skip_dump mr is not supposed to be addr-translated again ?
>
> Show me where a MemoryRegion with iommu_ops can be mr->ram.

I probably can't.
I was merely expressing a gut feeling.

>> There is a _shared_ 'io_mem_unassigned' mr.
>> Are we in danger to modify it ?
>> Would that hurt ?
>
> No.
>
>> Are we generally switching mrops "back and forth",
>> or is this a first ?
>
> mr->ram is expected to have mr->ops set to the default unassigned
> handler via memory_region_initfn().  All MemoryRegions start this way
> and have their ops reassigned for certain initialization types.
>
>> Can we afford not to implement size 8 or should we rather
>> force 8 -> 2*4 by setting specific mrop flags if possible ?
>> Or just hard code case 8: handle longword[1]; fallthru 4:
>
> The memory API code will automatically split a qword into multiple
> dword accesses.

I thought that would only happen if any of the mrop constraint flags were set ?
Like min size = 1, max_size = 4 ?
Are those default or propagated to the new op context from somewhere else
(i.e. dealt with before the new mrop callbacks are invoked) ?

>> When/where is memory_region_set_skip_dump() (supposed to be) called ?
>
> Use the source, it's called by vfio code after initializing the
> MemoryRegion to indicate that memory dumps should skip this region as
> it's backed by a physical device.

I should have phrased that completely differently.
I was wondering whether any future user of the skip_dump mechanism
would be aware enough of the new implications.
But I see Paolo has suggested a renaming, skip_dump to device_memory.
That looks good enough to me for raising that awareness.

>> Recommendations
>>
>> Add comment in skip_dump_mem_read/write NOT to support 64b,
>> because an error will not be recognised unless specific HW is present
>> (maybe even give examples of specific HW combinations).
>
> It's not clear to me that this is the correct long term behavior.  RTL
> does not support qword access, but other devices might.  The
> expectation would be that a guest driver does not use accesses beyond
> the capabilities of the device.  It's convenient that limiting to dword
> accesses fixes xp memory inspection in the monitor, but that's not a
> sufficient reason not to implement qword should we want it for other
> devices.
>
>> Add comments at more code locations that are break-subpage/mmap-sensible.
>> For example default vfio slow path mrops should also not support 64b ?
>
> Nope, same.
>
>> Add a trace message for each mrop.
>
> Yes, this is on my todo list post-RFC.
>
>> Additional patch suggestion(s)
>>
>> During former investigations I found it not easy to
>> identify runtime active/current mrops per mr, so:
>> Add .name to mr->ops/iommu_ops
>>      to be able to mon-list them together with mr names
>> OR
>> (this questions flag reuse/overlay)
>> skip_dump_flag should rather get a brother
>>      so (unamed) ops can be easily concluded for listing ?
>>      But is this the only mr<->mrop ambiguosity ?
>
> This is beyond the scope of this patch, you're welcome to pursue.

And would I find people willing to review ?
See, I did not have that much resonance for my recent patch initiative
'hmp: Improve 'info mtree' with optional parm for mapinfo'.

> Most importantly, you haven't indicated whether this patch resolves the
> issues you've been having.  Thanks,

I planned on testing the reviewed/revised patch,
but anyway,
I will only find time for testing later this week.

> Alex

Regards,

Thorsten