Re: [Qemu-devel] [RFC PATCH] memory: Don't use memcpy for ram marked as skip_dump

[Alex Williamson]

On Sat, 22 Oct 2016 11:10:59 +0200
Thorsten Kohfeldt <address@hidden> 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