xen/blkfront: WRITE_BARRIER and FLUSH_DISKCACHE require barrier
AbandonedPublic
Actions

Authored by ehem_freebsd_m5p.com on Apr 6 2021, 1:04 AM.

Details

Reviewers

royger
mhorne

Summary

For WRITE_BARRIER and FLUSH_DISKCACHE operation, we don't request any cache
operation. This will result to a panic in _bus_dmamap_sync on ARM because the
operation (op = 0) is not supported.

x86 platform doesn't seem to care about this and Xen is always requiring
memory shared with the backend to be cacheable. I'm wondering if we could drop
the call to bus_dmasync_map because the cache maintainance slows down the
process for no appareant reason?

For now, WRITE_BARRIER and FLUSH_DISKCACHE are an extension of the WRITE
command so require BUS_DMASYNC_PREWRITE for the cache maintenance operation.

Submitted by: Elliott Mitchell <ehem+freebsd@m5p.com>
Original implementation: Julien Grall <julien@xen.org>, 2015-10-05 10:35:43

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 38347
Build 35236: arc lint + arc unit

Event Timeline

ehem_freebsd_m5p.com created this revision.Apr 6 2021, 1:04 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 6 2021, 1:04 AM

ehem_freebsd_m5p.com requested review of this revision.Apr 6 2021, 1:04 AM

Harbormaster completed remote builds in B38347: Diff 86877.Apr 6 2021, 1:04 AM

The commit message needs to be adjusted, as it contains a question. All memory shared with Xen is of type WB (the default attribute), but I'm not sure I see the point of the question, the bus_dmamap_sync checks that the submitted segments match the requirement of the underlying device in terms of aligments, sizes and boundaries, or else bounces it to some temporary memory. This cannot be avoided AFAICT. A very good test for the dmamap stuff is newfs, which tends to do weird block writes that trigger the dmamap bouncing for blkfront IIRC.

sys/dev/xen/blkfront/blkfront.c
282	Aren't you missing the POST sides of those? I think a similar adjustment should be done in xbd_int for the POSTREAD/POSTWRITE variants?
284	If Arm bus_dmamap_sync panics when op == 0, we likely want to do: if (op != 0) bus_dmamap_sync(sc->xbd_io_dmat, cm->cm_map, op);

@julien_xen.org any idea?

I've mostly been doing very early testing so far, so my insight here is non-existent. Things appear to work with this, but so far I'm doing read()s and no write()s.

In D29598#672374, @ehem_freebsd_m5p.com wrote:

@julien_xen.org any idea?

I wish I kept the stack trace. I looked at the code and cannot find a panic() in the path. So I would suggest to drop this patch.

This is on my queue for investigation (alas, this is behind the queue for being submitted).

Marking D29598 as changed planned. This needs to be tested for whether this is now unneeded.

This could need to be brought back later, but for now I've been seeing apparently successful builds without this. This should be tested when we're near the stage of routine automated builds. Until then this appears unnecessary.

Revision Contents
Changeset List

Path

Size

sys/

dev/

xen/

blkfront/

blkfront.c

13 lines

Diff 86877

View Options

sys/dev/xen/blkfront/blkfront.c

xen/blkfront: WRITE_BARRIER and FLUSH_DISKCACHE require barrierAbandonedPublicActions