Re: [Xen-devel] [PATCH v3 05/13] x86/altp2m: basic data structures and support routines.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.15 at 20:09, <edmund.h.white@xxxxxxxxx> wrote:
> ---
>  xen/arch/x86/hvm/Makefile        |  1 +
>  xen/arch/x86/hvm/altp2m.c        | 92 +++++++++++++++++++++++++++++++++++++

Wouldn't this better go into xen/arch/x86/mm/?

> +int
> +altp2m_vcpu_initialise(struct vcpu *v)
> +{
> +    int rc = -EOPNOTSUPP;
> +
> +    if ( v != current )
> +        vcpu_pause(v);
> +
> +    if ( !hvm_funcs.ap2m_vcpu_initialise ||
> +         (hvm_funcs.ap2m_vcpu_initialise(v) == 0) )
> +    {
> +        rc = 0;

I think you would better honor the error code returned by
hvm_funcs.ap2m_vcpu_initialise() and enter this block only if it
was zero.

> +        altp2m_vcpu_reset(v);
> +        vcpu_altp2m(v).p2midx = 0;
> +        atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
> +
> +        ap2m_vcpu_update_eptp(v);

We're in vendor independent code here - either the function is
misnamed, or it shouldn't be called directly from here.

> +void
> +altp2m_vcpu_destroy(struct vcpu *v)
> +{
> +    struct p2m_domain *p2m;
> +
> +    if ( v != current )
> +        vcpu_pause(v);
> +
> +    if ( hvm_funcs.ap2m_vcpu_destroy )
> +        hvm_funcs.ap2m_vcpu_destroy(v);
> +
> +    if ( (p2m = p2m_get_altp2m(v)) )
> +        atomic_dec(&p2m->active_vcpus);

The ordering looks odd - from an abstract perspective I'd expect
p2m_get_altp2m() to not return the p2m anymore that was just
destroyed via hvm_funcs.ap2m_vcpu_destroy().

> +void ap2m_vcpu_update_eptp(struct vcpu *v)
> +{

As I think I said before, I consider these ap2m_ prefixes ambiguous -
the 'a' could also stand for accelerated, advanced, ... Consistently
staying with altp2m_ would seem better.

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -459,7 +459,7 @@ void hap_domain_init(struct domain *d)
>  int hap_enable(struct domain *d, u32 mode)
>  {
>      unsigned int old_pages;
> -    uint8_t i;
> +    uint16_t i;

unsigned int (also elsewhere, including uint8_t-s)

> @@ -498,6 +498,24 @@ int hap_enable(struct domain *d, u32 mode)
>             goto out;
>      }
>  
> +    /* Init alternate p2m data */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +    {
> +        rv = -ENOMEM;
> +        goto out;
> +    }
> +
> +    for (i = 0; i < MAX_EPTP; i++)
> +        d->arch.altp2m_eptp[i] = INVALID_MFN;

The above again seems EPT-specific in a vendor independent file.

> @@ -196,7 +234,14 @@ int p2m_init(struct domain *d)
>       * (p2m_init runs too early for HVM_PARAM_* options) */
>      rc = p2m_init_nestedp2m(d);
>      if ( rc )
> +    {
>          p2m_teardown_hostp2m(d);
> +        return rc;
> +    }
> +
> +    rc = p2m_init_altp2m(d);
> +    if ( rc )
> +        p2m_teardown_altp2m(d);
>  
>      return rc;
>  }

And why not also p2m_teardown_hostp2m() in this last error case?
And doesn't p2m_init_nestedp2m() need undoing too? (Overall this
suggests the error cleanup structuring here should be changed.)

> @@ -294,6 +298,12 @@ struct arch_domain
>      struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>      mm_lock_t nested_p2m_lock;
>  
> +    /* altp2m: allow multiple copies of host p2m */
> +    bool_t altp2m_active;
> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +    mm_lock_t altp2m_lock;
> +    uint64_t *altp2m_eptp;

This is a non-insignificant increase of the structure size - perhaps all
of these should hang off of struct arch_domain via a single,
separately allocated pointer?

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -210,6 +210,14 @@ struct hvm_function_table {
>                                    uint32_t *ecx, uint32_t *edx);
>  
>      void (*enable_msr_exit_interception)(struct domain *d);
> +
> +    /* Alternate p2m */
> +    int (*ap2m_vcpu_initialise)(struct vcpu *v);
> +    void (*ap2m_vcpu_destroy)(struct vcpu *v);
> +    int (*ap2m_vcpu_reset)(struct vcpu *v);

Why is this returning int when altp2m_vcpu_reset() returns void?

> +static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    uint16_t index = vcpu_altp2m(v).p2midx;
> +
> +    return (index == INVALID_ALTP2M) ? NULL : d->arch.altp2m_p2m[index];

It would feel better if you checked against MAX_ALTP2M here.

Jan

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