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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.07.15 at 01:58, <edmund.h.white@xxxxxxxxx> wrote:
> Add the basic data structures needed to support alternate p2m's and
> the functions to initialise them and tear them down.
> 
> Although Intel hardware can handle 512 EPTP's per hardware thread
> concurrently, only 10 per domain are supported in this patch for
> performance reasons.
> 
> The iterator in hap_enable() does need to handle 512, so that is now
> uint16_t.

Sigh - this one is still here (and the respective code unchanged).
I'm not going to NAK the patch just because of this, but it really
looks like you aren't trying to address comments even when
they're trivial (and quick) to carry out and testing of the change
comes as a side effect of you needing to test all the other changes
as well.

> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -1,6 +1,7 @@
>  subdir-y += svm
>  subdir-y += vmx
>  
> +obj-y += altp2m.o

Wasn't the outcome of the earlier discussion to put this in x86/mm,
or possibly not even introduce a new file?

> +void
> +altp2m_vcpu_initialise(struct vcpu *v)
> +{
> +    if ( v != current )
> +        vcpu_pause(v);
> +
> +    altp2m_vcpu_reset(v);
> +    vcpu_altp2m(v).p2midx = 0;
> +    atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
> +
> +    altp2m_vcpu_update_eptp(v);

Ahem - I'm not going to repeat the earlier comment yet another time,
the more that I see that the same issue didn't get addressed further
down either. Did you do _any_ of the requested changes at all? The
overview mail's list of changes seems to indicate you didn't. That also
reminds me to ask you yet another time to put the change info into
the individual patch mails; whether you _also_ put them in the
overview mail is up to you.

> @@ -196,7 +234,18 @@ 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_hostp2m(d);
> +        p2m_teardown_nestedp2m(d);
> +        p2m_teardown_altp2m(d);

I can see (and remember having asked for) the first two, but
p2m_init_altp2m() would better clean up after itself in case of
failure.

Overall the situation didn't really change from v5 - the code from
a pure functionality pov looks okay, but I don't see myself giving
in on all the "minor" issues the patch introduces. If some were
left adjusting of which really takes time to or risks breaking the
code, I'd (reluctantly) give my ack, but not this way, I'm afraid.

Jan


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