Re: [PATCH v3 26/32] KVM: arm64: Introduce PROT_NONE mappings for stage 2

[Will Deacon]

On Tue, Mar 02, 2021 at 02:59:56PM +0000, Quentin Perret wrote:
> Once we start unmapping portions of memory from the host stage 2 (such
> as e.g. the hypervisor memory sections, or pages that belong to
> protected guests), we will need a way to track page ownership. And
> given that all mappings in the host stage 2 will be identity-mapped, we
> can use the host stage 2 page-table itself as a simplistic rmap.
> 
> As a first step towards this, introduce a new protection attribute
> in the stage 2 page table code, called KVM_PGTABLE_PROT_NONE, which
> allows to annotate portions of the IPA space as inaccessible. For
> simplicity, PROT_NONE mappings are created as invalid mappings with a
> software bit set.

Just as an observation, but given that they're invalid we can use any bit
from [63:2] to indicate that it's a PROT_NONE mapping, and that way we
can keep the real "software bits" for live mappings.

But we can of course change that later when we need the bit for something
else.

> 
> Signed-off-by: Quentin Perret <qperret@xxxxxxxxxx>
> ---
>  arch/arm64/include/asm/kvm_pgtable.h |  2 ++
>  arch/arm64/kvm/hyp/pgtable.c         | 26 ++++++++++++++++++++++++--
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 9935dbae2cc1..c9f6ed76e0ad 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -80,6 +80,7 @@ struct kvm_pgtable {
>   * @KVM_PGTABLE_PROT_W:		Write permission.
>   * @KVM_PGTABLE_PROT_R:		Read permission.
>   * @KVM_PGTABLE_PROT_DEVICE:	Device attributes.
> + * @KVM_PGTABLE_PROT_NONE:	No permission.
>   */
>  enum kvm_pgtable_prot {
>  	KVM_PGTABLE_PROT_X			= BIT(0),
> @@ -87,6 +88,7 @@ enum kvm_pgtable_prot {
>  	KVM_PGTABLE_PROT_R			= BIT(2),
>  
>  	KVM_PGTABLE_PROT_DEVICE			= BIT(3),
> +	KVM_PGTABLE_PROT_NONE			= BIT(4),

Why do we need an extra entry here? Couldn't we just create PROT_NONE
entries when none of R,W or X are set?

>  };
>  
>  #define PAGE_HYP		(KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W)
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index bdd6e3d4eeb6..8e7059fcfd40 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -48,6 +48,8 @@
>  					 KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
>  					 KVM_PTE_LEAF_ATTR_HI_S2_XN)
>  
> +#define KVM_PTE_LEAF_SW_BIT_PROT_NONE	BIT(55)
> +
>  struct kvm_pgtable_walk_data {
>  	struct kvm_pgtable		*pgt;
>  	struct kvm_pgtable_walker	*walker;
> @@ -120,6 +122,16 @@ static bool kvm_pte_valid(kvm_pte_t pte)
>  	return pte & KVM_PTE_VALID;
>  }
>  
> +static bool kvm_pte_prot_none(kvm_pte_t pte)
> +{
> +	return pte & KVM_PTE_LEAF_SW_BIT_PROT_NONE;
> +}

I think it would be a good idea to check !kvm_pte_valid() in here too,
since it doesn't make sense to report true for valid (or table) entries.

Will