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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 23.07.15 at 16:56, <ravi.sahita@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>Sent: Thursday, July 23, 2015 3:22 AM
>>
>>>>> On 23.07.15 at 01:01, <edmund.h.white@intel.com> wrote:
>>> Signed-off-by: Ed White <edmund.h.white@intel.com>
>>>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>>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?

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

Jan