diff mbox series

xen/arm: Fix arm32 build failure when early printk is enabled

Message ID 20240228103555.172101-1-michal.orzel@amd.com (mailing list archive)
State New, archived
Headers show
Series xen/arm: Fix arm32 build failure when early printk is enabled | expand

Commit Message

Michal Orzel Feb. 28, 2024, 10:35 a.m. UTC
Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
failure on arm32, when early printk is enabled:
arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'

Fixes: 0441c3acc7e9 ("xen/arm: fixmap: Rename the fixmap slots to follow the x86 convention")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/include/asm/early_printk.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Julien Grall Feb. 28, 2024, 11:42 a.m. UTC | #1
Hi Michal,

On 28/02/2024 10:35, Michal Orzel wrote:
> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
> failure on arm32, when early printk is enabled:
> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'

Good catch! Somewhat related I wonder whether we should add earlyprintk 
testing in gitlab?

> 
> Fixes: 0441c3acc7e9 ("xen/arm: fixmap: Rename the fixmap slots to follow the x86 convention")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,
Julien Grall Feb. 28, 2024, 11:57 a.m. UTC | #2
On 28/02/2024 11:42, Julien Grall wrote:
> Hi Michal,
> 
> On 28/02/2024 10:35, Michal Orzel wrote:
>> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
>> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
>> failure on arm32, when early printk is enabled:
>> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and 
>> *ABS* sections) for `*'
> 
> Good catch! Somewhat related I wonder whether we should add earlyprintk 
> testing in gitlab?
> 
>>
>> Fixes: 0441c3acc7e9 ("xen/arm: fixmap: Rename the fixmap slots to 
>> follow the x86 convention")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

And committed.

Cheers,
Michal Orzel Feb. 28, 2024, 12:42 p.m. UTC | #3
Hi Julien,

On 28/02/2024 12:42, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 28/02/2024 10:35, Michal Orzel wrote:
>> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
>> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
>> failure on arm32, when early printk is enabled:
>> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'
> 
> Good catch! Somewhat related I wonder whether we should add earlyprintk
> testing in gitlab?
I thought about adding this and I think we should at least have build jobs (hypervisor only, no toolstack)
selecting early printk. When it comes to testing if early printk works, I'm not sure. It'd be nice
but FWIR we have limited bandwidth.

@Stefano, what's your opinion?

~Michal
Stefano Stabellini Feb. 28, 2024, 10:27 p.m. UTC | #4
On Wed, 28 Feb 2024, Michal Orzel wrote:
> Hi Julien,
> 
> On 28/02/2024 12:42, Julien Grall wrote:
> > 
> > 
> > Hi Michal,
> > 
> > On 28/02/2024 10:35, Michal Orzel wrote:
> >> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
> >> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
> >> failure on arm32, when early printk is enabled:
> >> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'
> > 
> > Good catch! Somewhat related I wonder whether we should add earlyprintk
> > testing in gitlab?
> I thought about adding this and I think we should at least have build jobs (hypervisor only, no toolstack)
> selecting early printk. When it comes to testing if early printk works, I'm not sure. It'd be nice
> but FWIR we have limited bandwidth.
> 
> @Stefano, what's your opinion?

I think it would be a good and quick test to have. To save testing
bandwidth I think we should reduce the amount of debug/non-debug
variations of the same tests that we have.
Michal Orzel Feb. 29, 2024, 10:07 a.m. UTC | #5
On 28/02/2024 23:27, Stefano Stabellini wrote:
> 
> 
> On Wed, 28 Feb 2024, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 28/02/2024 12:42, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 28/02/2024 10:35, Michal Orzel wrote:
>>>> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
>>>> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
>>>> failure on arm32, when early printk is enabled:
>>>> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'
>>>
>>> Good catch! Somewhat related I wonder whether we should add earlyprintk
>>> testing in gitlab?
>> I thought about adding this and I think we should at least have build jobs (hypervisor only, no toolstack)
>> selecting early printk. When it comes to testing if early printk works, I'm not sure. It'd be nice
>> but FWIR we have limited bandwidth.
>>
>> @Stefano, what's your opinion?
> 
> I think it would be a good and quick test to have. To save testing
> bandwidth I think we should reduce the amount of debug/non-debug
> variations of the same tests that we have.
Yes, I suggested that some time ago. We could keep both versions for generic tests,
but remove the non-debug version (unless you prefer to do the opposite) for:
Arm64:
- staticmem
- staticheap
- shared-mem
- boot-cpupools
Arm32:
- gzip
- without-dom0

This would save us some bandwidth that we could use for testing other features (e.g. early printk).

~Michal
Julien Grall Feb. 29, 2024, 10:10 a.m. UTC | #6
Hi,

On 29/02/2024 10:07, Michal Orzel wrote:
> 
> 
> On 28/02/2024 23:27, Stefano Stabellini wrote:
>>
>>
>> On Wed, 28 Feb 2024, Michal Orzel wrote:
>>> Hi Julien,
>>>
>>> On 28/02/2024 12:42, Julien Grall wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>> On 28/02/2024 10:35, Michal Orzel wrote:
>>>>> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
>>>>> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
>>>>> failure on arm32, when early printk is enabled:
>>>>> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'
>>>>
>>>> Good catch! Somewhat related I wonder whether we should add earlyprintk
>>>> testing in gitlab?
>>> I thought about adding this and I think we should at least have build jobs (hypervisor only, no toolstack)
>>> selecting early printk. When it comes to testing if early printk works, I'm not sure. It'd be nice
>>> but FWIR we have limited bandwidth.
>>>
>>> @Stefano, what's your opinion?
>>
>> I think it would be a good and quick test to have. To save testing
>> bandwidth I think we should reduce the amount of debug/non-debug
>> variations of the same tests that we have.
> Yes, I suggested that some time ago. We could keep both versions for generic tests,
> but remove the non-debug version (unless you prefer to do the opposite) for:

I think it makes sense during development window to use the debug 
version. However, I think we want some non-debug testing during the 
hardening phase.

Can gitlab read CONFIG_DEBUG from Config.mk?

Cheers,
Michal Orzel Feb. 29, 2024, 12:32 p.m. UTC | #7
On 29/02/2024 11:10, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 29/02/2024 10:07, Michal Orzel wrote:
>>
>>
>> On 28/02/2024 23:27, Stefano Stabellini wrote:
>>>
>>>
>>> On Wed, 28 Feb 2024, Michal Orzel wrote:
>>>> Hi Julien,
>>>>
>>>> On 28/02/2024 12:42, Julien Grall wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On 28/02/2024 10:35, Michal Orzel wrote:
>>>>>> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
>>>>>> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
>>>>>> failure on arm32, when early printk is enabled:
>>>>>> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'
>>>>>
>>>>> Good catch! Somewhat related I wonder whether we should add earlyprintk
>>>>> testing in gitlab?
>>>> I thought about adding this and I think we should at least have build jobs (hypervisor only, no toolstack)
>>>> selecting early printk. When it comes to testing if early printk works, I'm not sure. It'd be nice
>>>> but FWIR we have limited bandwidth.
>>>>
>>>> @Stefano, what's your opinion?
>>>
>>> I think it would be a good and quick test to have. To save testing
>>> bandwidth I think we should reduce the amount of debug/non-debug
>>> variations of the same tests that we have.
>> Yes, I suggested that some time ago. We could keep both versions for generic tests,
>> but remove the non-debug version (unless you prefer to do the opposite) for:
> 
> I think it makes sense during development window to use the debug
> version. However, I think we want some non-debug testing during the
> hardening phase.
> 
> Can gitlab read CONFIG_DEBUG from Config.mk?
At the moment, we have 2 types of jobs - non debug and debug (with -debug suffix).
They set "debug" variable accordingly, which is used later on to modify .config:
echo "CONFIG_DEBUG=${debug}" >> xen/.config

Without this line, Xen would be built according to default value of CONFIG_DEBUG.
That said, I don't think we want to get back to this behavior.

If we want to save some bandwidth, we should make a decision whether to keep debug or non-debug versions.
x86 has both versions for build jobs and mostly debug test jobs.

~Michal
Stefano Stabellini Feb. 29, 2024, 7:08 p.m. UTC | #8
On Thu, 29 Feb 2024, Michal Orzel wrote:
> On 29/02/2024 11:10, Julien Grall wrote:
> > 
> > 
> > Hi,
> > 
> > On 29/02/2024 10:07, Michal Orzel wrote:
> >>
> >>
> >> On 28/02/2024 23:27, Stefano Stabellini wrote:
> >>>
> >>>
> >>> On Wed, 28 Feb 2024, Michal Orzel wrote:
> >>>> Hi Julien,
> >>>>
> >>>> On 28/02/2024 12:42, Julien Grall wrote:
> >>>>>
> >>>>>
> >>>>> Hi Michal,
> >>>>>
> >>>>> On 28/02/2024 10:35, Michal Orzel wrote:
> >>>>>> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
> >>>>>> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
> >>>>>> failure on arm32, when early printk is enabled:
> >>>>>> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and *ABS* sections) for `*'
> >>>>>
> >>>>> Good catch! Somewhat related I wonder whether we should add earlyprintk
> >>>>> testing in gitlab?
> >>>> I thought about adding this and I think we should at least have build jobs (hypervisor only, no toolstack)
> >>>> selecting early printk. When it comes to testing if early printk works, I'm not sure. It'd be nice
> >>>> but FWIR we have limited bandwidth.
> >>>>
> >>>> @Stefano, what's your opinion?
> >>>
> >>> I think it would be a good and quick test to have. To save testing
> >>> bandwidth I think we should reduce the amount of debug/non-debug
> >>> variations of the same tests that we have.
> >> Yes, I suggested that some time ago. We could keep both versions for generic tests,
> >> but remove the non-debug version (unless you prefer to do the opposite) for:
> > 
> > I think it makes sense during development window to use the debug
> > version. However, I think we want some non-debug testing during the
> > hardening phase.
> > 
> > Can gitlab read CONFIG_DEBUG from Config.mk?
> At the moment, we have 2 types of jobs - non debug and debug (with -debug suffix).
> They set "debug" variable accordingly, which is used later on to modify .config:
> echo "CONFIG_DEBUG=${debug}" >> xen/.config
> 
> Without this line, Xen would be built according to default value of CONFIG_DEBUG.
> That said, I don't think we want to get back to this behavior.
> 
> If we want to save some bandwidth, we should make a decision whether to keep debug or non-debug versions.
> x86 has both versions for build jobs and mostly debug test jobs.

It is good to have some debug and non-debug jobs, but we probably don't
need both versions of every job.
diff mbox series

Patch

diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h
index f444e89a8638..46a5e562dd83 100644
--- a/xen/arch/arm/include/asm/early_printk.h
+++ b/xen/arch/arm/include/asm/early_printk.h
@@ -20,7 +20,7 @@ 
     (FIXMAP_ADDR(FIX_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
 
 #define TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS \
-    (TEMPORARY_FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
+    (TEMPORARY_FIXMAP_ADDR(FIX_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
 
 #endif /* !CONFIG_EARLY_PRINTK */