Patchwork memory-region: Report if region is read-only on info mtree

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 3, 2012, 12:02 p.m.
Message ID <4F2BCCBD.8050009@siemens.com>
Download mbox | patch
Permalink /patch/139362/
State New
Headers show

Comments

Jan Kiszka - Feb. 3, 2012, 12:02 p.m.
Helpful to understand guest configurations of things like the i440FX's
PAM.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 memory.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Blue Swirl - Feb. 4, 2012, 12:12 p.m.
On Fri, Feb 3, 2012 at 12:02, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Helpful to understand guest configurations of things like the i440FX's
> PAM.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  memory.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index ee4c98a..ea4adda 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1608,23 +1608,25 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>             ml->printed = false;
>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
>         }
> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
>                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
>                    base + mr->addr,
>                    base + mr->addr
>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>                    mr->priority,
> +                   mr->readonly ? "RO" : "RW",

I think the reserved regions which are unreadable and unwritable
should be shown as well. Then the output should be a combination of
'R', 'W' or neither ('-').

>                    mr->name,
>                    mr->alias->name,
>                    mr->alias_offset,
>                    mr->alias_offset
>                    + (target_phys_addr_t)int128_get64(mr->size) - 1);
>     } else {
> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): %s\n",
> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): %s\n",
>                    base + mr->addr,
>                    base + mr->addr
>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>                    mr->priority,
> +                   mr->readonly ? "RO" : "RW",
>                    mr->name);
>     }
>
> --
> 1.7.3.4
Jan Kiszka - Feb. 4, 2012, 12:23 p.m.
On 2012-02-04 13:12, Blue Swirl wrote:
> On Fri, Feb 3, 2012 at 12:02, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Helpful to understand guest configurations of things like the i440FX's
>> PAM.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  memory.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index ee4c98a..ea4adda 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1608,23 +1608,25 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>>             ml->printed = false;
>>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
>>         }
>> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
>> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
>>                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
>>                    base + mr->addr,
>>                    base + mr->addr
>>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>>                    mr->priority,
>> +                   mr->readonly ? "RO" : "RW",
> 
> I think the reserved regions which are unreadable and unwritable
> should be shown as well. Then the output should be a combination of
> 'R', 'W' or neither ('-').

Reserved regions are in the hand of some other device model (so far only
the KVM kernel). That says nothing about their R/W property. If we ever
have a reserved region that is not writable, the owner could still set
the corresponding flag for documentation purposes.

Jan
Blue Swirl - Feb. 4, 2012, 12:32 p.m.
On Sat, Feb 4, 2012 at 12:23, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-02-04 13:12, Blue Swirl wrote:
>> On Fri, Feb 3, 2012 at 12:02, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> Helpful to understand guest configurations of things like the i440FX's
>>> PAM.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  memory.c |    6 ++++--
>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index ee4c98a..ea4adda 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -1608,23 +1608,25 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>>>             ml->printed = false;
>>>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
>>>         }
>>> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
>>> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
>>>                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
>>>                    base + mr->addr,
>>>                    base + mr->addr
>>>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>>>                    mr->priority,
>>> +                   mr->readonly ? "RO" : "RW",
>>
>> I think the reserved regions which are unreadable and unwritable
>> should be shown as well. Then the output should be a combination of
>> 'R', 'W' or neither ('-').
>
> Reserved regions are in the hand of some other device model (so far only
> the KVM kernel). That says nothing about their R/W property. If we ever
> have a reserved region that is not writable, the owner could still set
> the corresponding flag for documentation purposes.

OK. But it's also possible for a region to have readable == false
while readonly == false, which would imply 'WO' or '-W'. That also
supports separate 'R', 'W' and '-' flags.
Jan Kiszka - Feb. 4, 2012, 2:51 p.m.
On 2012-02-04 13:32, Blue Swirl wrote:
> On Sat, Feb 4, 2012 at 12:23, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2012-02-04 13:12, Blue Swirl wrote:
>>> On Fri, Feb 3, 2012 at 12:02, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> Helpful to understand guest configurations of things like the i440FX's
>>>> PAM.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  memory.c |    6 ++++--
>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/memory.c b/memory.c
>>>> index ee4c98a..ea4adda 100644
>>>> --- a/memory.c
>>>> +++ b/memory.c
>>>> @@ -1608,23 +1608,25 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>>>>             ml->printed = false;
>>>>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
>>>>         }
>>>> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
>>>> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
>>>>                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
>>>>                    base + mr->addr,
>>>>                    base + mr->addr
>>>>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>>>>                    mr->priority,
>>>> +                   mr->readonly ? "RO" : "RW",
>>>
>>> I think the reserved regions which are unreadable and unwritable
>>> should be shown as well. Then the output should be a combination of
>>> 'R', 'W' or neither ('-').
>>
>> Reserved regions are in the hand of some other device model (so far only
>> the KVM kernel). That says nothing about their R/W property. If we ever
>> have a reserved region that is not writable, the owner could still set
>> the corresponding flag for documentation purposes.
> 
> OK. But it's also possible for a region to have readable == false
> while readonly == false, which would imply 'WO' or '-W'. That also
> supports separate 'R', 'W' and '-' flags.

