Re: [PATCH v4 4/9] powerpc/vas: Return paste instruction failure if no active window

2022-02-22 Thread Haren Myneni
On Wed, 2022-02-23 at 17:05 +1000, Nicholas Piggin wrote:
> Excerpts from Haren Myneni's message of February 20, 2022 5:58 am:
> > The VAS window may not be active if the system looses credits and
> > the NX generates page fault when it receives request on unmap
> > paste address.
> > 
> > The kernel handles the fault by remap new paste address if the
> > window is active again, Otherwise return the paste instruction
> > failure if the executed instruction that caused the fault was
> > a paste.
> 
> Looks good, thanks for fixin the SIGBUS thing, was that my
> fault? I vaguely remember writing some of this patch :P

Thanks for your reviews on all patches. 

No, it was my fault not handling the -EGAIN error. 

> 
> Thanks,
> Nick
> 
> > Signed-off-by: Nicholas Piggin 
> > Signed-off-by: Haren Myneni 
> > ---
> >  arch/powerpc/include/asm/ppc-opcode.h   |  2 +
> >  arch/powerpc/platforms/book3s/vas-api.c | 55
> > -
> >  2 files changed, 56 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> > b/arch/powerpc/include/asm/ppc-opcode.h
> > index 9675303b724e..82f1f0041c6f 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -262,6 +262,8 @@
> >  #define PPC_INST_MFSPR_PVR 0x7c1f42a6
> >  #define PPC_INST_MFSPR_PVR_MASK0xfc1e
> >  #define PPC_INST_MTMSRD0x7c000164
> > +#define PPC_INST_PASTE 0x7c20070d
> > +#define PPC_INST_PASTE_MASK0xfc2007ff
> >  #define PPC_INST_POPCNTB   0x7cf4
> >  #define PPC_INST_POPCNTB_MASK  0xfc0007fe
> >  #define PPC_INST_RFEBB 0x4c000124
> > diff --git a/arch/powerpc/platforms/book3s/vas-api.c
> > b/arch/powerpc/platforms/book3s/vas-api.c
> > index f359e7b2bf90..f3e421511ea6 100644
> > --- a/arch/powerpc/platforms/book3s/vas-api.c
> > +++ b/arch/powerpc/platforms/book3s/vas-api.c
> > @@ -351,6 +351,41 @@ static int coproc_release(struct inode *inode,
> > struct file *fp)
> > return 0;
> >  }
> >  
> > +/*
> > + * If the executed instruction that caused the fault was a paste,
> > then
> > + * clear regs CR0[EQ], advance NIP, and return 0. Else return
> > error code.
> > + */
> > +static int do_fail_paste(void)
> > +{
> > +   struct pt_regs *regs = current->thread.regs;
> > +   u32 instword;
> > +
> > +   if (WARN_ON_ONCE(!regs))
> > +   return -EINVAL;
> > +
> > +   if (WARN_ON_ONCE(!user_mode(regs)))
> > +   return -EINVAL;
> > +
> > +   /*
> > +* If we couldn't translate the instruction, the driver should
> > +* return success without handling the fault, it will be
> > retried
> > +* or the instruction fetch will fault.
> > +*/
> > +   if (get_user(instword, (u32 __user *)(regs->nip)))
> > +   return -EAGAIN;
> > +
> > +   /*
> > +* Not a paste instruction, driver may fail the fault.
> > +*/
> > +   if ((instword & PPC_INST_PASTE_MASK) != PPC_INST_PASTE)
> > +   return -ENOENT;
> > +
> > +   regs->ccr &= ~0xe000;   /* Clear CR0[0-2] to fail paste */
> > +   regs_add_return_ip(regs, 4);/* Emulate the paste */
> > +
> > +   return 0;
> > +}
> > +
> >  /*
> >   * This fault handler is invoked when the core generates page
> > fault on
> >   * the paste address. Happens if the kernel closes window in
> > hypervisor
> > @@ -408,9 +443,27 @@ static vm_fault_t vas_mmap_fault(struct
> > vm_fault *vmf)
> > }
> > mutex_unlock(>task_ref.mmap_mutex);
> >  
> > -   return VM_FAULT_SIGBUS;
> > +   /*
> > +* Received this fault due to closing the actual window.
> > +* It can happen during migration or lost credits.
> > +* Since no mapping, return the paste instruction failure
> > +* to the user space.
> > +*/
> > +   ret = do_fail_paste();
> > +   /*
> > +* The user space can retry several times until success (needed
> > +* for migration) or should fallback to SW compression or
> > +* manage with the existing open windows if available.
> > +* Looking at sysfs interface, it can determine whether these
> > +* failures are coming during migration or core removal:
> > +* nr_used_credits > nr_total_credits when lost credits
> > +*/
> > +   if (!ret || (ret == -EAGAIN))
> > +   return VM_FAULT_NOPAGE;
> >  
> > +   return VM_FAULT_SIGBUS;
> >  }
> > +
> >  static const struct vm_operations_struct vas_vm_ops = {
> > .fault = vas_mmap_fault,
> >  };
> > -- 
> > 2.27.0
> > 
> > 
> > 



