| Message ID | 20200207113958.7320-8-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Add support for protected VMs | expand |
On 07/02/2020 12.39, Christian Borntraeger wrote: > From: Janosch Frank <frankja@linux.ibm.com> > > This add 2 new variants of the UV CALL. > > The first variant handles UV CALLs that might have longer busy > conditions or just need longer when doing partial completion. We should > schedule when necessary. > > The second variant handles UV CALLs that only need the handle but have > no payload (e.g. destroying a VM). We can provide a simple wrapper for > those. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > [borntraeger@de.ibm.com: patch merging, splitting, fixing] > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/uv.h | 59 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h > index 1b97230a57ba..e1cef772fde1 100644 > --- a/arch/s390/include/asm/uv.h > +++ b/arch/s390/include/asm/uv.h > @@ -14,6 +14,7 @@ > #include <linux/types.h> > #include <linux/errno.h> > #include <linux/bug.h> > +#include <linux/sched.h> > #include <asm/page.h> > #include <asm/gmap.h> > > @@ -91,6 +92,19 @@ struct uv_cb_cfs { > u64 paddr; > } __packed __aligned(8); > > +/* > + * A common UV call struct for calls that take no payload > + * Examples: > + * Destroy cpu/config > + * Verify > + */ > +struct uv_cb_nodata { > + struct uv_cb_header header; > + u64 reserved08[2]; > + u64 handle; > + u64 reserved20[4]; > +} __packed __aligned(8); > + > struct uv_cb_share { > struct uv_cb_header header; > u64 reserved08[3]; > @@ -98,6 +112,31 @@ struct uv_cb_share { > u64 reserved28; > } __packed __aligned(8); > > +/* > + * Low level uv_call that takes r1 and r2 as parameter and avoids > + * stalls for long running busy conditions by doing schedule > + */ > +static inline int uv_call_sched(unsigned long r1, unsigned long r2) > +{ > + int cc; > + > + do { > + asm volatile( > + "0: .insn rrf,0xB9A40000,%[r1],%[r2],0,0\n" > + " ipm %[cc]\n" > + " srl %[cc],28\n" Maybe remove one TAB before "ipm" and "srl" ? Apart from that, patch looks fine to me now. Reviewed-by: Thomas Huth <thuth@redhat.com>
On 07.02.20 15:34, Thomas Huth wrote: [...] +static inline int uv_call_sched(unsigned long r1, unsigned long r2) >> +{ >> + int cc; >> + >> + do { >> + asm volatile( >> + "0: .insn rrf,0xB9A40000,%[r1],%[r2],0,0\n" >> + " ipm %[cc]\n" >> + " srl %[cc],28\n" > > Maybe remove one TAB before "ipm" and "srl" ? ack > > Apart from that, patch looks fine to me now. > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On Fri, 7 Feb 2020 06:39:30 -0500 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > From: Janosch Frank <frankja@linux.ibm.com> > > This add 2 new variants of the UV CALL. "This adds two new helper functions for doing UV CALLs." ? > > The first variant handles UV CALLs that might have longer busy > conditions or just need longer when doing partial completion. We should > schedule when necessary. > > The second variant handles UV CALLs that only need the handle but have > no payload (e.g. destroying a VM). We can provide a simple wrapper for > those. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > [borntraeger@de.ibm.com: patch merging, splitting, fixing] > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/uv.h | 59 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 10.02.20 13:16, Cornelia Huck wrote: > On Fri, 7 Feb 2020 06:39:30 -0500 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> From: Janosch Frank <frankja@linux.ibm.com> >> >> This add 2 new variants of the UV CALL. > > "This adds two new helper functions for doing UV CALLs." ack > > ? > >> >> The first variant handles UV CALLs that might have longer busy >> conditions or just need longer when doing partial completion. We should >> schedule when necessary. >> >> The second variant handles UV CALLs that only need the handle but have >> no payload (e.g. destroying a VM). We can provide a simple wrapper for >> those. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> [borntraeger@de.ibm.com: patch merging, splitting, fixing] >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/include/asm/uv.h | 59 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> >
On 07.02.20 12:39, Christian Borntraeger wrote: > From: Janosch Frank <frankja@linux.ibm.com> > > This add 2 new variants of the UV CALL. > > The first variant handles UV CALLs that might have longer busy > conditions or just need longer when doing partial completion. We should > schedule when necessary. > > The second variant handles UV CALLs that only need the handle but have > no payload (e.g. destroying a VM). We can provide a simple wrapper for > those. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > [borntraeger@de.ibm.com: patch merging, splitting, fixing] > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/uv.h | 59 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h > index 1b97230a57ba..e1cef772fde1 100644 > --- a/arch/s390/include/asm/uv.h > +++ b/arch/s390/include/asm/uv.h > @@ -14,6 +14,7 @@ > #include <linux/types.h> > #include <linux/errno.h> > #include <linux/bug.h> > +#include <linux/sched.h> > #include <asm/page.h> > #include <asm/gmap.h> > > @@ -91,6 +92,19 @@ struct uv_cb_cfs { > u64 paddr; > } __packed __aligned(8); > > +/* > + * A common UV call struct for calls that take no payload > + * Examples: > + * Destroy cpu/config > + * Verify > + */ > +struct uv_cb_nodata { > + struct uv_cb_header header; > + u64 reserved08[2]; > + u64 handle; > + u64 reserved20[4]; > +} __packed __aligned(8); > + > struct uv_cb_share { > struct uv_cb_header header; > u64 reserved08[3]; > @@ -98,6 +112,31 @@ struct uv_cb_share { > u64 reserved28; > } __packed __aligned(8); > > +/* > + * Low level uv_call that takes r1 and r2 as parameter and avoids > + * stalls for long running busy conditions by doing schedule > + */ > +static inline int uv_call_sched(unsigned long r1, unsigned long r2) > +{ > + int cc; > + > + do { > + asm volatile( > + "0: .insn rrf,0xB9A40000,%[r1],%[r2],0,0\n" label not necessary > + " ipm %[cc]\n" > + " srl %[cc],28\n" > + : [cc] "=d" (cc) > + : [r1] "d" (r1), [r2] "d" (r2) > + : "memory", "cc"); I was wondering if we could reuse uv_call() - something like static inline int __uv_call(unsigned long r1, unsigned long r2) { int cc; asm volatile( " .insn rrf,0xB9A40000,%[r1],%[r2],0,0\n" " ipm %[cc]\n" " srl %[cc],28\n" : [cc] "=d" (cc) : [r1] "a" (r1), [r2] "a" (r2) : "memory", "cc"); return cc; } static inline int uv_call(unsigned long r1, unsigned long r2) { int rc; do { cc = __uv_call(unsigned long r1, unsigned long r2); } while (cc > 1) return rc; } static inline int uv_call_sched(unsigned long r1, unsigned long r2) { int rc; do { cc = __uv_call(unsigned long r1, unsigned long r2); cond_resched(); } while (rc > 1) return rc; } > + if (need_resched()) > + schedule(); cond_resched(); > + } while (cc > 1); > + return cc; > +} > + > +/* > + * Low level uv_call that takes r1 and r2 as parameter > + */ This "r1 and r2" does not sound like relevant news. Same for the other variant above. > static inline int uv_call(unsigned long r1, unsigned long r2) > { > int cc; > @@ -113,6 +152,26 @@ static inline int uv_call(unsigned long r1, unsigned long r2) > return cc; > } > > +/* > + * special variant of uv_call that only transports the cpu or guest > + * handle and the command, like destroy or verify. > + */ > +static inline int uv_cmd_nodata(u64 handle, u16 cmd, u32 *ret) uv_call_sched_simple() ? > +{ > + int rc; > + struct uv_cb_nodata uvcb = { > + .header.cmd = cmd, > + .header.len = sizeof(uvcb), > + .handle = handle, > + }; > + > + WARN(!handle, "No handle provided to Ultravisor call cmd %x\n", cmd); > + rc = uv_call_sched(0, (u64)&uvcb); > + if (ret) > + *ret = *(u32 *)&uvcb.header.rc; > + return rc ? -EINVAL : 0; > +} > + > struct uv_info { > unsigned long inst_calls_list[4]; > unsigned long uv_base_stor_len; >
On 14.02.20 19:28, David Hildenbrand wrote: > On 07.02.20 12:39, Christian Borntraeger wrote: >> From: Janosch Frank <frankja@linux.ibm.com> >> >> This add 2 new variants of the UV CALL. >> >> The first variant handles UV CALLs that might have longer busy >> conditions or just need longer when doing partial completion. We should >> schedule when necessary. >> >> The second variant handles UV CALLs that only need the handle but have >> no payload (e.g. destroying a VM). We can provide a simple wrapper for >> those. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> [borntraeger@de.ibm.com: patch merging, splitting, fixing] >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/include/asm/uv.h | 59 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> >> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h >> index 1b97230a57ba..e1cef772fde1 100644 >> --- a/arch/s390/include/asm/uv.h >> +++ b/arch/s390/include/asm/uv.h >> @@ -14,6 +14,7 @@ >> #include <linux/types.h> >> #include <linux/errno.h> >> #include <linux/bug.h> >> +#include <linux/sched.h> >> #include <asm/page.h> >> #include <asm/gmap.h> >> >> @@ -91,6 +92,19 @@ struct uv_cb_cfs { >> u64 paddr; >> } __packed __aligned(8); >> >> +/* >> + * A common UV call struct for calls that take no payload >> + * Examples: >> + * Destroy cpu/config >> + * Verify >> + */ >> +struct uv_cb_nodata { >> + struct uv_cb_header header; >> + u64 reserved08[2]; >> + u64 handle; >> + u64 reserved20[4]; >> +} __packed __aligned(8); >> + >> struct uv_cb_share { >> struct uv_cb_header header; >> u64 reserved08[3]; >> @@ -98,6 +112,31 @@ struct uv_cb_share { >> u64 reserved28; >> } __packed __aligned(8); >> >> +/* >> + * Low level uv_call that takes r1 and r2 as parameter and avoids >> + * stalls for long running busy conditions by doing schedule >> + */ >> +static inline int uv_call_sched(unsigned long r1, unsigned long r2) >> +{ >> + int cc; >> + >> + do { >> + asm volatile( >> + "0: .insn rrf,0xB9A40000,%[r1],%[r2],0,0\n" > > label not necessary ack > >> + " ipm %[cc]\n" >> + " srl %[cc],28\n" >> + : [cc] "=d" (cc) >> + : [r1] "d" (r1), [r2] "d" (r2) >> + : "memory", "cc"); > > I was wondering if we could reuse uv_call() - something like > > static inline int __uv_call(unsigned long r1, unsigned long r2) > { > int cc; > > asm volatile( > " .insn rrf,0xB9A40000,%[r1],%[r2],0,0\n" > " ipm %[cc]\n" > " srl %[cc],28\n" > : [cc] "=d" (cc) > : [r1] "a" (r1), [r2] "a" (r2) > : "memory", "cc"); > return cc; > } > > static inline int uv_call(unsigned long r1, unsigned long r2) > { > int rc; > > do { > cc = __uv_call(unsigned long r1, unsigned long r2); > } while (cc > 1) > return rc; This will likely generate less efficient assembly code but it is certainly easier to read. WIll change. > } > > static inline int uv_call_sched(unsigned long r1, unsigned long r2) > { > int rc; > > do { > cc = __uv_call(unsigned long r1, unsigned long r2); > cond_resched(); > } while (rc > 1) > return rc; > } > >> + if (need_resched()) >> + schedule(); > > cond_resched(); ack > >> + } while (cc > 1); >> + return cc; >> +} >> + >> +/* >> + * Low level uv_call that takes r1 and r2 as parameter >> + */ > > This "r1 and r2" does not sound like relevant news. Same for the other > variant above. > >> static inline int uv_call(unsigned long r1, unsigned long r2) >> { >> int cc; >> @@ -113,6 +152,26 @@ static inline int uv_call(unsigned long r1, unsigned long r2) >> return cc; >> } >> >> +/* >> + * special variant of uv_call that only transports the cpu or guest >> + * handle and the command, like destroy or verify. >> + */ >> +static inline int uv_cmd_nodata(u64 handle, u16 cmd, u32 *ret) > > uv_call_sched_simple() ? I think nodata is actually a better description
diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h index 1b97230a57ba..e1cef772fde1 100644 --- a/arch/s390/include/asm/uv.h +++ b/arch/s390/include/asm/uv.h @@ -14,6 +14,7 @@ #include <linux/types.h> #include <linux/errno.h> #include <linux/bug.h> +#include <linux/sched.h> #include <asm/page.h> #include <asm/gmap.h> @@ -91,6 +92,19 @@ struct uv_cb_cfs { u64 paddr; } __packed __aligned(8); +/* + * A common UV call struct for calls that take no payload + * Examples: + * Destroy cpu/config + * Verify + */ +struct uv_cb_nodata { + struct uv_cb_header header; + u64 reserved08[2]; + u64 handle; + u64 reserved20[4]; +} __packed __aligned(8); + struct uv_cb_share { struct uv_cb_header header; u64 reserved08[3]; @@ -98,6 +112,31 @@ struct uv_cb_share { u64 reserved28; } __packed __aligned(8); +/* + * Low level uv_call that takes r1 and r2 as parameter and avoids + * stalls for long running busy conditions by doing schedule + */ +static inline int uv_call_sched(unsigned long r1, unsigned long r2) +{ + int cc; + + do { + asm volatile( + "0: .insn rrf,0xB9A40000,%[r1],%[r2],0,0\n" + " ipm %[cc]\n" + " srl %[cc],28\n" + : [cc] "=d" (cc) + : [r1] "d" (r1), [r2] "d" (r2) + : "memory", "cc"); + if (need_resched()) + schedule(); + } while (cc > 1); + return cc; +} + +/* + * Low level uv_call that takes r1 and r2 as parameter + */ static inline int uv_call(unsigned long r1, unsigned long r2) { int cc; @@ -113,6 +152,26 @@ static inline int uv_call(unsigned long r1, unsigned long r2) return cc; } +/* + * special variant of uv_call that only transports the cpu or guest + * handle and the command, like destroy or verify. + */ +static inline int uv_cmd_nodata(u64 handle, u16 cmd, u32 *ret) +{ + int rc; + struct uv_cb_nodata uvcb = { + .header.cmd = cmd, + .header.len = sizeof(uvcb), + .handle = handle, + }; + + WARN(!handle, "No handle provided to Ultravisor call cmd %x\n", cmd); + rc = uv_call_sched(0, (u64)&uvcb); + if (ret) + *ret = *(u32 *)&uvcb.header.rc; + return rc ? -EINVAL : 0; +} + struct uv_info { unsigned long inst_calls_list[4]; unsigned long uv_base_stor_len;