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

[Thorsten Kohfeldt]

Am 22.10.2016 um 17:09 schrieb 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.

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