[PATCH] kvm: bios: Put AP boot up code to 0x1000

[PATCH] kvm: bios: Put AP boot up code to 0x1000

[Yang, Sheng]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

From ba05b3c821400127074f65ee4d172fabbe3524cd Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang@intel.com>
Date: Thu, 31 Jul 2008 10:39:24 +0800
Subject: [PATCH] kvm: bios: Put AP boot up code to 0x1000

Rather than 0x10000, which can be overrided by userspace program like 
grub.

Signed-off-by: Sheng Yang <sheng.yang@intel.com>
---
 bios/rombios32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 2dc1d25..f7a4234 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -60,7 +60,7 @@ typedef unsigned long long uint64_t;

 #define APIC_ENABLED 0x0100

-#define AP_BOOT_ADDR 0x10000
+#define AP_BOOT_ADDR 0x1000

 #define MPTABLE_MAX_SIZE  0x00002000
 #define SMI_CMD_IO_ADDR   0xb2
--
1.5.6


[-- Attachment #2: 0001-kvm-bios-Put-AP-boot-up-code-to-0x1000.patch --]
[-- Type: text/x-diff, Size: 800 bytes --]

From ba05b3c821400127074f65ee4d172fabbe3524cd Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang@intel.com>
Date: Thu, 31 Jul 2008 10:39:24 +0800
Subject: [PATCH] kvm: bios: Put AP boot up code to 0x1000

Rather than 0x10000, which can be overrided by userspace program like grub.

Signed-off-by: Sheng Yang <sheng.yang@intel.com>
---
 bios/rombios32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 2dc1d25..f7a4234 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -60,7 +60,7 @@ typedef unsigned long long uint64_t;
 
 #define APIC_ENABLED 0x0100
 
-#define AP_BOOT_ADDR 0x10000
+#define AP_BOOT_ADDR 0x1000
 
 #define MPTABLE_MAX_SIZE  0x00002000
 #define SMI_CMD_IO_ADDR   0xb2
-- 
1.5.6

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Yang, Sheng]

On Thursday 31 July 2008 10:43:20 Yang, Sheng wrote:
> From ba05b3c821400127074f65ee4d172fabbe3524cd Mon Sep 17 00:00:00
> 2001 From: Sheng Yang <sheng.yang@intel.com>
> Date: Thu, 31 Jul 2008 10:39:24 +0800
> Subject: [PATCH] kvm: bios: Put AP boot up code to 0x1000
>
> Rather than 0x10000, which can be overrided by userspace program
> like grub.

Not good enough. It's still in userspace available memory region. Try 
to find a better place...

-- 
regards
Yang, Sheng
>
> Signed-off-by: Sheng Yang <sheng.yang@intel.com>
> ---
>  bios/rombios32.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/bios/rombios32.c b/bios/rombios32.c
> index 2dc1d25..f7a4234 100755
> --- a/bios/rombios32.c
> +++ b/bios/rombios32.c
> @@ -60,7 +60,7 @@ typedef unsigned long long uint64_t;
>
>  #define APIC_ENABLED 0x0100
>
> -#define AP_BOOT_ADDR 0x10000
> +#define AP_BOOT_ADDR 0x1000
>
>  #define MPTABLE_MAX_SIZE  0x00002000
>  #define SMI_CMD_IO_ADDR   0xb2
> --
> 1.5.6

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Avi Kivity]

Yang, Sheng wrote:
> On Thursday 31 July 2008 10:43:20 Yang, Sheng wrote:
>   
>> From ba05b3c821400127074f65ee4d172fabbe3524cd Mon Sep 17 00:00:00
>> 2001 From: Sheng Yang <sheng.yang@intel.com>
>> Date: Thu, 31 Jul 2008 10:39:24 +0800
>> Subject: [PATCH] kvm: bios: Put AP boot up code to 0x1000
>>
>> Rather than 0x10000, which can be overrided by userspace program
>> like grub.
>>     

How about 0xex000?  And avoid copying the bios to low memory?

Gleb tried it and it worked.  We'd have to change qemu to mark the 
memory as RAM, not ROM, but that's what real hardware does as well 
(shadowing the ROM into RAM).


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Yang, Sheng]

