22:54 -!- Irssi: Starting query in freenode with ickle
22:55 <jekstrand> From you on bugzilla:
22:55 <jekstrand> Any new bo returned to userspace is zeroed. That data may entirely reside in the CPU cache, and may remain there across the GPU write into main memory. The 
                  clflush performed before the read to invalidate the cache may in fact just cause the writeback of the cache to main memory, overwriting the results.
22:55 <jekstrand>  
22:55 <jekstrand> If this is true then I think we have a semi-major security hole.
22:57 <jekstrand> If the zeroing really does live in the CPU cache then it's possible for a client to create two BOs, blit from one to the other without updating the source 
                  BO's domain, and the destination BO will now contain a copy of the contents of the source BO's pages pre-zero.
22:57 <jekstrand> We can't trust the client's domain tracking.
22:58 <jekstrand> The only option is to either clflush or do a non-temporal zero operation to ensure that the physical pages are actually zeroed before we ever let a client 
                  touch it.
Day changed to 07 Jul 2017
02:14 <ickle> if you let the kernel do its thing, then nope.
02:15 <ickle> if you take over, then you need to clflush yourself, even if we added a new create ioctl to return a coherent buffer, there's still the old api hanging around
09:05 <jekstrand> If we have any uapi that allows a client to scrape truely undefined contents from memory, it's a kernel bug.
09:06 <jekstrand> It doesn't matter that userspace isn't "letting the kernel do its thing"
09:06 <ickle> it doesn't.
09:06 <ickle> if you use WC, the physical tags align with the cache
09:06 <ickle> if you use GTT, first access forces the clflush
09:08 <ickle> there are further problems with speculative CPU caching
09:10 <ickle> the recent issue is ASYNC, and the choice is between never allowing explicit fencing or assuming the userspace driver is doing its part
09:10 <jekstrand> I'm confused.  If the reason why stuff started failing in Vulkan was because the kernel's zeroing of memory got flushed out *after* the GPU rendered to it, 
                  then what's to prevent the gpu from reading it *before* the zeroing gets flushed to main memory?
09:27 <ickle> http://paste.debian.net/975349/
09:29 <jekstrand> Ugh...
09:29 <jekstrand> I'm fine with taking responsibility for synchronization of CPU and GPU.
09:30 <jekstrand> What i'm not fine with is the kernel having an API that lets me read other processes pages.
09:30 <jekstrand> A simple clflush on BO alloc is suffucient.
09:31 <ickle> but when? we either scrap ASYNC (either completely or add a check) or you do a set-domain on create and leave the kernel completely ignorant
09:32 <jekstrand> Ok, maybe I don't understand what that patch does.
09:33 <jekstrand> Maybe the right thing to do is just implicitly move to GTT whenever ASYNC is set.
09:33 <jekstrand> But that seems a little weird
09:33 <ickle> that the normal mode, but we use an async task with a fence
09:34 <ickle> which is where the fun with using ASYNC to ignore all those implicit fences begins
09:34 <jekstrand> Ok, let me make sure I have this straight.  The kernel does a CPU write to a user's BO in one of 3 circumstances:  initial zeroing, relocations, and pwrite.
09:35 <ickle> swapping
09:35 <jekstrand> Ok, 4.
09:35 <jekstrand> I hadn't thought about swapping but that's good to mention.
09:35 <ickle> and only relocations if the bo is in the CPU domain
09:35 <jekstrand> Right
09:36 <ickle> pwrite is also only if in CPU domain
09:36 <jekstrand> Otherwise it uses a GTT or WB map?
09:36 <ickle> GTT
09:36 <jekstrand> k
09:36 <ickle> (plonking a page into the GTT is actually cheaper than creating a WC mapping for a page)
09:37 <jekstrand> Next question:  Under what circumstances does the BO domain change?
09:37 <jekstrand> Can the kernel change it out from under me?  Can some other userspace with which I've shared the buffer change it?
09:37 <jekstrand> Also, what's the difference between GTT domain and RENDER/SAMPLER?
09:37 <ickle> it a single domain for all userspace, so shared buffers are a nightmare
09:38 <ickle> the only time it can change from !userspace action is random swapping
09:38 <jekstrand> Which changes it to CPU domain?
09:38 <ickle> there's a small difference, in that GTT writes are not fully coherent
09:38 <ickle> yes, swapping -> CPU
09:39 <jekstrand> In what way are GTT writes not fully cohereng?
09:39 <jekstrand> *coherent
09:39 <ickle> the domains are effective CPU (in cpu cache), GTT (in chipset buffering), NONE
09:39 <jekstrand> Ok
09:40 <ickle> if you do a write through GTT it takes a finite amount of time to land in the backing page, during which you can still read the old result
09:40 <jekstrand> Makes sense.
09:40 <ickle> (we used to assume that was impossible, but modern Atoms)
09:40 <jekstrand> So if you write through GTT and then try to read through CPU you may race.
09:40 <ickle> yes, and also possible via the GPU
09:41 <ickle> but we make sure we always hammer before the execbuf
09:41 <jekstrand> Ok
09:43 <jekstrand> Given the above conversation, I think I'm now convinced that we can't just do a single set_domain because some other process may have moved stuff around or 
                  into the CPU domain.
