Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 2007-07-16 at 17:03 -0500, James Bottomley wrote: > On Tue, 2007-07-17 at 07:49 +1000, Benjamin Herrenschmidt wrote: > > > No ... that was the point of flush_kernel_dcache_page(). The page in > > > question is page cache backed and contains user mappings. However, the > > > block layer has already done a flush_dcache_page() in get_user_pages() > > > and the user shouldn't be touching memory under I/O (unless they want > > > self induced aliasing problems) so we're free to assume all the user > > > cachelines are purged, hence all we have to do is flush the kernel alias > > > to bring the page up to date and make the users see it correctly. > > > > The block layer will have done that even in the swap-out path ? (Just > > asking... I'm not very familiar with the block layer) > > Er ... not really, this is the I/O path for user initiated I/O. The > page out path, by definition, can't have any extant user mappings. For > page out, the relevant page is flushed before its mapping is detached, > and then it can be paged to the backing store (or for anonymous pages to > the swap device) when no mappings remain. Ok, thanks. Ben. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Tue, 2007-07-17 at 07:49 +1000, Benjamin Herrenschmidt wrote: > > No ... that was the point of flush_kernel_dcache_page(). The page in > > question is page cache backed and contains user mappings. However, the > > block layer has already done a flush_dcache_page() in get_user_pages() > > and the user shouldn't be touching memory under I/O (unless they want > > self induced aliasing problems) so we're free to assume all the user > > cachelines are purged, hence all we have to do is flush the kernel alias > > to bring the page up to date and make the users see it correctly. > > The block layer will have done that even in the swap-out path ? (Just > asking... I'm not very familiar with the block layer) Er ... not really, this is the I/O path for user initiated I/O. The page out path, by definition, can't have any extant user mappings. For page out, the relevant page is flushed before its mapping is detached, and then it can be paged to the backing store (or for anonymous pages to the swap device) when no mappings remain. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
> No ... that was the point of flush_kernel_dcache_page(). The page in > question is page cache backed and contains user mappings. However, the > block layer has already done a flush_dcache_page() in get_user_pages() > and the user shouldn't be touching memory under I/O (unless they want > self induced aliasing problems) so we're free to assume all the user > cachelines are purged, hence all we have to do is flush the kernel alias > to bring the page up to date and make the users see it correctly. The block layer will have done that even in the swap-out path ? (Just asking... I'm not very familiar with the block layer) Ben. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 2007-07-16 at 08:47 -0500, James Bottomley wrote: > > No ... that was the point of flush_kernel_dcache_page(). The page in > question is page cache backed and contains user mappings. However, > the > block layer has already done a flush_dcache_page() in get_user_pages() > and the user shouldn't be touching memory under I/O (unless they want > self induced aliasing problems) so we're free to assume all the user > cachelines are purged, hence all we have to do is flush the kernel > alias > to bring the page up to date and make the users see it correctly. Ok. In our case the problem is not aliases though, it's the coherency between instruction and data caches for pages that may be executed from (such as swapped out text pages). Ben. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
> Upon closer look, while flush_kernel_dcache_page() is a no-op on ppc64, > flush_dcache_page() isn't. So I'd prefer to not call it if not really needed. > > And according to James, flush_kernel_dcache_page() should be sufficient... > > So I'm getting puzzled again... flush_dcache_page() handles icache vs. dcache coherency by clearing the PG_arch_1 bit in the struct page that indicates that the page is cache clean. You -must- call it if you're going to use any form of CPU access to write to the page (basically dirtying the data cache) and that page can be ever mapped into user space and executed from. I don't know what flush_kernel_dcache_page() does and if it needs a similar treatement, it's a new interface, so maybe Jens and or James can tell me more about it.. Cheers, Ben. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 16 Jul 2007, Jens Axboe wrote: > On Mon, Jul 16 2007, James Bottomley wrote: > > On Mon, 2007-07-16 at 14:16 +0200, Jens Axboe wrote: > > > On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > > > > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > > > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > > > > I'll update my patches. Thanks for the comments! > > > > > > > > Does this look OK? > > > > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > > > > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > > > > (ps3rom)) > > > > > > That looks good. > > > > > > > - Add a call to flush_kernel_dcache_page() in routines that write to > > > > buffers > > > > > > Hmm, I would have thought a flush_dcache_page() would be more > > > appropriate, the backing could be page cache pages. > > > > No ... that was the point of flush_kernel_dcache_page(). The page in > > question is page cache backed and contains user mappings. However, the > > block layer has already done a flush_dcache_page() in get_user_pages() > > and the user shouldn't be touching memory under I/O (unless they want > > self induced aliasing problems) so we're free to assume all the user > > cachelines are purged, hence all we have to do is flush the kernel alias > > to bring the page up to date and make the users see it correctly. > > Oh indeed, I missed the flush_dcache_page() in get_user_pages(). Good, thanks for reaching a consensus, so I can update my patch series. With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, Jul 16 2007, James Bottomley wrote: > On Mon, 2007-07-16 at 14:16 +0200, Jens Axboe wrote: > > On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > > > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > > > I'll update my patches. Thanks for the comments! > > > > > > Does this look OK? > > > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > > > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > > > (ps3rom)) > > > > That looks good. > > > > > - Add a call to flush_kernel_dcache_page() in routines that write to > > > buffers > > > > Hmm, I would have thought a flush_dcache_page() would be more > > appropriate, the backing could be page cache pages. > > No ... that was the point of flush_kernel_dcache_page(). The page in > question is page cache backed and contains user mappings. However, the > block layer has already done a flush_dcache_page() in get_user_pages() > and the user shouldn't be touching memory under I/O (unless they want > self induced aliasing problems) so we're free to assume all the user > cachelines are purged, hence all we have to do is flush the kernel alias > to bring the page up to date and make the users see it correctly. Oh indeed, I missed the flush_dcache_page() in get_user_pages(). -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 2007-07-16 at 14:16 +0200, Jens Axboe wrote: > On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > > I'll update my patches. Thanks for the comments! > > > > Does this look OK? > > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > > (ps3rom)) > > That looks good. > > > - Add a call to flush_kernel_dcache_page() in routines that write to > > buffers > > Hmm, I would have thought a flush_dcache_page() would be more > appropriate, the backing could be page cache pages. No ... that was the point of flush_kernel_dcache_page(). The page in question is page cache backed and contains user mappings. However, the block layer has already done a flush_dcache_page() in get_user_pages() and the user shouldn't be touching memory under I/O (unless they want self induced aliasing problems) so we're free to assume all the user cachelines are purged, hence all we have to do is flush the kernel alias to bring the page up to date and make the users see it correctly. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > On Mon, 16 Jul 2007, Geert Uytterhoeven wrote: > > On Mon, 16 Jul 2007, Jens Axboe wrote: > > > On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > > > > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > > > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > > > > I'll update my patches. Thanks for the comments! > > > > > > > > Does this look OK? > > > > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > > > > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > > > > (ps3rom)) > > > > > > That looks good. > > > > > > > - Add a call to flush_kernel_dcache_page() in routines that write to > > > > buffers > > > > > > Hmm, I would have thought a flush_dcache_page() would be more > > > appropriate, the backing could be page cache pages. > > > > OK, I'll change it to flush_dcache_page(). > > Upon closer look, while flush_kernel_dcache_page() is a no-op on ppc64, > flush_dcache_page() isn't. So I'd prefer to not call it if not really needed. > > And according to James, flush_kernel_dcache_page() should be sufficient... > > So I'm getting puzzled again... I'll let James expand on why he thinks flush_kernel_dcache_page() should be sufficient. If the backing is always user memory from get_user_pages(), then the flush_kernel_dcache_page() looks sufficient. For the normal IO paths, it could be that or page cache pages too for instance. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 16 Jul 2007, Geert Uytterhoeven wrote: > On Mon, 16 Jul 2007, Jens Axboe wrote: > > On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > > > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > > > I'll update my patches. Thanks for the comments! > > > > > > Does this look OK? > > > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > > > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > > > (ps3rom)) > > > > That looks good. > > > > > - Add a call to flush_kernel_dcache_page() in routines that write to > > > buffers > > > > Hmm, I would have thought a flush_dcache_page() would be more > > appropriate, the backing could be page cache pages. > > OK, I'll change it to flush_dcache_page(). Upon closer look, while flush_kernel_dcache_page() is a no-op on ppc64, flush_dcache_page() isn't. So I'd prefer to not call it if not really needed. And according to James, flush_kernel_dcache_page() should be sufficient... So I'm getting puzzled again... With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > On Mon, 16 Jul 2007, Jens Axboe wrote: > > On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > > > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > > > I'll update my patches. Thanks for the comments! > > > > > > Does this look OK? > > > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > > > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > > > (ps3rom)) > > > > That looks good. > > > > > - Add a call to flush_kernel_dcache_page() in routines that write to > > > buffers > > > > Hmm, I would have thought a flush_dcache_page() would be more > > appropriate, the backing could be page cache pages. > > OK, I'll change it to flush_dcache_page(). Then you may add my acked-by as well, if you want. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 16 Jul 2007, Jens Axboe wrote: > On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > > I'll update my patches. Thanks for the comments! > > > > Does this look OK? > > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > > (ps3rom)) > > That looks good. > > > - Add a call to flush_kernel_dcache_page() in routines that write to > > buffers > > Hmm, I would have thought a flush_dcache_page() would be more > appropriate, the backing could be page cache pages. OK, I'll change it to flush_dcache_page(). With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, Jul 16 2007, Geert Uytterhoeven wrote: > On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: > > Ah, that explains it. flush_dcache_page() is used in some drivers. > > I'll update my patches. Thanks for the comments! > > Does this look OK? > - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an > interrupt handler, from .request_fn (ps3disk), or from .queuecommand > (ps3rom)) That looks good. > - Add a call to flush_kernel_dcache_page() in routines that write to buffers Hmm, I would have thought a flush_dcache_page() would be more appropriate, the backing could be page cache pages. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
I'm pretty sure that no ppc64 machine needs alias resolution in the kernel, although some are VIPT. Last time we discussed this, Segher explained it to me, but I don't remember which way Cell does it. IIRC, it automatically flushes cache lines that are accessed through aliases. Ah yes, I remember reading about this automatic flushing thing. I don't know how the caches actually work on most of our PPC's, but the fact is, we don't have aliasing issues, so I can safely ignore it for a bit longer :-) That is the very short version of the story, yes: some PowerPC implementations are VIPT physically, but there are no aliasing issues we have to worry about. Anyone interested in how this works, can download the PPC970 UM (or 970FX or 970MP); it has a very detailed explanation of all this. Cell might be slightly different but the base idea is the same. Segher - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, Geert Uytterhoeven wrote:
> Ah, that explains it. flush_dcache_page() is used in some drivers.
> I'll update my patches. Thanks for the comments!
Does this look OK?
- Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an
interrupt handler, from .request_fn (ps3disk), or from .queuecommand
(ps3rom))
- Add a call to flush_kernel_dcache_page() in routines that write to buffers
If this is OK, I'll fold it into my original patch series and will resend.
Thanks!
---
drivers/block/ps3disk.c |5 +++--
drivers/scsi/ps3rom.c |9 +
2 files changed, 8 insertions(+), 6 deletions(-)
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -109,13 +109,14 @@ static void ps3disk_scatter_gather(struc
bio_sectors(bio), sector);
bio_for_each_segment(bvec, bio, j) {
size = bio_cur_sectors(bio)*KERNEL_SECTOR_SIZE;
- buf = __bio_kmap_atomic(bio, j, KM_USER0);
+ buf = __bio_kmap_atomic(bio, j, KM_IRQ0);
if (gather)
memcpy(dev->bounce_buf+offset, buf, size);
else
memcpy(buf, dev->bounce_buf+offset, size);
offset += size;
- __bio_kunmap_atomic(bio, KM_USER0);
+ flush_kernel_dcache_page(bio_iovec_idx(bio,
j)->bv_page);
+ __bio_kunmap_atomic(bio, KM_IRQ0);
}
sectors += bio_sectors(bio);
i++;
--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -112,7 +112,7 @@ static int fill_from_dev_buffer(struct s
active = 1;
for (k = 0, req_len = 0, act_len = 0; k < cmd->use_sg; ++k, ++sgpnt) {
if (active) {
- kaddr = kmap_atomic(sgpnt->page, KM_USER0);
+ kaddr = kmap_atomic(sgpnt->page, KM_IRQ0);
if (!kaddr)
return -1;
len = sgpnt->length;
@@ -121,7 +121,8 @@ static int fill_from_dev_buffer(struct s
len = buflen - req_len;
}
memcpy(kaddr + sgpnt->offset, buf + req_len, len);
- kunmap_atomic(kaddr, KM_USER0);
+ flush_kernel_dcache_page(sgpnt->page);
+ kunmap_atomic(kaddr, KM_IRQ0);
act_len += len;
}
req_len += sgpnt->length;
@@ -149,7 +150,7 @@ static int fetch_to_dev_buffer(struct sc
sgpnt = cmd->request_buffer;
for (k = 0, req_len = 0, fin = 0; k < cmd->use_sg; ++k, ++sgpnt) {
- kaddr = kmap_atomic(sgpnt->page, KM_USER0);
+ kaddr = kmap_atomic(sgpnt->page, KM_IRQ0);
if (!kaddr)
return -1;
len = sgpnt->length;
@@ -158,7 +159,7 @@ static int fetch_to_dev_buffer(struct sc
fin = 1;
}
memcpy(buf + req_len, kaddr + sgpnt->offset, len);
- kunmap_atomic(kaddr, KM_USER0);
+ kunmap_atomic(kaddr, KM_IRQ0);
if (fin)
return req_len + len;
req_len += sgpnt->length;
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone:+32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [EMAIL PROTECTED]
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 16:19 +0200, Arnd Bergmann wrote: > I'm pretty sure that no ppc64 machine needs alias resolution in the kernel, > although some are VIPT. Last time we discussed this, Segher explained it > to me, but I don't remember which way Cell does it. IIRC, it automatically > flushes cache lines that are accessed through aliases. Ah yes, I remember reading about this automatic flushing thing. I don't know how the caches actually work on most of our PPC's, but the fact is, we don't have aliasing issues, so I can safely ignore it for a bit longer :-) There are some aliasing issues with the instruction cache specifically on some 4xx models but that's irrelevant to this discussion (and I think we handle them elsewhere anyway). Ben. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 09:02 -0400, James Bottomley wrote:
> On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
> > + kaddr = kmap_atomic(sgpnt->page, KM_USER0);
> > + if (!kaddr)
> > + return -1;
> > + len = sgpnt->length;
> > + if ((req_len + len) > buflen) {
> > + active = 0;
> > + len = buflen - req_len;
> > + }
> > + memcpy(kaddr + sgpnt->offset, buf + req_len,
> > len);
> > + kunmap_atomic(kaddr, KM_USER0);
>
> This isn't a SCSI objection, but this sequence appears several times in
> this driver. It's wrong for a non-PIPT architecture (and I believe the
> PS3 is VIPT) because you copy into the kernel alias for the page, which
> dirties the line in the cache of that alias (the user alias cache line
> was already invalidated). However, unless you flush the kernel alias to
> main memory, the user could read stale data. The way this is supposed
> to be done is to do a
Nah, we have no cache aliasing on ppc, at least not that matter here and
not on cell.
Ben.
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, James Bottomley wrote:
> On Fri, 2007-07-13 at 17:10 +0200, Geert Uytterhoeven wrote:
> > On Fri, 13 Jul 2007, Arnd Bergmann wrote:
> > > On Friday 13 July 2007, James Bottomley wrote:
> > > > > IC.
> > > > >
> > > > > - flush_kernel_dcache_page() is a no-op on ppc64
> > > > > (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only).
> > > > >
> > > > > - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses
> > > > > a plain
> > > > > kmap/memcpy/kunmap sequence
> > > > >
> > > > > So what should I do?
> > > >
> > > > Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm
> > > > fairly certain PPC is VIPT and will need some kind of alias
> > > > resolution ... perhaps its associative enough not to let the aliases be
> > > > a problem.
> > >
> > > I'm pretty sure that no ppc64 machine needs alias resolution in the
> > > kernel,
> > > although some are VIPT. Last time we discussed this, Segher explained it
> > > to me, but I don't remember which way Cell does it. IIRC, it automatically
> > > flushes cache lines that are accessed through aliases.
> >
> > Thanks for confirming!
> >
> > > It's probably a good idea to have the flush_kernel_dcache_page() in there
> > > anyway, if only to serve as an example for people that copy it into
> > > architecture-independent drivers, same as what we do for the
> > > k{,un}map_atomic() that is also not required on ppc64.
> >
> > Now my next question: why should I add it, if currently no single driver in
> > mainline calls flush_kernel_dcache_page()?
> >
> > `git grep' finds it in the following files only:
> > Documentation/cachetlb.txt
> > arch/parisc/kernel/cache.c
> > arch/parisc/kernel/pacache.S
> > include/asm-parisc/cacheflush.h
> > include/linux/highmem.h
>
> It's a recent addition to the API ... the previous way of doing it was
> flush_dcache_page() but that's expensive and flushes all the user
> aliases. Since you know in this case there are no current user aliases
> and you only need to flush the kernel alias, flush_kernel_dcache_page()
> was introduced as the cheaper alternative.
Ah, that explains it. flush_dcache_page() is used in some drivers.
I'll update my patches. Thanks for the comments!
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone:+32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [EMAIL PROTECTED]
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, Jul 13 2007, Geert Uytterhoeven wrote:
> > It's probably a good idea to have the flush_kernel_dcache_page() in there
> > anyway, if only to serve as an example for people that copy it into
> > architecture-independent drivers, same as what we do for the
> > k{,un}map_atomic() that is also not required on ppc64.
>
> Now my next question: why should I add it, if currently no single driver in
> mainline calls flush_kernel_dcache_page()?
>
> `git grep' finds it in the following files only:
> Documentation/cachetlb.txt
> arch/parisc/kernel/cache.c
> arch/parisc/kernel/pacache.S
> include/asm-parisc/cacheflush.h
> include/linux/highmem.h
Not many drivers fiddle around with stuff like this, it's usually hidden
behind the dma api or in helpers.
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 17:10 +0200, Geert Uytterhoeven wrote:
> On Fri, 13 Jul 2007, Arnd Bergmann wrote:
> > On Friday 13 July 2007, James Bottomley wrote:
> > > > IC.
> > > >
> > > > - flush_kernel_dcache_page() is a no-op on ppc64
> > > > (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only).
> > > >
> > > > - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a
> > > > plain
> > > > kmap/memcpy/kunmap sequence
> > > >
> > > > So what should I do?
> > >
> > > Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm
> > > fairly certain PPC is VIPT and will need some kind of alias
> > > resolution ... perhaps its associative enough not to let the aliases be
> > > a problem.
> >
> > I'm pretty sure that no ppc64 machine needs alias resolution in the kernel,
> > although some are VIPT. Last time we discussed this, Segher explained it
> > to me, but I don't remember which way Cell does it. IIRC, it automatically
> > flushes cache lines that are accessed through aliases.
>
> Thanks for confirming!
>
> > It's probably a good idea to have the flush_kernel_dcache_page() in there
> > anyway, if only to serve as an example for people that copy it into
> > architecture-independent drivers, same as what we do for the
> > k{,un}map_atomic() that is also not required on ppc64.
>
> Now my next question: why should I add it, if currently no single driver in
> mainline calls flush_kernel_dcache_page()?
>
> `git grep' finds it in the following files only:
> Documentation/cachetlb.txt
> arch/parisc/kernel/cache.c
> arch/parisc/kernel/pacache.S
> include/asm-parisc/cacheflush.h
> include/linux/highmem.h
It's a recent addition to the API ... the previous way of doing it was
flush_dcache_page() but that's expensive and flushes all the user
aliases. Since you know in this case there are no current user aliases
and you only need to flush the kernel alias, flush_kernel_dcache_page()
was introduced as the cheaper alternative.
James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, Arnd Bergmann wrote:
> On Friday 13 July 2007, James Bottomley wrote:
> > > IC.
> > >
> > > � - flush_kernel_dcache_page() is a no-op on ppc64
> > > � � (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only).
> > >
> > > � - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a
> > > plain
> > > � � kmap/memcpy/kunmap sequence
> > >
> > > So what should I do?
> >
> > Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm
> > fairly certain PPC is VIPT and will need some kind of alias
> > resolution ... perhaps its associative enough not to let the aliases be
> > a problem.
>
> I'm pretty sure that no ppc64 machine needs alias resolution in the kernel,
> although some are VIPT. Last time we discussed this, Segher explained it
> to me, but I don't remember which way Cell does it. IIRC, it automatically
> flushes cache lines that are accessed through aliases.
Thanks for confirming!
> It's probably a good idea to have the flush_kernel_dcache_page() in there
> anyway, if only to serve as an example for people that copy it into
> architecture-independent drivers, same as what we do for the
> k{,un}map_atomic() that is also not required on ppc64.
Now my next question: why should I add it, if currently no single driver in
mainline calls flush_kernel_dcache_page()?
`git grep' finds it in the following files only:
Documentation/cachetlb.txt
arch/parisc/kernel/cache.c
arch/parisc/kernel/pacache.S
include/asm-parisc/cacheflush.h
include/linux/highmem.h
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone:+32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [EMAIL PROTECTED]
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, Jul 13 2007, Geert Uytterhoeven wrote:
> On Fri, 13 Jul 2007, Jens Axboe wrote:
> > On Fri, Jul 13 2007, James Bottomley wrote:
> > > On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
> > > > + kaddr = kmap_atomic(sgpnt->page, KM_USER0);
> > > > + if (!kaddr)
> > > > + return -1;
> > > > + len = sgpnt->length;
> > > > + if ((req_len + len) > buflen) {
> > > > + active = 0;
> > > > + len = buflen - req_len;
> > > > + }
> > > > + memcpy(kaddr + sgpnt->offset, buf + req_len,
> > > > len);
> > > > + kunmap_atomic(kaddr, KM_USER0);
> > >
> > > This isn't a SCSI objection, but this sequence appears several times in
> > > this driver. It's wrong for a non-PIPT architecture (and I believe the
> > > PS3 is VIPT) because you copy into the kernel alias for the page, which
> > > dirties the line in the cache of that alias (the user alias cache line
> > > was already invalidated). However, unless you flush the kernel alias to
> > > main memory, the user could read stale data. The way this is supposed
> > > to be done is to do a
> > >
> > > flush_kernel_dcache_page(kaddr)
> > >
> > > before doing the kunmap.
> > >
> > > Otherwise it looks OK from the SCSI point of view.
>
> kmap() just returns page_address() on ppc64, as there's no highmem.
> kunmap() is a no-op.
>
> So technically I could just use page_address() directly, but Christoph wanted
> me to keep the kmap()/kunmap() sequence because it's considered a good
> practice.
If you have the kmap sequence there, put the flush in as well. People
copy code, you know... Or put a big comment explaining why it isn't
needed.
> > Well, even worse is that fact that it's using KM_USER0 from interrupt
> > context.
>
> So should I replace it by e.g. KM_IRQ0?
> I'm not so familiar with these parts, and I couldn't find what these values
> really mean.
You corrupt data, using KM_USER0 from interrupt context. So it's a big
flaw right now. Use KM_IRQ0 for code where interrupts are always
disabled.
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Friday 13 July 2007, James Bottomley wrote:
>
> > IC.
> >
> > - flush_kernel_dcache_page() is a no-op on ppc64
> > (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only).
> >
> > - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a
> > plain
> > kmap/memcpy/kunmap sequence
> >
> > So what should I do?
>
> Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm
> fairly certain PPC is VIPT and will need some kind of alias
> resolution ... perhaps its associative enough not to let the aliases be
> a problem.
I'm pretty sure that no ppc64 machine needs alias resolution in the kernel,
although some are VIPT. Last time we discussed this, Segher explained it
to me, but I don't remember which way Cell does it. IIRC, it automatically
flushes cache lines that are accessed through aliases.
It's probably a good idea to have the flush_kernel_dcache_page() in there
anyway, if only to serve as an example for people that copy it into
architecture-independent drivers, same as what we do for the
k{,un}map_atomic() that is also not required on ppc64.
Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 15:45 +0200, Geert Uytterhoeven wrote: > On Fri, 13 Jul 2007, James Bottomley wrote: > > On Fri, 2007-07-13 at 15:25 +0200, Geert Uytterhoeven wrote: > > > kmap() just returns page_address() on ppc64, as there's no highmem. > > > kunmap() is a no-op. > > > > > So technically I could just use page_address() directly, but Christoph > > > wanted > > > me to keep the kmap()/kunmap() sequence because it's considered a good > > > practice. > > > > The point isn't what kmap and kunmap do ... it's the addresses they > > return. By and large, a kernel virtual address for a page is different > > from the user virtual address. If the cache is virtually indexed you > > get different cache lines for the same page ... and that sets you up > > with aliases you need to resolve. parisc is the same ... our > > kmap/kunmap are nops as well, but our kernel virtual addresses are still > > different from the user virtual ones. > > IC. > > - flush_kernel_dcache_page() is a no-op on ppc64 > (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only). > > - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a plain > kmap/memcpy/kunmap sequence > > So what should I do? Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm fairly certain PPC is VIPT and will need some kind of alias resolution ... perhaps its associative enough not to let the aliases be a problem. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, James Bottomley wrote: > On Fri, 2007-07-13 at 15:25 +0200, Geert Uytterhoeven wrote: > > kmap() just returns page_address() on ppc64, as there's no highmem. > > kunmap() is a no-op. > > > So technically I could just use page_address() directly, but Christoph > > wanted > > me to keep the kmap()/kunmap() sequence because it's considered a good > > practice. > > The point isn't what kmap and kunmap do ... it's the addresses they > return. By and large, a kernel virtual address for a page is different > from the user virtual address. If the cache is virtually indexed you > get different cache lines for the same page ... and that sets you up > with aliases you need to resolve. parisc is the same ... our > kmap/kunmap are nops as well, but our kernel virtual addresses are still > different from the user virtual ones. IC. - flush_kernel_dcache_page() is a no-op on ppc64 (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only). - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a plain kmap/memcpy/kunmap sequence So what should I do? With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 15:25 +0200, Geert Uytterhoeven wrote: > kmap() just returns page_address() on ppc64, as there's no highmem. > kunmap() is a no-op. > So technically I could just use page_address() directly, but Christoph > wanted > me to keep the kmap()/kunmap() sequence because it's considered a good > practice. The point isn't what kmap and kunmap do ... it's the addresses they return. By and large, a kernel virtual address for a page is different from the user virtual address. If the cache is virtually indexed you get different cache lines for the same page ... and that sets you up with aliases you need to resolve. parisc is the same ... our kmap/kunmap are nops as well, but our kernel virtual addresses are still different from the user virtual ones. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, Jens Axboe wrote:
> On Fri, Jul 13 2007, James Bottomley wrote:
> > On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
> > > + kaddr = kmap_atomic(sgpnt->page, KM_USER0);
> > > + if (!kaddr)
> > > + return -1;
> > > + len = sgpnt->length;
> > > + if ((req_len + len) > buflen) {
> > > + active = 0;
> > > + len = buflen - req_len;
> > > + }
> > > + memcpy(kaddr + sgpnt->offset, buf + req_len,
> > > len);
> > > + kunmap_atomic(kaddr, KM_USER0);
> >
> > This isn't a SCSI objection, but this sequence appears several times in
> > this driver. It's wrong for a non-PIPT architecture (and I believe the
> > PS3 is VIPT) because you copy into the kernel alias for the page, which
> > dirties the line in the cache of that alias (the user alias cache line
> > was already invalidated). However, unless you flush the kernel alias to
> > main memory, the user could read stale data. The way this is supposed
> > to be done is to do a
> >
> > flush_kernel_dcache_page(kaddr)
> >
> > before doing the kunmap.
> >
> > Otherwise it looks OK from the SCSI point of view.
kmap() just returns page_address() on ppc64, as there's no highmem.
kunmap() is a no-op.
So technically I could just use page_address() directly, but Christoph wanted
me to keep the kmap()/kunmap() sequence because it's considered a good
practice.
> Well, even worse is that fact that it's using KM_USER0 from interrupt
> context.
So should I replace it by e.g. KM_IRQ0?
I'm not so familiar with these parts, and I couldn't find what these values
really mean.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone:+32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [EMAIL PROTECTED]
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, Jul 13 2007, James Bottomley wrote:
> On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
> > + kaddr = kmap_atomic(sgpnt->page, KM_USER0);
> > + if (!kaddr)
> > + return -1;
> > + len = sgpnt->length;
> > + if ((req_len + len) > buflen) {
> > + active = 0;
> > + len = buflen - req_len;
> > + }
> > + memcpy(kaddr + sgpnt->offset, buf + req_len,
> > len);
> > + kunmap_atomic(kaddr, KM_USER0);
>
> This isn't a SCSI objection, but this sequence appears several times in
> this driver. It's wrong for a non-PIPT architecture (and I believe the
> PS3 is VIPT) because you copy into the kernel alias for the page, which
> dirties the line in the cache of that alias (the user alias cache line
> was already invalidated). However, unless you flush the kernel alias to
> main memory, the user could read stale data. The way this is supposed
> to be done is to do a
>
> flush_kernel_dcache_page(kaddr)
>
> before doing the kunmap.
>
> Otherwise it looks OK from the SCSI point of view.
Well, even worse is that fact that it's using KM_USER0 from interrupt
context.
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
> + kaddr = kmap_atomic(sgpnt->page, KM_USER0);
> + if (!kaddr)
> + return -1;
> + len = sgpnt->length;
> + if ((req_len + len) > buflen) {
> + active = 0;
> + len = buflen - req_len;
> + }
> + memcpy(kaddr + sgpnt->offset, buf + req_len,
> len);
> + kunmap_atomic(kaddr, KM_USER0);
This isn't a SCSI objection, but this sequence appears several times in
this driver. It's wrong for a non-PIPT architecture (and I believe the
PS3 is VIPT) because you copy into the kernel alias for the page, which
dirties the line in the cache of that alias (the user alias cache line
was already invalidated). However, unless you flush the kernel alias to
main memory, the user could read stale data. The way this is supposed
to be done is to do a
flush_kernel_dcache_page(kaddr)
before doing the kunmap.
Otherwise it looks OK from the SCSI point of view.
James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
