Re: [PATCH V2 7/8] KVM: PPC: Introduce new hcall H_COPY_TOFROM_GUEST to access quadrants 1 & 2

[Paul Mackerras <paulus@ozlabs.org>]

On Mon, Dec 10, 2018 at 02:58:24PM +1100, Suraj Jitindar Singh wrote:
> A guest cannot access quadrants 1 or 2 as this would result in an
> exception. Thus introduce the hcall H_COPY_TOFROM_GUEST to be used by a
> guest when it wants to perform an access to quadrants 1 or 2, for
> example when it wants to access memory for one of its nested guests.
> 
> Also provide an implementation for the kvm-hv module.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

[snip]

>  /*
> + * Handle the H_COPY_TOFROM_GUEST hcall.
> + * r4 = L1 lpid of nested guest
> + * r5 = pid
> + * r6 = eaddr to access
> + * r7 = to buffer (L1 gpa)
> + * r8 = from buffer (L1 gpa)

Comment says these are GPAs...

> + * r9 = n bytes to copy
> + */
> +long kvmhv_copy_tofrom_guest_nested(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_nested_guest *gp;
> +	int l1_lpid = kvmppc_get_gpr(vcpu, 4);
> +	int pid = kvmppc_get_gpr(vcpu, 5);
> +	gva_t eaddr = kvmppc_get_gpr(vcpu, 6);
> +	void *gp_to = (void *) kvmppc_get_gpr(vcpu, 7);
> +	void *gp_from = (void *) kvmppc_get_gpr(vcpu, 8);
> +	void *buf;
> +	unsigned long n = kvmppc_get_gpr(vcpu, 9);
> +	bool is_load = !!gp_to;
> +	long rc;
> +
> +	if (gp_to && gp_from) /* One must be NULL to determine the direction */
> +		return H_PARAMETER;
> +
> +	if (eaddr & (0xFFFUL << 52))
> +		return H_PARAMETER;
> +
> +	buf = kzalloc(n, GFP_KERNEL);
> +	if (!buf)
> +		return H_NO_MEM;
> +
> +	gp = kvmhv_get_nested(vcpu->kvm, l1_lpid, false);
> +	if (!gp) {
> +		rc = H_PARAMETER;
> +		goto out_free;
> +	}
> +
> +	mutex_lock(&gp->tlb_lock);
> +
> +	if (is_load) {
> +		/* Load from the nested guest into our buffer */
> +		rc = __kvmhv_copy_tofrom_guest_radix(gp->shadow_lpid, pid,
> +						     eaddr, buf, NULL, n);
> +		if (rc)
> +			goto not_found;
> +
> +		/* Write what was loaded into our buffer back to the L1 guest */
> +		rc = kvmppc_st(vcpu, (ulong *) &gp_to, n, buf, true);

but using kvmppc_st implies that it is an EA (and in fact when you
call it in the next patch you pass an EA).

It would be more like other hcalls to pass a GPA, meaning that you
would use kvm_write_guest() here.  On the other hand, with the
quadrant access, kvmppc_st() might well be faster than
kvm_write_guest.

So you need to decide which it is and either fix the comment or change
the code.

Paul.