Re: [Xen-devel] [PATCH v7 10/15] x86/altp2m: add remaining support routines.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.15 at 01:01, <edmund.h.white@xxxxxxxxx> wrote:
> Add the remaining routines required to support enabling the alternate
> p2m functionality.
> 
> Signed-off-by: Ed White <edmund.h.white@xxxxxxxxx>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> Changes since v6:
>         rename altp2m lazy copier, make bool_t, use __put_gfn throughout,
>           and move to p2m.c, eliminating altp2m_hap.c
>         change various p2m_altp2m... routines from long to int
>         change uint16_t's/uint8_t's to unsigned int's
>         optimize remapped gfn tracking and altp2m invalidation check
>         mechanical changes due to patch 5 changes

Taken together clearly requiring dropping any earlier review tags.

Non-mm parts
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> +void p2m_flush_altp2m(struct domain *d)
> +{
> +    unsigned int i;
> +
> +    altp2m_list_lock(d);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        p2m_flush_table(d->arch.altp2m_p2m[i]);
> +        /* Uninit and reinit ept to force TLB shootdown */
> +        ept_p2m_uninit(d->arch.altp2m_p2m[i]);
> +        ept_p2m_init(d->arch.altp2m_p2m[i]);
> +        d->arch.altp2m_eptp[i] = INVALID_MFN;
> +    }

And more EPT-specific code in a generic file...

> +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
> +{
> +    struct p2m_domain *p2m;
> +    int rc = -EINVAL;
> +
> +    if ( !idx || idx > MAX_ALTP2M )

>= (and then also elsewhere further down)?

> +        return rc;
> +
> +    domain_pause_except_self(d);
> +
> +    altp2m_list_lock(d);
> +
> +    if ( d->arch.altp2m_eptp[idx] != INVALID_MFN )
> +    {
> +        p2m = d->arch.altp2m_p2m[idx];
> +
> +        if ( !_atomic_read(p2m->active_vcpus) )
> +        {
> +            p2m_flush_table(d->arch.altp2m_p2m[idx]);
> +            /* Uninit and reinit ept to force TLB shootdown */
> +            ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
> +            ept_p2m_init(d->arch.altp2m_p2m[idx]);
> +            d->arch.altp2m_eptp[idx] = INVALID_MFN;

Perhaps worth making all of the above if() body a helper function
(considering that the loop body in p2m_flush_altp2m() does
exactly the same)?

> @@ -758,6 +765,38 @@ bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, 
> unsigned int idx);
>  /* Check to see if vcpu should be switched to a different p2m. */
>  void p2m_altp2m_check(struct vcpu *v, uint16_t idx);

The context here suggests that I overlooked a still not replaced
uint16_t.

> +/* Flush all the alternate p2m's for a domain */
> +void p2m_flush_altp2m(struct domain *d);
> +
> +/* Alternate p2m paging */
> +bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
> +    unsigned long gla, struct npfec npfec, struct p2m_domain **ap2m);
> +
> +/* Make a specific alternate p2m valid */
> +int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);
> +
> +/* Find an available alternate p2m and make it valid */
> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx);

For this one, however, things really depend on the intended call
sites, i.e. I could see reasons for this to be kept as is.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel