Re: [PATCH v8 4/6] KVM: PPC: Book3S HV: Nested support in H_RPT_INVALIDATE

[Bharata B Rao <bharata@linux.ibm.com>]

On Mon, Jun 21, 2021 at 10:12:42AM -0700, Nathan Chancellor wrote:
> > +long do_h_rpt_invalidate_pat(struct kvm_vcpu *vcpu, unsigned long lpid,
> > +			     unsigned long type, unsigned long pg_sizes,
> > +			     unsigned long start, unsigned long end)
> > +{
> > +	struct kvm_nested_guest *gp;
> > +	long ret;
> > +	unsigned long psize, ap;
> > +
> > +	/*
> > +	 * If L2 lpid isn't valid, we need to return H_PARAMETER.
> > +	 *
> > +	 * However, nested KVM issues a L2 lpid flush call when creating
> > +	 * partition table entries for L2. This happens even before the
> > +	 * corresponding shadow lpid is created in HV which happens in
> > +	 * H_ENTER_NESTED call. Since we can't differentiate this case from
> > +	 * the invalid case, we ignore such flush requests and return success.
> > +	 */
> > +	gp = kvmhv_find_nested(vcpu->kvm, lpid);
> > +	if (!gp)
> > +		return H_SUCCESS;
> > +
> > +	/*
> > +	 * A flush all request can be handled by a full lpid flush only.
> > +	 */
> > +	if ((type & H_RPTI_TYPE_NESTED_ALL) == H_RPTI_TYPE_NESTED_ALL)
> > +		return do_tlb_invalidate_nested_all(vcpu, lpid, RIC_FLUSH_ALL);
> > +
> > +	/*
> > +	 * We don't need to handle a PWC flush like process table here,
> > +	 * because intermediate partition scoped table in nested guest doesn't
> > +	 * really have PWC. Only level we have PWC is in L0 and for nested
> > +	 * invalidate at L0 we always do kvm_flush_lpid() which does
> > +	 * radix__flush_all_lpid(). For range invalidate at any level, we
> > +	 * are not removing the higher level page tables and hence there is
> > +	 * no PWC invalidate needed.
> > +	 *
> > +	 * if (type & H_RPTI_TYPE_PWC) {
> > +	 *	ret = do_tlb_invalidate_nested_all(vcpu, lpid, RIC_FLUSH_PWC);
> > +	 *	if (ret)
> > +	 *		return H_P4;
> > +	 * }
> > +	 */
> > +
> > +	if (start == 0 && end == -1)
> > +		return do_tlb_invalidate_nested_all(vcpu, lpid, RIC_FLUSH_TLB);
> > +
> > +	if (type & H_RPTI_TYPE_TLB) {
> > +		struct mmu_psize_def *def;
> > +		bool flush_lpid;
> > +		unsigned long nr_pages;
> > +
> > +		for (psize = 0; psize < MMU_PAGE_COUNT; psize++) {
> > +			def = &mmu_psize_defs[psize];
> > +			if (!(pg_sizes & def->h_rpt_pgsize))
> > +				continue;
> > +
> > +			nr_pages = (end - start) >> def->shift;
> > +			flush_lpid = nr_pages > tlb_range_flush_page_ceiling;
> > +			if (flush_lpid)
> > +				return do_tlb_invalidate_nested_all(vcpu, lpid,
> > +								RIC_FLUSH_TLB);
> > +
> > +			ret = do_tlb_invalidate_nested_tlb(vcpu, lpid,
> > +							   (1UL << def->shift),
> > +							   ap, start, end);
> 
> I have not seen this reported yet so apologies if it has and there is a
> fix I am missing:
> 
> arch/powerpc/kvm/book3s_hv_nested.c:1334:11: error: variable 'ap' is uninitialized when used here [-Werror,-Wuninitialized]
>                                                            ap, start, end);
>                                                            ^~
> arch/powerpc/kvm/book3s_hv_nested.c:1276:25: note: initialize the variable 'ap' to silence this warning
>         unsigned long psize, ap;
>                                ^
>                                 = 0

Thanks for catching this, this wasn't caught in my environment.

I will repost the series with proper initialization to ap.

Regards,
Bharata.