Re: [Xen-devel] [PATCH v6 2/4] xen/arm: use SYMBOL when required

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 10 Jan 2019, Jan Beulich wrote:
> >>> On 10.01.19 at 00:42, <sstabellini@xxxxxxxxxx> wrote:
> > @@ -1138,9 +1138,10 @@ void free_init_memory(void)
> >      for ( i = 0; i < nr; i++ )
> >          *(p + i) = insn;
> >  
> > -    set_pte_flags_on_range(__init_begin, len, mg_clear);
> > +    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_clear);
> >      init_domheap_pages(pa, pa + len);
> > -    printk("Freed %ldkB init memory.\n", 
> > (long)(__init_end-__init_begin)>>10);
> > +    printk("Freed %ldkB init memory.\n",
> > +           (long)(SYMBOL(__init_end)-SYMBOL(__init_begin))>>10);
> 
> I've noticed this only here, but I can't exclude I've overlooked other
> instances: I think it would be really nice if you corrected formatting
> at the same time (here: add the missing blanks).

OK

I tend not to do cleanups together with meaningful changes, because
typically I find the resulting patch harder to review, but I am OK with
doing cleanups if you the maintainer asks for them


> > --- a/xen/arch/arm/percpu.c
> > +++ b/xen/arch/arm/percpu.c
> > @@ -6,7 +6,7 @@
> >  
> >  unsigned long __per_cpu_offset[NR_CPUS];
> >  #define INVALID_PERCPU_AREA (-(long)__per_cpu_start)
> > -#define PERCPU_ORDER 
> > (get_order_from_bytes(__per_cpu_data_end-__per_cpu_start))
> > +#define PERCPU_ORDER (get_order_from_bytes(SYMBOL(__per_cpu_data_end) - 
> > SYMBOL(__per_cpu_start)))
> 
> Long line.

OK


> > @@ -37,7 +37,7 @@ static void _free_percpu_area(struct rcu_head *head)
> >  {
> >      struct free_info *info = container_of(head, struct free_info, rcu);
> >      unsigned int cpu = info->cpu;
> > -    char *p = __per_cpu_start + __per_cpu_offset[cpu];
> > +    char *p = SYMBOL(__per_cpu_start) + __per_cpu_offset[cpu];
> >      free_xenheap_pages(p, PERCPU_ORDER);
> >      __per_cpu_offset[cpu] = INVALID_PERCPU_AREA;
> >  }
> 
> As per above, to add the missing blank line would be quite nice at
> this occasion.

OK


> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -772,8 +772,10 @@ void __init start_xen(unsigned long boot_phys_offset,
> >  
> >      /* Register Xen's load address as a boot module. */
> >      xen_bootmodule = add_boot_module(BOOTMOD_XEN,
> > -                             (paddr_t)(uintptr_t)(_start + 
> > boot_phys_offset),
> > -                             (paddr_t)(uintptr_t)(_end - _start + 1), 
> > false);
> > +                             (paddr_t)(uintptr_t)(SYMBOL(_start) +
> > +                                                  boot_phys_offset),
> > +                             (paddr_t)(uintptr_t)(SYMBOL(_end) -
> > +                                                  SYMBOL(_start) + 1), 
> > false);
> 
> Why you need the double casts, i.e. why does (uintptr_t) alone not
> suffice?

The original reason was just not to change the existing code outside of
adding SYMBOL :-)

But to answer your question, uintptr_t is the same size of char*, while
paddr_t is always 64bit. uintptr_t casts to integer type, paddr_t casts
to the right size. I don't think it is allowed to change from pointer to
integer and change integer size in a single cast.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel