Date: Sun, 1 Mar 2015 16:47:13 -0800 From: Mark Millard <markmi@dsl-only.net> To: FreeBSD PowerPC ML <freebsd-ppc@freebsd.org> Subject: Re: cb_dumpdata vs. PowerMac G5's (or at least my SSD context): add involvement of dumpinfo's maxiosize value? Message-ID: <9F6AE9A1-CDE0-47F9-9EF6-1C3710A5C59B@dsl-only.net> In-Reply-To: <84EBF11A-FB29-440B-BA0F-9FA94F1501AD@dsl-only.net> References: <84EBF11A-FB29-440B-BA0F-9FA94F1501AD@dsl-only.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Unfortunately reporting the trace of finding what is happening for the =
DMA size panic-dump issue for powerpc64 on PowerMac G5's (and others =
FreeBSDs?) is rather long...
My test of trying a variant of my example code still ended up trying for =
a 64K DMA when only 32K is allowed according to the error message it =
generated.
It would appear the dumperinfo's maxiosize is too big as well. Looking =
around...
$ find sys -name '*.[hcsm]' -exec grep "\<oversized DMA\>" {} \; -print =
| more
"FAILURE - oversized DMA transfer attempt %d > =
%d\n",
sys/dev/ata/ata-dma.c
"FAILURE - oversized DMA transfer attempt %d > =
%d\n",
sys/powerpc/powermac/ata_dbdma.c
static int
ata_dmaload(struct ata_request *request, void *addr, int *entries)
{
struct ata_channel *ch =3D device_get_softc(request->parent);
...
if (request->bytecount > ch->dma.max_iosize) {
device_printf(request->parent,
"FAILURE - oversized DMA transfer attempt %d > =
%d\n",
request->bytecount, ch->dma.max_iosize);
return EIO;
}
...
}
and
static int
ata_dbdma_load(struct ata_request *request, void *addr, int *entries)
{
struct ata_channel *ch =3D device_get_softc(request->parent);
...
if (request->bytecount > ch->dma.max_iosize) {
device_printf(request->dev,
"FAILURE - oversized DMA transfer attempt %d > =
%d\n",
request->bytecount, ch->dma.max_iosize);
return EIO;
}
...
}
These suggest that the dumperinfo's maxiosize might sometimes be =
legitimately bigger than ata_channel's dma.max_iosize since =
dma.max_iosize would be very specific to DMA transfers.
So it would appear that either:
A) ata_channel's dma.max_iosize should also be considered in =
dump_machdep.c .
or
B) dumperinfo's maxiosize value should in part be based on ata_channel's =
dma.max_iosize when an ata_channel is involved and DMA would be =
involved.
Then looking...
$ find sys -name '*.[hcSm]' -exec grep "\<maxiosize\>" {} \; -print | =
more =20=
maxdumpsz =3D di->maxiosize;
sys/mips/mips/minidump_machdep.c
maxdumppgs =3D min(di->maxiosize / PAGE_SIZE, MAXDUMPPGS);
sys/x86/x86/dump_machdep.c
gkd->di.maxiosize =3D DFLTPHYS;
sys/geom/raid/g_raid.c
gkd->di.maxiosize =3D dp->d_maxsize;
sys/geom/geom_disk.c
maxdumpsz =3D min(di->maxiosize, MAXDUMPPGS * PAGE_SIZE);
sys/i386/i386/minidump_machdep.c
maxdumpsz =3D min(di->maxiosize, MAXDUMPPGS * PAGE_SIZE);
sys/amd64/amd64/minidump_machdep.c
u_int maxiosize; /* Max size allowed for an individual =
I/O */
sys/sys/conf.h
maxdumpsz =3D di->maxiosize;
sys/arm/arm/minidump_machdep.c
u_int max_trans_sz =3D (0=3D=3Ddi) ? 0 : =
(di->maxiosize<DFLTPHYS) ? di->maxiosize : DFLTPHYS;
* The original code that ignored =
di->maxiosize had that property
sys/powerpc/powerpc/dump_machdep.c
static void
g_disk_kerneldump(struct bio *bp, struct disk *dp)
{
struct g_kerneldump *gkd;
struct g_geom *gp;
gkd =3D (struct g_kerneldump*)bp->bio_data;
...
gkd->di.maxiosize =3D dp->d_maxsize;
...
}
So that would trace back to disk's d_maxsize. The only assignment to the =
field that I find is in g_disk_access:
g_disk_access(struct g_provider *pp, int r, int w, int e)
{
struct disk *dp;
struct g_disk_softc *sc;
int error;
g_trace(G_T_ACCESS, "g_disk_access(%s, %d, %d, %d)",
pp->name, r, w, e);
g_topology_assert();
sc =3D pp->private;
if (sc =3D=3D NULL || (dp =3D sc->dp) =3D=3D NULL || =
dp->d_destroyed) {
...
return (ENXIO);
}
...
if ((pp->acr + pp->acw + pp->ace) =3D=3D 0 && (r + w + e) > 0) {
...
if (dp->d_maxsize =3D=3D 0) {
printf("WARNING: Disk drive %s%d has no =
d_maxsize\n",
dp->d_name, dp->d_unit);
dp->d_maxsize =3D DFLTPHYS;
}
...
}
So it appears that the caller(s) of disk_create need to supply the value =
to the field. Looking at sys/cam/ata/ata_da.c's calling code...
#ifndef ata_disk_firmware_geom_adjust
#define ata_disk_firmware_geom_adjust(disk)
#endif
...
static cam_status
adaregister(struct cam_periph *periph, void *arg)
{
struct ada_softc *softc;
struct ccb_pathinq cpi;
struct ccb_getdev *cgd;
char announce_buf[80], buf1[32];
struct disk_params *dp;
caddr_t match;
u_int maxio;
int legacy_id, quirks;
...
bzero(&cpi, sizeof(cpi));
xpt_setup_ccb(&cpi.ccb_h, periph->path, CAM_PRIORITY_NONE);
cpi.ccb_h.func_code =3D XPT_PATH_INQ;
xpt_action((union ccb *)&cpi);
...
maxio =3D cpi.maxio; /* Honor max I/O size of SIM =
*/
if (maxio =3D=3D 0)
maxio =3D DFLTPHYS; /* traditional default */
else if (maxio > MAXPHYS)
maxio =3D MAXPHYS; /* for safety */
if (softc->flags & ADA_FLAG_CAN_48BIT)
maxio =3D min(maxio, 65536 * softc->params.secsize);
else /* 28bit ATA command =
limit */
maxio =3D min(maxio, 256 * softc->params.secsize);
softc->disk->d_maxsize =3D maxio;
...
ata_disk_firmware_geom_adjust(softc->disk);
...
}
So far I do not see any hint of a ata_channel's dma.max_iosize being =
involved in setting d_maxsize. (Only pc98 and sparc64 seem to have =
ata_disk_firmware_geom_adjust definitions.)
As overall there may not be a global requirement to use DMA this may =
well make sense.
But it would mean that when DMA is to be used and it is an ata_channel =
context that the ata_channel's dma.max_iosize should be consulted. (Not =
that ata_channel need be the only context with such an issue. But it is =
the one involved in my problem.)
Looking around I find the following assignments:
$ find sys -name '*.[hcSm]' -exec grep "\<max_iosize\>" {} \; -print | =
more =
=20
ch->dma.max_iosize =3D (ATA_SIIPRB_DMA_ENTRIES - 1) * PAGE_SIZE;
sys/dev/ata/chipsets/ata-siliconimage.c
ch->dma.max_iosize =3D 65536;
sys/dev/ata/chipsets/ata-promise.c
ch->dma.max_iosize =3D 64 * DEV_BSIZE;
sys/dev/ata/chipsets/ata-serverworks.c
ch->dma.max_iosize =3D (ATA_AHCI_DMA_ENTRIES - 1) * PAGE_SIZE;
sys/dev/ata/chipsets/ata-ahci.c
ch->dma.max_iosize =3D 64 * DEV_BSIZE;
sys/dev/ata/chipsets/ata-cyrix.c
ch->dma.max_iosize =3D 64 * DEV_BSIZE;
sys/dev/ata/chipsets/ata-national.c
if (ch->dma.max_iosize > 256 * 512)
ch->dma.max_iosize =3D 256 * 512;
sys/dev/ata/chipsets/ata-acerlabs.c
ch->dma.max_iosize =3D 64 * DEV_BSIZE;
sys/dev/ata/chipsets/ata-marvell.c
... (no assignment) ...
sys/dev/ata/ata-all.c
... (no assignment) ...
sys/dev/ata/ata-all.h
if (ch->dma.max_iosize =3D=3D 0)
ch->dma.max_iosize =3D MIN((ATA_DMA_ENTRIES - 1) * PAGE_SIZE, =
MAXPHYS);
NULL, NULL, ch->dma.max_iosize,
NULL, NULL, ch->dma.max_iosize,
if (request->bytecount > ch->dma.max_iosize) {
request->bytecount, ch->dma.max_iosize);
sys/dev/ata/ata-dma.c
... (no assignment) ...
sys/arm/mv/mv_sata.c
... (no assignment) ...
sys/powerpc/powermac/ata_dbdma.c
... (no assignment) ...
sys/cam/ctl/ctl_backend_block.c
All but possibly the last being ata contexts for the field name =
max_iosize .
In sys/sys/param.h I find:
#define DEV_BSHIFT 9 /* log2(DEV_BSIZE) */
#define DEV_BSIZE (1<<DEV_BSHIFT)
So DEV_BSIZE =3D=3D 512. That means several of the above assignment =
values are less than 64*1024 (a.k.a. DFLTPHYS), some being 64*512 (so =
32*1024) like the PowerMac G5 is ending up with for my configuration. =
There is also:
$ find sys -name '*.[h]' -exec grep "\<ATA_.*_ENTRIES\>" {} \; -print | =
more =20=
#define ATA_AHCI_DMA_ENTRIES 129
struct ata_ahci_dma_prd prd_tab[ATA_AHCI_DMA_ENTRIES];
#define ATA_DMA_ENTRIES 256
sys/dev/ata/ata-all.h
and sys/powerpc/include/param.h has:
#define PAGE_SHIFT 12
#define PAGE_SIZE (1L << PAGE_SHIFT) /* Page size */
#define PAGE_MASK (vm_offset_t)(PAGE_SIZE - 1)
So PAGE_SIZE =3D=3D 4096 for the context. So any ATA_..._ENTRIES * =
PAGE_SIZE alternatives are not involved in my problem.
Looking at what dumper is assigned...
static void
g_disk_kerneldump(struct bio *bp, struct disk *dp)
{
struct g_kerneldump *gkd;
struct g_geom *gp;
...
if (dp->d_dump =3D=3D NULL) {
g_io_deliver(bp, ENODEV);
return;
}
gkd->di.dumper =3D dp->d_dump;
...
}
where sys/cam/ata/ata_da.c has:
static cam_status
adaregister(struct cam_periph *periph, void *arg)
{
struct ada_softc *softc;
...
softc->disk->d_dump =3D adadump;
...
}
adadump has no logic to limit its I/O requests by the dma.max_iosize =
when given a length bigger than that. sys/cam/ata/ata_da.c has no text =
"dma" at all. But adadump does have:
adadump(void *arg, void *virtual, vm_offset_t physical, off_t offset, =
size_t length)
{
...
if ((softc->flags & ADA_FLAG_CAN_48BIT) &&
(lba + count >=3D ATA_MAX_28BIT_LBA ||
count >=3D 256)) {
ata_48bit_cmd(&ccb.ataio, ATA_WRITE_DMA48,
0, lba, count);
} else {
ata_28bit_cmd(&ccb.ataio, ATA_WRITE_DMA,
0, lba, count);
}
...
}
which makes the I/O a DMA based I/O for sure.
I do not see that sys/cam/ata/... explicitly uses or exposes =
ata_channel's dma.max_iosize in any way.
As a matter of interfacing I would expect that --just like the dumper =
field gets into the appropriate cam code-- any abstraction spanning =
dma.max_iosize sorts of issues would also reference something that gets =
into the cam code that matches up with the dumper cam code.
There seems to be no such interface. One could imagine a new dumpinfo =
field (look for the dumper_adjusted_maxsize below):
static int
cb_dumpdata(struct pmap_md *md, int seqnr, void *arg)
{
struct dumperinfo *di =3D (struct dumperinfo*)arg;
u_int max_trans_sz =3D (di->maxiosize<DFLTPHYS) ? di->maxiosize : =
DFLTPHYS;
...
max_trans_sz =3D di->dumper_adjusted_maxsize(di->priv, =
max_trans_sz);
...
printf(" chunk %d: %lu bytes ", seqnr, (u_long)resid);
while (resid) {
sz =3D (resid > max_trans_sz) ? max_trans_sz : resid;
...
error =3D di->dumper(di->priv, (void*)va, 0, dumplo, sz);
...
}
(Middle layers would need to be set up to allow plugging in the value =
into dumpinfo's new field and sys/cam/ata/... would need ata_channel's =
dma.max_iosize access and use of it.)
In many cases dumper_adjusted_maxsize might be an identity operation (no =
adjustment) or a simple maximum operation but for sys/cam/ata/ata_da.c =
contexts its main purpose would be to apply DMA size limitations if they =
are smaller than the parameter passed in.
Out of all this my overall conclusion is that there is a hole in the =
FreeBSD design and without some design changes the technique is to =
change the constant values used in cb_dumpdata to values that happen to =
work.
In this simpler line of things it leaves me wondering if something like =
DFLTPHYS/2 would be appropriate for dumper contexts in official =
10.1-STABLE FreeBSD for now:
static int
cb_dumpdata(struct pmap_md *md, int seqnr, void *arg)
{
struct dumperinfo *di =3D (struct dumperinfo*)arg;
u_int max_trans_sz =3D (di->maxiosize<DFLTPHYS/2) ? di->maxiosize =
: DFLTPHYS/2;
...
printf(" chunk %d: %lu bytes ", seqnr, (u_long)resid);
while (resid) {
sz =3D (resid > max_trans_sz) ? max_trans_sz : resid;
...
error =3D di->dumper(di->priv, (void*)va, 0, dumplo, sz);
...
}
More of the existing ch->dma.max_iosize assignments would not be =
exceeded and so more contexts could then generate panic dumps.
Context for the experiment that prompted the above looking around:
$ freebsd-version -ku; uname -a
10.1-RELEASE-p6
10.1-STABLE
FreeBSD FBSDG5M1 10.1-RELEASE-p6 FreeBSD 10.1-RELEASE-p6 #21 r279264M: =
Sun Mar 1 03:31:59 PST 2015 =
root@FBSDG5M1:/usr/obj/usr/home/markmi/src_10_1_releng/sys/GENERIC64vtsc =
powerpc
$ svnlite st
? .snap
M sys/ddb/db_main.c
M sys/ddb/db_script.c
M sys/powerpc/conf
? sys/powerpc/conf/GENERIC64vtsc
M sys/powerpc/ofw/ofw_machdep.c
M sys/powerpc/ofw/ofwcall64.S
M sys/powerpc/powerpc/dump_machdep.c
$ svnlite info
Path: .
Working Copy Root Path: /usr/src
URL: https://svn0.us-west.freebsd.org/base/stable/10
Relative URL: ^/stable/10
Repository Root: https://svn0.us-west.freebsd.org/base
Repository UUID: ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
Revision: 279201
Node Kind: directory
Schedule: normal
Last Changed Author: pluknet
Last Changed Rev: 279201
Last Changed Date: 2015-02-23 00:45:42 -0800 (Mon, 23 Feb 2015)
$ svnlite diff sys/powerpc/powerpc/dump_machdep.c | more
Index: sys/powerpc/powerpc/dump_machdep.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- sys/powerpc/powerpc/dump_machdep.c (revision 279201)
+++ sys/powerpc/powerpc/dump_machdep.c (working copy)
@@ -113,6 +113,11 @@
cb_dumpdata(struct pmap_md *md, int seqnr, void *arg)
{
struct dumperinfo *di =3D (struct dumperinfo*)arg;
+ u_int max_trans_sz =3D (0=3D=3Ddi) ? 0 : =
(di->maxiosize<DFLTPHYS) ? di->maxiosize : DFLTPHYS;
+ /* If md->md_size turns out to be 0 the =
above avoids derefercing di.
+ * The original code that ignored =
di->maxiosize had that property
+ * overall. So this update does too.
+ */
vm_offset_t va;
size_t counter, ofs, resid, sz;
int c, error, twiddle;
@@ -124,10 +129,10 @@
ofs =3D 0; /* Logical offset within the chunk */
resid =3D md->md_size;
=20
- printf(" chunk %d: %lu bytes ", seqnr, (u_long)resid);
+ printf(" chunk %d: %lu bytes max_trans_sz %lu ", seqnr, =
(u_long)resid, (u_long)max_trans_sz);
=20
while (resid) {
- sz =3D (resid > DFLTPHYS) ? DFLTPHYS : resid;
+ sz =3D (resid > max_trans_sz) ? max_trans_sz : resid;
va =3D pmap_dumpsys_map(md, ofs, &sz);
counter +=3D sz;
if (counter >> 24) {
=3D=3D=3D
Mark Millard
markmi at dsl-only.net
On 2015-Mar-1, at 04:14 AM, Mark Millard <markmi at dsl-only.net> wrote:
Context: 10.1-STABLE powerpc64 for PowerMac G5
I'm unable to get panic dumps because the DMA size is rejected, 64K =
being too big. I'd like to get things to the point where panic dumps are =
possible for that context.
Looking around I find:
root@FBSDG5M1:/usr/src # grep DFLTPHYS sys/sys/*.h
sys/sys/param.h:#ifndef DFLTPHYS
sys/sys/param.h:#define DFLTPHYS (64 * 1024) /* default max =
raw I/O transfer size */
sys/sys/param.h:#define MAXDUMPPGS (DFLTPHYS/PAGE_SIZE)
root@FBSDG5M1:/usr/src # grep DFLTPHYS sys/powerpc/powerpc/*
sys/powerpc/powerpc/dump_machdep.c: sz =3D (resid > =
DFLTPHYS) ? DFLTPHYS : resid;
static int
cb_dumpdata(struct pmap_md *md, int seqnr, void *arg)
{
struct dumperinfo *di =3D (struct dumperinfo*)arg;
...
resid =3D md->md_size;
...
while (resid) {
sz =3D (resid > DFLTPHYS) ? DFLTPHYS : resid;
...
error =3D di->dumper(di->priv, (void*)va, 0, dumplo, sz);
...
}
Which may well be where the 64K is from. sys/sys/conf.h has:
struct dumperinfo {
dumper_t *dumper; /* Dumping function. */
void *priv; /* Private parts. */
u_int blocksize; /* Size of block in bytes. */
u_int maxiosize; /* Max size allowed for an individual I/O =
*/
off_t mediaoffset; /* Initial offset in bytes. */
off_t mediasize; /* Space available in bytes. */
};
So it would appear that maxiosize from dumperinfo was supposed to be =
involved. But at this level nothing is checking dumperinfo's maxiosize =
field value to avoid using anything bigger.
It may be that the di->dumper is supposed to deal with allowed sizes =
being smaller than its size parameter indicates (sz here). But then =
exposing maxiosize in dumperinfo would seem a bit odd as an interface.
My initial guess would be that cb_dumpdata should be more like:
static int
cb_dumpdata(struct pmap_md *md, int seqnr, void *arg)
{
struct dumperinfo *di =3D (struct dumperinfo*)arg;
u_int max_trans_sz =3D (di->maxiosize<DFLTPHYS) ? di->maxiosize : =
DFLTPHYS;
...
printf(" chunk %d: %lu bytes ", seqnr, (u_long)resid);
while (resid) {
sz =3D (resid > max_trans_sz) ? max_trans_sz : resid;
...
error =3D di->dumper(di->priv, (void*)va, 0, dumplo, sz);
...
}
I have assumed that even when 0=3D=3Dresid that 0!=3Darg: the origin =
code would not dereference di in that kind of context so a little more =
conditional logic might be required if that property needs to be =
preserved.
Anyone know if I seem to be going in a reasonable direction for this?
I've only noted the large transfer size that I found. Many other places =
use a di->dumper with the size of something much smaller or with a =
smaller symbolic constant (such as having value 512). Again there is no =
maxiosize handling. It seems there there is an expected (implicit?) =
minimum size for maxiosize such that these various things would always =
fit. =46rom that point of view I'm just objecting to DFLTPHYS being that =
minimum I guess: I want a smaller minimum so that I can get dumps from =
my existing configuration.
=3D=3D=3D
Mark Millard
markmi at dsl-only.net
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9F6AE9A1-CDE0-47F9-9EF6-1C3710A5C59B>