[Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Mark Langsdorf]

From: Rob Herring <rob.herring@calxeda.com>

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>

Conflicts:

         target-arm/cpu.h
         target-arm/helper.c
---
  target-arm/cpu.h    |    1 +
  target-arm/helper.c |    7 +++++++
  2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 129edbb..b89c085 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -152,6 +152,7 @@ typedef struct CPUARMState {
          uint32_t c15_i_max; /* Maximum D-cache dirty line index.  */
          uint32_t c15_i_min; /* Minimum D-cache dirty line index.  */
          uint32_t c15_threadid; /* TI debugger thread-ID.  */
+        uint32_t c15_scubase; /* SCU base address.  */
      } cp15;

      struct {
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 816c4c4..37110bc 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2197,6 +2197,13 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t 
insn)
               * 0x200 << ($rn & 0xfff), when MMU is off.  */
              goto bad_reg;
          }
+        if (ARM_CPUID(env) == ARM_CPUID_CORTEXA9) {
+            switch (crm) {
+            case 0:
+                return env->cp15.c15_scubase;
+            }
+            goto bad_reg;
+        }
          return 0;
      }
  bad_reg:
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Peter Maydell]

On 20 December 2011 19:10, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
> From: Rob Herring <rob.herring@calxeda.com>
>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
>
> Conflicts:
>
>        target-arm/cpu.h
>        target-arm/helper.c

Commit messages shouldn't have Conflicts notes in them.
You could also use more detail here (for a start, you've
only added one register).

> ---
>  target-arm/cpu.h    |    1 +
>  target-arm/helper.c |    7 +++++++
>  2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 129edbb..b89c085 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -152,6 +152,7 @@ typedef struct CPUARMState {
>         uint32_t c15_i_max; /* Maximum D-cache dirty line index.  */
>         uint32_t c15_i_min; /* Minimum D-cache dirty line index.  */
>         uint32_t c15_threadid; /* TI debugger thread-ID.  */
> +        uint32_t c15_scubase; /* SCU base address.  */
>     } cp15;
>
>     struct {
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 816c4c4..37110bc 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2197,6 +2197,13 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t
> insn)
>              * 0x200 << ($rn & 0xfff), when MMU is off.  */
>             goto bad_reg;
>         }
> +        if (ARM_CPUID(env) == ARM_CPUID_CORTEXA9) {
> +            switch (crm) {
> +            case 0:
> +                return env->cp15.c15_scubase;
> +            }
> +            goto bad_reg;
> +        }

This is underdecoded: the A9 has two registers in c15,c0:
CRn     Op1     CRm     Op2     Name
15      0       c0      0       Power Control Register
15      4       c0      0       Configuration Base Address

I'm guessing you're after the Configuration Base Address register.
(please call the struct name something vaguely relating to the
official register name, incidentally.)

>         return 0;
>     }
>  bad_reg:

This commit leaves the register with a reset value of 0, which
isn't right (we only implement A9MP, not A9UP, so the reset value
should be settable by the board at init time somehow depending
where the a9mpcore_priv device is mapped. Not sure what the
cleanest way to do that is.)

-- PMM

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Mark Langsdorf]

On 12/20/2011 01:48 PM, Peter Maydell wrote:
> On 20 December 2011 19:10, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 816c4c4..37110bc 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -2197,6 +2197,13 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t
>> insn)
>>              * 0x200 << ($rn & 0xfff), when MMU is off.  */
>>             goto bad_reg;
>>         }
>> +        if (ARM_CPUID(env) == ARM_CPUID_CORTEXA9) {
>> +            switch (crm) {
>> +            case 0:
>> +                return env->cp15.c15_scubase;
>> +            }
>> +            goto bad_reg;
>> +        }
> 
> This is underdecoded: the A9 has two registers in c15,c0:
> CRn     Op1     CRm     Op2     Name
> 15      0       c0      0       Power Control Register
> 15      4       c0      0       Configuration Base Address
> 
> I'm guessing you're after the Configuration Base Address register.
> (please call the struct name something vaguely relating to the
> official register name, incidentally.)

Apparently it's called scubase in the Linux code, but I'll
fix the name for qemu.

>>         return 0;
>>     }
>>  bad_reg:
> 
> This commit leaves the register with a reset value of 0, which
> isn't right (we only implement A9MP, not A9UP, so the reset value
> should be settable by the board at init time somehow depending
> where the a9mpcore_priv device is mapped. Not sure what the
> cleanest way to do that is.)

I can add a DEFINE_PROP_UINT32 to a9mpcore_priv, which would give
anyone who wants to set the property an easy way to do so. I'm
not sure how to hook the a9mpcore_priv to the CPUARMState, though.
They don't appear to reference each other.

--Mark Langsdorf
Calxeda, Inc.

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Peter Maydell]

