| Message ID | 1585211927-784-1-git-send-email-linuxram@us.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] powerpc/XIVE: SVM: share the event-queue page with the Hypervisor. | expand |
On 3/26/20 9:38 AM, Ram Pai wrote: > XIVE interrupt controller use an Event Queue (EQ) to enqueue event The XIVE interrupt controller uses ... (my bad) > notifications when an exception occurs. The EQ is a single memory page > provided by the O/S defining a circular buffer, one per server and > priority couple. > > On baremetal, the EQ page is configured with an OPAL call. On pseries, > an extra hop is necessary and the guest OS uses the hcall > H_INT_SET_QUEUE_CONFIG to configure the XIVE interrupt controller. > > The XIVE controller being Hypervisor privileged, it will not be allowed > to enqueue event notifications for a Secure VM unless the EQ pages are > shared by the Secure VM. > > Hypervisor/Ultravisor still requires support for the TIMA and ESB page > fault handlers. Until this is complete, QEMU can use the emulated XIVE > device for Secure VMs, option "kernel_irqchip=off" on the QEMU pseries > machine. > > Cc: kvm-ppc@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com> > Cc: Michael Anderson <andmike@linux.ibm.com> > Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > Cc: Alexey Kardashevskiy <aik@ozlabs.ru> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Greg Kurz <groug@kaod.org> > Cc: Cedric Le Goater <clg@fr.ibm.com> clg@fr.ibm.com is insecure. Please use clg@kaod.org. > Cc: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Ram Pai <linuxram@us.ibm.com> Reviewed-by: Cedric Le Goater <clg@kaod.org> Thanks, C. > > v2: better description of the patch from Cedric. > --- > arch/powerpc/sysdev/xive/spapr.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c > index 55dc61c..608b52f 100644 > --- a/arch/powerpc/sysdev/xive/spapr.c > +++ b/arch/powerpc/sysdev/xive/spapr.c > @@ -26,6 +26,8 @@ > #include <asm/xive.h> > #include <asm/xive-regs.h> > #include <asm/hvcall.h> > +#include <asm/svm.h> > +#include <asm/ultravisor.h> > > #include "xive-internal.h" > > @@ -501,6 +503,9 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio, > rc = -EIO; > } else { > q->qpage = qpage; > + if (is_secure_guest()) > + uv_share_page(PHYS_PFN(qpage_phys), > + 1 << xive_alloc_order(order)); > } > fail: > return rc; > @@ -534,6 +539,8 @@ static void xive_spapr_cleanup_queue(unsigned int cpu, struct xive_cpu *xc, > hw_cpu, prio); > > alloc_order = xive_alloc_order(xive_queue_shift); > + if (is_secure_guest()) > + uv_unshare_page(PHYS_PFN(__pa(q->qpage)), 1 << alloc_order); > free_pages((unsigned long)q->qpage, alloc_order); > q->qpage = NULL; > } >
On Thu, 26 Mar 2020 01:38:47 -0700 Ram Pai <linuxram@us.ibm.com> wrote: > XIVE interrupt controller use an Event Queue (EQ) to enqueue event > notifications when an exception occurs. The EQ is a single memory page > provided by the O/S defining a circular buffer, one per server and > priority couple. > > On baremetal, the EQ page is configured with an OPAL call. On pseries, > an extra hop is necessary and the guest OS uses the hcall > H_INT_SET_QUEUE_CONFIG to configure the XIVE interrupt controller. > > The XIVE controller being Hypervisor privileged, it will not be allowed > to enqueue event notifications for a Secure VM unless the EQ pages are > shared by the Secure VM. > > Hypervisor/Ultravisor still requires support for the TIMA and ESB page > fault handlers. Until this is complete, QEMU can use the emulated XIVE > device for Secure VMs, option "kernel_irqchip=off" on the QEMU pseries > machine. > > Cc: kvm-ppc@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com> > Cc: Michael Anderson <andmike@linux.ibm.com> > Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > Cc: Alexey Kardashevskiy <aik@ozlabs.ru> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Greg Kurz <groug@kaod.org> > Cc: Cedric Le Goater <clg@fr.ibm.com> > Cc: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > v2: better description of the patch from Cedric. > --- Reviewed-by: Greg Kurz <groug@kaod.org> > arch/powerpc/sysdev/xive/spapr.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c > index 55dc61c..608b52f 100644 > --- a/arch/powerpc/sysdev/xive/spapr.c > +++ b/arch/powerpc/sysdev/xive/spapr.c > @@ -26,6 +26,8 @@ > #include <asm/xive.h> > #include <asm/xive-regs.h> > #include <asm/hvcall.h> > +#include <asm/svm.h> > +#include <asm/ultravisor.h> > > #include "xive-internal.h" > > @@ -501,6 +503,9 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio, > rc = -EIO; > } else { > q->qpage = qpage; > + if (is_secure_guest()) > + uv_share_page(PHYS_PFN(qpage_phys), > + 1 << xive_alloc_order(order)); > } > fail: > return rc; > @@ -534,6 +539,8 @@ static void xive_spapr_cleanup_queue(unsigned int cpu, struct xive_cpu *xc, > hw_cpu, prio); > > alloc_order = xive_alloc_order(xive_queue_shift); > + if (is_secure_guest()) > + uv_unshare_page(PHYS_PFN(__pa(q->qpage)), 1 << alloc_order); > free_pages((unsigned long)q->qpage, alloc_order); > q->qpage = NULL; > }
Hi Ram, Ram Pai <linuxram@us.ibm.com> writes: > diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c > index 55dc61c..608b52f 100644 > --- a/arch/powerpc/sysdev/xive/spapr.c > +++ b/arch/powerpc/sysdev/xive/spapr.c > @@ -26,6 +26,8 @@ > #include <asm/xive.h> > #include <asm/xive-regs.h> > #include <asm/hvcall.h> > +#include <asm/svm.h> > +#include <asm/ultravisor.h> > > #include "xive-internal.h" > > @@ -501,6 +503,9 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio, > rc = -EIO; > } else { > q->qpage = qpage; > + if (is_secure_guest()) > + uv_share_page(PHYS_PFN(qpage_phys), > + 1 << xive_alloc_order(order)); If I understand this correctly, you're passing the number of bytes of the queue to uv_share_page(), but that ultracall expects the number of pages to be shared. > } > fail: > return rc; > @@ -534,6 +539,8 @@ static void xive_spapr_cleanup_queue(unsigned int cpu, struct xive_cpu *xc, > hw_cpu, prio); > > alloc_order = xive_alloc_order(xive_queue_shift); > + if (is_secure_guest()) > + uv_unshare_page(PHYS_PFN(__pa(q->qpage)), 1 << alloc_order); > free_pages((unsigned long)q->qpage, alloc_order); > q->qpage = NULL; > } Same problem here.
On Tue, Mar 31, 2020 at 08:53:07PM -0300, Thiago Jung Bauermann wrote: > > Hi Ram, > > Ram Pai <linuxram@us.ibm.com> writes: > > > diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c > > index 55dc61c..608b52f 100644 > > --- a/arch/powerpc/sysdev/xive/spapr.c > > +++ b/arch/powerpc/sysdev/xive/spapr.c > > @@ -26,6 +26,8 @@ > > #include <asm/xive.h> > > #include <asm/xive-regs.h> > > #include <asm/hvcall.h> > > +#include <asm/svm.h> > > +#include <asm/ultravisor.h> > > > > #include "xive-internal.h" > > > > @@ -501,6 +503,9 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio, > > rc = -EIO; > > } else { > > q->qpage = qpage; > > + if (is_secure_guest()) > > + uv_share_page(PHYS_PFN(qpage_phys), > > + 1 << xive_alloc_order(order)); > > If I understand this correctly, you're passing the number of bytes of > the queue to uv_share_page(), but that ultracall expects the number of > pages to be shared. static inline u32 xive_alloc_order(u32 queue_shift) { return (queue_shift > PAGE_SHIFT) ? (queue_shift - PAGE_SHIFT) : 0; } xive_alloc_order(order) returns the order of PAGE_SIZE pages. Hence the value passed to uv_shared_pages is the number of pages, and not the number of bytes. BTW: I did verify through testing that it was indeed passing 1 page to the uv_share_page(). RP
Ram Pai <linuxram@us.ibm.com> writes: > On Tue, Mar 31, 2020 at 08:53:07PM -0300, Thiago Jung Bauermann wrote: >> >> Hi Ram, >> >> Ram Pai <linuxram@us.ibm.com> writes: >> >> > diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c >> > index 55dc61c..608b52f 100644 >> > --- a/arch/powerpc/sysdev/xive/spapr.c >> > +++ b/arch/powerpc/sysdev/xive/spapr.c >> > @@ -26,6 +26,8 @@ >> > #include <asm/xive.h> >> > #include <asm/xive-regs.h> >> > #include <asm/hvcall.h> >> > +#include <asm/svm.h> >> > +#include <asm/ultravisor.h> >> > >> > #include "xive-internal.h" >> > >> > @@ -501,6 +503,9 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio, >> > rc = -EIO; >> > } else { >> > q->qpage = qpage; >> > + if (is_secure_guest()) >> > + uv_share_page(PHYS_PFN(qpage_phys), >> > + 1 << xive_alloc_order(order)); >> >> If I understand this correctly, you're passing the number of bytes of >> the queue to uv_share_page(), but that ultracall expects the number of >> pages to be shared. > > > static inline u32 xive_alloc_order(u32 queue_shift) > { > return (queue_shift > PAGE_SHIFT) ? (queue_shift - PAGE_SHIFT) : 0; > } > > xive_alloc_order(order) returns the order of PAGE_SIZE pages. > Hence the value passed to uv_shared_pages is the number of pages, > and not the number of bytes. > > BTW: I did verify through testing that it was indeed passing 1 page to the > uv_share_page(). Ah, my mistake. I misunderstood the code. Sorry for the noise and thanks for clarifying.
diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c index 55dc61c..608b52f 100644 --- a/arch/powerpc/sysdev/xive/spapr.c +++ b/arch/powerpc/sysdev/xive/spapr.c @@ -26,6 +26,8 @@ #include <asm/xive.h> #include <asm/xive-regs.h> #include <asm/hvcall.h> +#include <asm/svm.h> +#include <asm/ultravisor.h> #include "xive-internal.h" @@ -501,6 +503,9 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio, rc = -EIO; } else { q->qpage = qpage; + if (is_secure_guest()) + uv_share_page(PHYS_PFN(qpage_phys), + 1 << xive_alloc_order(order)); } fail: return rc; @@ -534,6 +539,8 @@ static void xive_spapr_cleanup_queue(unsigned int cpu, struct xive_cpu *xc, hw_cpu, prio); alloc_order = xive_alloc_order(xive_queue_shift); + if (is_secure_guest()) + uv_unshare_page(PHYS_PFN(__pa(q->qpage)), 1 << alloc_order); free_pages((unsigned long)q->qpage, alloc_order); q->qpage = NULL; }
XIVE interrupt controller use an Event Queue (EQ) to enqueue event notifications when an exception occurs. The EQ is a single memory page provided by the O/S defining a circular buffer, one per server and priority couple. On baremetal, the EQ page is configured with an OPAL call. On pseries, an extra hop is necessary and the guest OS uses the hcall H_INT_SET_QUEUE_CONFIG to configure the XIVE interrupt controller. The XIVE controller being Hypervisor privileged, it will not be allowed to enqueue event notifications for a Secure VM unless the EQ pages are shared by the Secure VM. Hypervisor/Ultravisor still requires support for the TIMA and ESB page fault handlers. Until this is complete, QEMU can use the emulated XIVE device for Secure VMs, option "kernel_irqchip=off" on the QEMU pseries machine. Cc: kvm-ppc@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com> Cc: Michael Anderson <andmike@linux.ibm.com> Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Greg Kurz <groug@kaod.org> Cc: Cedric Le Goater <clg@fr.ibm.com> Cc: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Ram Pai <linuxram@us.ibm.com> v2: better description of the patch from Cedric. --- arch/powerpc/sysdev/xive/spapr.c | 7 +++++++ 1 file changed, 7 insertions(+)