09:43 <jekstrand> Or the kernel may have swapped it
09:44 <jekstrand> So we need to set the GTT domain every time we kick it off to the kernel.
09:44 <ickle> true, it's definitely the safe thing to do, my goal is simply not doing anything we don't need inside the kernel
09:44 <jekstrand> Agreed.  The kernel should do as little as possible to ensure correctness.
09:45 <jekstrand> One (somewhat terrible) option would be to just clflush on BO creation.
09:45 <jekstrand> That would provide the needed safety guarantees and still let us avoid the tracking.
09:46 <ickle> no, there's the swapping argument again
09:46 <jekstrand> One day in the future, we can zero our own pages and just use a non-temporal zero instruction.
09:46 <jekstrand> Oh...
09:46 <ickle> userspace simply doesn't know when the kernel touched it in that scenario
09:46 <jekstrand> Right, so the kernel either needs to do some implicit domain tracking or else use non-temporal moves to swap it in.
09:47 <jekstrand> Which I wouldn't be surprised if that's impossible in the kernel architecture.
09:47 <ickle> nothing is impossible, but that would be very hard to get in
09:47 <jekstrand> Right
09:47 <jekstrand> Easier to just notice the swap-in and mark it as CPU domain?
09:49 <jekstrand> In that case, I think the best thing to do is probably to take anything that's ASYNC and in the CPU domain and implicitly move it to the GTT domain.
09:49 <jekstrand> And userspace will just have to be aware that it should always use GTT domain if it wants ASYNC.
09:49 <jekstrand> Well, GTT, or WB.
09:49 <jekstrand> But not CPU.
09:50 <jekstrand> In other words, I think the fundamental problem is that ASYNC + GEM_DOMAIN_CPU don't mix.
09:51 <ickle> yup, that's what we have to do
09:51 <ickle> check everything on execbuf and cancel the async request if something has happened to the bo
09:51 <jekstrand> And we (userspace) will just always set the domain to GTT and just leave it there.
09:52 <jekstrand> Which brings up one more question:  How does this all work with async maps?
09:52 <jekstrand> I think it's probably safe.  Here's why:
09:54 <jekstrand>  1) If we map through the CPU cache and clflush like anv does then the BO accidentally being in the CPU domain due to a swap is completely ok because the 
                  worst that happens is that we (anv) clflush it on vkFlushMappedMemoryRange and then the kernel clflushes it again to move it to the GTT domain.
