Re: [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests

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

>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> As perform_tests() is going to clear memory past 4MB, we check that the
> memory can be use or we skip the tests.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Changes in v4:
> - move the check into the perform_test() function.
> - skip tests instead of using BUG.
> 
> New in V3
> ---
>  tools/firmware/hvmloader/tests.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/tools/firmware/hvmloader/tests.c 
> b/tools/firmware/hvmloader/tests.c
> index fea3ad3..7996206 100644
> --- a/tools/firmware/hvmloader/tests.c
> +++ b/tools/firmware/hvmloader/tests.c
> @@ -210,6 +210,26 @@ void perform_tests(void)
>          return;
>      }
>  
> +    /* Check that tests does not use memory where modules are stored */
> +    if ( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) > 4 << 20

    if ( (uint32_t)(hvm_start_info + 1) > (4 << 20)

> +         && (uint32_t)hvm_start_info < 8 << 20 )

Missing parentheses around operands of <<. Also commonly the
&& would belong on the previous line.

> +    {
> +        printf("Skipping tests due to memory used by hvm_start_info\n");
> +        return;
> +    }
> +    for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )

Bogus shadowing of an outer scope variable. And we prefer not to
use this C99 declaration style anyway.

> +    {
> +        const struct hvm_modlist_entry *modlist =
> +            (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
> +        if ( modlist[i].paddr

Blank line ...

> +             && modlist[i].paddr + modlist[i].size > 4ul << 20
> +             && modlist[i].paddr < 8ul << 20 )

Parentheses ... Also, why the ul suffixed here but not above?
Please be consistent.

> +        {
> +            printf("Skipping tests due to memory used by a module\n");

How about at least printing the module index as well?

> +            return;
> +        }
> +    }

How about cmdline_paddr (both variant) and rsdp_paddr?

Jan


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