Re: [PATCH v1 13/18] x86: generalize physmap logic

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 23:04, Daniel P. Smith wrote:
> The existing physmap code is specific to dom0.

I think this needs better wording. Either you name the function or you
explain what piece of code you're talking about. "physmap" alone is
just not meaningful enough. (Also applies to the title.)

> --- a/xen/arch/x86/include/asm/dom0_build.h
> +++ b/xen/arch/x86/include/asm/dom0_build.h
> @@ -21,7 +21,7 @@ int dom0_construct_pvh(struct boot_domain *bd);
>  unsigned long dom0_paging_pages(const struct domain *d,
>                                  unsigned long nr_pages);
>  
> -void dom0_update_physmap(bool compat, unsigned long pfn,
> +void dom_update_physmap(bool compat, unsigned long pfn,
>                           unsigned long mfn, unsigned long vphysmap_s);

So my initial inclination was to suggest domain_ as a name prefix,
matching what we have elsewhere. But when we're already giving the
thing a new name, its PV-only nature also wants expressing. Hence
I'd like to suggest pv_update_physmap(). And then please fix
indentation of the continuation lines here and below.

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -34,8 +34,8 @@
>  #define L3_PROT (BASE_PROT|_PAGE_DIRTY)
>  #define L4_PROT (BASE_PROT|_PAGE_DIRTY)
>  
> -void __init dom0_update_physmap(bool compat, unsigned long pfn,
> -                                unsigned long mfn, unsigned long vphysmap_s)
> +void __init dom_update_physmap(
> +    bool compat, unsigned long pfn, unsigned long mfn, unsigned long 
> vphysmap_s)
>  {

Personally I dislike this further change to re-flow the parameter
list, as I see no particular reason for doing so.

Jan