Re: [PATCH v3 10/21] KVM: arm64: Enable forcing page-level stage-2 mappings
Hi Quentin,
> > > +static bool stage2_block_mapping_allowed(u64 addr, u64 end, u32 level,
> > > +struct stage2_map_data *data)
> > > +{
> > > + if (data->force_pte && (level < (KVM_PGTABLE_MAX_LEVELS - 1)))
> > > + return false;
> >
> > I'm not sure I understand why checking the level is necessary. Can
> > there be block mapping at the last possible level?
>
> That's probably just a matter of naming, but this function is in fact
> called at every level, just like kvm_block_mapping_supported() was
> before. And we rely on it returning true at the last level, so I need to
> do that check here.
>
> Maybe renaming this stage2_leaf_mapping_allowed() would clarify?
Yes it would.
Thanks,
/fuad
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 10/21] KVM: arm64: Enable forcing page-level stage-2 mappings
Hi Fuad,
On Monday 02 Aug 2021 at 11:49:28 (+0200), Fuad Tabba wrote:
> On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret wrote:
> >
> > Much of the stage-2 manipulation logic relies on being able to destroy
> > block mappings if e.g. installing a smaller mapping in the range. The
> > rationale for this behaviour is that stage-2 mappings can always be
> > re-created lazily. However, this gets more complicated when the stage-2
> > page-table is used to store metadata about the underlying pages. In such
> > cases, destroying a block mapping may lead to losing part of the state,
> > and confuse the user of those metadata (such as the hypervisor in nVHE
> > protected mode).
> >
> > To avoid this, introduce a callback function in the pgtable struct which
> > is called during all map operations to determine whether the mappings
> > can use blocks, or should be forced to page granularity. This is used by
> > the hypervisor when creating the host stage-2 to force page-level
> > mappings when using non-default protection attributes.
> >
> > Signed-off-by: Quentin Perret
> > ---
> > arch/arm64/include/asm/kvm_pgtable.h | 65 ---
> > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 30 +++--
> > arch/arm64/kvm/hyp/pgtable.c | 29 +---
> > 3 files changed, 91 insertions(+), 33 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h
> > b/arch/arm64/include/asm/kvm_pgtable.h
> > index 83c5c97d9eac..ba7dcade2798 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -115,25 +115,6 @@ enum kvm_pgtable_stage2_flags {
> > KVM_PGTABLE_S2_IDMAP= BIT(1),
> > };
> >
> > -/**
> > - * struct kvm_pgtable - KVM page-table.
> > - * @ia_bits: Maximum input address size, in bits.
> > - * @start_level: Level at which the page-table walk starts.
> > - * @pgd: Pointer to the first top-level entry of the
> > page-table.
> > - * @mm_ops:Memory management callbacks.
> > - * @mmu: Stage-2 KVM MMU struct. Unused for stage-1
> > page-tables.
> > - */
> > -struct kvm_pgtable {
> > - u32 ia_bits;
> > - u32 start_level;
> > - kvm_pte_t *pgd;
> > - struct kvm_pgtable_mm_ops *mm_ops;
> > -
> > - /* Stage-2 only */
> > - struct kvm_s2_mmu *mmu;
> > - enum kvm_pgtable_stage2_flags flags;
> > -};
> > -
> > /**
> > * enum kvm_pgtable_prot - Page-table permissions and attributes.
> > * @KVM_PGTABLE_PROT_X:Execute permission.
> > @@ -149,11 +130,41 @@ enum kvm_pgtable_prot {
> > KVM_PGTABLE_PROT_DEVICE = BIT(3),
> > };
> >
> > -#define PAGE_HYP (KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W)
> > +#define KVM_PGTABLE_PROT_RW(KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W)
> > +#define KVM_PGTABLE_PROT_RWX (KVM_PGTABLE_PROT_RW | KVM_PGTABLE_PROT_X)
> > +
> > +#define PAGE_HYP KVM_PGTABLE_PROT_RW
> > #define PAGE_HYP_EXEC (KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_X)
> > #define PAGE_HYP_RO(KVM_PGTABLE_PROT_R)
> > #define PAGE_HYP_DEVICE(PAGE_HYP | KVM_PGTABLE_PROT_DEVICE)
>
> I wonder if it would be useful to add a couple of other aliases for
> default memory and default mmio protections, e.g.,
>
> #define KVM_PGTABLE_PROT_MEM KVM_PGTABLE_PROT_RWX
> #define KVM_PGTABLE_PROT_MMIO KVM_PGTABLE_PROT_RW
>
> I think that using these below, e.g., host_stage2_force_pte_cb(),
> might make it clearer and answer comments you had in earlier patches
> about why "RWX" for memory.
Sure I can add something. I'll probably call them something else than
KVM_PGTABLE_PROT_{MEM,MMIO} though, just to make it clear this is all
specific to the host stage-2 stuff and not a general requirement of the
pgtable code to map things like this.
> >
> > +typedef bool (*kvm_pgtable_force_pte_cb_t)(u64 addr, u64 end,
> > + enum kvm_pgtable_prot prot);
> > +
> > +/**
> > + * struct kvm_pgtable - KVM page-table.
> > + * @ia_bits: Maximum input address size, in bits.
> > + * @start_level: Level at which the page-table walk starts.
> > + * @pgd: Pointer to the first top-level entry of the
> > page-table.
> > + * @mm_ops:Memory management callbacks.
> > + * @mmu: Stage-2 KVM MMU struct. Unused for stage-1
> > page-tables.
> > + * @flags: Stage-2 page-table flags.
> > + * @force_pte_cb: Callback function used during map operations to
> > decide
> > + * whether block mappings can be used to map the given
> > IPA
> > + * range.
> > + */
>
> nit: I think it might be clearer (and probably not longer) to rephrase
> to describe in terms of the return value
Re: [PATCH v3 10/21] KVM: arm64: Enable forcing page-level stage-2 mappings
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret wrote:
>
> Much of the stage-2 manipulation logic relies on being able to destroy
> block mappings if e.g. installing a smaller mapping in the range. The
> rationale for this behaviour is that stage-2 mappings can always be
> re-created lazily. However, this gets more complicated when the stage-2
> page-table is used to store metadata about the underlying pages. In such
> cases, destroying a block mapping may lead to losing part of the state,
> and confuse the user of those metadata (such as the hypervisor in nVHE
> protected mode).
>
> To avoid this, introduce a callback function in the pgtable struct which
> is called during all map operations to determine whether the mappings
> can use blocks, or should be forced to page granularity. This is used by
> the hypervisor when creating the host stage-2 to force page-level
> mappings when using non-default protection attributes.
>
> Signed-off-by: Quentin Perret
> ---
> arch/arm64/include/asm/kvm_pgtable.h | 65 ---
> arch/arm64/kvm/hyp/nvhe/mem_protect.c | 30 +++--
> arch/arm64/kvm/hyp/pgtable.c | 29 +---
> 3 files changed, 91 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h
> b/arch/arm64/include/asm/kvm_pgtable.h
> index 83c5c97d9eac..ba7dcade2798 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -115,25 +115,6 @@ enum kvm_pgtable_stage2_flags {
> KVM_PGTABLE_S2_IDMAP= BIT(1),
> };
>
> -/**
> - * struct kvm_pgtable - KVM page-table.
> - * @ia_bits: Maximum input address size, in bits.
> - * @start_level: Level at which the page-table walk starts.
> - * @pgd: Pointer to the first top-level entry of the
> page-table.
> - * @mm_ops:Memory management callbacks.
> - * @mmu: Stage-2 KVM MMU struct. Unused for stage-1
> page-tables.
> - */
> -struct kvm_pgtable {
> - u32 ia_bits;
> - u32 start_level;
> - kvm_pte_t *pgd;
> - struct kvm_pgtable_mm_ops *mm_ops;
> -
> - /* Stage-2 only */
> - struct kvm_s2_mmu *mmu;
> - enum kvm_pgtable_stage2_flags flags;
> -};
> -
> /**
> * enum kvm_pgtable_prot - Page-table permissions and attributes.
> * @KVM_PGTABLE_PROT_X:Execute permission.
> @@ -149,11 +130,41 @@ enum kvm_pgtable_prot {
> KVM_PGTABLE_PROT_DEVICE = BIT(3),
> };
>
> -#define PAGE_HYP (KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W)
> +#define KVM_PGTABLE_PROT_RW(KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W)
> +#define KVM_PGTABLE_PROT_RWX (KVM_PGTABLE_PROT_RW | KVM_PGTABLE_PROT_X)
> +
> +#define PAGE_HYP KVM_PGTABLE_PROT_RW
> #define PAGE_HYP_EXEC (KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_X)
> #define PAGE_HYP_RO(KVM_PGTABLE_PROT_R)
> #define PAGE_HYP_DEVICE(PAGE_HYP | KVM_PGTABLE_PROT_DEVICE)
I wonder if it would be useful to add a couple of other aliases for
default memory and default mmio protections, e.g.,
#define KVM_PGTABLE_PROT_MEM KVM_PGTABLE_PROT_RWX
#define KVM_PGTABLE_PROT_MMIO KVM_PGTABLE_PROT_RW
I think that using these below, e.g., host_stage2_force_pte_cb(),
might make it clearer and answer comments you had in earlier patches
about why "RWX" for memory.
>
> +typedef bool (*kvm_pgtable_force_pte_cb_t)(u64 addr, u64 end,
> + enum kvm_pgtable_prot prot);
> +
> +/**
> + * struct kvm_pgtable - KVM page-table.
> + * @ia_bits: Maximum input address size, in bits.
> + * @start_level: Level at which the page-table walk starts.
> + * @pgd: Pointer to the first top-level entry of the
> page-table.
> + * @mm_ops:Memory management callbacks.
> + * @mmu: Stage-2 KVM MMU struct. Unused for stage-1
> page-tables.
> + * @flags: Stage-2 page-table flags.
> + * @force_pte_cb: Callback function used during map operations to decide
> + * whether block mappings can be used to map the given
> IPA
> + * range.
> + */
nit: I think it might be clearer (and probably not longer) to rephrase
to describe in terms of the return value of the callback, e.g., "...
function that returns true if page level mappings must be used instead
of block mappings."
> +struct kvm_pgtable {
> + u32 ia_bits;
> + u32 start_level;
> + kvm_pte_t *pgd;
> + struct kvm_pgtable_mm_ops *mm_ops;
> +
> + /* Stage-2 only */
> + struct kvm_s2_mmu *mmu;
> + enum kvm_pgtable_stage2_flags
