Re: [PATCH v7 8/8] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

some more remarks.

On 09/10/2023 02:03, Henry Wang wrote:
> +/*
> + * Invalidate all entries in the root page-tables. This is
> + * useful to get fault on entry and do an action.
> + *
> + * p2m_invalid_root() should not be called when the P2M is shared with
> + * the IOMMU because it will cause IOMMU fault.
> + */
> +void p2m_invalidate_root(struct p2m_domain *p2m)
I think this function doesn't make sense for the MPU. What would invalidate?

The only caller outside of the file is in 
arch_domain_creation_finished(). This was used for set/way emulation 
which I believe will be difficult to implement efficiently for the MPU 
(see my other reply to this patch).
So I would consider to move arch_domain_creation_finished() in mmu/p2m.c 
or creating a new function p2m_domain_creation_finished() which is 
called from arch_domain_creation_finished().

[...]

> +/*
> + * Clean & invalidate RAM associated to the guest vCPU.
> + *
> + * The function can only work with the current vCPU and should be called
> + * with IRQ enabled as the vCPU could get preempted.
> + */
> +void p2m_flush_vm(struct vcpu *v)
I believe an implementation of p2m_flush_vm() would be quite expensive 
for the MPU. It would be better to not emulate set/way for now.
It would also feel a bit odd to leave it unimplemented and called from 
check_for_vcpu_work(). So maybe we want to rename it. I don't have a 
goood name suggestion though. Bertrand, Stefano?
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> +    int rc;
> +    gfn_t start = _gfn(0);
> +
> +    ASSERT(v == current);
> +    ASSERT(local_irq_is_enabled());
> +    ASSERT(v->arch.need_flush_to_ram);
> +
> +    do
> +    {
> +        rc = p2m_cache_flush_range(v->domain, &start, _gfn(ULONG_MAX));
> +        if ( rc == -ERESTART )
> +            do_softirq();
> +    } while ( rc == -ERESTART );
> +
> +    if ( rc != 0 )
> +        gprintk(XENLOG_WARNING,
> +                "P2M has not been correctly cleaned (rc = %d)\n",
> +                rc);
> +
> +    /*
> +     * Invalidate the p2m to track which page was modified by the guest
> +     * between call of p2m_flush_vm().
> +     */
> +    p2m_invalidate_root(p2m);
> +
> +    v->arch.need_flush_to_ram = false;
> +}
> +
> +/* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
> +static register_t __read_mostly vtcr;
> +
> +void setup_virt_paging_one(void *data)
> +{
> +    WRITE_SYSREG(vtcr, VTCR_EL2);
> +
> +    /*
> +     * ARM64_WORKAROUND_AT_SPECULATE: We want to keep the TLBs free from
> +     * entries related to EL1/EL0 translation regime until a guest vCPU
> +     * is running. For that, we need to set-up VTTBR to point to an empty
> +     * page-table and turn on stage-2 translation. The TLB entries
> +     * associated with EL1/EL0 translation regime will also be flushed in case
> +     * an AT instruction was speculated before hand.
> +     */
> +    if ( cpus_have_cap(ARM64_WORKAROUND_AT_SPECULATE) )
> +    {
> +        WRITE_SYSREG64(generate_vttbr(INVALID_VMID, empty_root_mfn), 
> VTTBR_EL2);
> +        WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_VM, HCR_EL2);
> +        isb();
> +
> +        flush_all_guests_tlb_local();
> +    }
> +}

[...]

--
Julien Grall