1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
|
architecture: 0.940
device: 0.940
arm: 0.937
permissions: 0.936
assembly: 0.936
virtual: 0.935
user-level: 0.932
register: 0.930
semantic: 0.928
PID: 0.923
graphic: 0.916
files: 0.912
ppc: 0.906
TCG: 0.904
mistranslation: 0.888
network: 0.887
hypervisor: 0.885
peripherals: 0.884
performance: 0.881
vnc: 0.871
KVM: 0.870
risc-v: 0.868
boot: 0.852
debug: 0.849
VMM: 0.847
kernel: 0.837
socket: 0.809
x86: 0.793
i386: 0.682
dead code in pl080 functions
pl080 is arm dma controller virtual device.
source code path: hw/dma/pl080.c
I find there are two same dead code in pl080_read and pl080_write,
Here's the code, comments are my opinion:
=========================
240 static uint64_t pl080_read(void *opaque, hwaddr offset,
241 unsigned size)
242 {
243 PL080State *s = (PL080State *)opaque;
244 uint32_t i;
245 uint32_t mask;
246
247 if (offset >= 0xfe0 && offset < 0x1000) {
248 if (s->nchannels == 8) {
249 return pl080_id[(offset - 0xfe0) >> 2];
250 } else {
251 return pl081_id[(offset - 0xfe0) >> 2];
252 }
253 }
254 if (offset >= 0x100 && offset < 0x200) { //// here offset is limited in 0x100~0x200
255 i = (offset & 0xe0) >> 5;
256 if (i >= s->nchannels)
257 goto bad_offset;
258 switch (offset >> 2) { //// then here, offset>>2 is in range 64~128
259 case 0: /* SrcAddr */ //// while the switch case is 0,1,2,3,4,
260 return s->chan[i].src; //// so, NONE of the switch case would be selected !
261 case 1: /* DestAddr */ //// this switch is A DEAD CODE, it is contradictory with if.
262 return s->chan[i].dest;
263 case 2: /* LLI */
264 return s->chan[i].lli;
265 case 3: /* Control */
266 return s->chan[i].ctrl;
267 case 4: /* Configuration */
268 return s->chan[i].conf;
269 default:
270 goto bad_offset;
271 }
272 }
.....................................
=============================================
I guess, switch statement should like this :
switch((offset-0x100)>>2)
{
...
}
On 31 October 2016 at 10:48, jeyyej <email address hidden> wrote:
> Public bug reported:
>
> pl080 is arm dma controller virtual device.
> source code path: hw/dma/pl080.c
> I find there are two same dead code in pl080_read and pl080_write,
> + if this DEADCODE is not the author's original purpose,
> + Then there must be something in logic goes wrong, pl080 have NEVER works correctly ?
Yes, this code is incorrect. However the only registers
affected are those that set up the DMA channels, and
the code for actually doing DMA transfers will assert if
the guest ever tries to use it (see the hw_error() call in
pl080_run()), so even if the guest could set up the DMA
channels it would run into problems later.
This device is only used in the versatilepb board, and
I don't think Linux attempts to use it for DMA, so nobody's
ever needed to care that it's actually totally broken
for anything beyond "shouldn't crash the kernel when it
tries to probe for and initialize it".
thanks
-- PMM
@Peter Maydell Oh, I see, but would you try to fix this code ?
I have no requirement for a working pl080, since I have no guest code that requires one. If somebody else (you?) wants to send a patch that fixes it, I'm happy to review it.
Fixed here:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=156448ab640baaeca18
|