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