Re: [PATCH v4 23/32] KVM: PPC: Book3S HV: Implement H_TLB_INVALIDATE hcall

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Oct 04, 2018 at 09:56:00PM +1000, Paul Mackerras wrote:
11;rgb:ffff/ffff/ffff> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> 
> When running a nested (L2) guest the guest (L1) hypervisor will use
> the H_TLB_INVALIDATE hcall when it needs to change the partition
> scoped page tables or the partition table which it manages.  It will
> use this hcall in the situations where it would use a partition-scoped
> tlbie instruction if it were running in hypervisor mode.
> 
> The H_TLB_INVALIDATE hcall can invalidate different scopes:
> 
> Invalidate TLB for a given target address:
> - This invalidates a single L2 -> L1 pte
> - We need to invalidate any L2 -> L0 shadow_pgtable ptes which map the L2
>   address space which is being invalidated. This is because a single
>   L2 -> L1 pte may have been mapped with more than one pte in the
>   L2 -> L0 page tables.
> 
> Invalidate the entire TLB for a given LPID or for all LPIDs:
> - Invalidate the entire shadow_pgtable for a given nested guest, or
>   for all nested guests.
> 
> Invalidate the PWC (page walk cache) for a given LPID or for all LPIDs:
> - We don't cache the PWC, so nothing to do.
> 
> Invalidate the entire TLB, PWC and partition table for a given/all LPIDs:
> - Here we re-read the partition table entry and remove the nested state
>   for any nested guest for which the first doubleword of the partition
>   table entry is now zero.
> 
> The H_TLB_INVALIDATE hcall takes as parameters the tlbie instruction
> word (of which only the RIC, PRS and R fields are used), the rS value
> (giving the lpid, where required) and the rB value (giving the IS, AP
> and EPN values).
> 
> [paulus@ozlabs.org - adapted to having the partition table in guest
> memory, added the H_TLB_INVALIDATE implementation, removed tlbie
> instruction emulation, reworded the commit message.]
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

That said, there's one change I think could make it substantially
easier to read..

[snip]
> +static int kvmhv_emulate_tlbie_tlb_addr(struct kvm_vcpu *vcpu, int lpid,
> +					int ap, long epn)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	struct kvm_nested_guest *gp;
> +	long npages;
> +	int shift;
> +	unsigned long addr;
> +
> +	shift = ap_to_shift(ap);
> +	addr = epn << 12;
> +	if (shift < 0)
> +		/* Invalid ap encoding */
> +		return -EINVAL;
> +
> +	addr &= ~((1UL << shift) - 1);
> +	npages = 1UL << (shift - PAGE_SHIFT);
> +
> +	gp = kvmhv_get_nested(kvm, lpid, false);
> +	if (!gp) /* No such guest -> nothing to do */
> +		return 0;
> +	mutex_lock(&gp->tlb_lock);
> +
> +	/* There may be more than one host page backing this single guest pte */
> +	do {
> +		kvmhv_invalidate_shadow_pte(vcpu, gp, addr, &shift);
> +
> +		npages -= 1UL << (shift - PAGE_SHIFT);
> +		addr += 1UL << shift;

I read this about 6 times before realizing that 'shift' here has a
different value to what it did before the loop (which it has to in
order to be correct).  I'd suggest a loop local variable with a
different name (maybe 'shadow_shift') to make that more obvious.
-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]