Re: [PATCH 8/9] xen/ppc: Add stub function and symbol definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.08.2023 01:03, Shawn Anastasio wrote:
> --- a/xen/arch/ppc/mm-radix.c
> +++ b/xen/arch/ppc/mm-radix.c
> @@ -266,3 +266,47 @@ void __init setup_initial_pagetables(void)
>      /* Turn on the MMU */
>      enable_mmu();
>  }
> +
> +

Nit: No double blank lines please.

> +/*
> + * TODO: Implement the functions below
> + */
> +unsigned long total_pages;

Hmm, yet one more prereq patch for me to make: There should be no need
for every arch to have a definition, when common code requires the
variable. While looking there I found max_page, which common code
references as well. I'm surprised you get away without. I guess I'll
learn why that is when making the patch moving both.

> +unsigned long frametable_base_pdx __read_mostly;

While we still have many instances like this, we prefer this form:

unsigned long __read_mostly frametable_base_pdx;

> +
> +void put_page(struct page_info *p)
> +{
> +    BUG();
> +}
> +
> +void arch_dump_shared_mem_info(void)
> +{
> +    BUG();
> +}

And perhaps one further prereq patch to avoid the need for this.

> +int xenmem_add_to_physmap_one(struct domain *d,
> +                              unsigned int space,
> +                              union add_to_physmap_extra extra,
> +                              unsigned long idx,
> +                              gfn_t gfn)
> +{
> +    BUG();
> +}
> +
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> +{
> +    BUG();
> +}
> +
> +int map_pages_to_xen(unsigned long virt,
> +                     mfn_t mfn,
> +                     unsigned long nr_mfns,
> +                     unsigned int flags)

There's a patch in flight regarding the naming of this last parameter.
I guess PPC would best be in sync right away.

> --- a/xen/arch/ppc/setup.c
> +++ b/xen/arch/ppc/setup.c
> @@ -1,5 +1,8 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include <xen/lib.h>
>  #include <xen/init.h>
> +#include <xen/mm.h>
> +#include <public/version.h>
>  #include <asm/boot.h>
>  #include <asm/early_printk.h>
>  #include <asm/processor.h>

There's no need for xen/lib.h to come ahead of xen/init.h, is there?

> --- /dev/null
> +++ b/xen/arch/ppc/stubs.c
> @@ -0,0 +1,351 @@
>[...]
> +static void ack_none(struct irq_desc *irq)
> +{
> +    BUG();
> +}
> +
> +static void end_none(struct irq_desc *irq)
> +{
> +    BUG();
> +}
> +
> +hw_irq_controller no_irq_type = {
> +    .typename = "none",
> +    .startup = irq_startup_none,
> +    .shutdown = irq_shutdown_none,
> +    .enable = irq_enable_none,
> +    .disable = irq_disable_none,
> +    .ack = ack_none,
> +    .end = end_none
> +};

I would recommend to avoid filling pointers (and hence having private
hook functions) where it's not clear whether they'll be required. "end",
for example, is an optional hook on x86. Iirc common code doesn't use
any of the hooks.

Jan