[RFC,v2,13/22] i386/xen: implement HYPERVISOR_memory_op

Commit Message

David Woodhouse Dec. 9, 2022, 9:56 a.m. UTC

From: Joao Martins <[e-mail]>

Specifically XENMEM_add_to_physmap with space XENMAPSPACE_shared_info to
allow the guest to set its shared_info page.

Signed-off-by: Joao Martins <[e-mail]>
[dwmw2: Use the xen_overlay device]
Signed-off-by: David Woodhouse <[e-mail]>
---
 target/i386/trace-events |  1 +
 target/i386/xen.c        | 59 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 1 deletion(-)

Comments

Paul Durrant Dec. 12, 2022, 2:38 p.m. UTC | #1

On 09/12/2022 09:56, David Woodhouse wrote:
> From: Joao Martins <[e-mail]>
> 
> Specifically XENMEM_add_to_physmap with space XENMAPSPACE_shared_info to
> allow the guest to set its shared_info page.
> 
> Signed-off-by: Joao Martins <[e-mail]>
> [dwmw2: Use the xen_overlay device]
> Signed-off-by: David Woodhouse <[e-mail]>
> ---
>   target/i386/trace-events |  1 +
>   target/i386/xen.c        | 59 +++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 59 insertions(+), 1 deletion(-)
>
[snip]
> +static bool kvm_xen_hcall_memory_op(struct kvm_xen_exit *exit,
> +                                   int cmd, uint64_t arg, X86CPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +    int err = 0;
> +
> +    switch (cmd) {
> +    case XENMEM_add_to_physmap: {
> +            struct xen_add_to_physmap xatp;
> +
> +            err = kvm_copy_from_gva(cs, arg, &xatp, sizeof(xatp));
> +            if (err) {
> +                break;
> +            }
> +
> +            switch (xatp.space) {
> +            case XENMAPSPACE_shared_info:
> +                break;
> +            default:
> +                err = -ENOSYS;
> +                break;

Don't you want to return false here?

   Paul

> +            }
> +
> +            err = xen_set_shared_info(cs, xatp.gpfn);
> +            break;
> +         }
> +
> +    default:
> +            return false;
> +    }
> +
> +    exit->u.hcall.result = err;
> +    return true;
> +}
> +

David Woodhouse Dec. 13, 2022, 12:08 a.m. UTC | #2

On Mon, 2022-12-12 at 14:38 +0000, Paul Durrant wrote:
> 
> > +            switch (xatp.space) {
> > +            case XENMAPSPACE_shared_info:
> > +                break;
> > +            default:
> > +                err = -ENOSYS;
> > +                break;
> 
> Don't you want to return false here?

It doesn't make a lot of difference right now. Once we believe we've
implemented everything a sane guest would use, then the distinction
between {true, -ENOSYS} and just returning false actually starts to
matter.

Patch

diff --git a/target/i386/trace-events b/target/i386/trace-events
index 1bf9558811..fb999d0052 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -14,3 +14,4 @@  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data
 
 # target/i386/xen.c
 kvm_xen_hypercall(int cpu, uint8_t cpl, uint64_t input, uint64_t a0, uint64_t a1, uint64_t a2, uint64_t ret) "xen_hypercall: cpu %d cpl %d input %" PRIu64 " a0 0x%" PRIx64 " a1 0x%" PRIx64 " a2 0x%" PRIx64" ret 0x%" PRIx64
+kvm_xen_set_shared_info(uint64_t gfn) "shared info at gfn 0x%" PRIx64
diff --git a/target/i386/xen.c b/target/i386/xen.c
index 55beed1913..ddd144039a 100644
--- a/target/i386/xen.c
+++ b/target/i386/xen.c
@@ -15,8 +15,9 @@ 
 #include "exec/address-spaces.h"
 #include "xen.h"
 #include "trace.h"
-
+#include "hw/i386/kvm/xen_overlay.h"
 #include "standard-headers/xen/version.h"
+#include "standard-headers/xen/memory.h"
 
 static int kvm_gva_rw(CPUState *cs, uint64_t gva, void *_buf, size_t sz,
                       bool is_write)
@@ -126,6 +127,59 @@  static bool kvm_xen_hcall_xen_version(struct kvm_xen_exit *exit, X86CPU *cpu,
     return true;
 }
 
+static int xen_set_shared_info(CPUState *cs, uint64_t gfn)
+{
+    uint64_t gpa = gfn << TARGET_PAGE_BITS;
+    int err;
+
+    /* The xen_overlay device tells KVM about it too, since it had to
+     * do that on migration load anyway (unless we're going to jump
+     * through lots of hoops to maintain the fiction that this isn't
+     * KVM-specific */
+    err = xen_overlay_map_page(XENMAPSPACE_shared_info, 0, gpa);
+    if (err)
+            return err;
+
+    trace_kvm_xen_set_shared_info(gfn);
+
+    return err;
+}
+
+static bool kvm_xen_hcall_memory_op(struct kvm_xen_exit *exit,
+                                   int cmd, uint64_t arg, X86CPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    int err = 0;
+
+    switch (cmd) {
+    case XENMEM_add_to_physmap: {
+            struct xen_add_to_physmap xatp;
+
+            err = kvm_copy_from_gva(cs, arg, &xatp, sizeof(xatp));
+            if (err) {
+                break;
+            }
+
+            switch (xatp.space) {
+            case XENMAPSPACE_shared_info:
+                break;
+            default:
+                err = -ENOSYS;
+                break;
+            }
+
+            err = xen_set_shared_info(cs, xatp.gpfn);
+            break;
+         }
+
+    default:
+            return false;
+    }
+
+    exit->u.hcall.result = err;
+    return true;
+}
+
 static bool __kvm_xen_handle_exit(X86CPU *cpu, struct kvm_xen_exit *exit)
 {
     uint16_t code = exit->u.hcall.input;
@@ -136,6 +190,9 @@  static bool __kvm_xen_handle_exit(X86CPU *cpu, struct kvm_xen_exit *exit)
     }
 
     switch (code) {
+    case __HYPERVISOR_memory_op:
+        return kvm_xen_hcall_memory_op(exit, exit->u.hcall.params[0],
+                                       exit->u.hcall.params[1], cpu);
     case __HYPERVISOR_xen_version:
         return kvm_xen_hcall_xen_version(exit, cpu, exit->u.hcall.params[0],
                                          exit->u.hcall.params[1]);