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

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>-----Original Message-----
>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Monday, July 13, 2015 1:01 AM
>
>>>> On 10.07.15 at 23:48, <ravi.sahita@xxxxxxxxx> wrote:
>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>Sent: Thursday, July 09, 2015 6:30 AM
>>>
>>>>>> 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/?
>>
>> In this case we followed the pattern of nestedhvm - hope that's ok.
>
>Not really imo: Nested HVM obviously belongs in hvm/; alt-P2m is more of a
>mm extension than a HVM one afaict, and hence would rather belong in mm/.
>

Would like George's opinion also on this before we make this change (again want 
to avoid thrashing on things like this).

>>>> +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.
>>
>> The code is checking that condition - did I misinterpret?
>
>It is checking the condition, yes, but not propagating the error code.
>

We removed the unused wrapper out - so this one is moot.

>>>> +        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.
>>
>> Would it be reasonable to add if hap_enabled and cpu_has_vmx checks
>> like other code in this file that invokes ept specific ops?
>> Otherwise it seems ok that the function would be called from here for
>> p2m_altp2m interactions such as switching altp2m by id etc.
>> Open to any other suggestions from you, or we would like to leave it
>> as it is.
>
>Imo such should be abstracted out properly (if it's indeed EPT-specific), or 
>the
>function be renamed.
>

Renaming may make sense - checking first - Would a name like 
altp2m_vcpu_update_p2m() make sense?
(note - ap2m_ prefix is now altp2m_ prefix due to another review feedback to do 
that).


>>>> +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().
>>>
>>
>> ap2m_vcpu_destroy is for destroying vcpu context related to altp2m -
>> note this is not implemented since its not needed for Intel
>> implementation.  The idea is that if something needs to be done
>> specifically for for AMD then that could be done here.
>
>First of all this doesn't invalidate or address the concern raised.
>And then - if you don't need the hook, why don't you leave it out altogether,
>eliminating the need to decide about its caller's proper placement?
>

This unimplemented interface function is removed - so this is moot.

>>>> +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.
>>>
>>
>> We have a comment above the list of these ap2m_ functions in hvm.h
>> stating these are for Alternate p2m - do you feel strongly about us
>> changing this naming? Also this is the interface naming, and if we
>> renamed it altp2m_xxx it would cause confusion with the actual
>> altp2m_xx functionality - so we would like to leave it as proposed.
>
>I don't think there would be much confusion - structure member names and
>function names live in different name spaces anyway.
>So yes, I continue to think ap2m is a bad prefix...
>

Fixed - ap2m prefix is now altp2m prefix.

>>>> --- 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)
>>
>> We used existing iterator types that were being used (uint8_t was
>> being used in hap_final_teardown).
>> If you feel strongly we could change it but we would change code that
>> we didn't need to touch for this patch.
>
>I didn't say you should change code you otherwise don't need to touch. But
>both new code as well as code being changed anyway shouldn't
>repeat/continue pre-existing mistakes (or however you'd want to call such).
>

We will change this (didn't get to it in v5, saw it late and wanted to submit 
v5 since there are some other feedback we need anyway).

>>>> @@ -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?
>>
>> Is this a nice-to-have - again we modelled after the nestedhvm
>> extensions to the struct.
>> This will affect a lot of our patch without really changing how much
>> memory is allocated.
>
>I understand that. To a certain point I can agree to limit changes to what is
>there at this stage. But you wanting to avoid addressing concerns basically
>everywhere it's not a bug overextends this. Just because the series was
>submitted late doesn't mean you should now expect us to give in on any
>controversy regarding aspects we would normally expect to be changed. This
>would basically encourage others to submit their stuff late too in the future,
>hoping for relaxed review.
>

Couple things - first, we have absorbed a lot of (good) feedback - thanks for 
that.
Second, I don't think the series can be characterized as late (feedback from 
others welcome). 
V1 had almost the same structure and was submitted in January.
On this change - this will be a lot of effects on the code and we would like to 
avoid this one.

>>>> --- 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?
>>
>> Currently altp2m_vcpu_reset cannot fail - but at the interface level
>> from hvm, we wanted to allow someone to change that in the future, so
>> the interface allows for a return code.
>
>That's precisely what I assumed, and precisely what I want to see
>avoided: Either the operation can (theoretically) fail - then this should be
>catered for at all levels. Or it can't - then there's no point for the non-void
>return type here.
>

There is no wrapper any more - this comment also doesn't apply any more.

>>>> +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.
>>
>> There is no way for p2midx to be >= MAX_ALTP2M without being
>INVALID_ALTP2M.
>
>If there was an ASSERT() to that effect I'd be fine. Yet you have to accept 
>that
>bugs may exist somewhere, and being tight with checks like those for valid
>array indexes lowers the risk / impact of security issues.
>
>Jan

There is an ASSERT now.

Ravi


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