[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

>-----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 
>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 

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

There is an ASSERT now.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.