Re: [Xen-devel] [PATCH v7 11/15] x86/altp2m: define and implement alternate p2m HVMOP types.

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

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Thursday, July 23, 2015 8:09 AM
>
>>>> On 23.07.15 at 16:56, <ravi.sahita@xxxxxxxxx> wrote:
>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>Sent: Thursday, July 23, 2015 3:22 AM
>>>
>>>>>> On 23.07.15 at 01:01, <edmund.h.white@xxxxxxxxx> wrote:
>>>> Signed-off-by: Ed White <edmund.h.white@xxxxxxxxx>
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>>And I have to withdraw this ack pending clarification of (and perhaps
>>>adjustment to) the #VE info address interface.
>>>
>>
>> Could we have the ack back :-) I clarified the #VE info address
>> interface in the other email - repeating here:
>>
>> " If the "EPT-violation #VE" VM-execution control is 1, the
>> virtualization-exception information address must satisfy the
>> following checks:
>> - Bits 11:0 of the address must be 0.
>> - The address must not set any bits beyond the processor's
>> physical-address width."
>
>Yes, for this aspect.
>
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -6138,6 +6138,140 @@ static int hvmop_get_param(
>>>>      return rc;
>>>>  }
>>>>
>>>> +static int do_altp2m_op(
>>>> +    XEN_GUEST_HANDLE_PARAM(void) arg) {
>>>> +    struct xen_hvm_altp2m_op a;
>>>> +    struct domain *d = NULL;
>>>> +    int rc = 0;
>>>> +
>>>> +    if ( !hvm_altp2m_supported() )
>>>> +        return -EOPNOTSUPP;
>>>> +
>>>> +    if ( copy_from_guest(&a, arg, 1) )
>>>> +        return -EFAULT;
>>>> +
>>>> +    if ( a.pad1 || a.pad2 ||
>>>> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
>>>> +         (a.cmd < HVMOP_altp2m_get_domain_state) ||
>>>> +         (a.cmd > HVMOP_altp2m_change_gfn) )
>>>
>>>I'm afraid such a change invalidates any earlier ack, even if ti is correct.
>> Instead
>>>of this, why don't you start numbering of the sub-ops at zero? Or if
>>>you
>> really
>>>have a reason to start at 1, why not simply check a.cmd against zero
>>>(without using any particular sub-op value)? And then it escapes me
>>>why this can't be handled in a default case in the switch statement below
>anyway.
>>
>> Hmm - is that a requirement per se? we are consistently checking per
>> the sub-op definition we have.
>
>Well, in a way. But doing range checks like this means future additions of sub-
>ops would always need to touch this code. Quite different from doing it in the
>default case of a switch statement.
>Plus, can you see how the expression is going to look like if in interface 
>version
>2 you need to remove one or two of the current entries, replacing them with
>new, higher numbers?
>

Yes in a revision I would handle this with the default case.

>> Would like this to be considered as is.
>>
>> As I said in the cover letter we have constraints on how much more we
>> can do this week now - so requesting the maintainers to accept v7 with
>> the review comments you have on those recorded as pending to be
>> addressed by us.
>
>Yes, on that basis, albeit extremely hesitantly to be honest. If any other
>maintainer would be as hesitant as I am about this, I would likely put the two
>together to yield a NAK.

Thanks for your flexibility and honesty on this - 
hope other maintainers are ok with v7 in the same way, with keeping the other 
comments you have logged already as pending for post 4.6.

Ravi

>
>Jan


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