On Mon, 2022-03-21 at 17:27 +0000, Kaya, Metin wrote:
> From: Metin Kaya <metikaya@xxxxxxxxxx>
>
> This patch introduces compat version of struct sched_poll for
> SCHEDOP_poll sub-operation of sched_op hypercall, reads correct amount
> of data (16 bytes in 32-bit case, 24 bytes otherwise) by using new
> compat_sched_poll struct, copies it to sched_poll properly, and lets
> rest of the code run as is.
>
> Signed-off-by: Metin Kaya <metikaya@xxxxxxxxxx>
Could do with a test case. It's fairly simple to flip the 'longmode'
config even when the guest is actually in 64-bit mode, so it should be
fairly easy to add this in the xen_shinfo_test.
Other minor nits below...
> ---
> arch/x86/kvm/xen.c | 31 +++++++++++++++++++++++++++----
> arch/x86/kvm/xen.h | 7 +++++++
> 2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 7d01983d1087..2d0a5d2ca6f1 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -998,20 +998,43 @@ static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
> evtchn_port_t port, *ports;
> gpa_t gpa;
>
> - if (!longmode || !lapic_in_kernel(vcpu) ||
> + if (!lapic_in_kernel(vcpu) ||
> !(vcpu->kvm->arch.xen_hvm_config.flags & KVM_XEN_HVM_CONFIG_EVTCHN_SEND))
> return false;
>
> idx = srcu_read_lock(&vcpu->kvm->srcu);
> gpa = kvm_mmu_gva_to_gpa_system(vcpu, param, NULL);
> srcu_read_unlock(&vcpu->kvm->srcu, idx);
> -
> - if (!gpa || kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
> - sizeof(sched_poll))) {
> + if (!gpa) {
> *r = -EFAULT;
> return true;
> }
>
> + if (IS_ENABLED(CONFIG_64BIT) && longmode) {
Make this 'if (!IS_ENABLED(CONFIG_64BIT || longmode) {'
You want to take this trivial path for the 32-bit host kernel too,
since struct sched_poll and its compat version are identical there.
> + if (kvm_vcpu_read_guest(vcpu, gpa, &sched_poll,
> + sizeof(sched_poll))) {
> + *r = -EFAULT;
> + return true;
> + }
> + } else {
Then this code path is only for IS_ENABLED(CONFIG_64BIT) && !longmode,
which is what we want.
> + struct compat_sched_poll sp;
> +
> + /*
> + * Sanity check that __packed trick works fine and size of
> + * compat_sched_poll is 16 bytes just like in the real Xen
> + * 32-bit case.
> + */
> + BUILD_BUG_ON(sizeof(struct compat_sched_poll) != 16);
> +
> + if (kvm_vcpu_read_guest(vcpu, gpa, &sp, sizeof(sp))) {
> + *r = -EFAULT;
> + return true;
> + }
> + sched_poll.ports = (evtchn_port_t *)(unsigned long)(sp.ports);
> + sched_poll.nr_ports = sp.nr_ports;
> + sched_poll.timeout = sp.timeout;
> + }
> --- a/arch/x86/kvm/xen.h
> +++ b/arch/x86/kvm/xen.h
> @@ -196,6 +196,13 @@ struct compat_shared_info {
> struct compat_arch_shared_info arch;
> };
>
> +struct compat_sched_poll {
> + /* This is actually a guest virtual address which points to ports. */
> + uint32_t ports;
> + unsigned int nr_ports;
> + uint64_t timeout;
> +} __packed;
> +
We use __attribute__((packed)) elsewhere in the same file. I don't much
care, but consistency is good. If you want to use __packed, can you
change the other one too?
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
