[PATCH 5/6 v6] kvm: booke: clear host tlb reference flag on guest tlb invalidation

[Scott Wood]

On Fri, 2013-09-20 at 09:55 +0530, Bharat Bhushan wrote:
> On booke, "struct tlbe_ref" contains host tlb mapping information
> (pfn: for guest-pfn to pfn, flags: attribute associated with this mapping)
> for a guest tlb entry. So when a guest creates a TLB entry then
> "struct tlbe_ref" is set to point to valid "pfn" and set attributes in
> "flags" field of the above said structure. When a guest TLB entry is
> invalidated then flags field of corresponding "struct tlbe_ref" is
> updated to point that this is no more valid, also we selectively clear
> some other attribute bits, example: if E500_TLB_BITMAP was set then we clear
> E500_TLB_BITMAP, if E500_TLB_TLB0 is set then we clear this.
> 
> Ideally we should clear complete "flags" as this entry is invalid and does not
> have anything to re-used. The other part of the problem is that when we use
> the same entry again then also we do not clear (started doing or-ing etc).
> 
> So far it was working because the selectively clearing mentioned above
> actually clears "flags" what was set during TLB mapping. But the problem
> starts coming when we add more attributes to this then we need to selectively
> clear them and which is not needed.
> 
> This patch we do both
>         - Clear "flags" when invalidating;
>         - Clear "flags" when reusing same entry later
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan at freescale.com>
> ---
> v5->v6
>  - Fix flag clearing comment

The changes between v5 and v6 are not just about comments...

> 
>  arch/powerpc/kvm/e500_mmu_host.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
> index 1c6a9d7..7370e1c 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -230,15 +230,15 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
>  		ref->flags &= ~(E500_TLB_TLB0 | E500_TLB_VALID);
>  	}
>  
> -	/* Already invalidated in between */
> -	if (!(ref->flags & E500_TLB_VALID))
> -		return;
> -
> -	/* Guest tlbe is backed by at most one host tlbe per shadow pid. */
> -	kvmppc_e500_tlbil_one(vcpu_e500, gtlbe);
> +	/*
> +	 * Check whether TLB entry is already invalidated in between
> +	 * Guest tlbe is backed by at most one host tlbe per shadow pid.
> +	 */
> +	if (ref->flags & E500_TLB_VALID)
> +		kvmppc_e500_tlbil_one(vcpu_e500, gtlbe);

I'd phrase this combined comment as "If it's still valid, it's a TLB0
entry, and thus backed by at most one host tlbe per shadow pid".

Otherwise looks good.

-Scott