[v7] Reuse 32 bit C code more safely

[PATCH v7 5/5] x86/boot: Clarify comment

Posted by Frediano Ziglio 1 year, 3 months ago

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/reloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index e50e161b27..e725cfb6eb 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -65,7 +65,7 @@ typedef struct memctx {
     /*
      * Simple bump allocator.
      *
-     * It starts from the base of the trampoline and allocates downwards.
+     * It starts on top of space reserved for the trampoline and allocates downwards.
      */
     uint32_t ptr;
 } memctx;
-- 
2.34.1

Re: [PATCH v7 5/5] x86/boot: Clarify comment

Posted by Roger Pau Monné 1 year, 3 months ago

On Tue, Oct 29, 2024 at 10:29:42AM +0000, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
>  xen/arch/x86/boot/reloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> index e50e161b27..e725cfb6eb 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -65,7 +65,7 @@ typedef struct memctx {
>      /*
>       * Simple bump allocator.
>       *
> -     * It starts from the base of the trampoline and allocates downwards.
> +     * It starts on top of space reserved for the trampoline and allocates downwards.

I'm afraid this line is over 80 characters long, will need to be
adjusted.  Maybe:

    * Starts at top of the relocated trampoline space and allocates downwards.

Thanks.

Re: [PATCH v7 5/5] x86/boot: Clarify comment

Posted by Andrew Cooper 1 year, 3 months ago

On 29/10/2024 2:53 pm, Roger Pau Monné wrote:
> On Tue, Oct 29, 2024 at 10:29:42AM +0000, Frediano Ziglio wrote:
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
>> ---
>>  xen/arch/x86/boot/reloc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
>> index e50e161b27..e725cfb6eb 100644
>> --- a/xen/arch/x86/boot/reloc.c
>> +++ b/xen/arch/x86/boot/reloc.c
>> @@ -65,7 +65,7 @@ typedef struct memctx {
>>      /*
>>       * Simple bump allocator.
>>       *
>> -     * It starts from the base of the trampoline and allocates downwards.
>> +     * It starts on top of space reserved for the trampoline and allocates downwards.
> I'm afraid this line is over 80 characters long, will need to be
> adjusted.  Maybe:
>
>     * Starts at top of the relocated trampoline space and allocates downwards.

This patch miss misses 2 of the 3 incorrect statements about how the
trampoline works, and Alejandro had some better suggestions in the
thread on the matter.

~Andrew

Re: [PATCH v7 5/5] x86/boot: Clarify comment

Posted by Frediano Ziglio 1 year, 3 months ago

On Tue, Oct 29, 2024 at 3:07 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 29/10/2024 2:53 pm, Roger Pau Monné wrote:
> > On Tue, Oct 29, 2024 at 10:29:42AM +0000, Frediano Ziglio wrote:
> >> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> >> ---
> >>  xen/arch/x86/boot/reloc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> >> index e50e161b27..e725cfb6eb 100644
> >> --- a/xen/arch/x86/boot/reloc.c
> >> +++ b/xen/arch/x86/boot/reloc.c
> >> @@ -65,7 +65,7 @@ typedef struct memctx {
> >>      /*
> >>       * Simple bump allocator.
> >>       *
> >> -     * It starts from the base of the trampoline and allocates downwards.
> >> +     * It starts on top of space reserved for the trampoline and allocates downwards.
> > I'm afraid this line is over 80 characters long, will need to be
> > adjusted.  Maybe:
> >
> >     * Starts at top of the relocated trampoline space and allocates downwards.
>
> This patch miss misses 2 of the 3 incorrect statements about how the
> trampoline works, and Alejandro had some better suggestions in the
> thread on the matter.
>
> ~Andrew

Hi,
  changed to "Starts at the end of the relocated trampoline space and
allocates backwards".

See https://gitlab.com/xen-project/people/fziglio/xen/-/commit/21be0b9d2813db9c578e8a6ace76eee2445908f5.

Frediano

Re: [PATCH v7 5/5] x86/boot: Clarify comment

Posted by Alejandro Vallejo 1 year, 3 months ago

On Tue Oct 29, 2024 at 4:40 PM GMT, Frediano Ziglio wrote:
> On Tue, Oct 29, 2024 at 3:07 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> > On 29/10/2024 2:53 pm, Roger Pau Monné wrote:
> > > On Tue, Oct 29, 2024 at 10:29:42AM +0000, Frediano Ziglio wrote:
> > >> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> > >> ---
> > >>  xen/arch/x86/boot/reloc.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> > >> index e50e161b27..e725cfb6eb 100644
> > >> --- a/xen/arch/x86/boot/reloc.c
> > >> +++ b/xen/arch/x86/boot/reloc.c
> > >> @@ -65,7 +65,7 @@ typedef struct memctx {
> > >>      /*
> > >>       * Simple bump allocator.
> > >>       *
> > >> -     * It starts from the base of the trampoline and allocates downwards.
> > >> +     * It starts on top of space reserved for the trampoline and allocates downwards.
> > > I'm afraid this line is over 80 characters long, will need to be
> > > adjusted.  Maybe:
> > >
> > >     * Starts at top of the relocated trampoline space and allocates downwards.
> >
> > This patch miss misses 2 of the 3 incorrect statements about how the
> > trampoline works, and Alejandro had some better suggestions in the
> > thread on the matter.
> >
> > ~Andrew
>
> Hi,
>   changed to "Starts at the end of the relocated trampoline space and
> allocates backwards".
>
> See https://gitlab.com/xen-project/people/fziglio/xen/-/commit/21be0b9d2813db9c578e8a6ace76eee2445908f5.
>
> Frediano

with that:

  Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Cheers,
Alejandro