Re: [PATCH 04/20] KVM: arm64: Use block-level annotations when setting up the host stage-2

[Vincent Donnefort <vdonnefort@google.com>]

On Mon, Sep 07, 2026 at 03:37:56PM +0100, Wei-Lin Chang wrote:
> On Mon, Aug 03, 2026 at 11:08:48AM +0100, Vincent Donnefort wrote:
> 
> [...]
> 
> > +static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx,
> > +				     enum kvm_pgtable_walk_flags visit)
> > +{
> > +	struct fix_host_ownership_data *data = ctx->arg;
> > +	enum kvm_pgtable_prot prot = 0;
> > +	phys_addr_t phys = 0;
> > +	int ret;
> > +
> > +	if (kvm_pte_valid(ctx->old))
> > +		prot = kvm_pgtable_hyp_pte_prot(ctx->old);
> > +
> > +	if (!prot) {
> > +		/* Unchanged region */
> > +		if (!data->prot)
> > +			return 0;
> > +
> > +		goto apply_ownership;
> > +	}
> > +
> > +	phys = kvm_pte_to_phys(ctx->old);
> > +	if (!addr_is_memory(phys))
> > +		return -EINVAL;
> > +
> > +	/* We already know phys is contiguous as we walk the linear map */
> > +
> > +	if (prot != data->prot)
> > +		goto apply_ownership;
> > +
> > +	/* Accumulate in the current region */
> > +	data->size += kvm_granule_size(ctx->level);
> > +
> > +	return 0;
> > +
> > +apply_ownership:
> > +	ret = __fix_host_ownership(data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	data->phys = phys;
> > +	data->size = kvm_granule_size(ctx->level);
> > +	data->prot = prot;
> > +
> > +	return 0;
> > +}
> 
> I think this function can be simplified, if we check phys first:
> 
> static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx,
> 				     enum kvm_pgtable_walk_flags visit)
> {
> 	struct fix_host_ownership_data *data = ctx->arg;
> 	enum kvm_pgtable_prot prot = 0;
> 	phys_addr_t phys = 0;
> 	int ret;
> 
> 	if (kvm_pte_valid(ctx->old))
> 		prot = kvm_pgtable_hyp_pte_prot(ctx->old);
> 
> 	if (prot) {
> 		phys = kvm_pte_to_phys(ctx->old);
> 		if (!addr_is_memory(phys))
> 			return -EINVAL;
> 	}
> 
> 	if (prot != data->prot) {
> 		ret = __fix_host_ownership(data);
> 		if (ret)
> 			return ret;
> 
> 		data->phys = phys;
> 		data->size = kvm_granule_size(ctx->level);
> 		data->prot = prot;
> 	} else {
> 		data->size += kvm_granule_size(ctx->level);
> 	}
> 	return 0;
> }
> 
> Did I miss anything?
> 
> Thanks,
> Wei-Lin Chang

Indeed, I think it's my:

   if (prot != data->prot)
       goto apply_ownership

that is clumsy. With

  if (prot == data->prot) {
      data->size += kvm_granule_size();
      return 0;
  }

(which is basically what you suggested)

I can get rid of the label.

-- 
Vincent