On Thursday 31 July 2008 14:41:04 Avi Kivity wrote:
> Yang, Sheng wrote:
> > On Thursday 31 July 2008 10:43:20 Yang, Sheng wrote:
> >> From ba05b3c821400127074f65ee4d172fabbe3524cd Mon Sep 17
> >> 00:00:00 2001 From: Sheng Yang <sheng.yang@intel.com>
> >> Date: Thu, 31 Jul 2008 10:39:24 +0800
> >> Subject: [PATCH] kvm: bios: Put AP boot up code to 0x1000
> >>
> >> Rather than 0x10000, which can be overrided by userspace program
> >> like grub.
>
> How about 0xex000?  And avoid copying the bios to low memory?
>
> Gleb tried it and it worked.  We'd have to change qemu to mark the
> memory as RAM, not ROM, but that's what real hardware does as well
> (shadowing the ROM into RAM).

I tried 0xea000, and it's fine. But avoid copying is better. 

But I don't understand the RAM/ROM issue in qemu here...

And do you mean you have patch on hand now? :)

-- 
regards
Yang, Sheng

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Avi Kivity]

Yang, Sheng wrote:
> On Thursday 31 July 2008 14:41:04 Avi Kivity wrote:
>   
>> Yang, Sheng wrote:
>>     
>>> On Thursday 31 July 2008 10:43:20 Yang, Sheng wrote:
>>>       
>>>> From ba05b3c821400127074f65ee4d172fabbe3524cd Mon Sep 17
>>>> 00:00:00 2001 From: Sheng Yang <sheng.yang@intel.com>
>>>> Date: Thu, 31 Jul 2008 10:39:24 +0800
>>>> Subject: [PATCH] kvm: bios: Put AP boot up code to 0x1000
>>>>
>>>> Rather than 0x10000, which can be overrided by userspace program
>>>> like grub.
>>>>         
>> How about 0xex000?  And avoid copying the bios to low memory?
>>
>> Gleb tried it and it worked.  We'd have to change qemu to mark the
>> memory as RAM, not ROM, but that's what real hardware does as well
>> (shadowing the ROM into RAM).
>>     
>
> I tried 0xea000, and it's fine. But avoid copying is better. 
>
> But I don't understand the RAM/ROM issue in qemu here...
>   

IIRC the rombios32.c writes to the memory it is in, so it expects RAM, 
not ROM.

kvm doesn't support ROM, so it would work.  Qemu doesn't, so it would fail.

> And do you mean you have patch on hand now? :)
>
>   

No, but maybe Gleb does.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[H. Peter Anvin]

Avi Kivity wrote:
> 
> IIRC the rombios32.c writes to the memory it is in, so it expects RAM, 
> not ROM.
> 
> kvm doesn't support ROM, so it would work.  Qemu doesn't, so it would fail.
> 

You can specify either RAM or ROM in Qemu, but you have to edit the C code.

If you need RAM in the low 640K, your choices are roughly 0x300..0x340 
or in the EBDA.

	-hpa

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Sebastian Herbszt]

Yang, Sheng wrote:

> Rather than 0x10000, which can be overrided by userspace program like 
> grub.

Could you please explain this problem?
If i don't misunderstand this smp_ap_boot_code_start from rombios32start.S
gets copied to AP_BOOT_CODE and then there is a broadcast INIT and SIPI.
After the code is run it's not used anymore so there should be no need to care
where it's located.

- Sebastian

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Yang, Sheng]

On Friday 01 August 2008 05:15:11 Sebastian Herbszt wrote:
> Yang, Sheng wrote:
> > Rather than 0x10000, which can be overrided by userspace program
> > like grub.
>
> Could you please explain this problem?
> If i don't misunderstand this smp_ap_boot_code_start from
> rombios32start.S gets copied to AP_BOOT_CODE and then there is a
> broadcast INIT and SIPI. After the code is run it's not used
> anymore so there should be no need to care where it's located.
>
> - Sebastian

Hi Sebastian

Please refer to my another patch comment. Basicly, AP is running HLT 
in 0x10038, but it can exit due to some reason, here QEmu want to 
raise it up . But after AP exit from HLT, it can't return to it 
because the code was overrided by grub and no HLT loop there. Then AP 
had gone away...

--
regards
Yang, Sheng

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Sebastian Herbszt]

Yang, Sheng wrote:
> Please refer to my another patch comment. Basicly, AP is running HLT 
> in 0x10038, but it can exit due to some reason, here QEmu want to 
> raise it up . But after AP exit from HLT, it can't return to it 
> because the code was overrided by grub and no HLT loop there. Then AP 
> had gone away...

