Re: ATI/AMD VGA passthru report

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, May 08, 2012 at 01:10:32AM +0200, Pavel Mateja wrote:
> > On Mon, May 07, 2012 at 01:45:23PM +0200, Pavel Matěja wrote:
> >> Hi,
> >> I'm trying to run AMD VGA passthru on latest XEN unstable with latest
> >> kernel.
> >> I already have working setup with ancient xen-devel 4.2 and 2.6.32.x
> >> xenified
> >> kernel. See attached log.
> >>
> >> So I tried just to update and patch xen and kernel, but I had no luck so
> >> far.
> >>
> >> Xen has ati_vbios_patch_respin.txt sent to xen-devel by Wei Huang
> >> recently.
> >> http://lists.xen.org/archives/html/xen-devel/2012-04/msg00291.html
> >>
> >> I tried two kernels
> >> testing-3.5-with-extra and xen/next-3.2 from
> >> git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git
> >> and
> >> git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git
> >> Both with ioperm opcode patch which is required by the ATI patch. See
> >> attachement.
> >>
> >> The xen/next-3.2 branch kernel was able to start booting windows.
> >> With Catalyst driver installed I just saw bluescreen during Windows
> >> boot.
> >> Without Catalyst driver Windows were able to boot but Windows ended
> >> frozen or
> >> USB was not working. It was hard to tell because input devices had no
> >> response
> >> at all.
> >>
> >> The testing-3.5-with-extra (reported as 3.4.0-rc4) just crashed dom0
> >> kernel
> >> during Windows boot. I guess I have to rework the io_bitmap patch
> >> somehow.
> >
> > When you say 'io_bitmap' patch are you referring to these ones:
> >
> > 70a357d xen: implement IO permission bitmap
> > 0c596c5 x86/paravirt: paravirtualize IO permission bitmap
> > 93b7a2a x86: demacro set_iopl_mask()
> >
> > ? I don't think those are in that tag. They are in the kitchensink -
> > #testing
> > - does it work with that branch?
> >
> > The next-3.2 is a bit old. I should actually delete it.
> 
> Hi,
> I mean this patch below.
> I think it was originally here:
> http://old-list-archives.xen.org/archives/html/xen-devel/2009-05/msg01139.html
> Then I was told by you to use devel/ioperm branch which I think doesn't
> exist anymore:
> http://old-list-archives.xen.org/archives/html/xen-devel/2011-11/msg01213.html

It certainly does :-)
> 
> I'm too tired to test #testing branch now. I'll try it tomorrow after some
> sleep.

OK, take your time.

> 
> >> diff --git a/arch/x86/include/asm/paravirt.h

Looks like those patches all squashed together.