Re: [PATCH v4 4/9] powerpc/vas: Return paste instruction failure if no active window

2022-02-22 Thread Nicholas Piggin
Excerpts from Haren Myneni's message of February 20, 2022 5:58 am:
> 
> The VAS window may not be active if the system looses credits and
> the NX generates page fault when it receives request on unmap
> paste address.
> 
> The kernel handles the fault by remap new paste address if the
> window is active again, Otherwise return the paste instruction
> failure if the executed instruction that caused the fault was
> a paste.

Looks good, thanks for fixin the SIGBUS thing, was that my
fault? I vaguely remember writing some of this patch :P

Thanks,
Nick

> 
> Signed-off-by: Nicholas Piggin 
> Signed-off-by: Haren Myneni 
> ---
>  arch/powerpc/include/asm/ppc-opcode.h   |  2 +
>  arch/powerpc/platforms/book3s/vas-api.c | 55 -
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h 
> b/arch/powerpc/include/asm/ppc-opcode.h
> index 9675303b724e..82f1f0041c6f 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -262,6 +262,8 @@
>  #define PPC_INST_MFSPR_PVR   0x7c1f42a6
>  #define PPC_INST_MFSPR_PVR_MASK  0xfc1e
>  #define PPC_INST_MTMSRD  0x7c000164
> +#define PPC_INST_PASTE   0x7c20070d
> +#define PPC_INST_PASTE_MASK  0xfc2007ff
>  #define PPC_INST_POPCNTB 0x7cf4
>  #define PPC_INST_POPCNTB_MASK0xfc0007fe
>  #define PPC_INST_RFEBB   0x4c000124
> diff --git a/arch/powerpc/platforms/book3s/vas-api.c 
> b/arch/powerpc/platforms/book3s/vas-api.c
> index f359e7b2bf90..f3e421511ea6 100644
> --- a/arch/powerpc/platforms/book3s/vas-api.c
> +++ b/arch/powerpc/platforms/book3s/vas-api.c
> @@ -351,6 +351,41 @@ static int coproc_release(struct inode *inode, struct 
> file *fp)
>   return 0;
>  }
>  
> +/*
> + * If the executed instruction that caused the fault was a paste, then
> + * clear regs CR0[EQ], advance NIP, and return 0. Else return error code.
> + */
> +static int do_fail_paste(void)
> +{
> + struct pt_regs *regs = current->thread.regs;
> + u32 instword;
> +
> + if (WARN_ON_ONCE(!regs))
> + return -EINVAL;
> +
> + if (WARN_ON_ONCE(!user_mode(regs)))
> + return -EINVAL;
> +
> + /*
> +  * If we couldn't translate the instruction, the driver should
> +  * return success without handling the fault, it will be retried
> +  * or the instruction fetch will fault.
> +  */
> + if (get_user(instword, (u32 __user *)(regs->nip)))
> + return -EAGAIN;
> +
> + /*
> +  * Not a paste instruction, driver may fail the fault.
> +  */
> + if ((instword & PPC_INST_PASTE_MASK) != PPC_INST_PASTE)
> + return -ENOENT;
> +
> + regs->ccr &= ~0xe000;   /* Clear CR0[0-2] to fail paste */
> + regs_add_return_ip(regs, 4);/* Emulate the paste */
> +
> + return 0;
> +}
> +
>  /*
>   * This fault handler is invoked when the core generates page fault on
>   * the paste address. Happens if the kernel closes window in hypervisor
> @@ -408,9 +443,27 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf)
>   }
>   mutex_unlock(>task_ref.mmap_mutex);
>  
> - return VM_FAULT_SIGBUS;
> + /*
> +  * Received this fault due to closing the actual window.
> +  * It can happen during migration or lost credits.
> +  * Since no mapping, return the paste instruction failure
> +  * to the user space.
> +  */
> + ret = do_fail_paste();
> + /*
> +  * The user space can retry several times until success (needed
> +  * for migration) or should fallback to SW compression or
> +  * manage with the existing open windows if available.
> +  * Looking at sysfs interface, it can determine whether these
> +  * failures are coming during migration or core removal:
> +  * nr_used_credits > nr_total_credits when lost credits
> +  */
> + if (!ret || (ret == -EAGAIN))
> + return VM_FAULT_NOPAGE;
>  
> + return VM_FAULT_SIGBUS;
>  }
> +
>  static const struct vm_operations_struct vas_vm_ops = {
>   .fault = vas_mmap_fault,
>  };
> -- 
> 2.27.0
> 
> 
>