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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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

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

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

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

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

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