Re: [Xen-devel] [PATCH 4/4] SVM: clean up svm_vmcb_isvalid()

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/31/2017 11:32 AM, Andrew Cooper wrote:
> On 31/05/17 15:50, Boris Ostrovsky wrote:
>> On 05/31/2017 08:14 AM, Andrew Cooper wrote:
>>> On 31/05/17 08:23, Jan Beulich wrote:
>>>> - correct CR3 and CR4 checks
>>>> - add vcpu parameter (to include in log messages) and constify vmcb one
>>>> - use bool/true/false
>>>> - use accessors
>>>> - adjust formatting
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>
>>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> @@ -658,13 +658,13 @@ static int nsvm_vmcb_prepare4vmrun(struc
>>>>      /* Cleanbits */
>>>>      n2vmcb->cleanbits.bytes = 0;
>>>>  
>>>> -    rc = svm_vmcb_isvalid(__func__, ns_vmcb, 1);
>>>> +    rc = svm_vmcb_isvalid(__func__, ns_vmcb, v, true);
>>>>      if (rc) {
>>>>          gdprintk(XENLOG_ERR, "virtual vmcb invalid\n");
>>>>          return NSVM_ERROR_VVMCB;
>>>>      }
>>>>  
>>>> -    rc = svm_vmcb_isvalid(__func__, n2vmcb, 1);
>>>> +    rc = svm_vmcb_isvalid(__func__, n2vmcb, v, true);
>>> As these are the only two callsites, I don't think the __func__ or
>>> verbose parameters are useful.  I'd just drop them.
>> I actually think keeping this is useful. We indeed have only two
>> invocations but someone debugging a problem may want to add a few more.
> Why?  Its clear where it is being called from by the following "$FOO
> invalid" log message.

You won't have to print anything at the call site if this is kept. (I am
not suggesting to get rid of existing printks, this would only be useful
for one-off debugging).

-boris


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