[Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Peter Maydell]

The ethernet device in the musicpal only has two tx queues,
but we modelled it with four CTDP registers, presumably a
cut and paste from the rx queue registers. Since the tx_queue[]
array is only 2 entries long this allowed a guest to overrun
this buffer. Remove the nonexistent registers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
There's no readily available documentation for this SoC,
but I'm told the BSP for it indicates that there are
indeed only two tx queues.

 hw/arm/musicpal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 023e875..a8d0086 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -92,8 +92,6 @@
 #define MP_ETH_CRDP3            0x4AC
 #define MP_ETH_CTDP0            0x4E0
 #define MP_ETH_CTDP1            0x4E4
-#define MP_ETH_CTDP2            0x4E8
-#define MP_ETH_CTDP3            0x4EC
 
 /* MII PHY access */
 #define MP_ETH_SMIR_DATA        0x0000FFFF
@@ -308,7 +306,7 @@ static uint64_t mv88w8618_eth_read(void *opaque, hwaddr offset,
     case MP_ETH_CRDP0 ... MP_ETH_CRDP3:
         return s->rx_queue[(offset - MP_ETH_CRDP0)/4];
 
-    case MP_ETH_CTDP0 ... MP_ETH_CTDP3:
+    case MP_ETH_CTDP0 ... MP_ETH_CTDP1:
         return s->tx_queue[(offset - MP_ETH_CTDP0)/4];
 
     default:
@@ -362,7 +360,7 @@ static void mv88w8618_eth_write(void *opaque, hwaddr offset,
             s->cur_rx[(offset - MP_ETH_CRDP0)/4] = value;
         break;
 
-    case MP_ETH_CTDP0 ... MP_ETH_CTDP3:
+    case MP_ETH_CTDP0 ... MP_ETH_CTDP1:
         s->tx_queue[(offset - MP_ETH_CTDP0)/4] = value;
         break;
     }
-- 
1.8.5

Re: [Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Jan Kiszka]

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

On 2014-02-18 16:28, Peter Maydell wrote:
> The ethernet device in the musicpal only has two tx queues,
> but we modelled it with four CTDP registers, presumably a
> cut and paste from the rx queue registers. Since the tx_queue[]
> array is only 2 entries long this allowed a guest to overrun
> this buffer. Remove the nonexistent registers.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: Jan Kiszka <jan.kiszka@web.de>

> ---
> There's no readily available documentation for this SoC,
> but I'm told the BSP for it indicates that there are
> indeed only two tx queues.
> 
>  hw/arm/musicpal.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 023e875..a8d0086 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -92,8 +92,6 @@
>  #define MP_ETH_CRDP3            0x4AC
>  #define MP_ETH_CTDP0            0x4E0
>  #define MP_ETH_CTDP1            0x4E4
> -#define MP_ETH_CTDP2            0x4E8
> -#define MP_ETH_CTDP3            0x4EC
>  
>  /* MII PHY access */
>  #define MP_ETH_SMIR_DATA        0x0000FFFF
> @@ -308,7 +306,7 @@ static uint64_t mv88w8618_eth_read(void *opaque, hwaddr offset,
>      case MP_ETH_CRDP0 ... MP_ETH_CRDP3:
>          return s->rx_queue[(offset - MP_ETH_CRDP0)/4];
>  
> -    case MP_ETH_CTDP0 ... MP_ETH_CTDP3:
> +    case MP_ETH_CTDP0 ... MP_ETH_CTDP1:
>          return s->tx_queue[(offset - MP_ETH_CTDP0)/4];
>  
>      default:
> @@ -362,7 +360,7 @@ static void mv88w8618_eth_write(void *opaque, hwaddr offset,
>              s->cur_rx[(offset - MP_ETH_CRDP0)/4] = value;
>          break;
>  
> -    case MP_ETH_CTDP0 ... MP_ETH_CTDP3:
> +    case MP_ETH_CTDP0 ... MP_ETH_CTDP1:
>          s->tx_queue[(offset - MP_ETH_CTDP0)/4] = value;
>          break;
>      }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Peter Maydell]

On 21 February 2014 07:15, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2014-02-18 16:28, Peter Maydell wrote:
>> The ethernet device in the musicpal only has two tx queues,
>> but we modelled it with four CTDP registers, presumably a
>> cut and paste from the rx queue registers. Since the tx_queue[]
>> array is only 2 entries long this allowed a guest to overrun
>> this buffer. Remove the nonexistent registers.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Acked-by: Jan Kiszka <jan.kiszka@web.de>

Thanks, applied to target-arm.next (with added cc:stable line).

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Andreas Färber]

Am 24.02.2014 16:45, schrieb Peter Maydell:
> On 21 February 2014 07:15, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2014-02-18 16:28, Peter Maydell wrote:
>>> The ethernet device in the musicpal only has two tx queues,
>>> but we modelled it with four CTDP registers, presumably a
>>> cut and paste from the rx queue registers. Since the tx_queue[]
>>> array is only 2 entries long this allowed a guest to overrun
>>> this buffer. Remove the nonexistent registers.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Acked-by: Jan Kiszka <jan.kiszka@web.de>
> 
> Thanks, applied to target-arm.next (with added cc:stable line).

Jan/Peter, is there any other outstanding work on musicpal that I should
be aware of? If not, I'd like to base the long-planned file splitup on
Peter's next pull. Hopefully we can still get that done for 2.0.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Peter Maydell]

On 24 February 2014 15:55, Andreas Färber <afaerber@suse.de> wrote:
> Jan/Peter, is there any other outstanding work on musicpal that I should
> be aware of? If not, I'd like to base the long-planned file splitup on
> Peter's next pull. Hopefully we can still get that done for 2.0.

I have a trivial patch on list for it:
http://patchwork.ozlabs.org/patch/322857/

which I'm probably not going to put into my next pull unless
somebody reviews those patches since they've not been on
list very long. Other than that I'm not aware of anything.
(I can trivially rebase that on top of a file split, obviously.)

I assume you just mean splitting the devices currently in
hw/arm/musicpal.c out into their own files? That would be
good to do, yes.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Andreas Färber]

Am 24.02.2014 17:02, schrieb Peter Maydell:
> On 24 February 2014 15:55, Andreas Färber <afaerber@suse.de> wrote:
>> Jan/Peter, is there any other outstanding work on musicpal that I should
>> be aware of? If not, I'd like to base the long-planned file splitup on
>> Peter's next pull. Hopefully we can still get that done for 2.0.
> 
> I have a trivial patch on list for it:
> http://patchwork.ozlabs.org/patch/322857/
> 
> which I'm probably not going to put into my next pull unless
> somebody reviews those patches since they've not been on
> list very long. Other than that I'm not aware of anything.

Maybe Jan as author can review it? :)

> (I can trivially rebase that on top of a file split, obviously.)
> 
> I assume you just mean splitting the devices currently in
> hw/arm/musicpal.c out into their own files?

Yes, in particular I had noticed that the NIC (which your linked patch
indeed seems to be touching) was missing in hw/net/, we had previously
talked about doing this. Now that we have qom-test for musicpal and the
Soft Freeze coming up might be a good time to finally do this.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg