diff mbox

[01/40] memory: assert that PhysPageEntry's ptr does not overflow

Message ID 1367936238-12196-2-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini May 7, 2013, 2:16 p.m. UTC 
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Comments

Peter Maydell May 7, 2013, 3:44 p.m. UTC | #1 
On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 19725db..2e5b89a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -719,6 +719,8 @@ static void destroy_all_mappings(AddressSpaceDispatch *d)
>
>  static uint16_t phys_section_add(MemoryRegionSection *section)
>  {
> +    assert(phys_sections_nb < TARGET_PAGE_SIZE);
> +
>      if (phys_sections_nb == phys_sections_nb_alloc) {
>          phys_sections_nb_alloc = MAX(phys_sections_nb_alloc * 2, 16);
>          phys_sections = g_renew(MemoryRegionSection, phys_sections,

Why is the limit we're asserting not the same as the maximum
size that we pass to g_renew() below?

-- PMM
Paolo Bonzini May 7, 2013, 4:08 p.m. UTC | #2 
----- Messaggio originale -----
> Da: "Peter Maydell" <peter.maydell@linaro.org>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, aik@ozlabs.ru, "jan kiszka" <jan.kiszka@siemens.com>, qemulist@gmail.com,
> stefanha@redhat.com, david@gibson.dropbear.id.au
> Inviato: Martedì, 7 maggio 2013 17:44:59
> Oggetto: Re: [Qemu-devel] [PATCH 01/40] memory: assert that PhysPageEntry's ptr does not overflow
> 
> On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  exec.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 19725db..2e5b89a 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -719,6 +719,8 @@ static void destroy_all_mappings(AddressSpaceDispatch
> > *d)
> >
> >  static uint16_t phys_section_add(MemoryRegionSection *section)
> >  {
> > +    assert(phys_sections_nb < TARGET_PAGE_SIZE);
> > +
> >      if (phys_sections_nb == phys_sections_nb_alloc) {
> >          phys_sections_nb_alloc = MAX(phys_sections_nb_alloc * 2, 16);
> >          phys_sections = g_renew(MemoryRegionSection, phys_sections,
> 
> Why is the limit we're asserting not the same as the maximum
> size that we pass to g_renew() below?

That's a minimum size, isn't it?

I'm asserting that the physical section number doesn't overflow into
the page, since the TLB entries are stored as a combination of the two.

Paolo
Peter Maydell May 7, 2013, 4:17 p.m. UTC | #3 
On 7 May 2013 17:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Da: "Peter Maydell" <peter.maydell@linaro.org>
>> Why is the limit we're asserting not the same as the maximum
>> size that we pass to g_renew() below?
>
> That's a minimum size, isn't it?

Doh, so it is.

> I'm asserting that the physical section number doesn't overflow into
> the page, since the TLB entries are stored as a combination of the two.

Maybe this could use a comment about where (in what data
structure) the section number lives, because it's not
entirely obvious (well, to me anyway).

-- PMM
pingfan liu May 9, 2013, 3:41 a.m. UTC | #4 
On Wed, May 8, 2013 at 12:08 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> ----- Messaggio originale -----
>> Da: "Peter Maydell" <peter.maydell@linaro.org>
>> A: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: qemu-devel@nongnu.org, aik@ozlabs.ru, "jan kiszka" <jan.kiszka@siemens.com>, qemulist@gmail.com,
>> stefanha@redhat.com, david@gibson.dropbear.id.au
>> Inviato: Martedì, 7 maggio 2013 17:44:59
>> Oggetto: Re: [Qemu-devel] [PATCH 01/40] memory: assert that PhysPageEntry's ptr does not overflow
>>
>> On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> > ---
>> >  exec.c |    2 ++
>> >  1 files changed, 2 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/exec.c b/exec.c
>> > index 19725db..2e5b89a 100644
>> > --- a/exec.c
>> > +++ b/exec.c
>> > @@ -719,6 +719,8 @@ static void destroy_all_mappings(AddressSpaceDispatch
>> > *d)
>> >
>> >  static uint16_t phys_section_add(MemoryRegionSection *section)
>> >  {
>> > +    assert(phys_sections_nb < TARGET_PAGE_SIZE);
>> > +
>> >      if (phys_sections_nb == phys_sections_nb_alloc) {
>> >          phys_sections_nb_alloc = MAX(phys_sections_nb_alloc * 2, 16);
>> >          phys_sections = g_renew(MemoryRegionSection, phys_sections,
>>
>> Why is the limit we're asserting not the same as the maximum
>> size that we pass to g_renew() below?
>
> That's a minimum size, isn't it?
>
> I'm asserting that the physical section number doesn't overflow into
> the page, since the TLB entries are stored as a combination of the two.
>
Could you explain more detail? Why < TARGET_PAGE_SIZE, not 2^15?

Thanks,
Pingfan

> Paolo
Paolo Bonzini May 9, 2013, 4:46 p.m. UTC | #5 
Il 09/05/2013 05:41, liu ping fan ha scritto:
> On Wed, May 8, 2013 at 12:08 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> ----- Messaggio originale -----
>>> Da: "Peter Maydell" <peter.maydell@linaro.org>
>>> A: "Paolo Bonzini" <pbonzini@redhat.com>
>>> Cc: qemu-devel@nongnu.org, aik@ozlabs.ru, "jan kiszka" <jan.kiszka@siemens.com>, qemulist@gmail.com,
>>> stefanha@redhat.com, david@gibson.dropbear.id.au
>>> Inviato: Martedì, 7 maggio 2013 17:44:59
>>> Oggetto: Re: [Qemu-devel] [PATCH 01/40] memory: assert that PhysPageEntry's ptr does not overflow
>>>
>>> On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>  exec.c |    2 ++
>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/exec.c b/exec.c
>>>> index 19725db..2e5b89a 100644
>>>> --- a/exec.c
>>>> +++ b/exec.c
>>>> @@ -719,6 +719,8 @@ static void destroy_all_mappings(AddressSpaceDispatch
>>>> *d)
>>>>
>>>>  static uint16_t phys_section_add(MemoryRegionSection *section)
>>>>  {
>>>> +    assert(phys_sections_nb < TARGET_PAGE_SIZE);
>>>> +
>>>>      if (phys_sections_nb == phys_sections_nb_alloc) {
>>>>          phys_sections_nb_alloc = MAX(phys_sections_nb_alloc * 2, 16);
>>>>          phys_sections = g_renew(MemoryRegionSection, phys_sections,
>>>
>>> Why is the limit we're asserting not the same as the maximum
>>> size that we pass to g_renew() below?
>>
>> That's a minimum size, isn't it?
>>
>> I'm asserting that the physical section number doesn't overflow into
>> the page, since the TLB entries are stored as a combination of the two.
>>
> Could you explain more detail? Why < TARGET_PAGE_SIZE, not 2^15?

Because the TLB entry is the "or" of the page address and the
phys_section.  Look here:

hwaddr memory_region_section_get_iotlb(CPUArchState *env,
                                                   MemoryRegionSection *section,
                                                   target_ulong vaddr,
                                                   hwaddr paddr,
                                                   int prot,
                                                   target_ulong *address)
{
    hwaddr iotlb;
    CPUWatchpoint *wp;

    if (memory_region_is_ram(section->mr)) {
        /* Normal RAM.  */
        iotlb = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
            + memory_region_section_addr(section, paddr);
        if (!section->readonly) {
            iotlb |= phys_section_notdirty;
        } else {
            iotlb |= phys_section_rom;
        }
    } else {
        iotlb = section - phys_sections;
        iotlb += memory_region_section_addr(section, paddr);
    }

where the else could be written better as:

        iotlb = memory_region_section_addr(section, paddr);
        iotlb |= section - phys_sections;

memory_region_section_addr will return a page-aligned value.

Paolo
diff mbox

Patch

diff --git a/exec.c b/exec.c
index 19725db..2e5b89a 100644
--- a/exec.c
+++ b/exec.c
@@ -719,6 +719,8 @@  static void destroy_all_mappings(AddressSpaceDispatch *d)
 
 static uint16_t phys_section_add(MemoryRegionSection *section)
 {
+    assert(phys_sections_nb < TARGET_PAGE_SIZE);
+
     if (phys_sections_nb == phys_sections_nb_alloc) {
         phys_sections_nb_alloc = MAX(phys_sections_nb_alloc * 2, 16);
         phys_sections = g_renew(MemoryRegionSection, phys_sections,