Thank you for pointing me to "[PATCH] KVM: Fix QEmu interrupted HLT
emulation". You wrote there:
"Though I also sent a patch for BIOS, it's necessary to get correct behavior here."

Would i be correct saying the problem is kvm/qemu only and with that patch
applied the bios fix is not needed?

If you still want to change the bios code i would suggest jumping to the
original smp_ap_boot_code location in rombios32.bin which should not
get overwritten. The following patch is based on the bochs version.

- Sebastian

--- rombios32start.orig 2008-08-02 19:37:34.000000000 +0200
+++ rombios32start.S    2008-08-02 20:19:15.000000000 +0200
@@ -40,9 +40,11 @@

   .code16
 smp_ap_boot_code_start:
+  cli
   xor %ax, %ax
   mov %ax, %ds
   lock incw CPU_COUNT_ADDR
+  ljmp $0xe000, $(1f-_start)
 1:
   hlt
   jmp 1b

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Yang, Sheng]

On Sunday 03 August 2008 03:30:41 Sebastian Herbszt wrote:
> Yang, Sheng wrote:
> > Please refer to my another patch comment. Basicly, AP is running
> > HLT in 0x10038, but it can exit due to some reason, here QEmu
> > want to raise it up . But after AP exit from HLT, it can't return
> > to it because the code was overrided by grub and no HLT loop
> > there. Then AP had gone away...
>
> Thank you for pointing me to "[PATCH] KVM: Fix QEmu interrupted HLT
> emulation". You wrote there:
> "Though I also sent a patch for BIOS, it's necessary to get correct
> behavior here."
>
> Would i be correct saying the problem is kvm/qemu only and with
> that patch applied the bios fix is not needed?

No... I think more important is get bios fixed. Anyway, copy BIOS part 
of running code to a place that may overwritten by userspace program 
is wrong, if you expect AP still running when userspace program is 
running.

In fact, if BIOS code in right place, we won't need fix HLT, for HLT 
have a assumption that it might be interrupt by some event, so 
usually it is wrapped by a loop, as current BIOS did. 

>
> If you still want to change the bios code i would suggest jumping
> to the original smp_ap_boot_code location in rombios32.bin which
> should not get overwritten. The following patch is based on the
> bochs version.
>

Yeah, I just curious why we need to copy BIOS here and there in the 
beginning... 

Thanks for your patch, but is it based on bochs upstream? I found the 
patch don't apply... I also want to know why we need copy AP boot 
code, if anyone know the detail... 

Thanks!

--
regards
Yang, Sheng

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Sebastian Herbszt]

Yang, Sheng wrote:

>> > Please refer to my another patch comment. Basicly, AP is running
>> > HLT in 0x10038, but it can exit due to some reason, here QEmu
>> > want to raise it up . But after AP exit from HLT, it can't return
>> > to it because the code was overrided by grub and no HLT loop
>> > there. Then AP had gone away...
>>
>> Thank you for pointing me to "[PATCH] KVM: Fix QEmu interrupted HLT
>> emulation". You wrote there:
>> "Though I also sent a patch for BIOS, it's necessary to get correct
>> behavior here."
>>
>> Would i be correct saying the problem is kvm/qemu only and with
>> that patch applied the bios fix is not needed?
> 
> No... I think more important is get bios fixed. Anyway, copy BIOS part 
> of running code to a place that may overwritten by userspace program 
> is wrong, if you expect AP still running when userspace program is 
> running.
> 
> In fact, if BIOS code in right place, we won't need fix HLT, for HLT 
> have a assumption that it might be interrupt by some event, so 
> usually it is wrapped by a loop, as current BIOS did. 

The AP enters HALT state with IF=0. What can make it resume at the
following "jmp 1b"? NMI? SMI?

>> If you still want to change the bios code i would suggest jumping
>> to the original smp_ap_boot_code location in rombios32.bin which
>> should not get overwritten. The following patch is based on the
>> bochs version.
>>
> 
> Yeah, I just curious why we need to copy BIOS here and there in the 
> beginning... 
> 
> Thanks for your patch, but is it based on bochs upstream?

Yes it's based on bochs upstream rombios32start.S r1.5.

> I found the 
> patch don't apply...

This one should apply to current kvm version.

> I also want to know why we need copy AP boot 
> code, if anyone know the detail... 

I think i read something about a need for 4kb alignment for the
startup vector in the mp specs. AP_BOOT_ADDR is currently
at 0x10000 (16 x 4kb).