> >> b/arch/x86/include/asm/paravirt.h
> >> index aa0f913..259ef7f 100644
> >> --- a/arch/x86/include/asm/paravirt.h
> >> +++ b/arch/x86/include/asm/paravirt.h
> >> @@ -340,11 +340,18 @@ static inline void write_idt_entry(gate_desc *dt,
> >> int entry, const gate_desc *g)
> >>  {
> >>  	PVOP_VCALL3(pv_cpu_ops.write_idt_entry, dt, entry, g);
> >>  }
> >> +
> >>  static inline void set_iopl_mask(unsigned mask)
> >>  {
> >>  	PVOP_VCALL1(pv_cpu_ops.set_iopl_mask, mask);
> >>  }
> >>
> >> +static inline void set_io_bitmap(struct thread_struct *thread,
> >> +                                unsigned long bytes_updated)
> >> +{
> >> +       PVOP_VCALL2(pv_cpu_ops.set_io_bitmap, thread, bytes_updated);
> >> +}
> >> +
> >>  /* The paravirtualized I/O functions */
> >>  static inline void slow_down_io(void)
> >>  {
> >> diff --git a/arch/x86/include/asm/paravirt_types.h
> >> b/arch/x86/include/asm/paravirt_types.h
> >> index 8e8b9a4..96f267c 100644
> >> --- a/arch/x86/include/asm/paravirt_types.h
> >> +++ b/arch/x86/include/asm/paravirt_types.h
> >> @@ -142,6 +142,8 @@ struct pv_cpu_ops {
> >>  	void (*load_sp0)(struct tss_struct *tss, struct thread_struct *t);
> >>
> >>  	void (*set_iopl_mask)(unsigned mask);
> >> +        void (*set_io_bitmap)(struct thread_struct *thread,
> >> +                              unsigned long bytes_updated);
> >>
> >>  	void (*wbinvd)(void);
> >>  	void (*io_delay)(void);
> >> diff --git a/arch/x86/include/asm/processor.h
> >> b/arch/x86/include/asm/processor.h
> >> index 4fa7dcc..62becce 100644
> >> --- a/arch/x86/include/asm/processor.h
> >> +++ b/arch/x86/include/asm/processor.h
> >> @@ -503,6 +503,9 @@ static inline void native_set_iopl_mask(unsigned
> >> mask)
> >>  #endif
> >>  }
> >>
> >> +extern void native_set_io_bitmap(struct thread_struct *thread,
> >> +                                unsigned long updated_bytes);
> >> +
> >>  static inline void
> >>  native_load_sp0(struct tss_struct *tss, struct thread_struct *thread)
> >>  {
> >> @@ -536,6 +539,7 @@ static inline void load_sp0(struct tss_struct *tss,
> >>  }
> >>
> >>  #define set_iopl_mask native_set_iopl_mask
> >> +#define set_io_bitmap native_set_io_bitmap
> >>  #endif /* CONFIG_PARAVIRT */
> >>
> >>  /*
> >> diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
> >> index 8c96897..c372ef0 100644
> >> --- a/arch/x86/kernel/ioport.c
> >> +++ b/arch/x86/kernel/ioport.c
> >> @@ -17,13 +17,29 @@
> >>  #include <linux/bitmap.h>
> >>  #include <asm/syscalls.h>
> >>
> >> +void native_set_io_bitmap(struct thread_struct *t,
> >> +                         unsigned long bytes_updated)
> >> +{
> >> +       struct tss_struct *tss;
> >> +
> >> +       if (!bytes_updated)
> >> +               return;
> >> +
> >> +       tss = &__get_cpu_var(init_tss);
> >> +
> >> +       /* Update the TSS: */
> >> +       if (t->io_bitmap_ptr)
> >> +               memcpy(tss->io_bitmap, t->io_bitmap_ptr, bytes_updated);
> >> +       else
> >> +               memset(tss->io_bitmap, 0xff, bytes_updated);
> >> +}
> >> +
> >>  /*
> >>   * this changes the io permissions bitmap in the current task.
> >>   */
> >>  asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int
> >> turn_on)
> >>  {
> >>  	struct thread_struct *t = &current->thread;
> >> -	struct tss_struct *tss;
> >>  	unsigned int i, max_long, bytes, bytes_updated;
> >>
> >>  	if ((from + num <= from) || (from + num > IO_BITMAP_BITS))
> >> @@ -48,13 +64,13 @@ asmlinkage long sys_ioperm(unsigned long from,
> >> unsigned long num, int turn_on)
> >>  	}
> >>
> >>  	/*
> >> -	 * do it in the per-thread copy and in the TSS ...
> >> +	 * do it in the per-thread copy
> >>  	 *
> >> -	 * Disable preemption via get_cpu() - we must not switch away
> >> +	 * Disable preemption - we must not switch away
> >>  	 * because the ->io_bitmap_max value must match the bitmap
> >>  	 * contents:
> >>  	 */
> >> -	tss = &per_cpu(init_tss, get_cpu());
> >> +        preempt_disable();
> >>
> >>  	if (turn_on)
> >>  		bitmap_clear(t->io_bitmap_ptr, from, num);
> >> @@ -75,10 +91,9 @@ asmlinkage long sys_ioperm(unsigned long from,
> >> unsigned long num, int turn_on)
> >>
> >>  	t->io_bitmap_max = bytes;
> >>
> >> -	/* Update the TSS: */
> >> -	memcpy(tss->io_bitmap, t->io_bitmap_ptr, bytes_updated);
> >> +        set_io_bitmap(t, bytes_updated);
> >>
> >> -	put_cpu();
> >> +        preempt_enable();
> >>
> >>  	return 0;
> >>  }
> >> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> >> index ab13760..aa420e7 100644
> >> --- a/arch/x86/kernel/paravirt.c
> >> +++ b/arch/x86/kernel/paravirt.c
> >> @@ -391,6 +391,7 @@ struct pv_cpu_ops pv_cpu_ops = {
> >>  	.swapgs = native_swapgs,
> >>
> >>  	.set_iopl_mask = native_set_iopl_mask,
> >> +        .set_io_bitmap = native_set_io_bitmap,
> >>  	.io_delay = native_io_delay,
> >>
> >>  	.start_context_switch = paravirt_nop,
> >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> >> index 1d92a5a..e8436cc 100644
> >> --- a/arch/x86/kernel/process.c
> >> +++ b/arch/x86/kernel/process.c
> >> @@ -91,16 +91,12 @@ void exit_thread(void)
> >>  	unsigned long *bp = t->io_bitmap_ptr;
> >>
> >>  	if (bp) {
> >> -		struct tss_struct *tss = &per_cpu(init_tss, get_cpu());
> >> -
> >> +                preempt_disable();
> >>  		t->io_bitmap_ptr = NULL;
> >>  		clear_thread_flag(TIF_IO_BITMAP);
> >> -		/*
> >> -		 * Careful, clear this in the TSS too:
> >> -		 */
> >> -		memset(tss->io_bitmap, 0xff, t->io_bitmap_max);
> >> +                set_io_bitmap(t, t->io_bitmap_max);
> >>  		t->io_bitmap_max = 0;
> >> -		put_cpu();
> >> +                preempt_enable();
> >>  		kfree(bp);
> >>  	}
> >>  }
> >> @@ -237,19 +233,11 @@ void __switch_to_xtra(struct task_struct *prev_p,
> >> struct task_struct *next_p,
> >>  			hard_enable_TSC();
> >>  	}
> >>
> >> -	if (test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
> >> -		/*
> >> -		 * Copy the relevant range of the IO bitmap.
> >> -		 * Normally this is 128 bytes or less:
> >> -		 */
> >> -		memcpy(tss->io_bitmap, next->io_bitmap_ptr,
> >> -		       max(prev->io_bitmap_max, next->io_bitmap_max));
> >> -	} else if (test_tsk_thread_flag(prev_p, TIF_IO_BITMAP)) {
> >> -		/*
> >> -		 * Clear any possible leftover bits:
> >> -		 */
> >> -		memset(tss->io_bitmap, 0xff, prev->io_bitmap_max);
> >> -	}
> >> +        if (test_tsk_thread_flag(next_p, TIF_IO_BITMAP) ||
> >> +            test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> >> +                set_io_bitmap(next,
> >> +                              max(prev->io_bitmap_max,
> >> next->io_bitmap_max));
> >> +
> >>  	propagate_user_return_notify(prev_p, next_p);
> >>  }
> >>
> >> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> >> index 8853204..7a05509 100644
> >> --- a/arch/x86/xen/enlighten.c
> >> +++ b/arch/x86/xen/enlighten.c
> >> @@ -808,6 +808,18 @@ static void xen_set_iopl_mask(unsigned mask)
> >>  	HYPERVISOR_physdev_op(PHYSDEVOP_set_iopl, &set_iopl);
> >>  }
> >>
> >> +static void xen_set_io_bitmap(struct thread_struct *thread,
> >> +	int changed, unsigned long bytes_updated)
> >> +{
> >> +	struct physdev_set_iobitmap set_iobitmap;
> >> +
> >> +	set_xen_guest_handle(set_iobitmap.bitmap,
> >> +		(char *)thread->io_bitmap_ptr);
> >> +	set_iobitmap.nr_ports = thread->io_bitmap_ptr ? IO_BITMAP_BITS : 0;
> >> +	WARN_ON(HYPERVISOR_physdev_op(PHYSDEVOP_set_iobitmap,
> >> +		&set_iobitmap));
> >> +}
> >> +
> >>  static void xen_io_delay(void)
> >>  {
> >>  }
> >> @@ -1117,6 +1129,7 @@ static const struct pv_cpu_ops xen_cpu_ops
> >> __initconst = {
> >>  	.load_sp0 = xen_load_sp0,
> >>
> >>  	.set_iopl_mask = xen_set_iopl_mask,
> >> +	.set_io_bitmap = xen_set_io_bitmap,
> >>  	.io_delay = xen_io_delay,
> >>
> >>  	/* Xen takes care of %gs when switching to usermode for us */
> 
> -- 
> Pavel Mateja