Yep, I encoded the ROM device state as well. And this revealed a
regression of the memory region conversion of the cfi02. Gave up
counting how often I fixed this type of bug in the flash code.

Jan
Stefan Hajnoczi - Feb. 9, 2012, 8:35 a.m.
On Sat, Feb 04, 2012 at 03:51:43PM +0100, Jan Kiszka wrote:
> On 2012-02-04 13:32, Blue Swirl wrote:
> > On Sat, Feb 4, 2012 at 12:23, Jan Kiszka <jan.kiszka@web.de> wrote:
> >> On 2012-02-04 13:12, Blue Swirl wrote:
> >>> On Fri, Feb 3, 2012 at 12:02, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>> Helpful to understand guest configurations of things like the i440FX's
> >>>> PAM.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> ---
> >>>>  memory.c |    6 ++++--
> >>>>  1 files changed, 4 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/memory.c b/memory.c
> >>>> index ee4c98a..ea4adda 100644
> >>>> --- a/memory.c
> >>>> +++ b/memory.c
> >>>> @@ -1608,23 +1608,25 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
> >>>>             ml->printed = false;
> >>>>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
> >>>>         }
> >>>> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
> >>>> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
> >>>>                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
> >>>>                    base + mr->addr,
> >>>>                    base + mr->addr
> >>>>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
> >>>>                    mr->priority,
> >>>> +                   mr->readonly ? "RO" : "RW",
> >>>
> >>> I think the reserved regions which are unreadable and unwritable
> >>> should be shown as well. Then the output should be a combination of
> >>> 'R', 'W' or neither ('-').
> >>
> >> Reserved regions are in the hand of some other device model (so far only
> >> the KVM kernel). That says nothing about their R/W property. If we ever
> >> have a reserved region that is not writable, the owner could still set
> >> the corresponding flag for documentation purposes.
> > 
> > OK. But it's also possible for a region to have readable == false
> > while readonly == false, which would imply 'WO' or '-W'. That also
> > supports separate 'R', 'W' and '-' flags.
> 
> Yep, I encoded the ROM device state as well. And this revealed a
> regression of the memory region conversion of the cfi02. Gave up
> counting how often I fixed this type of bug in the flash code.

Is this patch ready to go?  Seems fine to me but do you still want to
add the 'WO' output that Blue Swirl suggested?

Stefan
Jan Kiszka - Feb. 9, 2012, 10:08 a.m.
On 2012-02-09 09:35, Stefan Hajnoczi wrote:
> On Sat, Feb 04, 2012 at 03:51:43PM +0100, Jan Kiszka wrote:
>> On 2012-02-04 13:32, Blue Swirl wrote:
>>> On Sat, Feb 4, 2012 at 12:23, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2012-02-04 13:12, Blue Swirl wrote:
>>>>> On Fri, Feb 3, 2012 at 12:02, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> Helpful to understand guest configurations of things like the i440FX's
>>>>>> PAM.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>  memory.c |    6 ++++--
>>>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/memory.c b/memory.c
>>>>>> index ee4c98a..ea4adda 100644
>>>>>> --- a/memory.c
>>>>>> +++ b/memory.c
>>>>>> @@ -1608,23 +1608,25 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>>>>>>             ml->printed = false;
>>>>>>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
>>>>>>         }
>>>>>> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
>>>>>> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
>>>>>>                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
>>>>>>                    base + mr->addr,
>>>>>>                    base + mr->addr
>>>>>>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>>>>>>                    mr->priority,
>>>>>> +                   mr->readonly ? "RO" : "RW",
>>>>>
>>>>> I think the reserved regions which are unreadable and unwritable
>>>>> should be shown as well. Then the output should be a combination of
>>>>> 'R', 'W' or neither ('-').
>>>>
>>>> Reserved regions are in the hand of some other device model (so far only
>>>> the KVM kernel). That says nothing about their R/W property. If we ever
>>>> have a reserved region that is not writable, the owner could still set
>>>> the corresponding flag for documentation purposes.
>>>
>>> OK. But it's also possible for a region to have readable == false
>>> while readonly == false, which would imply 'WO' or '-W'. That also
>>> supports separate 'R', 'W' and '-' flags.
>>
>> Yep, I encoded the ROM device state as well. And this revealed a
>> regression of the memory region conversion of the cfi02. Gave up
>> counting how often I fixed this type of bug in the flash code.
> 
> Is this patch ready to go?  Seems fine to me but do you still want to
> add the 'WO' output that Blue Swirl suggested?

