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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 8/8/23 5:27 AM, Jan Beulich wrote:
> 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.
>

Will fix.

>> +/*
>> + * 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.
>

Since your patch for this has gone though, I'll drop this in v2.

>> +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;
>

Will update.

>> +
>> +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.
>

Will remove from this series pending merge of your prereq patch.

>> +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.
>

I can't seem to find the patch in question and it doesn't seem like it
has been merged in the meantime. Could you provide a link?

>> --- 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?
>

There is not -- I'll fix the ordering.

>> --- /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.
>

Alright, I'll drop the `end_none` stub and leave the .end pointer as
NULL.

> Jan

Thanks,
Shawn