09:54 <jekstrand>  2) If we map it through the GTT then the kernel will move it back to the GTT domain when it faults in the pages for the GTT read/write?
09:54 <jekstrand>  3) If we map WC, I have no idea...
09:55 <ickle> WC should just work, because the CPU guys tell me that cachelines are tracked on physical tags
09:55 <ickle> GTT is touch and go
09:55 <ickle> first fault is fine
09:55 <ickle> post-swap fault is fine, since we had to evict it from the GTT for swapping
09:56 <ickle> so that is fine from the anv perspective
09:56 <ickle> two clients one reading from GTT, the other writing via the CPU, may run into problems
09:57 <ickle> (but equally will likely run into problems with set-domain being a one sided barrier anyway)
09:57 <jekstrand> Yeah
09:57 <jekstrand> If two clients are trying to access it through GTT/CPU, you're in sketchy territory anyway
09:58 <jekstrand> In that case, I would expect that the CPU writing client needs to either clflush or move the BO to GTT domain prior to the other client reading or risk 
                  undefined results.
09:58 <jekstrand> But there's not much we can do about that.
10:05 <jekstrand> Ok, I think I'm convinced that your patch is correct though it could probably use a slightly less passive-agressive commit message. :-)
10:06 <ickle> definitely, rewrote it in terms of swapping and its urgency is then apparent
10:06 <jekstrand> Cool.
10:07 <jekstrand> I'll get anv updated to just always use GTT domain.
10:07 <ickle> oh, for passing to relocs. just set RENDER
10:07 <jekstrand> ?
10:07 <ickle> I think we do check for !GPU_DOMAINS and complain
10:08 <jekstrand> So I should put batches in RENDER domain?
10:08 <jekstrand> or INSTRUCTION I guess would be more appropriate.
10:08 <ickle> sure, we don't really care
10:08 <jekstrand> But I shouldn't put it in GTT?
10:08 <ickle> inside it boils down to a single bit of whether you write to the bo
10:09 <ickle>         if (unlikely((reloc->write_domain | reloc->read_domains)
10:09 <ickle>                      & ~I915_GEM_GPU_DOMAINS)) {
10:09 <ickle> return -EINVAL
10:09 <jekstrand> Gotcha
10:09 <jekstrand> So should I just put everything in RENDER domain?
10:10 <jekstrand> Or is GTT better for something?
10:10 <ickle> there's a complication with gen6 pipecontrols
10:10 <ickle> (do you care about those?)
10:10 <jekstrand> For anv, I'm not liable to care about gen6 pipe controls. :-)
10:10 <jekstrand> What's the complication?
10:10 <ickle> they only used GGTT addresses even though we are using ppgtt
10:11 <ickle> it's easiest to set execobject.flags |= EXEC_OBJECT_NEEDS_GTT
10:11 <ickle> but the older method was to use reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION
10:11 <jekstrand> Ah
10:12 <jekstrand> Sounds like I should just set write_domain = RENDER for everything and call it a day.
10:13 <jekstrand> Unless that will cause the kernel to do piles of extra flushing
10:14 <ickle> no, we haven't done gpu cache tracking with those domains for ~10 years
10:14 <ickle> we just f;ush everthing at the end of the batch (and inva;date before evry batch)
10:20 <jekstrand> Reading a bit, it seems safer to just set read_domain
10:23 <ickle> inded, since you can rely on the kernel you use support EXEC_OBJECT_WRITE, it doesn't need reloc.write_domain at all
10:35 <jekstrand> If I set NO_RELOC, will the kernel still walk the relocation list to gather up domains?
10:39 <jekstrand> Wait, never mind...
10:39 <jekstrand> As discussed above, the kernel will automatically move it to some GPU domain if ASYNC is set.
10:40 <jekstrand> But what if ASYNC isn't set...
10:48 <jekstrand> Hrm... I think I just managed to convince myself that userspace doesn't need to do anything with domains.
10:49 <jekstrand> The two things that we need to set domains for, as far as I can tell are implicit fences for X11 and relocations.
10:51 <jekstrand> However, fences are taken care of by EXEC_OBJECT_WRITE
10:51 <jekstrand> For relocations, vma_move_to_active automatically does Iobj->base.read_domains |= I915_GEM_GPU_DOMAINS;
10:52 <jekstrand> so the only time the BO is in the CPU domain is if it's inactive in which case the kernel will do the relocation in the CPU domain and then clflush as part of eb_move_to_gpu.