Re: [Xen-devel] Re: how to handle paged hypercall args?

[Keir Fraser <keir@xxxxxxx>]

On 02/12/2010 10:11, "Olaf Hering" <olaf@xxxxxxxxx> wrote:

> On Thu, Nov 18, Keir Fraser wrote:
> 
>> I've done something along these lines now as xen-unstable:22402. It actually
>> seems to work okay! So you can go ahead and use waitqueues in __hvm_copy()
>> now.
> 
> This is my first attempt to do it.
> It crashed Xen on the very first try in a spectacular way. But it
> happend only once for some reason.
> See my other mail.

Firstly, the usage of waitqueues is broken. The waitqueue_head should be
shared with code that pages in, so that vcpus can be *woken* at some point
after they start waiting. As it is, if a vcpu does sleep on its local
waitqueue_head, it will never wake. You might start with a single global
waitqueue_head and wake everyone on it every time a page (or maybe page
batch) is paged in. More sophisticated might be to hash page numbers into a
array of waitqueue_heads, to reduce false wakeups. This is all similar to
Linux waitqueues by the way -- your current code would be just as broken in
Linux as it is Xen.

Secondly, you should be able to hide the waiting inside __hvm_copy(). I
doubt you really need to touch the callers.

 -- Keir

> 
> Olaf
> 
> --- xen-unstable.hg-4.1.22447.orig/xen/arch/x86/hvm/hvm.c
> +++ xen-unstable.hg-4.1.22447/xen/arch/x86/hvm/hvm.c
> @@ -1986,69 +1986,117 @@ static enum hvm_copy_result __hvm_copy(
>  enum hvm_copy_result hvm_copy_to_guest_phys(
>      paddr_t paddr, void *buf, int size)
>  {
> -    return __hvm_copy(buf, paddr, size,
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, paddr, size,
>                        HVMCOPY_to_guest | HVMCOPY_fault | HVMCOPY_phys,
> -                      0);
> +                      0)) != HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  enum hvm_copy_result hvm_copy_from_guest_phys(
>      void *buf, paddr_t paddr, int size)
>  {
> -    return __hvm_copy(buf, paddr, size,
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, paddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_fault | HVMCOPY_phys,
> -                      0);
> +                      0)) != HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  enum hvm_copy_result hvm_copy_to_guest_virt(
>      unsigned long vaddr, void *buf, int size, uint32_t pfec)
>  {
> -    return __hvm_copy(buf, vaddr, size,
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_to_guest | HVMCOPY_fault | HVMCOPY_virt,
> -                      PFEC_page_present | PFEC_write_access | pfec);
> +                      PFEC_page_present | PFEC_write_access | pfec)) !=
> HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  enum hvm_copy_result hvm_copy_from_guest_virt(
>      void *buf, unsigned long vaddr, int size, uint32_t pfec)
>  {
> -    return __hvm_copy(buf, vaddr, size,
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | pfec)) != HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  enum hvm_copy_result hvm_fetch_from_guest_virt(
>      void *buf, unsigned long vaddr, int size, uint32_t pfec)
>  {
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
>      if ( hvm_nx_enabled(current) )
>          pfec |= PFEC_insn_fetch;
> -    return __hvm_copy(buf, vaddr, size,
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | pfec)) != HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  enum hvm_copy_result hvm_copy_to_guest_virt_nofault(
>      unsigned long vaddr, void *buf, int size, uint32_t pfec)
>  {
> -    return __hvm_copy(buf, vaddr, size,
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_to_guest | HVMCOPY_no_fault | HVMCOPY_virt,
> -                      PFEC_page_present | PFEC_write_access | pfec);
> +                      PFEC_page_present | PFEC_write_access | pfec)) !=
> HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
>      void *buf, unsigned long vaddr, int size, uint32_t pfec)
>  {
> -    return __hvm_copy(buf, vaddr, size,
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_no_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | pfec)) != HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  enum hvm_copy_result hvm_fetch_from_guest_virt_nofault(
>      void *buf, unsigned long vaddr, int size, uint32_t pfec)
>  {
> +    enum hvm_copy_result res;
> +    struct waitqueue_head wq;
>      if ( hvm_nx_enabled(current) )
>          pfec |= PFEC_insn_fetch;
> -    return __hvm_copy(buf, vaddr, size,
> +    init_waitqueue_head(&wq);
> +
> +    wait_event(wq, (
> +    res = __hvm_copy(buf, vaddr, size,
>                        HVMCOPY_from_guest | HVMCOPY_no_fault | HVMCOPY_virt,
> -                      PFEC_page_present | pfec);
> +                      PFEC_page_present | pfec)) != HVMCOPY_gfn_paged_out);
> +    return res;
>  }
>  
>  unsigned long copy_to_user_hvm(void *to, const void *from, unsigned int len)



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel