On Wednesday 17 Mar 2021 at 14:42:46 (+0000), Will Deacon wrote:
> On Wed, Mar 17, 2021 at 02:17:13PM +0000, Quentin Perret wrote:
> > In order to further configure stage-2 page-tables, pass flags to the
> > init function using a new enum.
> >
> > The first of these flags allows to disable FWB even if the hardware
> > supports it as we will need to do so for the host stage-2.
> >
> > Signed-off-by: Quentin Perret <qperret@xxxxxxxxxx>
> >
> > ---
> >
> > One question is, do we want to use stage2_has_fwb() everywhere, including
> > guest-specific paths (e.g. kvm_arch_prepare_memory_region(), ...) ?
> >
> > That'd make this patch more intrusive, but would make the whole codebase
> > work with FWB enabled on a guest by guest basis. I don't see us use that
> > anytime soon (other than maybe debug of some sort?) but it'd be good to
> > have an agreement.
>
> I don't see the value in spreading this everywhere for now.
Good. Sounds like we're all in agreement.
> > arch/arm64/include/asm/kvm_pgtable.h | 19 +++++++++--
> > arch/arm64/include/asm/pgtable-prot.h | 4 +--
> > arch/arm64/kvm/hyp/pgtable.c | 49 +++++++++++++++++----------
> > 3 files changed, 50 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index b93a2a3526ab..7382bdfb6284 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -56,6 +56,15 @@ struct kvm_pgtable_mm_ops {
> > phys_addr_t (*virt_to_phys)(void *addr);
> > };
> >
> > +/**
> > + * enum kvm_pgtable_stage2_flags - Stage-2 page-table flags.
> > + * @KVM_PGTABLE_S2_NOFWB: Don't enforce Normal-WB even if the CPUs have
> > + * ARM64_HAS_STAGE2_FWB.
> > + */
> > +enum kvm_pgtable_stage2_flags {
> > + KVM_PGTABLE_S2_NOFWB = BIT(0),
> > +};
> > +
> > /**
> > * struct kvm_pgtable - KVM page-table.
> > * @ia_bits: Maximum input address size, in bits.
> > @@ -72,6 +81,7 @@ struct kvm_pgtable {
> >
> > /* Stage-2 only */
> > struct kvm_s2_mmu *mmu;
> > + enum kvm_pgtable_stage2_flags flags;
> > };
> >
> > /**
> > @@ -201,11 +211,16 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift);
> > * @arch: Arch-specific KVM structure representing the guest virtual
> > * machine.
> > * @mm_ops: Memory management callbacks.
> > + * @flags: Stage-2 configuration flags.
> > *
> > * Return: 0 on success, negative error code on failure.
> > */
> > -int kvm_pgtable_stage2_init(struct kvm_pgtable *pgt, struct kvm_arch *arch,
> > - struct kvm_pgtable_mm_ops *mm_ops);
> > +int kvm_pgtable_stage2_init_flags(struct kvm_pgtable *pgt, struct kvm_arch *arch,
> > + struct kvm_pgtable_mm_ops *mm_ops,
> > + enum kvm_pgtable_stage2_flags flags);
> > +
> > +#define kvm_pgtable_stage2_init(pgt, arch, mm_ops) \
> > + kvm_pgtable_stage2_init_flags(pgt, arch, mm_ops, 0)
>
> nit: I think some of the kerneldoc refers to "kvm_pgtable_stage_init()"
> so that needs a trivial update to e.g. "kvm_pgtable_stage_init*()".
Will do.
> > diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> > index 046be789fbb4..beeb722a82d3 100644
> > --- a/arch/arm64/include/asm/pgtable-prot.h
> > +++ b/arch/arm64/include/asm/pgtable-prot.h
> > @@ -72,10 +72,10 @@ extern bool arm64_use_ng_mappings;
> > #define PAGE_KERNEL_EXEC __pgprot(PROT_NORMAL & ~PTE_PXN)
> > #define PAGE_KERNEL_EXEC_CONT __pgprot((PROT_NORMAL & ~PTE_PXN) | PTE_CONT)
> >
> > -#define PAGE_S2_MEMATTR(attr) \
> > +#define PAGE_S2_MEMATTR(attr, has_fwb) \
> > ({ \
> > u64 __val; \
> > - if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) \
> > + if (has_fwb) \
> > __val = PTE_S2_MEMATTR(MT_S2_FWB_ ## attr); \
> > else \
> > __val = PTE_S2_MEMATTR(MT_S2_ ## attr); \
>
> Can you take the pgt structure instead of a bool here, or does it end up
> being really ugly?
It means I need to expose the stage2_has_fwb() helper in pgtable.h so I
can use it here. But Marc suggested that I introduce another macro along
the lines of
#define KVM_S2_MEMATTR(pgt, attr) PAGE_S2_MEMATTR(attr, stage2_has_fwb(pgt))
which can be defined in pgtable.c and keep everything neatly contained
in there. So I think I'll go ahead with that unless you feel strongly
about it.
Cheers,
Quentin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
