Re: [Intel-gfx] Ugly patches for stolen reservation
On Thu, 25 Jul 2013 20:31:27 -0700 H. Peter Anvin h...@zytor.com wrote: On 07/25/2013 07:14 PM, Jesse Barnes wrote: To clarify: it'll either be marked reserved or not listed at all in e820, which is why I did this early, before any other e820 stuff like the RAM buffer are allocated, and before we could use the iomem resource (or maybe we could even early per Linus? I'll check). Jesse If it is marked reserved or not listed at all it is much less of an issue. Reserved is in fact the correct thing; not listed at all really isn't very problematic in most cases. Yeah the problems seem to fall into two categories: 1) mmio space is allocated in this range: https://bugs.freedesktop.org/show_bug.cgi?id=66726 2) range gets partially allocated to the RAM buffer https://bugs.freedesktop.org/show_bug.cgi?id=66844 Case (1) is the one that worries me. I'm guessing it'll mainly be a problem on machines where MMIO space is limited or somehow structured such that PCI resources end up there when we allocate them. Depending on what gets put there and the decode priority, behavior may be poor. Case (2) isn't harmful, but ends up causing our driver to skip stolen memory initialization, because of the conflict. Anyway I'll look at Linus's suggestion of reserving in the iomem resource really early and roll in Chris's stuff if that doesn't work out. Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On Thu, 25 Jul 2013 17:31:29 -0700 Linus Torvalds torva...@linux-foundation.org wrote: On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin h...@zytor.com wrote: So the bootloader is just as likely to step on things... what happens when/if it does? This isn't a new problem. We've had this firmware tables don't show all devices issue before. The only odd thing about this one is how the quirk in question uses e820_add_region() instead of just adding things to the MMIO list. And I think that's actually likely a mistake. So Jesse, why don't you do what the other quirks do, and claim an actual MMIO resource? If you make it a real resource, you'll get to use fancy things like REAL NAMES, and actually document it. With human-readable strings. See quirk_io_region() in drivers/pci/quirks.c for example. The same code except for IORESOURCE_MEM should do a lovely job.. And even *if* it's already marked reserved in the e820 table, it just looks nice in /proc/iomem. Hmm? I should have mentioned yesterday that we *do* try to reserve the resource in our driver init, with pretty name and everything. The issue here is making sure we are actually *able* to reserve it without conflicts at driver init time. If the PCI layer has put something there (misc MMIO or the RAM buffer intended to prevent stuff like this) because it's not listed in the E820 map, we'll fail to get at this memory in our driver init. Thus the early reservation in early-quirks, followed by a real request_mem_region later on. Doing the request_mem_region before the PCI layer gets its hands on it isn't really possible, because __request_region depends on the memory allocator being initialized. So rather than add a new hook elsewhere in setup_arch or start_kernel I figured I'd use an early quirk. Reasonable? Note iomem in both cases. Before (RAM buffer prevents our allocation): cafff000-caff : System RAM cb00-cbff : RAM buffer cfa0-feaf : PCI Bus :00 d000-dfff : :00:02.0 After (yay): cb00-cb9f : RAM buffer cba0-cf9f : reserved cba0-cf9f : Graphics Stolen Memory cfa0-feaf : PCI Bus :00 Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
So the bootloader is just as likely to step on things... what happens when/if it does? Ingo Molnar mi...@kernel.org wrote: * Jesse Barnes jbar...@virtuousgeek.org wrote: Patch 2/2 has the description, but suffice it to say I'm not really pleased with this, though it does solve a problem we have. On some machines, we get MMIO space allocated on top of this hidden memory, which can cause problems. I'm not sure if there are similar problems for other hunks of the address space; if so it's possible this could be made more general (though the bits for looking up the address of this region are definitely Intel graphics specific). It looks pretty hardware specific. Discovering it the hard way and marking it e820 reserved in an early quirk is what the firmware should have done to begin with - and I doubt the kernel could do anything significantly cleaner. How does Windows manage to not crash? By luckily never allocating PCI resources on top of the RAM? Or does it have a quirk? Chris has some patches on top to add a new E820 type so we can look up the region later, which removes some redundant code in the i915 driver at least. Any comments? I assume no one likes this, but maybe it's just another early quirk we'll have to live with... No strong feelings against it - my only suggestion would be to make this more visible - right now it's added as e820 reserved which hides amongst other areas already marked reserved - would a low-key printk() of the range added make it more apparent that a kernel quirk activated here? Just so that people know that it came from the kernel, not the firmware. But in any case: Acked-by: Ingo Molnar mi...@kernel.org Thanks, Ingo -- Sent from my mobile phone. Please excuse brevity and lack of formatting. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On 07/25/2013 05:31 PM, Linus Torvalds wrote: On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin h...@zytor.com wrote: So the bootloader is just as likely to step on things... what happens when/if it does? This isn't a new problem. We've had this firmware tables don't show all devices issue before. Yes, I just want to know what happens. The only odd thing about this one is how the quirk in question uses e820_add_region() instead of just adding things to the MMIO list. And I think that's actually likely a mistake. So Jesse, why don't you do what the other quirks do, and claim an actual MMIO resource? If you make it a real resource, you'll get to use fancy things like REAL NAMES, and actually document it. With human-readable strings. See quirk_io_region() in drivers/pci/quirks.c for example. The same code except for IORESOURCE_MEM should do a lovely job.. And even *if* it's already marked reserved in the e820 table, it just looks nice in /proc/iomem. We should do both -- mark it reserved in early boot, and add it as an MMIO region later during boot. The problem here, if I'm reading this right, is that this memory region is marked as normal RAM in e820, which is much worse than just not marking it as reserved; we need to intercept this memory before we genuinely turn it into normal RAM. At the same time, we have no protection against the bootloader using this as memory or even placing the kernel there. The BIOS needs to be fixed regardless of what workarounds we do in the kernel. -hpa ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On 07/25/2013 04:17 PM, Jesse Barnes wrote: Well, it's ok if the boot loader writes to this memory, the worst that'll happen is you'll see garbage on the screen. If the boot loader tries to do MMIO mapping on top it'll get into trouble... but why would it do that? Jesse Much worse: it could be hunting for a place to put the kernel, and put it there. -hpa ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On 07/25/2013 07:14 PM, Jesse Barnes wrote: To clarify: it'll either be marked reserved or not listed at all in e820, which is why I did this early, before any other e820 stuff like the RAM buffer are allocated, and before we could use the iomem resource (or maybe we could even early per Linus? I'll check). Jesse If it is marked reserved or not listed at all it is much less of an issue. Reserved is in fact the correct thing; not listed at all really isn't very problematic in most cases. -hpa ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On Thu, Jul 25, 2013 at 05:48:42PM -0700, H. Peter Anvin wrote: On 07/25/2013 05:31 PM, Linus Torvalds wrote: On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin h...@zytor.com wrote: So the bootloader is just as likely to step on things... what happens when/if it does? This isn't a new problem. We've had this firmware tables don't show all devices issue before. Yes, I just want to know what happens. The only odd thing about this one is how the quirk in question uses e820_add_region() instead of just adding things to the MMIO list. And I think that's actually likely a mistake. So Jesse, why don't you do what the other quirks do, and claim an actual MMIO resource? If you make it a real resource, you'll get to use fancy things like REAL NAMES, and actually document it. With human-readable strings. See quirk_io_region() in drivers/pci/quirks.c for example. The same code except for IORESOURCE_MEM should do a lovely job.. And even *if* it's already marked reserved in the e820 table, it just looks nice in /proc/iomem. We should do both -- mark it reserved in early boot, and add it as an MMIO region later during boot. We started to reserve this range in drm/i915 (and still do), that's why we've noticed that something is amiss. The problem here, if I'm reading this right, is that this memory region is marked as normal RAM in e820, which is much worse than just not marking it as reserved; we need to intercept this memory before we genuinely turn it into normal RAM. I haven't seen a case where it's marked as ram, but many systems don't mark it as reserved. Then the pci code can go around and put a bar into the middle of that range. The other issue (benign but ugly) is that the RAM buffer (used to protect stolen memory without knowing where exactly it is because it's not properly reserved in the e820 map) sometimes ends up in the graphics stolen range. With the new iomeme reserve code in drm/i915 we've stopped using stolen in such cases to avoid surprises, but that kills a few neat features. Thus far we haven't tracked down any bugs to such overlaps yet, but otoh we don't use stolen all that much yet. But we have plans to put it to real use (and use the full range) and I'd like to get the reservation conflicts sorted out before we do so. At the same time, we have no protection against the bootloader using this as memory or even placing the kernel there. The BIOS needs to be fixed regardless of what workarounds we do in the kernel. Like I've said we haven't seen it earmarked as ram anywhere yet, so I don't think this is something we need to concern ousrselves with. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On 07/26/2013 01:24 PM, Ingo Molnar wrote: Am I being too pedantic in expecting that we still mark it e820-reserved? This area really isnt an ordinary PCI resource such as a BAR or an MMIO region. It's truly system RAM (which cannot be moved/reallocated), used by graphics hardware, and the firmware should have marked it reserved. (The end result should be the same in any case, so this is a detail.) I, too, would prefer to see it marked as reserved. -hpa ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Ugly patches for stolen reservation
Patch 2/2 has the description, but suffice it to say I'm not really pleased with this, though it does solve a problem we have. On some machines, we get MMIO space allocated on top of this hidden memory, which can cause problems. I'm not sure if there are similar problems for other hunks of the address space; if so it's possible this could be made more general (though the bits for looking up the address of this region are definitely Intel graphics specific). Chris has some patches on top to add a new E820 type so we can look up the region later, which removes some redundant code in the i915 driver at least. Any comments? I assume no one likes this, but maybe it's just another early quirk we'll have to live with... Thanks, Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On Thu, 25 Jul 2013 22:05:51 +0200 Ingo Molnar mi...@kernel.org wrote: * Jesse Barnes jbar...@virtuousgeek.org wrote: Patch 2/2 has the description, but suffice it to say I'm not really pleased with this, though it does solve a problem we have. On some machines, we get MMIO space allocated on top of this hidden memory, which can cause problems. I'm not sure if there are similar problems for other hunks of the address space; if so it's possible this could be made more general (though the bits for looking up the address of this region are definitely Intel graphics specific). It looks pretty hardware specific. Discovering it the hard way and marking it e820 reserved in an early quirk is what the firmware should have done to begin with - and I doubt the kernel could do anything significantly cleaner. How does Windows manage to not crash? By luckily never allocating PCI resources on top of the RAM? Or does it have a quirk? Pretty sure Windows doesn't allocate MMIO space the same way we do, so doesn't run into this on platforms where it's not E820_RESERVED. On top of that, BIOS vendors probably just move things around until Windows boots and the device manager doesn't have any dreaded yellow bang icons that would prevent them from getting their designed for windows sticker. Chris has some patches on top to add a new E820 type so we can look up the region later, which removes some redundant code in the i915 driver at least. Any comments? I assume no one likes this, but maybe it's just another early quirk we'll have to live with... No strong feelings against it - my only suggestion would be to make this more visible - right now it's added as e820 reserved which hides amongst other areas already marked reserved - would a low-key printk() of the range added make it more apparent that a kernel quirk activated here? Sounds good, I think Chris's patches should satisfy there. They make it a new E820 type so it's clear in /proc/iomem too. Just so that people know that it came from the kernel, not the firmware. But in any case: Acked-by: Ingo Molnar mi...@kernel.org Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On Thu, Jul 25, 2013 at 01:16:48PM -0700, Jesse Barnes wrote: On Thu, 25 Jul 2013 22:05:51 +0200 Ingo Molnar mi...@kernel.org wrote: Chris has some patches on top to add a new E820 type so we can look up the region later, which removes some redundant code in the i915 driver at least. Any comments? I assume no one likes this, but maybe it's just another early quirk we'll have to live with... No strong feelings against it - my only suggestion would be to make this more visible - right now it's added as e820 reserved which hides amongst other areas already marked reserved - would a low-key printk() of the range added make it more apparent that a kernel quirk activated here? Sounds good, I think Chris's patches should satisfy there. They make it a new E820 type so it's clear in /proc/iomem too. I think it'd be good to get it all in in one go, since with Chris' patches i915 will also use the detection logic from the quirk code, so more testing for it. And imo the i915 cleanups shouldn't conflict really with ongoing work in drm/i915, so could all go in through x86 trees. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
Well, it's ok if the boot loader writes to this memory, the worst that'll happen is you'll see garbage on the screen. If the boot loader tries to do MMIO mapping on top it'll get into trouble... but why would it do that? Jesse On Thu, 25 Jul 2013 15:42:25 -0700 H. Peter Anvin h...@zytor.com wrote: So the bootloader is just as likely to step on things... what happens when/if it does? Ingo Molnar mi...@kernel.org wrote: * Jesse Barnes jbar...@virtuousgeek.org wrote: Patch 2/2 has the description, but suffice it to say I'm not really pleased with this, though it does solve a problem we have. On some machines, we get MMIO space allocated on top of this hidden memory, which can cause problems. I'm not sure if there are similar problems for other hunks of the address space; if so it's possible this could be made more general (though the bits for looking up the address of this region are definitely Intel graphics specific). It looks pretty hardware specific. Discovering it the hard way and marking it e820 reserved in an early quirk is what the firmware should have done to begin with - and I doubt the kernel could do anything significantly cleaner. How does Windows manage to not crash? By luckily never allocating PCI resources on top of the RAM? Or does it have a quirk? Chris has some patches on top to add a new E820 type so we can look up the region later, which removes some redundant code in the i915 driver at least. Any comments? I assume no one likes this, but maybe it's just another early quirk we'll have to live with... No strong feelings against it - my only suggestion would be to make this more visible - right now it's added as e820 reserved which hides amongst other areas already marked reserved - would a low-key printk() of the range added make it more apparent that a kernel quirk activated here? Just so that people know that it came from the kernel, not the firmware. But in any case: Acked-by: Ingo Molnar mi...@kernel.org Thanks, Ingo -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin h...@zytor.com wrote: So the bootloader is just as likely to step on things... what happens when/if it does? This isn't a new problem. We've had this firmware tables don't show all devices issue before. The only odd thing about this one is how the quirk in question uses e820_add_region() instead of just adding things to the MMIO list. And I think that's actually likely a mistake. So Jesse, why don't you do what the other quirks do, and claim an actual MMIO resource? If you make it a real resource, you'll get to use fancy things like REAL NAMES, and actually document it. With human-readable strings. See quirk_io_region() in drivers/pci/quirks.c for example. The same code except for IORESOURCE_MEM should do a lovely job.. And even *if* it's already marked reserved in the e820 table, it just looks nice in /proc/iomem. Hmm? Linus ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Ugly patches for stolen reservation
To clarify: it'll either be marked reserved or not listed at all in e820, which is why I did this early, before any other e820 stuff like the RAM buffer are allocated, and before we could use the iomem resource (or maybe we could even early per Linus? I'll check). Jesse -- Jesse Barnes, Intel Open Source Technology Center Original message From: H. Peter Anvin h...@zytor.com Date: 07/25/2013 5:49 PM (GMT-08:00) To: Jesse Barnes jbar...@virtuousgeek.org Cc: Ingo Molnar mi...@kernel.org,intel-gfx@lists.freedesktop.org,linux-ker...@vger.kernel.org,mi...@elte.hu,t...@linutronix.de,torva...@linux-foundation.org Subject: Re: Ugly patches for stolen reservation On 07/25/2013 04:17 PM, Jesse Barnes wrote: Well, it's ok if the boot loader writes to this memory, the worst that'll happen is you'll see garbage on the screen. If the boot loader tries to do MMIO mapping on top it'll get into trouble... but why would it do that? Jesse Much worse: it could be hunting for a place to put the kernel, and put it there. -hpa ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
