Re: libata/sata_sil24 cache alignment problem?
Alan Cox [EMAIL PROTECTED] wrote:
Has anyone else reported a problem like this? It requires
non-coherent DMA, and a lack of a cache invalidate instruction, and
one of the drivers that has this problem (it looks like sata_qstor
does too, I haven't looked at others), so maybe that doesn't cover
any other architectures.
Nobody has, not even PA-RISC which is normally guaranteed to make life
miserable in the caching area but I agree entirely with your diagnosis
and that buffer should indeed be marked cache aligned
I can provide a patch if you're interested.
Please do.
Here it is. I'd appreciate it if someone could sanity check where I'm
freeing the buffer. It works, but I don't know the code well enough
to know if this covers all cases.
I'm counting on kmalloc to return a cache aligned buffer. I found
some reason to think it does, but I don't remember offhand what that
reason was, or if it's configurable per-architecture. The buffer has
to be both physically and virtually contiguous, I was tempted to just
allocate a page and waste some space but we've got 64K pages, so I'm a
bit more sensitive about that.
*** drivers/scsi/libata-core.c 2008-02-13 13:34:32.141489000 -0500
--- drivers/scsi/libata-core.c.orig 2008-02-13 13:29:35.62036 -0500
***
*** 5335,5369
}
host = scsi_host_alloc(ent-sht, sizeof(struct ata_port));
if (!host)
return NULL;
host-transportt = ata_scsi_transport_template;
ap = ata_shost_to_port(host);
- ap-sector_buf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
- if (!ap-sector_buf)
- goto err_out;
-
ata_host_init(ap, host, host_set, ent, port_no);
rc = ap-ops-port_start(ap);
if (rc)
goto err_out;
return ap;
err_out:
- if (ap-sector_buf)
- kfree(ap-sector_buf);
scsi_host_put(host);
return NULL;
}
/**
*ata_device_add - Register hardware device with ATA and SCSI layers
*@ent: Probe information describing hardware device to be registered
*
*This function processes the information provided in the probe
*information struct @ent, allocates the necessary ATA and SCSI
--- 5335,5363
***
*** 5602,5645
*Unregister all objects associated with this host set. Free those
*objects.
*
*LOCKING:
*Inherited from calling layer (may sleep).
*/
void ata_host_set_remove(struct ata_host_set *host_set)
{
unsigned int i;
- u8 *sector_buf;
for (i = 0; i host_set-n_ports; i++)
ata_port_detach(host_set-ports[i]);
free_irq(host_set-irq, host_set);
for (i = 0; i host_set-n_ports; i++) {
struct ata_port *ap = host_set-ports[i];
ata_scsi_release(ap-host);
if ((ap-flags ATA_FLAG_NO_LEGACY) == 0) {
struct ata_ioports *ioaddr = ap-ioaddr;
if (ioaddr-cmd_addr == 0x1f0)
release_region(0x1f0, 8);
else if (ioaddr-cmd_addr == 0x170)
release_region(0x170, 8);
}
- sector_buf = ap-sector_buf;
scsi_host_put(ap-host);
- kfree(sector_buf);
}
if (host_set-ops-host_stop)
host_set-ops-host_stop(host_set);
kfree(host_set);
}
/**
*ata_scsi_release - SCSI layer callback hook for host unload
--- 5596,5636
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
Alan Cox [EMAIL PROTECTED] wrote:
Has anyone else reported a problem like this? It requires
non-coherent DMA, and a lack of a cache invalidate instruction, and
one of the drivers that has this problem (it looks like sata_qstor
does too, I haven't looked at others), so maybe that doesn't cover
any other architectures.
Nobody has, not even PA-RISC which is normally guaranteed to make life
miserable in the caching area but I agree entirely with your diagnosis
and that buffer should indeed be marked cache aligned
I can provide a patch if you're interested.
Please do.
Oops, sorry, I left a file out of that diff. Here it is again...
*** drivers/scsi/libata-core.c 2008-02-13 13:34:32.141489000 -0500
--- drivers/scsi/libata-core.c.orig 2008-02-13 13:29:35.62036 -0500
***
*** 5335,5369
}
host = scsi_host_alloc(ent-sht, sizeof(struct ata_port));
if (!host)
return NULL;
host-transportt = ata_scsi_transport_template;
ap = ata_shost_to_port(host);
- ap-sector_buf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
- if (!ap-sector_buf)
- goto err_out;
-
ata_host_init(ap, host, host_set, ent, port_no);
rc = ap-ops-port_start(ap);
if (rc)
goto err_out;
return ap;
err_out:
- if (ap-sector_buf)
- kfree(ap-sector_buf);
scsi_host_put(host);
return NULL;
}
/**
*ata_device_add - Register hardware device with ATA and SCSI layers
*@ent: Probe information describing hardware device to be registered
*
*This function processes the information provided in the probe
*information struct @ent, allocates the necessary ATA and SCSI
--- 5335,5363
***
*** 5602,5645
*Unregister all objects associated with this host set. Free those
*objects.
*
*LOCKING:
*Inherited from calling layer (may sleep).
*/
void ata_host_set_remove(struct ata_host_set *host_set)
{
unsigned int i;
- u8 *sector_buf;
for (i = 0; i host_set-n_ports; i++)
ata_port_detach(host_set-ports[i]);
free_irq(host_set-irq, host_set);
for (i = 0; i host_set-n_ports; i++) {
struct ata_port *ap = host_set-ports[i];
ata_scsi_release(ap-host);
if ((ap-flags ATA_FLAG_NO_LEGACY) == 0) {
struct ata_ioports *ioaddr = ap-ioaddr;
if (ioaddr-cmd_addr == 0x1f0)
release_region(0x1f0, 8);
else if (ioaddr-cmd_addr == 0x170)
release_region(0x170, 8);
}
- sector_buf = ap-sector_buf;
scsi_host_put(ap-host);
- kfree(sector_buf);
}
if (host_set-ops-host_stop)
host_set-ops-host_stop(host_set);
kfree(host_set);
}
/**
*ata_scsi_release - SCSI layer callback hook for host unload
--- 5596,5636
*** include/linux/libata.h 2008-02-13 10:51:39.151202000 -0500
--- include/linux/libata.h.orig 2008-02-13 13:50:26.531572000 -0500
***
*** 550,570
u32 msg_enable;
struct list_headeh_done_q;
wait_queue_head_t eh_wait_q;
pm_message_tpm_mesg;
int *pm_result;
void*private_data;
! u8 *sector_buf; /* owned by EH */
};
struct ata_port_operations {
void (*port_disable) (struct ata_port *);
void (*dev_config) (struct ata_port *, struct ata_device *);
void (*set_piomode) (struct ata_port *, struct ata_device *);
void (*set_dmamode) (struct ata_port *, struct ata_device *);
unsigned long (*mode_filter) (const struct ata_port *, struct
ata_device *, unsigned long);
--- 550,570
u32 msg_enable;
struct list_headeh_done_q;
wait_queue_head_t eh_wait_q;
pm_message_tpm_mesg;
int *pm_result;
void*private_data;
! u8 sector_buf[ATA_SECT_SIZE]; /* owned by EH */
};
struct ata_port_operations {
void (*port_disable) (struct ata_port *);
void (*dev_config) (struct ata_port *, struct ata_device *);
void (*set_piomode) (struct ata_port *, struct ata_device *);
void (*set_dmamode) (struct ata_port *, struct ata_device *);
unsigned long (*mode_filter) (const struct ata_port *, struct
ata_device *, unsigned long);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at
Re: libata/sata_sil24 cache alignment problem?
O I'm counting on kmalloc to return a cache aligned buffer. I found some reason to think it does, but I don't remember offhand what that Its defined to reason was, or if it's configurable per-architecture. The buffer has to be both physically and virtually contiguous, I was tempted to just allocate a page and waste some space but we've got 64K pages, so I'm a bit more sensitive about that. Ok I was expecting a different approach if you mark the field with the magic cacheline_aligned tag after it (ie int foo blah_aligned;) the compiler should align it all for you , which is probably cleaner if it works. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
Alan Cox [EMAIL PROTECTED] wrote:
O I'm counting on kmalloc to return a cache aligned buffer. I found
some reason to think it does, but I don't remember offhand what that
Its defined to
reason was, or if it's configurable per-architecture. The buffer has
to be both physically and virtually contiguous, I was tempted to just
allocate a page and waste some space but we've got 64K pages, so I'm a
bit more sensitive about that.
Ok I was expecting a different approach if you mark the field with the
magic cacheline_aligned tag after it (ie int foo blah_aligned;)
the compiler should align it all for you , which is probably cleaner if
it works.
I hadn't considered that approach due to the way the ata_port is allocated:
libata-core.c:
host = scsi_host_alloc(ent-sht, sizeof(struct ata_port));
hosts.c:
struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int
privsize)
{
shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
}
The ata_port allocation is tacked onto the end of the Scsi_Host
allocation, so the start of ata_port will only be cache aligned if the
end of the Scsi_Host struct is, although that would be easy enough to
fix since it's currently aligned to an unsigned long boundary.
I like that approach better, since it's clearer what the intent is,
and it's easier. Is there any other way that the ata_port struct
might be used that would invalidate this? This one issue is the
extent of my knowledge of libata.
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
I hadn't considered that approach due to the way the ata_port is allocated:
libata-core.c:
host = scsi_host_alloc(ent-sht, sizeof(struct ata_port));
hosts.c:
struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int
privsize)
{
shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
}
The ata_port allocation is tacked onto the end of the Scsi_Host
allocation, so the start of ata_port will only be cache aligned if the
end of the Scsi_Host struct is, although that would be easy enough to
fix since it's currently aligned to an unsigned long boundary.
You are right.
I like that approach better, since it's clearer what the intent is,
and it's easier. Is there any other way that the ata_port struct
might be used that would invalidate this?
I can't think of one. The object lifetime is right - the ata_port is
created before the port buffer is used and destroyed after it is finished
(obviously or embedding it in the struct wouldn't work either)
Alan
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
Alan Cox wrote:
I hadn't considered that approach due to the way the ata_port is allocated:
libata-core.c:
host = scsi_host_alloc(ent-sht, sizeof(struct ata_port));
hosts.c:
struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int
privsize)
{
shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
}
The ata_port allocation is tacked onto the end of the Scsi_Host
allocation, so the start of ata_port will only be cache aligned if the
end of the Scsi_Host struct is, although that would be easy enough to
fix since it's currently aligned to an unsigned long boundary.
You are right.
For recent kernels, ata_port is allocated separately so
cacheline_aligned should be enough.
I like that approach better, since it's clearer what the intent is,
But, yeah, allocating separately is probably safer.
and it's easier. Is there any other way that the ata_port struct
might be used that would invalidate this?
I can't think of one. The object lifetime is right - the ata_port is
created before the port buffer is used and destroyed after it is finished
(obviously or embedding it in the struct wouldn't work either)
I'll prep a patch for the current kernel. Hmmm... This means that any
DMA buffer should be aligned to cacheline. Block layer DMA alignment
helpers should probably take this into consideration and we better add
warnings to dma map functions. This only affects DMA-to-memory, right?
Thanks.
--
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
I tell you what ... find me a parisc box that actually has IDE and we might have told you ... The NS87415 variant IDE has been tested on parisc and didn't blow up - must just be lucky. (actually, the pa8800's have IDE CD's on a cmd640 chip, but that oopses on boot for no reason we've tracked yet). With libata, old IDE or both ? and what does the oops look like ? - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
On Wed, 2008-02-13 at 02:13 +, Alan Cox wrote: I tell you what ... find me a parisc box that actually has IDE and we might have told you ... The NS87415 variant IDE has been tested on parisc and didn't blow up - must just be lucky. Actually, it's only a specific class of machines: the PCX (that's most of the 715,725 series) that are fully incoherent. PCXL hack around this by making the page uncacheable and the 64 bit machines have coherence index handling which fixes this up (mostly). (actually, the pa8800's have IDE CD's on a cmd640 chip, but that oopses on boot for no reason we've tracked yet). With libata, old IDE or both ? and what does the oops look like ? Unfortunately, there's no oops; we get a high priority machine check which means something has gone wrong somewhere and it's not pretty. It's on my todo list to track down ... I just don't have one of these boxes and CDs are hard to play with remotely. James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
Has anyone else reported a problem like this? It requires non-coherent DMA, and a lack of a cache invalidate instruction, and one of the drivers that has this problem (it looks like sata_qstor does too, I haven't looked at others), so maybe that doesn't cover any other architectures. Nobody has, not even PA-RISC which is normally guaranteed to make life miserable in the caching area but I agree entirely with your diagnosis and that buffer should indeed be marked cache aligned I can provide a patch if you're interested. Please do. Alan - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
I wonder if this may be what I am seeing with the Si3124 on my Alpha based setup. I'm not sure if Alpha meets all the criteria, but the thing refuses to recognize any drivers when they are connected and I see what are supposed pci parity failures. maybe not ... ...tom Mark Mason wrote: Hi All, I've implemented the Linux PCI support for a new architecture, and have run into what appears to be a bug in libata, but I don't understand why it wouldn't have been seen on other architectures. The processor is a Tile64, which is a 64 core, 64 bit VLIW processor with non-coherent DMA - DMA transfers bypass the cache, so cache coherence must be maintained manually. I am working with libata v2.0, from Linux 2.6.18, and the sata_sil24 driver for a Silicon Images SIL3531A chip. The problem occurs when the drive's model and serial numbers are read at startup via ata_dev_read_id(). They are read once on driver startup, then when the the error handler first runs it verifies that the model and serial numbers match what was read when the driver started. The problem is with the proximity of the private_data and sector_buf members of the ata_port struct, and the sector_buf not being cache aligned. ata_qc_issue() calls dma_map_single(), which in my case flushes and invalidates the cache for the buffer it's about DMA into (ata_port-sector_buf), then calls sil24_qc_issue(). sil24_qc_issue() then dereferences ata_port-private_data, which causes the cache line containing it to be read in, bringing part of the sector_buf with it, before the DMA transfer has taken place. The Tile64 processor does not have a cache invalidate instruction, only a flush/invalidate, so dma_unmap_single() can't invalidate the portion of the sector_buf that's been inadvertently read in, and I get a serial number and/or model number mismatch, due to part of the buffer being stale. Documentation/DMA-API.txt states that addresses passed to dma_map_single() and dma_unmap_single() must be cache aligned for this reason. Both calls to ata_dev_read_id() request DMA into a non cache-aligned buffer, but only the second call - via ata_dev_same_device() - has a conflict that can cause the cache line to be read back in during the very narrow window for which this vulnerability exists. Has anyone else reported a problem like this? It requires non-coherent DMA, and a lack of a cache invalidate instruction, and one of the drivers that has this problem (it looks like sata_qstor does too, I haven't looked at others), so maybe that doesn't cover any other architectures. I can provide a patch if you're interested. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