On 21 December 2011 16:37, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
> On 12/20/2011 01:48 PM, Peter Maydell wrote:
>> This commit leaves the register with a reset value of 0, which
>> isn't right (we only implement A9MP, not A9UP, so the reset value
>> should be settable by the board at init time somehow depending
>> where the a9mpcore_priv device is mapped. Not sure what the
>> cleanest way to do that is.)
>
> I can add a DEFINE_PROP_UINT32 to a9mpcore_priv, which would give
> anyone who wants to set the property an easy way to do so. I'm
> not sure how to hook the a9mpcore_priv to the CPUARMState, though.
> They don't appear to reference each other.

Yes, we don't really model the private peripherals very sensibly:
they should be an integral part of the CPU but for historical
reasons they've been done as a totally separate device. I don't
think a property is the right thing, though -- ideally the a9mpcore
device should be able to find out for itself where it was mapped.

Avi: is there a way for a device (sysbus device in this case) to
find out for one of its memory regions where it has been mapped
in the address space? The context here is that the Cortex-A9
has a cp15 register whose value is "base address of the private
peripherals", and it would be nice not to have to have boards
saying both "map mmio region at X" and "set property so register
reads as X"... [You could argue that hardware implementations
have to do the equivalent of both of these things separately,
I suppose, but it's still not very pretty.]

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Avi Kivity]

On 12/21/2011 11:10 PM, Peter Maydell wrote:
> On 21 December 2011 16:37, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
> > On 12/20/2011 01:48 PM, Peter Maydell wrote:
> >> This commit leaves the register with a reset value of 0, which
> >> isn't right (we only implement A9MP, not A9UP, so the reset value
> >> should be settable by the board at init time somehow depending
> >> where the a9mpcore_priv device is mapped. Not sure what the
> >> cleanest way to do that is.)
> >
> > I can add a DEFINE_PROP_UINT32 to a9mpcore_priv, which would give
> > anyone who wants to set the property an easy way to do so. I'm
> > not sure how to hook the a9mpcore_priv to the CPUARMState, though.
> > They don't appear to reference each other.
>
> Yes, we don't really model the private peripherals very sensibly:
> they should be an integral part of the CPU but for historical
> reasons they've been done as a totally separate device. I don't
> think a property is the right thing, though -- ideally the a9mpcore
> device should be able to find out for itself where it was mapped.
>
> Avi: is there a way for a device (sysbus device in this case) to
> find out for one of its memory regions where it has been mapped
> in the address space? 

Where and if.

> The context here is that the Cortex-A9
> has a cp15 register whose value is "base address of the private
> peripherals", and it would be nice not to have to have boards
> saying both "map mmio region at X" and "set property so register
> reads as X"... [You could argue that hardware implementations
> have to do the equivalent of both of these things separately,
> I suppose, but it's still not very pretty.]

I don't really follow, can you explain?

Anyway, with the new MemoryListener API (not yet merged), you can
register a callback to be called when MemoryRegions become visible or
invisible.  You can filter there for your pet region and get all the
info about it.

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

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Peter Maydell]

On 22 December 2011 10:14, Avi Kivity <avi@redhat.com> wrote:
> On 12/21/2011 11:10 PM, Peter Maydell wrote:
>> Avi: is there a way for a device (sysbus device in this case) to
>> find out for one of its memory regions where it has been mapped
>> in the address space?
>
> Where and if.
>
>> The context here is that the Cortex-A9
>> has a cp15 register whose value is "base address of the private
>> peripherals", and it would be nice not to have to have boards
>> saying both "map mmio region at X" and "set property so register
>> reads as X"... [You could argue that hardware implementations
>> have to do the equivalent of both of these things separately,
>> I suppose, but it's still not very pretty.]
>
> I don't really follow, can you explain?

So in real hardware, these devices (interrupt controller,
timers, etc) are an integral part of the CPU. They appear in
the memory map at an address which is configured by hardwiring
the CPU's PERIPHBASE signals to specify that address. Since
obviously software needs to know where the devices are, there
is a coprocessor register which simply returns the value
of PERIPHBASE. (So I was wrong that hardware does the mapping
and the register separately -- sorry.)

Part of the problem we have is that in QEMU we don't model
the CPU as a single qdev device which includes both the
core and its builtin devices -- the two are completely
separate and the board model ends up having to do a lot of
the work of wiring things up, and in cases like this where
one bit of config affects both the core CPU and the builtin
devices you end up having to specify it twice.

> Anyway, with the new MemoryListener API (not yet merged), you can
> register a callback to be called when MemoryRegions become visible or
> invisible.  You can filter there for your pet region and get all the
> info about it.

Mmm, that would work.

-- PMM

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Avi Kivity]