- Sebastian

--- rombios32start.S.orig Wed Aug  6 21:37:22 2008
+++ rombios32start.S Wed Aug  6 21:39:17 2008
@@ -40,6 +40,7 @@ _start:
 
   .code16
 smp_ap_boot_code_start:
+  cli
   xor %ax, %ax
   mov %ax, %ds
   xor %eax, %eax
@@ -60,6 +61,7 @@ smp_ap_boot_code_start:
 12:
 
   incw CPU_COUNT_ADDR
+  ljmp $0xe000, $(1f-_start)
 1:
   hlt
   jmp 1b

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Avi Kivity]

Sebastian Herbszt wrote:
>
> The AP enters HALT state with IF=0. What can make it resume at the
> following "jmp 1b"? NMI? SMI?
>

Yes, but also INIT.

We might ignore IF=0 for hlt (which would be a bug).

> I think i read something about a need for 4kb alignment for the
> startup vector in the mp specs. AP_BOOT_ADDR is currently
> at 0x10000 (16 x 4kb).
>

Yes, the SIPI address is 4K x sipi vector.

> - Sebastian
>
> --- rombios32start.S.orig Wed Aug  6 21:37:22 2008
> +++ rombios32start.S Wed Aug  6 21:39:17 2008
> @@ -40,6 +40,7 @@ _start:
>
>   .code16
> smp_ap_boot_code_start:
> +  cli
>   xor %ax, %ax
>   mov %ax, %ds
>   xor %eax, %eax
> @@ -60,6 +61,7 @@ smp_ap_boot_code_start:
> 12:
>
>   incw CPU_COUNT_ADDR
> +  ljmp $0xe000, $(1f-_start)
> 1:
>   hlt
>   jmp 1b
>

Looks good.  Please post with signoff and changelog.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Sebastian Herbszt]

Avi Kivity wrote:
>>
>> The AP enters HALT state with IF=0. What can make it resume at the
>> following "jmp 1b"? NMI? SMI?
>>
> 
> Yes, but also INIT.

Are you sure the AP will continue after the hlt instruction on INIT?
I think it will start to execute code from the reset vector (0xfffffff0).

> We might ignore IF=0 for hlt (which would be a bug).
> 
>> I think i read something about a need for 4kb alignment for the
>> startup vector in the mp specs. AP_BOOT_ADDR is currently
>> at 0x10000 (16 x 4kb).
>>
> 
> Yes, the SIPI address is 4K x sipi vector.
> 

- Sebastian

Re: [PATCH] kvm: bios: Put AP boot up code to 0x1000

[Avi Kivity]

Sebastian Herbszt wrote:
> Avi Kivity wrote:
>>>
>>> The AP enters HALT state with IF=0. What can make it resume at the
>>> following "jmp 1b"? NMI? SMI?
>>>
>>
>> Yes, but also INIT.
>
> Are you sure the AP will continue after the hlt instruction on INIT?
> I think it will start to execute code from the reset vector (0xfffffff0).
>

I think an AP will actually wait for SIPI after INIT.  I misread the 
question.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[PATCH] kvm: bios: end AP boot code execution in rombios

[Sebastian Herbszt]

Jump to rombios before executing the halt loop.

Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>

diff -uNrp a/bios/rombios32start.S b/bios/rombios32start.S
--- a/bios/rombios32start.S	2008-08-17 23:30:42.000000000 +0200
+++ b/bios/rombios32start.S	2008-08-17 23:38:42.000000000 +0200
@@ -40,6 +40,7 @@ _start:
 
   .code16
 smp_ap_boot_code_start:
+  cli
   xor %ax, %ax
   mov %ax, %ds
   xor %eax, %eax
@@ -60,6 +61,7 @@ smp_ap_boot_code_start:
 12:
 
   incw CPU_COUNT_ADDR
+  ljmp $0xe000, $(1f-_start)
 1:
   hlt
   jmp 1b

Re: [PATCH] kvm: bios: end AP boot code execution in rombios

[Anthony Liguori]

Sebastian Herbszt wrote:
> Jump to rombios before executing the halt loop.
>   

Why?  More importantly, why is this specific to KVM?

Regards,

Anthony Liguori

> Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>
>
> diff -uNrp a/bios/rombios32start.S b/bios/rombios32start.S
> --- a/bios/rombios32start.S	2008-08-17 23:30:42.000000000 +0200
> +++ b/bios/rombios32start.S	2008-08-17 23:38:42.000000000 +0200
> @@ -40,6 +40,7 @@ _start:
>  
>    .code16
>  smp_ap_boot_code_start:
> +  cli
>    xor %ax, %ax
>    mov %ax, %ds
>    xor %eax, %eax
> @@ -60,6 +61,7 @@ smp_ap_boot_code_start:
>  12:
>  
>    incw CPU_COUNT_ADDR
> +  ljmp $0xe000, $(1f-_start)
>  1:
>    hlt
>    jmp 1b
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH] kvm: bios: end AP boot code execution in rombios

[Yang, Sheng]

On Monday 18 August 2008 10:33:11 Anthony Liguori wrote:
> Sebastian Herbszt wrote:
> > Jump to rombios before executing the halt loop.
>
> Why?  More importantly, why is this specific to KVM?

The bios copy AP boot up code to 0x10000 now in KVM, so it can be 
overwrite by userspace program like grub. I found it caused 
stop/cont/info cpus in grub corrupt AP. Please refer to "
KVM: Fix exiting from HLT emulation with MP_STATE_HALTED"
(well, I found the patch duplicated with Marcelo's patch later, then 
drop it...)

Yeah, I am agree this should not be a specific issue to KVM.

-- 
regards
Yang, Sheng
>
> Regards,
>
> Anthony Liguori
>
> > Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>
> >
> > diff -uNrp a/bios/rombios32start.S b/bios/rombios32start.S
> > --- a/bios/rombios32start.S	2008-08-17 23:30:42.000000000 +0200
> > +++ b/bios/rombios32start.S	2008-08-17 23:38:42.000000000 +0200
> > @@ -40,6 +40,7 @@ _start:
> >
> >    .code16
> >  smp_ap_boot_code_start:
> > +  cli
> >    xor %ax, %ax
> >    mov %ax, %ds
> >    xor %eax, %eax
> > @@ -60,6 +61,7 @@ smp_ap_boot_code_start:
> >  12:
> >
> >    incw CPU_COUNT_ADDR
> > +  ljmp $0xe000, $(1f-_start)
> >  1:
> >    hlt
> >    jmp 1b
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at 
> > http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: bios: end AP boot code execution in rombios

[Avi Kivity]

Yang, Sheng wrote:
> On Monday 18 August 2008 10:33:11 Anthony Liguori wrote:
>   
>> Sebastian Herbszt wrote:
>>     
>>> Jump to rombios before executing the halt loop.
>>>       
>> Why?  More importantly, why is this specific to KVM?
>>     
>
> The bios copy AP boot up code to 0x10000 now in KVM, so it can be 
> overwrite by userspace program like grub. I found it caused 
> stop/cont/info cpus in grub corrupt AP. Please refer to "
> KVM: Fix exiting from HLT emulation with MP_STATE_HALTED"
> (well, I found the patch duplicated with Marcelo's patch later, then 
> drop it...)
>
> Yeah, I am agree this should not be a specific issue to KVM.
>   

It is actually kvm specific.  The SIPI code runs with interrupts 
disabled, so the 'hlt' instruction cannot be resumed due to an interrupt 
(and as the apic hasn't been configured yet, no nmis either).  But due 
to a bug in kvm, an exit to userspace (like the one caused by 'info 
cpus' or stop+cont) can cause kvm to resume executing after the hlt 
instruction, which is now arbitrary application code.

Marcelo has a patch that addresses this, unfortunately with a 
regression, so hopefully the kvm bug will be closed soon.  That's not to 
say running out of ROM space is a bad idea, so I'll apply this.

(would be even better not to copy the code at all)

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm: bios: end AP boot code execution in rombios

[Avi Kivity]

Sebastian Herbszt wrote:
> Jump to rombios before executing the halt loop.
>
>   

Applied, thanks.

>  
>    .code16
>  smp_ap_boot_code_start:
> +  cli
>   

Redundant (but no harm done).


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm: bios: end AP boot code execution in rombios

[Sebastian Herbszt]

Avi Kivity wrote:
>>    .code16
>>  smp_ap_boot_code_start:
>> +  cli
>>   
> 
> Redundant (but no harm done).

I added it so it's explicit this code is run with IF=0. Kind of
serves a documentation purpose so we don't have to look up
whether the cpu starts with IF=0 or not.

- Sebastian