On 2014-03-18 14:00, Luiz Capitulino wrote:
> On Tue, 18 Mar 2014 11:30:42 +0100
> Jan Kiszka <jan.kis...@siemens.com> wrote:
>
>> On 2014-03-18 08:36, Paolo Bonzini wrote:
>>> Il 18/03/2014 08:19, Jan Kiszka ha scritto:
>>>> On 2014-03-18 02:54, Luiz Capitulino wrote:
>>>>> If you start a Linux guest with more than 4GB of memory and try to
>>>>> look at a
>>>>> memory address, you will get an error from gdb:
>>>>>
>>>>> (gdb) p node_data[0]->node_id
>>>>> Cannot access memory at address 0xffff88013fffd3a0
>>>>> (gdb)
>>>>
>>>> I suppose this is x86-64, not 32-bit with PTE, right?
>>>>
>>>>>
>>>>> I debugged this down to x86_cpu_get_phys_page_debug(), it doesn't
>>>>> handle the
>>>>> case where the PDPTE has the PS bit set (although I didn't check
>>>>> where Linux
>>>>> sets that bit). This commit adds the PS bit handling, which fixes the
>>>>> problem
>>>>> for me.
>>>>>
>>>>> Signed-off-by: Luiz capitulino <lcapitul...@redhat.com>
>>>>> ---
>>>>>
>>>>> Two observations:
>>>>>
>>>>> 1. This bug has always existed, so it's not a regression, so I'm not
>>>>> sure
>>>>> it's worth it to fix for 2.0
>>>
>>> Sure, why not?
>>>
>>>>> 2. I'm not familiar with every detail of x86_cpu_get_phys_page_debug(),
>>>>> so I'm not completely sure this is the right thing to do
>>>>>
>>>>> target-i386/helper.c | 8 ++++++++
>>>>> 1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>>> index 4f447b8..9b7803f 100644
>>>>> --- a/target-i386/helper.c
>>>>> +++ b/target-i386/helper.c
>>>>> @@ -951,6 +951,13 @@ hwaddr x86_cpu_get_phys_page_debug(CPUState *cs,
>>>>> vaddr addr)
>>>>> return -1;
>>>>> }
>>>>>
>>>>> + if (pdpe & PG_PSE_MASK) {
>>>>> + page_size = 1024 * 1024 * 1024;
>>>>> + pte = pdpe & ~( (page_size - 1) & ~0xfff);
>>>>> + pte &= ~(PG_NX_MASK | PG_HI_USER_MASK);
>>>>> + goto out;
>>>>> + }
>>>>
>>>> Does this also apply if we are not in long mode?
>>>
>>> No, it doesn't. The only valid bits in a PAE PDPTE are P, PWT and PCD.
>>> Bit 7 (PS) is reserved.
>>
>> Right, this belongs in the "if (env->hflags & HF_LMA_MASK)" block.
>>
>> And the subject or description should mention that
>> x86_cpu_get_phys_page_debug was lacking support for 1G hugepages.
>
> To be honest, although the PS bit is set and that indicates a 1GB page,
> I didn't know Linux does that. I thought Linux would use 4KB pages for
> everything unless it's explicitly asked to use bigger pages. Also, note that
> I was using gdb to debug really early kernel boot code (start_kernel()).
I could imagine that Linux initially creates a giant identity mapping
page table for the startup process and only later on switches to
fine-grained tables of 4K and 2M pages. Giant pages still require
hughtlbfs, IIRC.
>
> I'd feel more confident to have such a changelog after I find out where
> exactly Linux sets that bit, but I won't have time in the next days. On the
> other hand, the patch does fix the problem to me.
Don't worry about Linux (the code should work with any OS anyway), just
believe your reviewers. ;) Alternatively, check Intel IA32 SDM on page
table structures.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
