Re: [Xen-devel] [PATCH 2/2] xen/x86: hap: Clean-up and harden hap_enable()

From: George Dunlap <george.dunlap@citrix.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>,
	"Julien Grall" <julien@xen.org>
Cc: Wei Liu <wl@xen.org>, George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Julien Grall <jgrall@amazon.com>, Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 2/2] xen/x86: hap: Clean-up and harden hap_enable()
Date: Tue, 4 Feb 2020 11:33:31 +0000	[thread overview]
Message-ID: <eaf932d8-79f2-61dd-9648-189c9e2ea955@citrix.com> (raw)
In-Reply-To: <20200204112807.GK4679@Air-de-Roger>

On 2/4/20 11:28 AM, Roger Pau Monné wrote:
> On Tue, Feb 04, 2020 at 11:11:11AM +0000, Julien Grall wrote:
>>
>>
>> On 04/02/2020 10:51, Roger Pau Monné wrote:
>>> On Tue, Feb 04, 2020 at 09:34:11AM +0000, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Unlike shadow_enable(), hap_enable() can only be called once during
>>>> domain creation and with the mode equal to mode equal to
>>>                                      ^ equals to
>>
>> Will fix it.
>>
>>>> PG_external | PG_translate | PG_refcounts.
>>>>
>>>> If it were called twice, then we might have something interesting
>>>                                                ^ a problem
>>>> problem as the p2m tables would be re-allocated (and therefore all the
>>>> mappings would be lost).
>>>>
>>>> Add code to sanity check the mode and that the function is only called
>>>> once. Take the opportunity to an if checking that PG_translate is set.
>>>                                  ^ add an if
>>
>> Will fix it.
>>
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>
>>>> ---
>>>>
>>>> It is not entirely clear when PG_translate was enforced.
>>>> ---
>>>>   xen/arch/x86/mm/hap/hap.c | 18 +++++++++++-------
>>>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
>>>> index 31362a31b6..b734e2e6d3 100644
>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
>>>>       unsigned int i;
>>>>       int rv = 0;
>>>> +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
>>>> +        return -EINVAL;
>>>> +
>>>> +    /* The function can only be called once */
>>>> +    if ( d->arch.paging.mode != 0 )
>>>> +        return -EINVAL;
>>>
>>> If you want to return EINVAL for both they can be merged into a single
>>> if. Also note that this would usually be written as
>>> if ( d->arch.paging.mode ) to keep it shorter.
>>
>> To be honest, this is a matter of taste. There is also an argument that for
>> MISRA, your suggestion is not compliant (see Rule 14.4).
> 
> Oh, then we should add those rules to CODING_STYLE if they are to be
> enforced.
> 
> So far the style of most of the hypervisor code is to omit the value
> when comparing against 0 or NULL AFAIK.
> 
> I don't have an issue with requiring explicit comparisons, but it
> needs to be documented so we can aim to have an homogeneous style,
> because so far I've been recommending the other way around.

Indeed, the general preference of the codebase as a whole is to favor
conciseness in this case; there's value in being consistent.

I don't want to be annoying about this.  I don't agree with the MISRA
rule here; but I do think that MISRA is important.  OTOH this is in x86
code, which I don't think anyone has suggested become MISRA compliant.
And if we're going to start making these sorts of changes, I agree that
we should have a discussion about it, rather than implicitly do things
sometimes one way and sometimes another.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel