Re: r259580 breaks radeonkms

On Wed, Aug 06, 2014 at 06:52:14PM +0200, Roger Pau Monn?? wrote:
> On 06/08/14 16:35, Nathan Whitehorn wrote:
> > 
> > 
> > On 2014-08-06 02:35, Roger Pau Monn?? wrote:
> >> On 06/08/14 02:37, Nathan Whitehorn wrote:
> >>> Kernels with r269580 will panic when loading the radeonkms driver in
> >>> pmap_page_set_memattr(). This probably indicates a bug in radeonkms, but
> >>> the system is unusable in the meantime.
> >>> -Nathan
> >>
> >> I seem to be able to load radeonkms just fine after r269580:
> > 
> > It's possible that it is related to actually using it, rather than
> > loading the module. I was only testing them together. I'm using vt and
> > the panic (page fault in kernel mode) occurs when TTM tries to set
> > memory attributes on some page while starting X. Before the panic, I see
> > all the normal Radeon module messages as you do, so the module seems to
> > have actually loaded correctly.  The KMS console also seems to be
> > functional enough to display the panic message, so I suspect it's X that
> > triggers it.
> > -Nathan
> 
> Please try the attached patch, it seems to solve the panic for me. It
> also includes a fix for Intel i915 gem, which I'm not able to test
> because I don't have the hardware.
> 
> Roger.
> 

> From 9dd3a21d99ff2fc7bf3299359751d2399eee912a Mon Sep 17 00:00:00 2001
> From: Roger Pau Monne <roger.pau_at_citrix.com>
> Date: Wed, 6 Aug 2014 18:16:53 +0200
> Subject: [PATCH] drm: fix usage of vm_phys_fictitious_to_vm_page
> 
> vm_phys_fictitious_to_vm_page should not be called directly, even when
> operating on a range that has been registered using
> vm_phys_fictitious_reg_range. PHYS_TO_VM_PAGE should be used instead
> because on arches that use VM_PHYSSEG_DENSE the page might come
> directly from vm_page_array.
> 
> Reported by: nwhitehorn
> Sponsored by: Citrix Systems R&D
> ---
>  sys/dev/drm2/i915/i915_gem.c |    6 ++++--
>  sys/dev/drm2/ttm/ttm_bo_vm.c |    8 ++++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/sys/dev/drm2/i915/i915_gem.c b/sys/dev/drm2/i915/i915_gem.c
> index a3acb60..6d46207 100644
> --- a/sys/dev/drm2/i915/i915_gem.c
> +++ b/sys/dev/drm2/i915/i915_gem.c
> _at__at_ -1428,8 +1428,10 _at__at_ retry:
>  
>  	obj->fault_mappable = true;
>  	VM_OBJECT_WLOCK(vm_obj);
> -	m = vm_phys_fictitious_to_vm_page(dev->agp->base + obj->gtt_offset +
> -	    offset);
> +	m = PHYS_TO_VM_PAGE(dev->agp->base + obj->gtt_offset + offset);
> +	KASSERT((m->flags & PG_FICTITIOUS) != 0,
> +	    ("physical address %#jx not fictitious",
> +	    (uintmax_t)(dev->agp->base + obj->gtt_offset + offset)));
>  	if (m == NULL) {
>  		VM_OBJECT_WUNLOCK(vm_obj);
>  		cause = 60;
> diff --git a/sys/dev/drm2/ttm/ttm_bo_vm.c b/sys/dev/drm2/ttm/ttm_bo_vm.c
> index 83ec76c..7aa1ac0 100644
> --- a/sys/dev/drm2/ttm/ttm_bo_vm.c
> +++ b/sys/dev/drm2/ttm/ttm_bo_vm.c
> _at__at_ -216,8 +216,12 _at__at_ reserve:
>  	}
>  
>  	if (bo->mem.bus.is_iomem) {
> -		m = vm_phys_fictitious_to_vm_page(bo->mem.bus.base +
> -		    bo->mem.bus.offset + offset);
> +		m = PHYS_TO_VM_PAGE(bo->mem.bus.base + bo->mem.bus.offset +
> +		    offset);
> +		KASSERT((m->flags & PG_FICTITIOUS) != 0,
> +		    ("physical address %#jx not fictitious",
> +		    (uintmax_t)(bo->mem.bus.base + bo->mem.bus.offset
> +		    + offset)));
>  		pmap_page_set_memattr(m, ttm_io_prot(bo->mem.placement));
>  	} else {
>  		ttm = bo->ttm;

This should be fine.  In fact I considered doing similar change some
time ago, but for some reasons decided not to.  Still, it is not clear
why does it break with your changes, in particular, the PCI hole
where the aperture is mapped should be covered by vm_page_array.