| Message ID | 20170103173822.4120-1-fbarrat@linux.vnet.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
Do we also need the same fix to hw/phb3.c and hw/npu.c? I haven't tested this but it looks reasonable. If I'm not mistaken the old behaviour would have allowed a PCIe device to access addresses below it's allocated PCIe bus address so this just increases protection by further restricting device access. Reviewed-By: Alistair Popple <alistair@popple.id.au> On Tue, 3 Jan 2017 06:38:22 PM Frederic Barrat wrote: > From the phb4 spec, when encoding the TVE, the pci start address bits > 49:24 are encoded in bits TVE[52:53]||[0:23]. > The mask to select bits 47:24 is incorrectly set. It should be > 0xffffff000000, shifted left by 16, i.e. 0xffffff << 40 > > Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> > --- > Already discussed with Benh > > hw/phb4.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/phb4.c b/hw/phb4.c > index b2723ca..c2c7cc7 100644 > --- a/hw/phb4.c > +++ b/hw/phb4.c > @@ -1345,7 +1345,7 @@ static int64_t phb4_map_pe_dma_window_real(struct phb *phb, > * and end address bits 49:24 into TVE[54:55]||[24:47] > * and set TVE[51] > */ > - tve = (pci_start_addr << 16) & (0xffffffull << 48); > + tve = (pci_start_addr << 16) & (0xffffffull << 40); > tve |= (pci_start_addr >> 38) & (3ull << 10); > tve |= (end >> 8) & (0xfffffful << 16); > tve |= (end >> 40) & (3ull << 8); >
On Fri, 2017-02-03 at 14:34 +1100, Alistair Popple wrote: > Do we also need the same fix to hw/phb3.c and hw/npu.c? > > I haven't tested this but it looks reasonable. If I'm not mistaken > the > old behaviour would have allowed a PCIe device to access addresses > below it's allocated PCIe bus address so this just increases > protection by further restricting device access. I *think* we always put 0 in there anyway, don't we ? As for PHB3 and NPU, check the spec to see where the bits are. > Reviewed-By: Alistair Popple <alistair@popple.id.au> > > On Tue, 3 Jan 2017 06:38:22 PM Frederic Barrat wrote: > > From the phb4 spec, when encoding the TVE, the pci start address > > bits > > 49:24 are encoded in bits TVE[52:53]||[0:23]. > > The mask to select bits 47:24 is incorrectly set. It should be > > 0xffffff000000, shifted left by 16, i.e. 0xffffff << 40 > > > > Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> > > --- > > Already discussed with Benh > > > > hw/phb4.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/phb4.c b/hw/phb4.c > > index b2723ca..c2c7cc7 100644 > > --- a/hw/phb4.c > > +++ b/hw/phb4.c > > @@ -1345,7 +1345,7 @@ static int64_t > > phb4_map_pe_dma_window_real(struct phb *phb, > > * and end address bits 49:24 into > > TVE[54:55]||[24:47] > > * and set TVE[51] > > */ > > - tve = (pci_start_addr << 16) & (0xffffffull << > > 48); > > + tve = (pci_start_addr << 16) & (0xffffffull << > > 40); > > tve |= (pci_start_addr >> 38) & (3ull << 10); > > tve |= (end >> 8) & (0xfffffful << 16); > > tve |= (end >> 40) & (3ull << 8); > >
Le 03/02/2017 à 04:37, Benjamin Herrenschmidt a écrit : > On Fri, 2017-02-03 at 14:34 +1100, Alistair Popple wrote: >> Do we also need the same fix to hw/phb3.c and hw/npu.c? >> >> I haven't tested this but it looks reasonable. If I'm not mistaken >> the >> old behaviour would have allowed a PCIe device to access addresses >> below it's allocated PCIe bus address so this just increases >> protection by further restricting device access. > > I *think* we always put 0 in there anyway, don't we ? > > As for PHB3 and NPU, check the spec to see where the bits are. yeah, I think the OS only tries to write 0 in there. Though for capi2/CX5, I intend to try restricting to the capi window, which is why it caught my attention (DMA ops will be on tvt1). But that won't be in that function anyway. For the sake of it, I've checked the phb3 spec. Same thing. I don't have the npu v1 spec. Alistair, if you can check, I could submit a followup patch to cover phb3 and npu. Fred
Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > From the phb4 spec, when encoding the TVE, the pci start address bits > 49:24 are encoded in bits TVE[52:53]||[0:23]. > The mask to select bits 47:24 is incorrectly set. It should be > 0xffffff000000, shifted left by 16, i.e. 0xffffff << 40 > > Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> Thanks, merged to master as of 95c59d192376a4360278393da5df53e21ae3fc72
diff --git a/hw/phb4.c b/hw/phb4.c index b2723ca..c2c7cc7 100644 --- a/hw/phb4.c +++ b/hw/phb4.c @@ -1345,7 +1345,7 @@ static int64_t phb4_map_pe_dma_window_real(struct phb *phb, * and end address bits 49:24 into TVE[54:55]||[24:47] * and set TVE[51] */ - tve = (pci_start_addr << 16) & (0xffffffull << 48); + tve = (pci_start_addr << 16) & (0xffffffull << 40); tve |= (pci_start_addr >> 38) & (3ull << 10); tve |= (end >> 8) & (0xfffffful << 16); tve |= (end >> 40) & (3ull << 8);
From the phb4 spec, when encoding the TVE, the pci start address bits 49:24 are encoded in bits TVE[52:53]||[0:23]. The mask to select bits 47:24 is incorrectly set. It should be 0xffffff000000, shifted left by 16, i.e. 0xffffff << 40 Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> --- Already discussed with Benh hw/phb4.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)