Re: [Intel-gfx] [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
..snip.. > > > I think the main question I have is how would you like to see patches for > > > 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? > > > > Yes that would be perfect. If there are any dependencies on the rc1, I > > can rebase it on top of that. > > Yes, please, rebasing would be very helpful. The broader rework of > 'io_tlb_default_mem' is going to conflict quite badly otherwise. There is a devel/for-linus-5.15 (based on v5.14-rc1) now. Thank you! > > Cheers, > > Will ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Tue, Jul 06, 2021 at 05:57:21PM +0100, Will Deacon wrote: > On Tue, Jul 06, 2021 at 10:46:07AM -0400, Konrad Rzeszutek Wilk wrote: > > On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > > FWIW I was pondering the question of whether to do something along > > > > those > > > > lines or just scrap the default assignment entirely, so since I hadn't > > > > got > > > > round to saying that I've gone ahead and hacked up the alternative > > > > (similarly untested) for comparison :) > > > > > > > > TBH I'm still not sure which one I prefer... > > > > > > Claire did implement something like your suggestion originally, but > > > I don't really like it as it doesn't scale for adding multiple global > > > pools, e.g. for the 64-bit addressable one for the various encrypted > > > secure guest schemes. > > > > Couple of things: > > - I am not pushing to Linus the Claire's patchset until we have a > >resolution on this. I hope you all agree that is a sensible way > >forward as much as I hate doing that. > > Sure, it's a pity but we could clearly use a bit more time to get these > just right and we've run out of time for 5.14. > > I think the main question I have is how would you like to see patches for > 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? Yes that would be perfect. If there are any dependencies on the rc1, I can rebase it on top of that. > > Cheers, > > Will ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > FWIW I was pondering the question of whether to do something along those > > lines or just scrap the default assignment entirely, so since I hadn't got > > round to saying that I've gone ahead and hacked up the alternative > > (similarly untested) for comparison :) > > > > TBH I'm still not sure which one I prefer... > > Claire did implement something like your suggestion originally, but > I don't really like it as it doesn't scale for adding multiple global > pools, e.g. for the 64-bit addressable one for the various encrypted > secure guest schemes. Couple of things: - I am not pushing to Linus the Claire's patchset until we have a resolution on this. I hope you all agree that is a sensible way forward as much as I hate doing that. - I like Robin's fix as it is simplest looking. Would love to see if it does fix the problem. - Christopher - we can always add multiple pools as the next milestone and just focus on this feature getting tested extensively during this release. - Would it be worth (for future or maybe in another tiny fix) to also add a printk in swiotlb when we de-allocate the buffer so when someone looks through the `dmesg` it becomes much easier to diagnose issues? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Thu, Jun 24, 2021 at 11:58:57PM +0800, Claire Chang wrote: > On Thu, Jun 24, 2021 at 11:56 PM Konrad Rzeszutek Wilk > wrote: > > > > On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote: > > > > > > > > > On 6/24/2021 7:48 AM, Will Deacon wrote: > > > > Ok, diff below which attempts to tackle the offset issue I mentioned as > > > > well. Qian Cai -- please can you try with these changes? > > > > > > This works fine. > > > > Cool. Let me squash this patch in #6 and rebase the rest of them. > > > > Claire, could you check the devel/for-linus-5.14 say by end of today to > > double check that I didn't mess anything up please? > > I just submitted v15 here > (https://lore.kernel.org/patchwork/cover/1451322/) in case it's > helpful. Oh! Nice! > I'll double check of course. Thanks for the efforts! I ended up using your patch #6 and #7. Please double-check. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v15 00/12] Restricted DMA
On Thu, Jun 24, 2021 at 11:55:14PM +0800, Claire Chang wrote: > This series implements mitigations for lack of DMA access control on > systems without an IOMMU, which could result in the DMA accessing the > system memory at unexpected times and/or unexpected addresses, possibly > leading to data leakage or corruption. > > For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is > not behind an IOMMU. As PCI-e, by design, gives the device full access to > system memory, a vulnerability in the Wi-Fi firmware could easily escalate > to a full system exploit (remote wifi exploits: [1a], [1b] that shows a > full chain of exploits; [2], [3]). > > To mitigate the security concerns, we introduce restricted DMA. Restricted > DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a > specially allocated region and does memory allocation from the same region. > The feature on its own provides a basic level of protection against the DMA > overwriting buffer contents at unexpected times. However, to protect > against general data leakage and system memory corruption, the system needs > to provide a way to restrict the DMA to a predefined memory region (this is > usually done at firmware level, e.g. MPU in ATF on some ARM platforms [4]). > > [1a] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_4.html > [1b] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_11.html > [2] https://blade.tencent.com/en/advisories/qualpwn/ > [3] > https://www.bleepingcomputer.com/news/security/vulnerabilities-found-in-highly-popular-firmware-for-wifi-chips/ > [4] > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132 > > v15: > - Apply Will's diff (https://lore.kernel.org/patchwork/patch/1448957/#1647521) > to fix the crash reported by Qian. > - Add Stefano's Acked-by tag for patch 01/12 from v14 That all should be now be on https://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git/ devel/for-linus-5.14 (and linux-next) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote:
>
>
> On 6/24/2021 7:48 AM, Will Deacon wrote:
> > Ok, diff below which attempts to tackle the offset issue I mentioned as
> > well. Qian Cai -- please can you try with these changes?
>
> This works fine.
Cool. Let me squash this patch in #6 and rebase the rest of them.
Claire, could you check the devel/for-linus-5.14 say by end of today to
double check that I didn't mess anything up please?
Will,
Thank you for generating the fix! I am going to run it on x86 and Xen
to make sure all is good (granted last time I ran devel/for-linus-5.14
on that setup I didn't see any errors so I need to double check
I didn't do something silly like run a wrong kernel).
>
> >
> > Will
> >
> > --->8
> >
> > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> > index 175b6c113ed8..39284ff2a6cd 100644
> > --- a/include/linux/swiotlb.h
> > +++ b/include/linux/swiotlb.h
> > @@ -116,7 +116,9 @@ static inline bool is_swiotlb_buffer(struct device
> > *dev, phys_addr_t paddr)
> >
> > static inline bool is_swiotlb_force_bounce(struct device *dev)
> > {
> > - return dev->dma_io_tlb_mem->force_bounce;
> > + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > +
> > + return mem && mem->force_bounce;
> > }
> >
> > void __init swiotlb_exit(void);
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index 44be8258e27b..0ffbaae9fba2 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -449,6 +449,7 @@ static int swiotlb_find_slots(struct device *dev,
> > phys_addr_t orig_addr,
> > dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
> > unsigned int nslots = nr_slots(alloc_size), stride;
> > unsigned int index, wrap, count = 0, i;
> > + unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> > unsigned long flags;
> >
> > BUG_ON(!nslots);
> > @@ -497,7 +498,7 @@ static int swiotlb_find_slots(struct device *dev,
> > phys_addr_t orig_addr,
> > for (i = index; i < index + nslots; i++) {
> > mem->slots[i].list = 0;
> > mem->slots[i].alloc_size =
> > - alloc_size - ((i - index) << IO_TLB_SHIFT);
> > + alloc_size - (offset + ((i - index) <<
> > IO_TLB_SHIFT));
> > }
> > for (i = index - 1;
> > io_tlb_offset(i) != IO_TLB_SEGSIZE - 1 &&
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v14 00/12] Restricted DMA
On Sat, Jun 19, 2021 at 11:40:31AM +0800, Claire Chang wrote: > This series implements mitigations for lack of DMA access control on > systems without an IOMMU, which could result in the DMA accessing the > system memory at unexpected times and/or unexpected addresses, possibly > leading to data leakage or corruption. > > For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is > not behind an IOMMU. As PCI-e, by design, gives the device full access to > system memory, a vulnerability in the Wi-Fi firmware could easily escalate > to a full system exploit (remote wifi exploits: [1a], [1b] that shows a > full chain of exploits; [2], [3]). > > To mitigate the security concerns, we introduce restricted DMA. Restricted > DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a > specially allocated region and does memory allocation from the same region. > The feature on its own provides a basic level of protection against the DMA > overwriting buffer contents at unexpected times. However, to protect > against general data leakage and system memory corruption, the system needs > to provide a way to restrict the DMA to a predefined memory region (this is > usually done at firmware level, e.g. MPU in ATF on some ARM platforms [4]). > > [1a] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_4.html > [1b] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_11.html > [2] https://blade.tencent.com/en/advisories/qualpwn/ > [3] > https://www.bleepingcomputer.com/news/security/vulnerabilities-found-in-highly-popular-firmware-for-wifi-chips/ > [4] > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132 Heya Claire, I put all your patches on https://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git/log/?h=devel/for-linus-5.14 Please double-check that they all look ok. Thank you! ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 03/12] swiotlb: Set dev->dma_io_tlb_mem to the swiotlb pool used
On Tue, Jun 15, 2021 at 09:27:02PM +0800, Claire Chang wrote:
> Always have the pointer to the swiotlb pool used in struct device. This
> could help simplify the code for other pools.
Applying: swiotlb: Set dev->dma_io_tlb_mem to the swiotlb pool used
error: patch failed: kernel/dma/swiotlb.c:339
error: kernel/dma/swiotlb.c: patch does not apply
..
Would you be OK rebasing this against devel/for-linus-5.14 please?
(And please send out with the Reviewed-by from Christopher)
Thank you!
>
> Signed-off-by: Claire Chang
> ---
> drivers/base/core.c| 4
> include/linux/device.h | 4
> kernel/dma/swiotlb.c | 8
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b8a8c96dca58..eeb2d49d3aa3 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include /* for dma_default_coherent */
>
> @@ -2846,6 +2847,9 @@ void device_initialize(struct device *dev)
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> dev->dma_coherent = dma_default_coherent;
> #endif
> +#ifdef CONFIG_SWIOTLB
> + dev->dma_io_tlb_mem = io_tlb_default_mem;
> +#endif
> }
> EXPORT_SYMBOL_GPL(device_initialize);
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 4443e12238a0..2e9a378c9100 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -432,6 +432,7 @@ struct dev_links_info {
> * @dma_pools: Dma pools (if dma'ble device).
> * @dma_mem: Internal for coherent mem override.
> * @cma_area:Contiguous memory area for dma allocations
> + * @dma_io_tlb_mem: Pointer to the swiotlb pool used. Not for driver use.
> * @archdata:For arch-specific additions.
> * @of_node: Associated device tree node.
> * @fwnode: Associated device node supplied by platform firmware.
> @@ -540,6 +541,9 @@ struct device {
> #ifdef CONFIG_DMA_CMA
> struct cma *cma_area; /* contiguous memory area for dma
> allocations */
> +#endif
> +#ifdef CONFIG_SWIOTLB
> + struct io_tlb_mem *dma_io_tlb_mem;
> #endif
> /* arch specific additions */
> struct dev_archdata archdata;
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 97c6ad50fdc2..949a6bb21343 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -339,7 +339,7 @@ void __init swiotlb_exit(void)
> static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t
> size,
> enum dma_data_direction dir)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;
> phys_addr_t orig_addr = mem->slots[index].orig_addr;
> size_t alloc_size = mem->slots[index].alloc_size;
> @@ -421,7 +421,7 @@ static unsigned int wrap_index(struct io_tlb_mem *mem,
> unsigned int index)
> static int find_slots(struct device *dev, phys_addr_t orig_addr,
> size_t alloc_size)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> unsigned long boundary_mask = dma_get_seg_boundary(dev);
> dma_addr_t tbl_dma_addr =
> phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
> @@ -498,7 +498,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev,
> phys_addr_t orig_addr,
> size_t mapping_size, size_t alloc_size,
> enum dma_data_direction dir, unsigned long attrs)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> unsigned int i;
> int index;
> @@ -549,7 +549,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev,
> phys_addr_t tlb_addr,
> size_t mapping_size, enum dma_data_direction dir,
> unsigned long attrs)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = hwdev->dma_io_tlb_mem;
> unsigned long flags;
> unsigned int offset = swiotlb_align_offset(hwdev, tlb_addr);
> int index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT;
> --
> 2.32.0.272.g935e593368-goog
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 01/15] swiotlb: Refactor swiotlb init functions
> > do the set_memory_decrypted()+memset(). Is this okay or should > > swiotlb_init_io_tlb_mem() add an additional argument to do this > > conditionally? > > I'm actually not sure if this it okay. If not, will add an additional > argument for it. Any observations discovered? (Want to make sure my memory-cache has the correct semantics for set_memory_decrypted in mind). > > > -- > > Florian ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 05/15] swiotlb: Add a new get_io_tlb_mem getter
On Tue, May 18, 2021 at 02:51:52PM +0800, Claire Chang wrote: > Still keep this function because directly using dev->dma_io_tlb_mem > will cause issues for memory allocation for existing devices. The pool > can't support atomic coherent allocation so we need to distinguish the > per device pool and the default pool in swiotlb_alloc. This above should really be rolled in the commit. You can prefix it by "The reason it was done this way was because directly using .." ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 04/15] swiotlb: Add restricted DMA pool initialization
On Tue, May 18, 2021 at 02:48:35PM +0800, Claire Chang wrote: > I didn't move this to a separate file because I feel it might be > confusing for swiotlb_alloc/free (and need more functions to be > non-static). > Maybe instead of moving to a separate file, we can try to come up with > a better naming? I think you are referring to: rmem_swiotlb_setup ? Which is ARM specific and inside the generic code? Christopher wants to unify it in all the code so there is one single source, but the "you seperate arch code out from generic" saying makes me want to move it out. I agree that if you move it out from generic to arch-specific we have to expose more of the swiotlb functions, which will undo's Christopher cleanup code. How about this - lets leave it as is now, and when there are more use-cases we can revisit it and then if need to move the code? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 and swiotlb_max_segment
On Mon, May 10, 2021 at 05:25:25PM +0200, Christoph Hellwig wrote: > Hi all, > > swiotlb_max_segment is a rather strange "API" export by swiotlb.c, > and i915 is the only (remaining) user. > > swiotlb_max_segment returns 0 if swiotlb is not in use, 1 if > SWIOTLB_FORCE is set or swiotlb-zen is set, and the swiotlb segment > size when swiotlb is otherwise enabled. > > i915 then uses it to: > > a) decided on the max order in i915_gem_object_get_pages_internal > b) decide on a max segment size in i915_sg_segment_size > > for a) it really seems i915 should switch to dma_alloc_noncoherent > or dma_alloc_noncontigous ASAP instead of using alloc_page and > streaming DMA mappings. Any chance I could trick one of the i915 > maintaines into doing just that given that the callchain is not > exactly trivial? > > For b) I'm not sure swiotlb and i915 really agree on the meaning > of the value. swiotlb_set_max_segment basically returns the entire > size of the swiotlb buffer, while i915 seems to use it to limit > the size each scatterlist entry. It seems like dma_max_mapping_size > might be the best value to use here. Yes. The background behind that was SWIOTLB would fail because well, the size of the sg was too large. And some way to limit it to max size was needed - the dma_max_mapping_size "should" be just fine. > > Once that is fixed I'd like to kill off swiotlb_max_segment as it is > a horribly confusing API. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v1 5/6] xen-swiotlb: convert variables to arrays
On Sun, Feb 07, 2021 at 04:56:01PM +0100, Christoph Hellwig wrote:
> On Thu, Feb 04, 2021 at 09:40:23AM +0100, Christoph Hellwig wrote:
> > So one thing that has been on my mind for a while: I'd really like
> > to kill the separate dma ops in Xen swiotlb. If we compare xen-swiotlb
> > to swiotlb the main difference seems to be:
> >
> > - additional reasons to bounce I/O vs the plain DMA capable
> > - the possibility to do a hypercall on arm/arm64
> > - an extra translation layer before doing the phys_to_dma and vice
> >versa
> > - an special memory allocator
> >
> > I wonder if inbetween a few jump labels or other no overhead enablement
> > options and possibly better use of the dma_range_map we could kill
> > off most of swiotlb-xen instead of maintaining all this code duplication?
>
> So I looked at this a bit more.
>
> For x86 with XENFEAT_auto_translated_physmap (how common is that?)
Juergen, Boris please correct me if I am wrong, but that
XENFEAT_auto_translated_physmap
only works for PVH guests?
> pfn_to_gfn is a nop, so plain phys_to_dma/dma_to_phys do work as-is.
>
> xen_arch_need_swiotlb always returns true for x86, and
> range_straddles_page_boundary should never be true for the
> XENFEAT_auto_translated_physmap case.
Correct. The kernel should have no clue of what the real MFNs are
for PFNs.
>
> So as far as I can tell the mapping fast path for the
> XENFEAT_auto_translated_physmap can be trivially reused from swiotlb.
>
> That leaves us with the next more complicated case, x86 or fully cache
> coherent arm{,64} without XENFEAT_auto_translated_physmap. In that case
> we need to patch in a phys_to_dma/dma_to_phys that performs the MFN
> lookup, which could be done using alternatives or jump labels.
> I think if that is done right we should also be able to let that cover
> the foreign pages in is_xen_swiotlb_buffer/is_swiotlb_buffer, but
> in that worst case that would need another alternative / jump label.
>
> For non-coherent arm{,64} we'd also need to use alternatives or jump
> labels to for the cache maintainance ops, but that isn't a hard problem
> either.
>
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v1 2/6] swiotlb: convert variables to arrays
On Thu, Feb 04, 2021 at 11:49:23AM +, Robin Murphy wrote: > On 2021-02-04 07:29, Christoph Hellwig wrote: > > On Wed, Feb 03, 2021 at 03:37:05PM -0800, Dongli Zhang wrote: > > > This patch converts several swiotlb related variables to arrays, in > > > order to maintain stat/status for different swiotlb buffers. Here are > > > variables involved: > > > > > > - io_tlb_start and io_tlb_end > > > - io_tlb_nslabs and io_tlb_used > > > - io_tlb_list > > > - io_tlb_index > > > - max_segment > > > - io_tlb_orig_addr > > > - no_iotlb_memory > > > > > > There is no functional change and this is to prepare to enable 64-bit > > > swiotlb. > > > > Claire Chang (on Cc) already posted a patch like this a month ago, > > which looks much better because it actually uses a struct instead > > of all the random variables. > > Indeed, I skimmed the cover letter and immediately thought that this whole > thing is just the restricted DMA pool concept[1] again, only from a slightly > different angle. Kind of. Let me lay out how some of these pieces are right now: +---+ +--+ | | | | | | | | | a)Xen-SWIOTLB | | b)SWIOTLB (for !Xen) | | | | | +---XX--+ +---X--+ X XX XXX X XX +--XX---+ | | | | | c) SWIOTLB generic | | | +---+ Dongli's patches modify the SWIOTLB generic c), and Xen-SWIOTLB a) parts. Also see the IOMMU_INIT logic which lays this a bit more deepth (for example how to enable SWIOTLB on AMD boxes, or IBM with Calgary IOMMU, etc - see iommu_table.h). Furtheremore it lays the groundwork to allocate AMD SEV SWIOTLB buffers later after boot (so that you can stich different pools together). All the bits are kind of inside of the SWIOTLB code. And also it changes the Xen-SWIOTLB to do something similar. The mempool did it similarly by taking the internal parts (aka the various io_tlb) of SWIOTLB and exposing them out and having other code: +---+ +--+ | | | | | | | | | a)Xen-SWIOTLB | | b)SWIOTLB (for !Xen) | | | | | +---XX--+ +---X--+ X XX XXX X XX +--XX---+ +--+ | | | Device tree | | +<+ enabling SWIOTLB | |c) SWIOTLB generic | | | | | | mempool | +---+ +--+ What I was suggesting to Clarie to follow Xen model, that is do something like this: +---+ +--+ ++ | | | | || | | | | || | a)Xen-SWIOTLB | | b)SWIOTLB (for !Xen) | | e) DT-SWIOTLB | | | | | || +---XX--+ +---X--+ +XX-X+ XXXX X X XX X XX XX XXX X XX X +--XXX--+ | | | | |c) SWIOTLB generic | | | +---+ so using the SWIOTLB generic parts, and then bolt on top of the device-tree logic, along with the mempool logic. But Christopher has an interesting suggestion which is to squash the all the existing code (a, b, c) all together and pepper it with various jump-tables. So: -+ | SWIOTLB: | || | a) SWIOTLB (for non-Xen) | | b) Xen-SWIOTLB| | c) DT-SWIOTLB | || || -+ with all the various bits (M2P/P2M for Xen, mempool for ARM, and normal allocation for BM) in one big file. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/li
Re: [Intel-gfx] [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
On Tue, Apr 18, 2017 at 09:42:20AM -0600, Logan Gunthorpe wrote:
>
>
> On 18/04/17 08:27 AM, Konrad Rzeszutek Wilk wrote:
> > Interesting that you didn't CC any of the maintainers. Could you
> > do that in the future please?
>
> Please read the cover letter. The distribution list for the patchset
> would have been way too large to cc every maintainer (even as limited as
> it was, I had mailing lists yelling at me). My plan was to get buy in
I am not sure if you know, but you can add on each patch the respective
maintainer via 'CC'. That way you can have certain maintainers CCed only
on the subsystems they cover. You put it after (or before) your SoB and
git send-email happilly picks it up.
It does mean that for every patch you have to run something like this:
$ more add_cc
#!/bin/bash
git diff HEAD^.. > /tmp/a
echo "---"
scripts/get_maintainer.pl --no-l /tmp/a | while read file
do
echo "Cc: $file"
done
Or such.
> for the first patch, get it merged and resend the rest independently to
> their respective maintainers. Of course, though, I'd be open to other
> suggestions.
>
> >>>
> >>> Signed-off-by: Logan Gunthorpe
> >>> ---
> >>> drivers/block/xen-blkfront.c | 33 +++--
> >>> 1 file changed, 27 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> >>> index 5067a0a..7dcf41d 100644
> >>> --- a/drivers/block/xen-blkfront.c
> >>> +++ b/drivers/block/xen-blkfront.c
> >>> @@ -807,8 +807,19 @@ static int blkif_queue_rw_req(struct request *req,
> >>> struct blkfront_ring_info *ri
> >>> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
> >>>
> >>> if (setup.need_copy) {
> >>> - setup.bvec_off = sg->offset;
> >>> - setup.bvec_data = kmap_atomic(sg_page(sg));
> >>> + setup.bvec_off = 0;
> >>> + setup.bvec_data = sg_map(sg, SG_KMAP_ATOMIC);
> >>> + if (IS_ERR(setup.bvec_data)) {
> >>> + /*
> >>> + * This should really never happen unless
> >>> + * the code is changed to use memory that is
> >>> + * not mappable in the sg. Seeing there is a
> >>> + * questionable error path out of here,
> >>> + * we WARN.
> >>> + */
> >>> + WARN(1, "Non-mappable memory used in sg!");
> >>> + return 1;
> >>> + }
> >> ...
> >>
> >> Perhaps add a flag to mark failure as 'unexpected' and trace (and panic?)
> >> inside sg_map().
>
> Thanks, that's a good suggestion. I'll make the change for v2.
>
> Logan
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
On Tue, Apr 18, 2017 at 02:13:59PM +, David Laight wrote:
> From: Logan Gunthorpe
> > Sent: 13 April 2017 23:05
> > Straightforward conversion to the new helper, except due to
> > the lack of error path, we have to warn if unmapable memory
> > is ever present in the sgl.
Interesting that you didn't CC any of the maintainers. Could you
do that in the future please?
> >
> > Signed-off-by: Logan Gunthorpe
> > ---
> > drivers/block/xen-blkfront.c | 33 +++--
> > 1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 5067a0a..7dcf41d 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -807,8 +807,19 @@ static int blkif_queue_rw_req(struct request *req,
> > struct blkfront_ring_info *ri
> > BUG_ON(sg->offset + sg->length > PAGE_SIZE);
> >
> > if (setup.need_copy) {
> > - setup.bvec_off = sg->offset;
> > - setup.bvec_data = kmap_atomic(sg_page(sg));
> > + setup.bvec_off = 0;
> > + setup.bvec_data = sg_map(sg, SG_KMAP_ATOMIC);
> > + if (IS_ERR(setup.bvec_data)) {
> > + /*
> > +* This should really never happen unless
> > +* the code is changed to use memory that is
> > +* not mappable in the sg. Seeing there is a
> > +* questionable error path out of here,
> > +* we WARN.
> > +*/
> > + WARN(1, "Non-mappable memory used in sg!");
> > + return 1;
> > + }
> ...
>
> Perhaps add a flag to mark failure as 'unexpected' and trace (and panic?)
> inside sg_map().
>
> David
>
>
> ___
> Linux-nvdimm mailing list
> linux-nvd...@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 regression in kernel 4.10
On Tue, Dec 20, 2016 at 09:42:46AM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 19, 2016 at 03:16:44PM +0100, Juergen Gross wrote:
> > On 19/12/16 13:29, Chris Wilson wrote:
> > > On Mon, Dec 19, 2016 at 12:39:16PM +0100, Juergen Gross wrote:
> > >> With recent 4.10 kernel the graphics isn't coming up under Xen. First
> > >> failure message is:
> > >>
> > >> [ 46.656649] i915 :00:02.0: swiotlb buffer is full (sz: 1630208
> > >> bytes)
> > >
> > > Do we get a silent failure? i915_gem_gtt_prepare_pages() is where we
> > > call dma_map_sg() and pass the sg to swiotlb (in this case) for
> > > remapping, and we do check for an error value of 0. After that error,
> > > SWIOTLB_MAP_ERROR is propagated back and converted to 0 for
> > > dma_map_sg(). That looks valid, and we should report ENOMEM back to the
> > > caller.
> > >
> > >> Later I see splats like:
> > >>
> > >> [ 49.393583] general protection fault: [#1] SMP
> > >
> > > What was the faulting address? RAX is particularly non-pointer-like so I
> > > wonder if we walked onto an uninitialised portion of the sgtable. We may
> > > have tripped over a bug in our sg_page iterator.
> >
> > During the bisect process there have been either GP or NULL pointer
> > dereferences or other page faults. Typical addresses where:
> >
> > xen_swiotlb_unmap_sg_attrs+0x1f/0x50: access to 0018
> > xen_swiotlb_unmap_sg_attrs+0x1f/0x50: access to 03020118
> >
> > >
> > > The attached patch should prevent an early ENOMEM following the swiotlb
> > > allocation failure. But I suspect that we will still be tripping up the
> > > failure in the sg walker when binding to the GPU.
> > > -Chris
> > >
> >
> > The patch is working not too bad. :-)
> >
> > Still several "swiotlb buffer is full" messages (some with sz:, most
> > without), but no faults any more (neither GP nor NULL pointer
> > dereference). Graphical login is working now.
>
>
> I think I know why. The optimization that was added assumes that
> bus addresses is the same as physical address. Hence it packs all
> of the virtual addresses in the sg, and hands it off to SWIOTLB
> which walks each one and realizes that it has to use the bounce
> buffer.
>
> I am wondering if would make sense to pull 'swiotlb_max_size' inside
> of SWIOTLB and make it an library-ish - so Xen-SWIOTLB can register
> as well and report say that it can only provide one page
> (unless it is running under baremtal).
>
> Or make the usage of 'max_segement' and 'page_to_pfn(page) != last_pfn + 1'
> in i915_gem_object_Get_pages_gtt use something similar to
> xen_biovec_phys_mergeable?
Chris,
I was thinking of something like this (which Juergen has already tested).
From f196f1294fd25f1402c3dd1231babb8d7f5db2e7 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk
Date: Tue, 20 Dec 2016 10:02:02 -0500
Subject: [PATCH] swiotlb: Export swiotlb_max_segment to users
So they can figure out what is the optimal number of pages
that can be contingously stitched together without fear of
bounce buffer.
We also expose an mechanism for sub-users of SWIOTLB API, such
as Xen-SWIOTLB to set the max segment value. And lastly
if swiotlb=force is set (which mandates we bounce buffer everything)
we set max_segment so at least we can bounce buffer one 4K page
instead of a giant 512KB one for which we may not have space.
Reported-and-Tested-by: Juergen Gross
Signed-off-by: Konrad Rzeszutek Wilk
---
drivers/gpu/drm/i915/i915_gem.c | 11 +--
drivers/xen/swiotlb-xen.c | 4
include/linux/swiotlb.h | 3 +++
lib/swiotlb.c | 26 ++
4 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d0dcaf3..115fa39 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2290,15 +2290,6 @@ void __i915_gem_object_put_pages(struct
drm_i915_gem_object *obj,
mutex_unlock(&obj->mm.lock);
}
-static unsigned int swiotlb_max_size(void)
-{
-#if IS_ENABLED(CONFIG_SWIOTLB)
- return rounddown(swiotlb_nr_tbl() << IO_TLB_SHIFT, PAGE_SIZE);
-#else
- return 0;
-#endif
-}
-
static void i915_sg_trim(struct sg_table *orig_st)
{
struct sg_table new_st;
@@ -2345,7 +2336,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object
*obj)
GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAIN
Re: [Intel-gfx] i915 regression in kernel 4.10
On Mon, Dec 19, 2016 at 03:16:44PM +0100, Juergen Gross wrote: > On 19/12/16 13:29, Chris Wilson wrote: > > On Mon, Dec 19, 2016 at 12:39:16PM +0100, Juergen Gross wrote: > >> With recent 4.10 kernel the graphics isn't coming up under Xen. First > >> failure message is: > >> > >> [ 46.656649] i915 :00:02.0: swiotlb buffer is full (sz: 1630208 > >> bytes) > > > > Do we get a silent failure? i915_gem_gtt_prepare_pages() is where we > > call dma_map_sg() and pass the sg to swiotlb (in this case) for > > remapping, and we do check for an error value of 0. After that error, > > SWIOTLB_MAP_ERROR is propagated back and converted to 0 for > > dma_map_sg(). That looks valid, and we should report ENOMEM back to the > > caller. > > > >> Later I see splats like: > >> > >> [ 49.393583] general protection fault: [#1] SMP > > > > What was the faulting address? RAX is particularly non-pointer-like so I > > wonder if we walked onto an uninitialised portion of the sgtable. We may > > have tripped over a bug in our sg_page iterator. > > During the bisect process there have been either GP or NULL pointer > dereferences or other page faults. Typical addresses where: > > xen_swiotlb_unmap_sg_attrs+0x1f/0x50: access to 0018 > xen_swiotlb_unmap_sg_attrs+0x1f/0x50: access to 03020118 > > > > > The attached patch should prevent an early ENOMEM following the swiotlb > > allocation failure. But I suspect that we will still be tripping up the > > failure in the sg walker when binding to the GPU. > > -Chris > > > > The patch is working not too bad. :-) > > Still several "swiotlb buffer is full" messages (some with sz:, most > without), but no faults any more (neither GP nor NULL pointer > dereference). Graphical login is working now. I think I know why. The optimization that was added assumes that bus addresses is the same as physical address. Hence it packs all of the virtual addresses in the sg, and hands it off to SWIOTLB which walks each one and realizes that it has to use the bounce buffer. I am wondering if would make sense to pull 'swiotlb_max_size' inside of SWIOTLB and make it an library-ish - so Xen-SWIOTLB can register as well and report say that it can only provide one page (unless it is running under baremtal). Or make the usage of 'max_segement' and 'page_to_pfn(page) != last_pfn + 1' in i915_gem_object_Get_pages_gtt use something similar to xen_biovec_phys_mergeable? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Allow compaction upto SWIOTLB max segment size
On Wed, Oct 12, 2016 at 10:51:57PM +0100, Chris Wilson wrote:
> On Wed, Oct 12, 2016 at 05:19:14PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Mon, Oct 10, 2016 at 11:27:00PM +0100, Chris Wilson wrote:
> > > commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> > > work with SWIOTLB backend") took a heavy handed approach to undo the
> > > scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> > > whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> > > can be a little more intelligent and try compacting the scatterlist up
> > > to the maximum SWIOTLB segment size (when using SWIOTLB).
> > >
> >
> > Won't this cause a bigger usage of the SWIOTLB bounce buffer ?
>
> It won't change the frequency of the usage of the bounce buffer, if that
> is what you mean. Either you have intel-iommu and so will not go through
> swiotlb, or you are forced to use swiotlb even though the hw doesn't
> require it (swiotlb config is byzantium and always enabled unless you
> hack it out and can rejoice at the lower cpu usage).
Hahah. Wish that was possible.
Anyhow my concern was with it under Xen, since the page_to_pfn(page) != last_pfn
check is useless there (the PFNs may be contingous but underneath
the machine frame numbers may be discontingous).
And was thinking that this check should be moved to some form of 'DMA-API'
type check, but that screams to me lots of work.
Perhaps expose another swiotlb call to query the preferred size of the
segments. So that you can get the 128 under baremetal SWIOTLB but under Xen
it may expose 1.
But that should not hold up this patch, and can be a followup patch I can
write.
Anyhow for *this* patch:
Acked-by: Konrad Rzeszutek Wilk
[as [xen,]swiotlb maintainer, in case you need this].
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Allow compaction upto SWIOTLB max segment size
On Mon, Oct 10, 2016 at 11:27:00PM +0100, Chris Wilson wrote:
> commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> work with SWIOTLB backend") took a heavy handed approach to undo the
> scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> can be a little more intelligent and try compacting the scatterlist up
> to the maximum SWIOTLB segment size (when using SWIOTLB).
>
Won't this cause a bigger usage of the SWIOTLB bounce buffer ?
> v2: Tidy sg_mark_end() and cpp
>
> Signed-off-by: Chris Wilson
> CC: Imre Deak
> CC: Daniel Vetter
> Cc: Konrad Rzeszutek Wilk
> Cc: Tvrtko Ursulin
> ---
> drivers/gpu/drm/i915/i915_gem.c | 30 ++
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dff8d05d80ee..50fd611926cb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2201,6 +2201,15 @@ unlock:
> mutex_unlock(&obj->mm.lock);
> }
>
> +static unsigned long swiotlb_max_size(void)
> +{
> +#if IS_ENABLED(CONFIG_SWIOTLB)
> + return swiotlb_nr_tbl() << IO_TLB_SHIFT;
> +#else
> + return 0;
> +#endif
> +}
> +
> static struct sg_table *
> i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> {
> @@ -2212,6 +2221,7 @@ i915_gem_object_get_pages_gtt(struct
> drm_i915_gem_object *obj)
> struct sgt_iter sgt_iter;
> struct page *page;
> unsigned long last_pfn = 0; /* suppress gcc warning */
> + unsigned long max_segment;
> int ret;
> gfp_t gfp;
>
> @@ -,6 +2232,10 @@ i915_gem_object_get_pages_gtt(struct
> drm_i915_gem_object *obj)
> GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
> GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>
> + max_segment = swiotlb_max_size();
> + if (!max_segment)
> + max_segment = obj->base.size;
> +
> st = kmalloc(sizeof(*st), GFP_KERNEL);
> if (st == NULL)
> return ERR_PTR(-ENOMEM);
> @@ -2263,15 +2277,9 @@ i915_gem_object_get_pages_gtt(struct
> drm_i915_gem_object *obj)
> goto err_pages;
> }
> }
> -#ifdef CONFIG_SWIOTLB
> - if (swiotlb_nr_tbl()) {
> - st->nents++;
> - sg_set_page(sg, page, PAGE_SIZE, 0);
> - sg = sg_next(sg);
> - continue;
> - }
> -#endif
> - if (!i || page_to_pfn(page) != last_pfn + 1) {
> + if (!i ||
> + sg->length >= max_segment ||
> + page_to_pfn(page) != last_pfn + 1) {
> if (i)
> sg = sg_next(sg);
> st->nents++;
> @@ -2284,9 +2292,7 @@ i915_gem_object_get_pages_gtt(struct
> drm_i915_gem_object *obj)
> /* Check that the i965g/gm workaround works. */
> WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x0010UL));
> }
> -#ifdef CONFIG_SWIOTLB
> - if (!swiotlb_nr_tbl())
> -#endif
> + if (sg) /* loop terminated early; short sg table */
> sg_mark_end(sg);
>
> ret = i915_gem_gtt_prepare_pages(obj, st);
> --
> 2.9.3
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Thu, Jul 24, 2014 at 09:44:41AM +0800, Chen, Tiejun wrote: > On 2014/7/24 4:54, Konrad Rzeszutek Wilk wrote: > >On Sat, Jul 19, 2014 at 12:27:21AM +, Kay, Allen M wrote: > >>>For the MCH PCI registers that do need to be read - can you tell us which > >>>ones those are? > >> > >>In qemu/hw/xen_pt_igd.c/igd_pci_read(), following MCH PCI config register > >>reads are passthrough to the host HW. Some of the registers are needed by > >>Ironlake GFX driver which we probably can remove. I did a trace recently > >>on Broadwell, the number of register accessed are even smaller (0, 2, 2c, > >>2e, 50, 52, a0, a4). Given that we now have integrated MCH and GPU in the > >>same socket, looks like driver can easily remove reads for offsets 0 - 0x2e. > >> > >>case 0x00:/* vendor id */ > >>case 0x02:/* device id */ > >>case 0x08:/* revision id */ > >>case 0x2c:/* sybsystem vendor id */ > >>case 0x2e:/* sybsystem id */ > > > >Right. We can fix the i915 to use the mechanism that Michael mentioned. > >(see attached RFC patches) > > > >>case 0x50:/* SNB: processor graphics control register */ > >>case 0x52:/* processor graphics control register */ > >>case 0xa0:/* top of memory */ > >>case 0xb0:/* ILK: BSM: should read from dev 2 offset > >> 0x5c */ > >>case 0x58:/* SNB: PAVPC Offset */ > >>case 0xa4:/* SNB: graphics base of stolen memory */ > >>case 0xa8:/* SNB: base of GTT stolen memory */ > > > >I dug in the intel-gtt.c (see ironlake_gtt_driver) to figure out this > >a bit more. As in, I speculated, what if we returned 0 (and not implement > >any support for reading from these registers). What would happen? > > > >0x52 for Ironlake (g5): > >-- > >It looks like intel_gmch_probe is called when i915_gem_gtt_init > >starts (there is a lot of code that looks to be used between > >intel-gtt.c and i915.c). > > > >Anyhow the interesting parts are that i9xx_setup ends up calling > >ioremap the i915 BAR to setup some of these registers for older generations. > > > >Then i965_gtt_total_entries gets which reads 0x52, but it is only > >needed for v5 generation. For other (v4 and G33) it reads it from the GPU's > >0x2020 register. > > > >If there is a mismatch, it writes to the GPU at 0x2020 to update the > >the size based on the bridge. And then it reads from 0x2020 and that > >is returned and stuck in intel_private.gtt_total_entries. > > > >So 0x52 in the emulated bridge could be populated with what the > >GPU has at 0x2020. And the writes go in the emulated area. > > > >0x52 for v6 -> v8: > >- > >We seem to go to gen6_gmch_probe which just figures out the > >the GTT based on the GPU's BAR sizes. The stolen values > >are read from 0x50 from the GPU. So no need to touch the bridge > >(see gen6_gmch_probe) > > > >OK, so no need to have 0x52 or 0x50 in the bridge. > > > >0xA0: > >- > >Could not find any reference in the Linux code. Why would > >Windows driver need this? If we returned the _real_ TOM would > >it matter? Is it used to figure out the device should use 32-bit > >DMA operations or 40-bit? > > > >0xb0 or 0x5c: > >- > >No mention of them in the Linux code. > > > >0x58, 0xa4, 0xa8: > >- > >No usage of them in the Linux code. We seem to be using the 0x52 > >from the bridge and 0x2020 from the GPU for this functionality. > > > > > >So in theory*, if using Ironlake we need to have a proper value > >in 0x52 in the bridge. But for the later chipsets we do not need > >these values (I am assuming that intel_setup_mchbar can safely > >return as it does that for Ironlake and could very well for later > >generations). > > > >Can this be reflected in the Windows driver as well? > > > >P.S. > >*theory: That is assuming we modify the Linux i915_drv.c:intel_detect_pch > >to pick up the id as suggested earlier. See the RFC patches attached. > >(Not compile tested at all!) > > I take a look these patches, looks we still scan all PCI Bridge to walk all > PCHs. So this means we still need to fake a PCI bridge, right? Or maybe you > don't cover this problem this time. I totall
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Sat, Jul 19, 2014 at 12:27:21AM +, Kay, Allen M wrote: > > For the MCH PCI registers that do need to be read - can you tell us which > > ones those are? > > In qemu/hw/xen_pt_igd.c/igd_pci_read(), following MCH PCI config register > reads are passthrough to the host HW. Some of the registers are needed by > Ironlake GFX driver which we probably can remove. I did a trace recently on > Broadwell, the number of register accessed are even smaller (0, 2, 2c, 2e, > 50, 52, a0, a4). Given that we now have integrated MCH and GPU in the same > socket, looks like driver can easily remove reads for offsets 0 - 0x2e. > > case 0x00:/* vendor id */ > case 0x02:/* device id */ > case 0x08:/* revision id */ > case 0x2c:/* sybsystem vendor id */ > case 0x2e:/* sybsystem id */ Right. We can fix the i915 to use the mechanism that Michael mentioned. (see attached RFC patches) > case 0x50:/* SNB: processor graphics control register */ > case 0x52:/* processor graphics control register */ > case 0xa0:/* top of memory */ > case 0xb0:/* ILK: BSM: should read from dev 2 offset > 0x5c */ > case 0x58:/* SNB: PAVPC Offset */ > case 0xa4:/* SNB: graphics base of stolen memory */ > case 0xa8:/* SNB: base of GTT stolen memory */ I dug in the intel-gtt.c (see ironlake_gtt_driver) to figure out this a bit more. As in, I speculated, what if we returned 0 (and not implement any support for reading from these registers). What would happen? 0x52 for Ironlake (g5): -- It looks like intel_gmch_probe is called when i915_gem_gtt_init starts (there is a lot of code that looks to be used between intel-gtt.c and i915.c). Anyhow the interesting parts are that i9xx_setup ends up calling ioremap the i915 BAR to setup some of these registers for older generations. Then i965_gtt_total_entries gets which reads 0x52, but it is only needed for v5 generation. For other (v4 and G33) it reads it from the GPU's 0x2020 register. If there is a mismatch, it writes to the GPU at 0x2020 to update the the size based on the bridge. And then it reads from 0x2020 and that is returned and stuck in intel_private.gtt_total_entries. So 0x52 in the emulated bridge could be populated with what the GPU has at 0x2020. And the writes go in the emulated area. 0x52 for v6 -> v8: - We seem to go to gen6_gmch_probe which just figures out the the GTT based on the GPU's BAR sizes. The stolen values are read from 0x50 from the GPU. So no need to touch the bridge (see gen6_gmch_probe) OK, so no need to have 0x52 or 0x50 in the bridge. 0xA0: - Could not find any reference in the Linux code. Why would Windows driver need this? If we returned the _real_ TOM would it matter? Is it used to figure out the device should use 32-bit DMA operations or 40-bit? 0xb0 or 0x5c: - No mention of them in the Linux code. 0x58, 0xa4, 0xa8: - No usage of them in the Linux code. We seem to be using the 0x52 from the bridge and 0x2020 from the GPU for this functionality. So in theory*, if using Ironlake we need to have a proper value in 0x52 in the bridge. But for the later chipsets we do not need these values (I am assuming that intel_setup_mchbar can safely return as it does that for Ironlake and could very well for later generations). Can this be reflected in the Windows driver as well? P.S. *theory: That is assuming we modify the Linux i915_drv.c:intel_detect_pch to pick up the id as suggested earlier. See the RFC patches attached. (Not compile tested at all!) > > Allen > > -Original Message- > From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com] > Sent: Friday, July 18, 2014 6:45 AM > To: Kay, Allen M > Cc: Michael S. Tsirkin; Jesse Barnes; peter.mayd...@linaro.org; > xen-de...@lists.xensource.com; Ross Philipson; airl...@linux.ie; > daniel.vet...@ffwll.ch; intel-gfx@lists.freedesktop.org; > kelly.zyta...@amd.com; qemu-de...@nongnu.org; Anthony Perard; Stefano > Stabellini; anth...@codemonkey.ws; Paolo Bonzini; Zhang, Yang Z; Chen, Tiejun > Subject: Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel > IGD passthrough support > > On Thu, Jul 17, 2014 at 05:37:12PM +, Kay, Allen M wrote: > > > That sounds great. Tiejun could you confirm that with windows driver guys > > > too? > > > > I believe windows driver can also assume specific CPU/PCH combos. I will > > discuss this with native Windows driver guys. Preferably, the same code > > path can be used for both native and virtualization cases to avoid frequent &g
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Thu, Jul 17, 2014 at 05:37:12PM +, Kay, Allen M wrote:
> > That sounds great. Tiejun could you confirm that with windows driver guys
> > too?
>
> I believe windows driver can also assume specific CPU/PCH combos. I will
> discuss this with native Windows driver guys. Preferably, the same code path
> can be used for both native and virtualization cases to avoid frequent
> breakage as most developers and QA do not test new code changes in
> virtualization environment.
>
> I have verified that Windows driver do not need to write to any MCH PCI
> registers on HSW/BDW so the PCI write function can be removed. The MCH PCI
> registers that need to be read, we are working with HW team to get them
> mirrored in GPU MMIO registers in future HW.
>
For the MCH PCI registers that do need to be read - can you tell us which
ones those are?
Thank you!
> Allen
>
> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> Michael S. Tsirkin
> Sent: Thursday, July 03, 2014 1:28 PM
> To: Jesse Barnes
> Cc: peter.mayd...@linaro.org; xen-de...@lists.xensource.com; Ross Philipson;
> Konrad Rzeszutek Wilk; airl...@linux.ie; daniel.vet...@ffwll.ch;
> intel-gfx@lists.freedesktop.org; kelly.zyta...@amd.com;
> qemu-de...@nongnu.org; Anthony Perard; Stefano Stabellini;
> anth...@codemonkey.ws; Paolo Bonzini; Zhang, Yang Z; Chen, Tiejun
> Subject: Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel
> IGD passthrough support
>
> On Thu, Jul 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote:
> > On Thu, 3 Jul 2014 14:26:12 -0400
> > Konrad Rzeszutek Wilk wrote:
> >
> > > On Thu, Jul 03, 2014 at 10:32:12AM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jul 02, 2014 at 12:23:37PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > > On Wed, Jul 02, 2014 at 04:50:15PM +0200, Paolo Bonzini wrote:
> > > > > > Il 02/07/2014 16:00, Konrad Rzeszutek Wilk ha scritto:
> > > > > > >With this long thread I lost a bit context about the
> > > > > > >challenges that exists. But let me try summarizing it here -
> > > > > > >which will hopefully get some consensus.
> > > > > > >
> > > > > > >1). Fix IGD hardware to not use Southbridge magic addresses.
> > > > > > >We can moan and moan but I doubt it is going to change.
> > > > > >
> > > > > > There are two problems:
> > > > > >
> > > > > > - Northbridge (i.e. MCH i.e. PCI host bridge) configuration
> > > > > > space addresses
> > > > >
> > > > > Right. So in drivers/gpu/drm/i915/i915_dma.c:
> > > > > 1135 #define MCHBAR_I915 0x44
> > > > >
> > > > > 1136 #define MCHBAR_I965 0x48
> > > > >
> > > > > 1147 int reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965 :
> > > > > MCHBAR_I915;
> > > > > 1152 if (INTEL_INFO(dev)->gen >= 4)
> > > > >
> > > > > 1153 pci_read_config_dword(dev_priv->bridge_dev, reg
> > > > > + 4, &temp_hi);
> > > > > 1154 pci_read_config_dword(dev_priv->bridge_dev, reg,
> > > > > &temp_lo);
> > > > > 1155 mchbar_addr = ((u64)temp_hi << 32) | temp_lo;
> > > > >
> > > > >
> > > > > and
> > > > >
> > > > > 1139 #define DEVEN_REG 0x54
> > > > >
> > > > >
> > > > > 1193 int mchbar_reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965
> > > > > : MCHBAR_I915;
> > > > > 1202 if (IS_I915G(dev) || IS_I915GM(dev)) {
> > > > >
> > > > > 1203 pci_read_config_dword(dev_priv->bridge_dev,
> > > > > DEVEN_REG, &temp);
> > > > > 1204 enabled = !!(temp & DEVEN_MCHBAR_EN);
> > > > >
> > > > > 1205 } else {
> > > > >
> > > > > 1206 pci_read_config_dwo
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Thu, Jul 03, 2014 at 11:27:40PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote:
> > On Thu, 3 Jul 2014 14:26:12 -0400
> > Konrad Rzeszutek Wilk wrote:
> >
> > > On Thu, Jul 03, 2014 at 10:32:12AM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jul 02, 2014 at 12:23:37PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > > On Wed, Jul 02, 2014 at 04:50:15PM +0200, Paolo Bonzini wrote:
> > > > > > Il 02/07/2014 16:00, Konrad Rzeszutek Wilk ha scritto:
> > > > > > >With this long thread I lost a bit context about the challenges
> > > > > > >that exists. But let me try summarizing it here - which will
> > > > > > >hopefully
> > > > > > >get some consensus.
> > > > > > >
> > > > > > >1). Fix IGD hardware to not use Southbridge magic addresses.
> > > > > > >We can moan and moan but I doubt it is going to change.
> > > > > >
> > > > > > There are two problems:
> > > > > >
> > > > > > - Northbridge (i.e. MCH i.e. PCI host bridge) configuration space
> > > > > > addresses
> > > > >
> > > > > Right. So in drivers/gpu/drm/i915/i915_dma.c:
> > > > > 1135 #define MCHBAR_I915 0x44
> > > > >
> > > > > 1136 #define MCHBAR_I965 0x48
> > > > >
> > > > > 1147 int reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965 :
> > > > > MCHBAR_I915;
> > > > > 1152 if (INTEL_INFO(dev)->gen >= 4)
> > > > >
> > > > > 1153 pci_read_config_dword(dev_priv->bridge_dev, reg
> > > > > + 4, &temp_hi);
> > > > > 1154 pci_read_config_dword(dev_priv->bridge_dev, reg,
> > > > > &temp_lo);
> > > > > 1155 mchbar_addr = ((u64)temp_hi << 32) | temp_lo;
> > > > >
> > > > >
> > > > > and
> > > > >
> > > > > 1139 #define DEVEN_REG 0x54
> > > > >
> > > > >
> > > > > 1193 int mchbar_reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965
> > > > > : MCHBAR_I915;
> > > > > 1202 if (IS_I915G(dev) || IS_I915GM(dev)) {
> > > > >
> > > > > 1203 pci_read_config_dword(dev_priv->bridge_dev,
> > > > > DEVEN_REG, &temp);
> > > > > 1204 enabled = !!(temp & DEVEN_MCHBAR_EN);
> > > > >
> > > > > 1205 } else {
> > > > >
> > > > > 1206 pci_read_config_dword(dev_priv->bridge_dev,
> > > > > mchbar_reg, &temp);
> > > > > 1207 enabled = temp & 1;
> > > > >
> > > > > 1208 }
> > > > > >
> > > > > > - Southbridge (i.e. PCH i.e. ISA bridge) vendor/device ID; some
> > > > > > versions of
> > > > > > the driver identify it by class, some versions identify it by slot
> > > > > > (1f.0).
> > > > >
> > > > > Right, So in drivers/gpu/drm/i915/i915_drv.c the giant
> > > > > intel_detect_pch
> > > > > which sets the pch_type based on :
> > > > >
> > > > > 432 if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> > > > >
> > > > > 433 unsigned short id = pch->device &
> > > > > INTEL_PCH_DEVICE_ID_MASK;
> > > > > 434 dev_priv->pch_id = id;
> > > > >
> > > > > 435
> > > > >
> > > > > 436 if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE)
> > > > > {
> > > > >
> > > > > It chec
Re: [Intel-gfx] [Xen-devel] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type
On Fri, Jul 11, 2014 at 08:29:56AM +0200, Daniel Vetter wrote:
> On Thu, Jul 10, 2014 at 09:08:24PM +, Tian, Kevin wrote:
> > actually I'm curious whether it's still necessary to __detect__ PCH. Could
> > we assume a 1:1 mapping between GPU and PCH, e.g. BDW already hard
> > code the knowledge:
> >
> > } else if (IS_BROADWELL(dev)) {
> > dev_priv->pch_type = PCH_LPT;
> > dev_priv->pch_id =
> > INTEL_PCH_LPT_LP_DEVICE_ID_TYPE;
> > DRM_DEBUG_KMS("This is Broadwell, assuming "
> > "LynxPoint LP PCH\n");
> >
> > Or if there is real usage on non-fixed mapping (not majority), could it be
> > a
> > better option to have fixed mapping as a fallback instead of leaving as
> > PCH_NONE? Then even when Qemu doesn't provide a special tweaked PCH,
> > the majority case just works.
>
> I guess we can do it, at least I haven't seen any strange combinations in
> the wild outside of Intel ...
How big is the QA matrix for this? Would it make sense to just
include the latest hardware (say going two generations back)
and ignore the older one?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Thu, Jul 03, 2014 at 10:32:12AM +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 02, 2014 at 12:23:37PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jul 02, 2014 at 04:50:15PM +0200, Paolo Bonzini wrote:
> > > Il 02/07/2014 16:00, Konrad Rzeszutek Wilk ha scritto:
> > > >With this long thread I lost a bit context about the challenges
> > > >that exists. But let me try summarizing it here - which will hopefully
> > > >get some consensus.
> > > >
> > > >1). Fix IGD hardware to not use Southbridge magic addresses.
> > > >We can moan and moan but I doubt it is going to change.
> > >
> > > There are two problems:
> > >
> > > - Northbridge (i.e. MCH i.e. PCI host bridge) configuration space
> > > addresses
> >
> > Right. So in drivers/gpu/drm/i915/i915_dma.c:
> > 1135 #define MCHBAR_I915 0x44
> >
> > 1136 #define MCHBAR_I965 0x48
> >
> > 1147 int reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965 :
> > MCHBAR_I915;
> > 1152 if (INTEL_INFO(dev)->gen >= 4)
> >
> > 1153 pci_read_config_dword(dev_priv->bridge_dev, reg + 4,
> > &temp_hi);
> > 1154 pci_read_config_dword(dev_priv->bridge_dev, reg, &temp_lo);
> >
> > 1155 mchbar_addr = ((u64)temp_hi << 32) | temp_lo;
> >
> > and
> >
> > 1139 #define DEVEN_REG 0x54
> >
> >
> > 1193 int mchbar_reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965 :
> > MCHBAR_I915;
> > 1202 if (IS_I915G(dev) || IS_I915GM(dev)) {
> >
> > 1203 pci_read_config_dword(dev_priv->bridge_dev, DEVEN_REG,
> > &temp);
> > 1204 enabled = !!(temp & DEVEN_MCHBAR_EN);
> >
> > 1205 } else {
> >
> > 1206 pci_read_config_dword(dev_priv->bridge_dev,
> > mchbar_reg, &temp);
> > 1207 enabled = temp & 1;
> >
> > 1208 }
> > >
> > > - Southbridge (i.e. PCH i.e. ISA bridge) vendor/device ID; some versions
> > > of
> > > the driver identify it by class, some versions identify it by slot (1f.0).
> >
> > Right, So in drivers/gpu/drm/i915/i915_drv.c the giant intel_detect_pch
> > which sets the pch_type based on :
> >
> > 432 if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> >
> > 433 unsigned short id = pch->device &
> > INTEL_PCH_DEVICE_ID_MASK;
> > 434 dev_priv->pch_id = id;
> >
> > 435
> >
> > 436 if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
> >
> > It checks for 0x3b00, 0x1c00, 0x1e00, 0x8c00 and 0x9c00.
> > The INTEL_PCH_DEVICE_ID_MASK is 0xff00
> > >
> > > To solve the first, make a new machine type, PIIX4-based, and pass through
> > > the registers you need. The patch must document _exactly_ why the
> > > registers
> > > are safe to pass. If they are not reserved on PIIX4, the patch must
> > > document what the same offsets mean on PIIX4, and why it's sensible to
> > > assume that firmware for virtual machine will not read/write them. Bonus
> > > point for also documenting the same for Q35.
> >
> > OK. They look to be related to setting up an MBAR , but I don't understand
> > why it is needed. Hopefully some of the i915 folks CC-ed here can answer.
>
> In particular, I think setting up MBAR should move out of i915 and become
> the bridge driver tweak on linux and windows.
That is an excellent idea.
However I am still curious to _why_ it has to be done in the first place.
> It would then run on the priveledged host
> and we won't need to deal with it in QEMU.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Wed, Jul 02, 2014 at 06:29:23PM +0200, Paolo Bonzini wrote: > Il 02/07/2014 17:27, Michael S. Tsirkin ha scritto: > > At some level, maybe Paolo is right. Ignore existing drivers and ask > > intel developers to update their drivers to do something sane on > > hypervisors, even if they do ugly things on real hardware. > > > > A simple proposal since what I wrote earlier though apparently wasn't > > very clear: > > > > Detect Xen subsystem vendor id on vga card. > > If there, avoid poking at chipset. Instead > > - use subsystem device # for card type > > You mean for PCH type (aka PCH device id). > > > - use second half of BAR0 of device > > - instead of access to pci host > > > > hypervisors will simply take BAR0 and double it in size, > > make second part map to what would be the pci host. > > Nice. Detecting the backdoor via the subsystem vendor id > is clever. > > I'm not sure if it's possible to just double the size of BAR0 > or not, but my laptop has: > > Region 0: Memory at d000 (64-bit, non-prefetchable) [size=4M] > Region 2: Memory at c000 (64-bit, prefetchable) [size=256M] > Region 4: I/O ports at 5000 [size=64] > > and I hope we can reserve a few KB for hypervisors within those > 4M, or 8 bytes for an address/data pair (like cf8/cfc) within BAR4's > 64 bytes (or grow BAR4 to 128 bytes, or something like that). > > Xen can still add the hacky machine type if they want for existing > hosts, but this would be a nice way forward. It would be good to understand first why i915 in the first place needs to setup the bridge MBAR if it has not been set. As in, why is this region needed? Is it needed to flush the pipeline (as in you need to write there?) or .. Perhaps it is not needed anymore with the current hardware and what can be done is put a stake in the ground saying that only genX or later will be supported. The commit ids allude to power managament and the earlier commits did poke there - but I don't see it on the latest tree. > > Paolo > > ___ > Xen-devel mailing list > xen-de...@lists.xen.org > http://lists.xen.org/xen-devel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Wed, Jul 02, 2014 at 04:50:15PM +0200, Paolo Bonzini wrote:
> Il 02/07/2014 16:00, Konrad Rzeszutek Wilk ha scritto:
> >With this long thread I lost a bit context about the challenges
> >that exists. But let me try summarizing it here - which will hopefully
> >get some consensus.
> >
> >1). Fix IGD hardware to not use Southbridge magic addresses.
> >We can moan and moan but I doubt it is going to change.
>
> There are two problems:
>
> - Northbridge (i.e. MCH i.e. PCI host bridge) configuration space addresses
Right. So in drivers/gpu/drm/i915/i915_dma.c:
1135 #define MCHBAR_I915 0x44
1136 #define MCHBAR_I965 0x48
1147 int reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965 : MCHBAR_I915;
1152 if (INTEL_INFO(dev)->gen >= 4)
1153 pci_read_config_dword(dev_priv->bridge_dev, reg + 4,
&temp_hi);
1154 pci_read_config_dword(dev_priv->bridge_dev, reg, &temp_lo);
1155 mchbar_addr = ((u64)temp_hi << 32) | temp_lo;
and
1139 #define DEVEN_REG 0x54
1193 int mchbar_reg = INTEL_INFO(dev)->gen >= 4 ? MCHBAR_I965 :
MCHBAR_I915;
1202 if (IS_I915G(dev) || IS_I915GM(dev)) {
1203 pci_read_config_dword(dev_priv->bridge_dev, DEVEN_REG,
&temp);
1204 enabled = !!(temp & DEVEN_MCHBAR_EN);
1205 } else {
1206 pci_read_config_dword(dev_priv->bridge_dev, mchbar_reg,
&temp);
1207 enabled = temp & 1;
1208 }
>
> - Southbridge (i.e. PCH i.e. ISA bridge) vendor/device ID; some versions of
> the driver identify it by class, some versions identify it by slot (1f.0).
Right, So in drivers/gpu/drm/i915/i915_drv.c the giant intel_detect_pch
which sets the pch_type based on :
432 if (pch->vendor == PCI_VENDOR_ID_INTEL) {
433 unsigned short id = pch->device &
INTEL_PCH_DEVICE_ID_MASK;
434 dev_priv->pch_id = id;
435
436 if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
It checks for 0x3b00, 0x1c00, 0x1e00, 0x8c00 and 0x9c00.
The INTEL_PCH_DEVICE_ID_MASK is 0xff00
>
> To solve the first, make a new machine type, PIIX4-based, and pass through
> the registers you need. The patch must document _exactly_ why the registers
> are safe to pass. If they are not reserved on PIIX4, the patch must
> document what the same offsets mean on PIIX4, and why it's sensible to
> assume that firmware for virtual machine will not read/write them. Bonus
> point for also documenting the same for Q35.
OK. They look to be related to setting up an MBAR , but I don't understand
why it is needed. Hopefully some of the i915 folks CC-ed here can answer.
>
> Regarding the second, fixing IGD hardware to not rely on chipset magic is a
> no-go, I agree. I disagree that it's a no-go to define a "backdoor" that
> lets a hypervisor pass the right information to the driver without hacking
> the chipset device model.
>
> The hardware folks would have to give us a place for a pair of registers
> (something like data/address), and a bit somewhere else that would be always
> 0 on hardware and always 1 if the hypervisor is implementing the pair of
> registers. This is similar to CPUID, which has the HYPERVISOR bit +
> hypervisor-defined leaves at 0x4000.
>
> The data/address pair could be in a BAR, in configuration space, in the low
> VGA ports at 0x3c0-0x3df, wherever. The hypervisor bit can be in the same
> place or somewhere else---again, whatever is convenient for the hardware
> folks. We just need *one bit* that is known-zero on all hardware, and 8
> bytes in a reserved area. I don't think it's too hard to find this space,
> and I really, really would like Intel to follow up on a paravirtualized
> backdoor.
>
> That said, we have the problem of existing guests, so I agree something else
> is needed.
>
> > a) Two bridges - one 'passthrough' and the legacy ISA bridge
> >that QEMU emulates. Both Linux and Windows are OK with
> >two bridges (even thought it is pretty weird).
>
> This is pretty much the only solution for existing Linux gue
Re: [Intel-gfx] [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Wed, Jul 02, 2014 at 05:08:43PM +0300, Michael S. Tsirkin wrote: > On Wed, Jul 02, 2014 at 10:00:33AM -0400, Konrad Rzeszutek Wilk wrote: > > On Wed, Jul 02, 2014 at 01:33:09PM +0200, Paolo Bonzini wrote: > > > Il 01/07/2014 19:39, Ross Philipson ha scritto: > > > > > > > >We do IGD pass-through in our project (XenClient). The patches > > > >originally came from our project. We surface the same ISA bridge and > > > >have never had activation issues on any version of Widows from XP to > > > >Win8. We do not normally run server platforms so I can't say for sure > > > >there. > > > > > > The problem is not activation, the problem is that the patches are making > > > assumptions on the driver and the firmware that might work today but are > > > IMHO just not sane. > > > > > > I would have no problem with a clean patchset that adds a new machine type > > > and doesn't touch code in "-M pc", but it looks like mst disagrees. > > > Ultimately, if a patchset is too hacky for upstream, you can include it in > > > your downstream XenClient (and XenServer) QEMU branch. It happens. > > > > And then this discussion will come back again in a year when folks > > rebase and ask: Why hasn't this been done upstream. > > > > Then the discussion resumes .. > > > > With this long thread I lost a bit context about the challenges > > that exists. But let me try summarizing it here - which will hopefully > > get some consensus. > > Before I answer could you clarify please: > by Southbridge do you mean the PCH at slot 1f or the MCH at slot 0 or both? MCH slot. We read/write from this (see intel_setup_mchbar) from couple of registers (0x44 and 0x48 if gen >= 4, otherwise 0x54). It is hard-coded in the i915_get_bridge_dev (see ec2a4c3fdc8e82fe82a25d800e85c1ea06b74372) as 0:0.0 BDF. The PCH (does not matter where it sits) we only use the model:vendor id to figure out the pch_type (see intel_detect_pch). I don't see why that model:vendor_id can't be exposed via checking the type of device:vendor_id of the IGD itself. CC-ing some Intel i915 authors. So for the discussion here, when I say Southbridge I mean MCH. > > > 1). Fix IGD hardware to not use Southbridge magic addresses. > > We can moan and moan but I doubt it is going to change. > > > > 2). Since we need the Southbridge magic addresses, we can expose > > an bridge. [I think everybody agrees that we need to do > > that since 1) is no go). > > > > 3). What kind of bridge. We can do: > > > > a) Two bridges - one 'passthrough' and the legacy ISA bridge > > that QEMU emulates. Both Linux and Windows are OK with > > two bridges (even thought it is pretty weird). > > > > b) One bridge - the one that QEMU emulates - and lets emulate > > more of the registers (by emulate - I mean for some get the > > data from the real hardware). > > > >b1). We can't use the legacy because the registers are > > above 256 (is that correct? Did I miss something?) > > > >b2) We would need to use the Q35. > > b2a). If we need Q35, that needs to be exposed in > > for Xen guests. That means exposing the > > MMCONFIG and restructing the E820 to fit that > > in. > > Problem: > > - Migration is not working with Q35. > > (But for v1 you wouldn't migrate, however > >later hardware will surely have SR-IOV so > >we will need to migrate). > > > > - There are no developers who have an OK > > from their management to focus on this. > >(Potential solution: Poke Intel management to see > > if they can get more developers on it) > > > > > > 4). Code does a bit of sysfs that could use some refacturing with > > the KVM code. > > Problem: More time needed to do the code restructing. > > > > > > Is that about correct? > > > > What are folks timezones and the best days next week to talk about > > this on either Google Hangout or the phone? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] ACPI: Fix acpi_evaluate_object() return value check
Yijing Wang wrote:
>Fix acpi_evaluate_object() return value check,
>shoud acpi_status not int.
Should be?
Your mailer also ate the word 'to' .
>
>Signed-off-by: Yijing Wang
>---
>
>v1->v2: Add CC to the related subsystem MAINTAINERS.
>
>---
> drivers/gpu/drm/i915/intel_acpi.c | 13 +++--
> drivers/gpu/drm/nouveau/core/subdev/mxm/base.c |6 +++---
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 13 +++--
> drivers/pci/pci-label.c|6 +++---
> 4 files changed, 20 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_acpi.c
>b/drivers/gpu/drm/i915/intel_acpi.c
>index dfff090..7ea00e5 100644
>--- a/drivers/gpu/drm/i915/intel_acpi.c
>+++ b/drivers/gpu/drm/i915/intel_acpi.c
>@@ -35,7 +35,7 @@ static int intel_dsm(acpi_handle handle, int func)
> union acpi_object params[4];
> union acpi_object *obj;
> u32 result;
>- int ret = 0;
>+ acpi_status status;
>
> input.count = 4;
> input.pointer = params;
>@@ -50,8 +50,8 @@ static int intel_dsm(acpi_handle handle, int func)
> params[3].package.count = 0;
> params[3].package.elements = NULL;
>
>- ret = acpi_evaluate_object(handle, "_DSM", &input, &output);
>- if (ret) {
>+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
>+ if (ACPI_FAILURE(status)) {
> DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
> return ret;
> }
>@@ -141,7 +141,8 @@ static void intel_dsm_platform_mux_info(void)
> struct acpi_object_list input;
> union acpi_object params[4];
> union acpi_object *pkg;
>- int i, ret;
>+ acpi_status status;
>+ int i;
>
> input.count = 4;
> input.pointer = params;
>@@ -156,9 +157,9 @@ static void intel_dsm_platform_mux_info(void)
> params[3].package.count = 0;
> params[3].package.elements = NULL;
>
>- ret = acpi_evaluate_object(intel_dsm_priv.dhandle, "_DSM", &input,
>+ acpi_status = acpi_evaluate_object(intel_dsm_priv.dhandle, "_DSM",
>&input,
> &output);
>- if (ret) {
>+ if (ACPI_FAILURE(status)) {
> DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
> goto out;
> }
>diff --git a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
>b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
>index 1291204..3920943 100644
>--- a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
>+++ b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
>@@ -114,14 +114,14 @@ mxm_shadow_dsm(struct nouveau_mxm *mxm, u8
>version)
> struct acpi_buffer retn = { ACPI_ALLOCATE_BUFFER, NULL };
> union acpi_object *obj;
> acpi_handle handle;
>- int ret;
>+ acpi_status status;
>
> handle = ACPI_HANDLE(&device->pdev->dev);
> if (!handle)
> return false;
>
>- ret = acpi_evaluate_object(handle, "_DSM", &list, &retn);
>- if (ret) {
>+ status = acpi_evaluate_object(handle, "_DSM", &list, &retn);
>+ if (ACPI_FAILURE(status)) {
> nv_debug(mxm, "DSM MXMS failed: %d\n", ret);
> return false;
> }
>diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
>b/drivers/gpu/drm/nouveau/nouveau_acpi.c
>index ba0183f..6f810f2 100644
>--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
>+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
>@@ -82,7 +82,8 @@ static int nouveau_optimus_dsm(acpi_handle handle,
>int func, int arg, uint32_t *
> struct acpi_object_list input;
> union acpi_object params[4];
> union acpi_object *obj;
>- int i, err;
>+ acpi_status status;
>+ int i;
> char args_buff[4];
>
> input.count = 4;
>@@ -101,8 +102,8 @@ static int nouveau_optimus_dsm(acpi_handle handle,
>int func, int arg, uint32_t *
> args_buff[i] = (arg >> i * 8) & 0xFF;
> params[3].buffer.pointer = args_buff;
>
>- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
>- if (err) {
>+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
>+ if (ACPI_FAILURE(status)) {
> printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
> return err;
> }
>@@ -134,7 +135,7 @@ static int nouveau_dsm(acpi_handle handle, int
>func, int arg, uint32_t *result)
> struct acpi_object_list input;
> union acpi_object params[4];
> union acpi_object *obj;
>- int err;
>+ acpi_status status;
>
> input.count = 4;
> input.pointer = params;
>@@ -148,8 +149,8 @@ static int nouveau_dsm(acpi_handle handle, int
>func, int arg, uint32_t *result)
> params[3].type = ACPI_TYPE_INTEGER;
> params[3].integer.value = arg;
>
>- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
>- if (err) {
>+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
>+ if (ACPI_FAILURE(status)) {
> printk(KERN_INFO "failed to
Re: [Intel-gfx] [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
On Wed, Aug 07, 2013 at 12:09:32PM +0200, Daniel Vetter wrote:
> This fixes a WARN in i915_gem_free_object when the
> obj->pages_pin_count isn't 0.
>
> v2: Add locking to unmap, noticed by Chris Wilson. Note that even
> though we call unmap with our own dev->struct_mutex held that won't
> result in an immediate deadlock since we never go through the dma_buf
> interfaces for our own, reimported buffers. But it's still easy to
> blow up and anger lockdep, but that's already the case with our ->map
> implementation. Fixing this for real will involve per dma-buf ww mutex
> locking by the callers. And lots of fun. So go with the duct-tape
> approach for now.
>
> Cc: Chris Wilson
> Reported-by: Maarten Lankhorst
> Cc: Maarten Lankhorst
> Tested-by: Armin K. (v1)
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 63ee1a9..f7e1682 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct
> dma_buf_attachment *attachment,
> struct sg_table *sg,
> enum dma_data_direction dir)
> {
> + struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
> +
> + mutex_lock(&obj->base.dev->struct_mutex);
> +
> dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
> sg_free_table(sg);
> kfree(sg);
> +
> + i915_gem_object_unpin_pages(obj);
I am curious - would it logic of first unpinning, and then doing the
dma_unmap_sg
make more sense? As in, in the map path we do:
dma_map
pin
And in here you do the same:
dma_unmap
unpin
But I would have thought that on a unroll you would do it in reverse
order, so:
unpin
dma_unmap
> +
> + mutex_unlock(&obj->base.dev->struct_mutex);
> }
>
> static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> --
> 1.8.3.2
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/3] fbdev no more!
On Sun, Jun 16, 2013 at 04:57:17PM +0200, Daniel Vetter wrote: > Hi all, > > So I've taken a look again at the locking mess in our fbdev support and cried. > Fixing up the console_lock mess around the fbdev notifier will be real work, > semanatically the fbdev layer does lots of stupid things (like the radeon > resume > issue I've just debugged) and the panic notifier is pretty much a lost cause. > > So I've decided to instead rip it all out. It seems to work \o/ When you say 'locking mess in our fbdev support' you mean the general core fbdev driver? Not neccessarily the i915 driver? I am asking b/c that would imply that the other fbdev drivers still hit the locking mess? Thanks! > > Of course a general purpose distro propably wants David's kmscon for any > fallbacks needs and a system compositor to ditch the VT subsystem - atm my > machine here runs with the dummy console so that VT switching between > different > X sessions still works ;-) > > Oh and: At least fedora's boot splash seems to be unhappy about the lack of an > fbdev (it doesn't seem to do anything), which breaks early disk encryption a > bit. The black screen itself shouldn't be a big issue at least for i915, since > with all the fastboot work we can just hang onto the current config and > framebuffer (one missing patch from Chris for the fb preservartion). So as > long > as the bios/grub put up something nice, it'll look ok. > > So just a small step here really, but imo into the right direction. Now, > please > bring on the flames! > > Aside: We can hide the #ifdef mess a bit better in drm/i915 I think, but I'd > like to wait for a bit of feedback first. And one more: This also removes the > console_lock completely from our critical path in suspend/resume! > > One thing I haven't wasted a single thought about is kgdb and panic notifier > support. But since the current code is pretty decently broken already (we have > _tons_ of mutex grabbing and waits in there) I don't think people care that > much > about it anyway. Using a sprite to smash the kgdb/panic output on top of > whatever's currently displaying might be an approach. > > Cheers, Daniel > > Daniel Vetter (3): > drm: Add separate Kconfig option for fbdev helpers > drm/i915: Kconfig option to disable the legacy fbdev support > drm/i915: rename intel_fb.c to intel_fbdev.c > > drivers/gpu/drm/Kconfig | 57 ++- > drivers/gpu/drm/Makefile | 3 +- > drivers/gpu/drm/ast/Kconfig | 1 + > drivers/gpu/drm/cirrus/Kconfig | 1 + > drivers/gpu/drm/exynos/Kconfig | 1 + > drivers/gpu/drm/gma500/Kconfig | 1 + > drivers/gpu/drm/i915/Kconfig | 56 +++ > drivers/gpu/drm/i915/Makefile| 3 +- > drivers/gpu/drm/i915/i915_debugfs.c | 4 +- > drivers/gpu/drm/i915/i915_dma.c | 8 +- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/intel_display.c | 12 +- > drivers/gpu/drm/i915/intel_drv.h | 39 - > drivers/gpu/drm/i915/intel_fb.c | 314 > --- > drivers/gpu/drm/i915/intel_fbdev.c | 314 > +++ > drivers/gpu/drm/mgag200/Kconfig | 1 + > drivers/gpu/drm/nouveau/Kconfig | 1 + > drivers/gpu/drm/omapdrm/Kconfig | 1 + > drivers/gpu/drm/qxl/Kconfig | 1 + > drivers/gpu/drm/shmobile/Kconfig | 1 + > drivers/gpu/drm/tilcdc/Kconfig | 1 + > drivers/gpu/drm/udl/Kconfig | 1 + > drivers/gpu/host1x/drm/Kconfig | 1 + > drivers/staging/imx-drm/Kconfig | 1 + > 24 files changed, 452 insertions(+), 373 deletions(-) > create mode 100644 drivers/gpu/drm/i915/Kconfig > delete mode 100644 drivers/gpu/drm/i915/intel_fb.c > create mode 100644 drivers/gpu/drm/i915/intel_fbdev.c > > -- > 1.7.11.7 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