On 12/22/2011 02:37 PM, Peter Maydell wrote:
> On 22 December 2011 10:14, Avi Kivity <avi@redhat.com> wrote:
> > On 12/21/2011 11:10 PM, Peter Maydell wrote:
> >> Avi: is there a way for a device (sysbus device in this case) to
> >> find out for one of its memory regions where it has been mapped
> >> in the address space?
> >
> > Where and if.
> >
> >> The context here is that the Cortex-A9
> >> has a cp15 register whose value is "base address of the private
> >> peripherals", and it would be nice not to have to have boards
> >> saying both "map mmio region at X" and "set property so register
> >> reads as X"... [You could argue that hardware implementations
> >> have to do the equivalent of both of these things separately,
> >> I suppose, but it's still not very pretty.]
> >
> > I don't really follow, can you explain?
>
> So in real hardware, these devices (interrupt controller,
> timers, etc) are an integral part of the CPU. They appear in
> the memory map at an address which is configured by hardwiring
> the CPU's PERIPHBASE signals to specify that address. Since
> obviously software needs to know where the devices are, there
> is a coprocessor register which simply returns the value
> of PERIPHBASE. (So I was wrong that hardware does the mapping
> and the register separately -- sorry.)
>
> Part of the problem we have is that in QEMU we don't model
> the CPU as a single qdev device which includes both the
> core and its builtin devices -- the two are completely
> separate and the board model ends up having to do a lot of
> the work of wiring things up, and in cases like this where
> one bit of config affects both the core CPU and the builtin
> devices you end up having to specify it twice.

Sounds like a qom Link<Blah> or some other magic should handle it? 
#include "cc/aliguori".

> > Anyway, with the new MemoryListener API (not yet merged), you can
> > register a callback to be called when MemoryRegions become visible or
> > invisible.  You can filter there for your pet region and get all the
> > info about it.
>
> Mmm, that would work.

Now that I better understand the issue, I think it's overkill.  You just
want to replicate a register between devices.  The MemoryListener API is
designed to allow you to see the memory map from the cpu's side, after a
region has been transformed by overlapping regions, address
transformation by bridges, enables/disables, etc.  You just want to copy
a value from one object to another, you don't care about any of this.

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

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Anthony Liguori]

On 12/22/2011 06:43 AM, Avi Kivity wrote:
> On 12/22/2011 02:37 PM, Peter Maydell wrote:
>> On 22 December 2011 10:14, Avi Kivity<avi@redhat.com>  wrote:
>>> On 12/21/2011 11:10 PM, Peter Maydell wrote:
>>>> Avi: is there a way for a device (sysbus device in this case) to
>>>> find out for one of its memory regions where it has been mapped
>>>> in the address space?
>>>
>>> Where and if.
>>>
>>>> The context here is that the Cortex-A9
>>>> has a cp15 register whose value is "base address of the private
>>>> peripherals", and it would be nice not to have to have boards
>>>> saying both "map mmio region at X" and "set property so register
>>>> reads as X"... [You could argue that hardware implementations
>>>> have to do the equivalent of both of these things separately,
>>>> I suppose, but it's still not very pretty.]
>>>
>>> I don't really follow, can you explain?
>>
>> So in real hardware, these devices (interrupt controller,
>> timers, etc) are an integral part of the CPU. They appear in
>> the memory map at an address which is configured by hardwiring
>> the CPU's PERIPHBASE signals to specify that address. Since
>> obviously software needs to know where the devices are, there
>> is a coprocessor register which simply returns the value
>> of PERIPHBASE. (So I was wrong that hardware does the mapping
>> and the register separately -- sorry.)
>>
>> Part of the problem we have is that in QEMU we don't model
>> the CPU as a single qdev device which includes both the
>> core and its builtin devices -- the two are completely
>> separate and the board model ends up having to do a lot of
>> the work of wiring things up, and in cases like this where
>> one bit of config affects both the core CPU and the builtin
>> devices you end up having to specify it twice.
>
> Sounds like a qom Link<Blah>  or some other magic should handle it?
> #include "cc/aliguori".

First, we need to actually model the CPUs.  The general trouble with that today 
is that everything needs to live on a bus after we get QOM merged, that 
requirement will be relaxed.

Once the CPUs are objects, they can create children devices (builtin interrupt 
controller for instance) in their instance_init function.  It would be a child<> 
property in this case.

This is how I want to model the local apic, for instance.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/9] arm: add missing v7 cp15 registers

[Peter Maydell]

On 22 December 2011 14:25, Anthony Liguori <anthony@codemonkey.ws> wrote:
> First, we need to actually model the CPUs.  The general trouble with that
> today is that everything needs to live on a bus after we get QOM merged,
> that requirement will be relaxed.
>
> Once the CPUs are objects, they can create children devices (builtin
> interrupt controller for instance) in their instance_init function.  It
> would be a child<> property in this case.

So, in summary to Mark, if "always reads as zero" is sufficient
for your purposes, I'll accept this as a patch which implements
that (if you fix the other issues and add a comment that it
ought to be the base address of the private peripheral region).
We can implement it properly when we have enough object model
infrastructure to do so.

-- PMM