Re: [PATCH v2 1/1] KVM: x86/xen: add support for 32-bit guests in SCHEDOP_poll

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

On Mon, 2022-03-21 at 17:27 +0000, Kaya, Metin wrote:
> From: Metin Kaya <metikaya@amazon.com>
> 
> 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@amazon.com>

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 #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]