On Thu, Oct 04, 2018 at 09:56:00PM +1000, Paul Mackerras wrote:
11;rgb:ffff/ffff/ffff> From: Suraj Jitindar Singh <sjitindarsingh@xxxxxxxxx>
>
> 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@xxxxxxxxxx - 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@xxxxxxxxx>
> Signed-off-by: Paul Mackerras <paulus@xxxxxxxxxx>
Reviewed-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
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:
signature.asc
Description: PGP signature