There is v2 on the list, but I removed trival from CC due to the
discussion. Needs an ack from Blue, I think.

Jan
Stefan Hajnoczi - Feb. 9, 2012, 11:54 a.m.
On Thu, Feb 9, 2012 at 10:08 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-02-09 09:35, Stefan Hajnoczi wrote:
>> On Sat, Feb 04, 2012 at 03:51:43PM +0100, Jan Kiszka wrote:
>>> On 2012-02-04 13:32, Blue Swirl wrote:
>>>> On Sat, Feb 4, 2012 at 12:23, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> On 2012-02-04 13:12, Blue Swirl wrote:
>>>>>> On Fri, Feb 3, 2012 at 12:02, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>> Helpful to understand guest configurations of things like the i440FX's
>>>>>>> PAM.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>>>  memory.c |    6 ++++--
>>>>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/memory.c b/memory.c
>>>>>>> index ee4c98a..ea4adda 100644
>>>>>>> --- a/memory.c
>>>>>>> +++ b/memory.c
>>>>>>> @@ -1608,23 +1608,25 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>>>>>>>             ml->printed = false;
>>>>>>>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
>>>>>>>         }
>>>>>>> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
>>>>>>> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
>>>>>>>                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
>>>>>>>                    base + mr->addr,
>>>>>>>                    base + mr->addr
>>>>>>>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>>>>>>>                    mr->priority,
>>>>>>> +                   mr->readonly ? "RO" : "RW",
>>>>>>
>>>>>> I think the reserved regions which are unreadable and unwritable
>>>>>> should be shown as well. Then the output should be a combination of
>>>>>> 'R', 'W' or neither ('-').
>>>>>
>>>>> Reserved regions are in the hand of some other device model (so far only
>>>>> the KVM kernel). That says nothing about their R/W property. If we ever
>>>>> have a reserved region that is not writable, the owner could still set
>>>>> the corresponding flag for documentation purposes.
>>>>
>>>> OK. But it's also possible for a region to have readable == false
>>>> while readonly == false, which would imply 'WO' or '-W'. That also
>>>> supports separate 'R', 'W' and '-' flags.
>>>
>>> Yep, I encoded the ROM device state as well. And this revealed a
>>> regression of the memory region conversion of the cfi02. Gave up
>>> counting how often I fixed this type of bug in the flash code.
>>
>> Is this patch ready to go?  Seems fine to me but do you still want to
>> add the 'WO' output that Blue Swirl suggested?
>
> There is v2 on the list, but I removed trival from CC due to the
> discussion. Needs an ack from Blue, I think.

Okay, I missed it.  Thanks for the update!

Stefan

Patch

diff --git a/memory.c b/memory.c
index ee4c98a..ea4adda 100644
--- a/memory.c
+++ b/memory.c
@@ -1608,23 +1608,25 @@  static void mtree_print_mr(fprintf_function mon_printf, void *f,
             ml->printed = false;
             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
         }
-        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias %s @%s "
+        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): alias %s @%s "
                    TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
                    base + mr->addr,
                    base + mr->addr
                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
                    mr->priority,
+                   mr->readonly ? "RO" : "RW",
                    mr->name,
                    mr->alias->name,
                    mr->alias_offset,
                    mr->alias_offset
                    + (target_phys_addr_t)int128_get64(mr->size) - 1);
     } else {
-        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): %s\n",
+        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): %s\n",
                    base + mr->addr,
                    base + mr->addr
                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
                    mr->priority,
+                   mr->readonly ? "RO" : "RW",
                    mr->name);
     }